On Thu, Sep 06, 2012 at 06:00:32PM +0800, Miao Xie wrote:
> This patch fixes the following problem:
> - If we failed to deal with the delayed dir items, we should abort
> transaction,
> just as its comment said. Fix it.
> - If root reference or root back reference insertion failed, we should
> abort transaction. Fix it.
> - Fix the double free problem of pending->inherit.
> - Do not restore the trans->rsv if we doesn't change it.
> - make the error path more clearly.
I've noticed a pattern in the error + transaction abort paths, that is
touched in this patch and would like to ask you to update it:
> @@ -1018,10 +1016,9 @@ static noinline int create_pending_snapshot(struct
> btrfs_trans_handle *trans,
> BTRFS_FT_DIR, index);
> if (ret == -EEXIST) {
> pending->error = -EEXIST;
> - dput(parent);
> goto fail;
normal exit path: here we don't abort transaction, just go the exit
block and do the cleanup
> } else if (ret) {
> - goto abort_trans_dput;
> + goto abort_trans;
a transaction abort path: here we jump to a common block that calls
abort, but we lose the information where the abort occured
I went through the code and saw several uses of this pattern (and I
remember more than one bugreport that pointed to a abort_transaction
call without leaving any traces what condition failed).
(Search regex I used 'goto.*abort')
So the proposed pattern to use is
---
if (condition) {
btrfs_transaction_abort(...);
goto fail;
}
fail:
<cleanup>
return ...;
---
> @@ -1120,15 +1114,15 @@ static noinline int create_pending_snapshot(struct
> btrfs_trans_handle *trans,
> ret = btrfs_reloc_post_snapshot(trans, pending);
> if (ret)
> goto abort_trans;
> - ret = 0;
> fail:
> - kfree(new_root_item);
> + dput(parent);
> trans->block_rsv = rsv;
> +no_free_objectid:
> + kfree(new_root_item);
> +root_item_alloc_fail:
> btrfs_block_rsv_release(root, &pending->block_rsv, (u64)-1);
> return ret;
>
> -abort_trans_dput:
> - dput(parent);
> abort_trans:
> btrfs_abort_transaction(trans, root, ret);
> goto fail;
^^^^^^^^^^^^^^^^
(end of function here)
this will also remove all the instances where a function ends with a
'goto'. All instances are convertible to the pattern described above.
Atlernate approach that I originally considered for fixing was to
introduce a call like 'btrfs_mark_transaction_abort_callsite' which
would need to add a field to fs_info and print it later. But, if we're
going to touch all the code, it makes sense to utilize the
infrastructure we already have.
Please consider updating your patch, I'll send a separate patch that
deals with aborts outside of create_pending_snapshot.
TIA,
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
