On mon, 19 Nov 2012 18:18:48 +0800, Liu Bo wrote:
>> @@ -2316,14 +2315,12 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans,
>> if (ret) {
>> printk(KERN_DEBUG "btrfs: run_delayed_extent_op returned %d\n", ret);
>> spin_lock(&delayed_refs->lock);
>> + btrfs_delayed_ref_unlock(locked_ref);
>> return ret;
>> }
>>
>> goto next;
>> }
>> -
>> - list_del_init(&locked_ref->cluster);
>> - locked_ref = NULL;
>> }
>>
>> ref->in_tree = 0;
>> @@ -2350,11 +2347,24 @@ static noinline int run_clustered_refs(struct btrfs_trans_handle *trans,
>>
>> ret = run_one_delayed_ref(trans, root, ref, extent_op,
>> must_insert_reserved);
>> -
>> - btrfs_put_delayed_ref(ref);
>> kfree(extent_op);
>> count++;
>>
>> + /*
>> + * If this node is a head, we will pick the next head to deal
>> + * with. If there is something wrong when we process the
>> + * delayed ref, we will end our operation. So in these two
>> + * cases, we have to unlock the head and drop it from the
>> + * cluster list before we release it though the code is ugly.
>> + */
>> + if (btrfs_delayed_ref_is_head(ref) || ret) {
>> + list_del_init(&locked_ref->cluster);
>> + btrfs_delayed_ref_unlock(locked_ref);
>> + locked_ref = NULL;
>> + }
>> +
>
> In case that we don't remove mutex_unlock above,
>
> if ret is non-zero, either
> A)locked_ref is not NULL, or
> B)locked_ref is NULL, and it has done list_del_init above and
> also done mutex_unlock in run_one_delayed_ref().
>
> So in the case A), it is ok to do list_del_init() and mutex_unlock(),
> while in the case B), we need to do nothing.
>
> Then the code can be clean as we wish,
> if (ret) {
> if (locked_ref) {
> list_del_init();
> mutex_unlock();
> }
> ...
> }
I think it is not good style that locking/unlocking a lock in the different functions, because
it is error prone and the readability of the code is very bad, so I remove mutex_unlock() in
run_one_delayed_ref().
Maybe I should not mix the code of the error path into the normal one, I will send out a new patch
to make the code cleaner and more readable.
Thanks
Miao
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
