Btrfs devel - Sep 2012 - [PATCH V4 01/12] Btrfs: fix error path in create_pending_snapshot()

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.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
Changelog v3 -> v4:
- No change.

Changelog v2 -> v3:
- rebase on the latest for-linus branch
- fix double free problem of pending->inherit

Changelog v1 -> v2:
- fix double dput() when aborting transaction. In the previous version of the
  patches, this problem was fixed in the second patch, it is not good because
  this problem is the bug of the patch in fact.
---
 fs/btrfs/transaction.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 3ee8d58..b259d22f2 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -962,18 +962,16 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 	u64 root_flags;
 	uuid_le new_uuid;
 
-	rsv = trans->block_rsv;
-
 	new_root_item = kmalloc(sizeof(*new_root_item), GFP_NOFS);
 	if (!new_root_item) {
 		ret = pending->error = -ENOMEM;
-		goto fail;
+		goto root_item_alloc_fail;
 	}
 
 	ret = btrfs_find_free_objectid(tree_root, &objectid);
 	if (ret) {
 		pending->error = ret;
-		goto fail;
+		goto no_free_objectid;
 	}
 
 	btrfs_reloc_pre_snapshot(trans, pending, &to_reserve);
@@ -983,22 +981,22 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 						  to_reserve);
 		if (ret) {
 			pending->error = ret;
-			goto fail;
+			goto no_free_objectid;
 		}
 	}
 
 	ret = btrfs_qgroup_inherit(trans, fs_info, root->root_key.objectid,
 				   objectid, pending->inherit);
-	kfree(pending->inherit);
 	if (ret) {
 		pending->error = ret;
-		goto fail;
+		goto no_free_objectid;
 	}
 
 	key.objectid = objectid;
 	key.offset = (u64)-1;
 	key.type = BTRFS_ROOT_ITEM_KEY;
 
+	rsv = trans->block_rsv;
 	trans->block_rsv = &pending->block_rsv;
 
 	dentry = pending->dentry;
@@ -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;
 	} else if (ret) {
-		goto abort_trans_dput;
+		goto abort_trans;
 	}
 
 	btrfs_i_size_write(parent_inode, parent_inode->i_size +
@@ -1029,7 +1026,7 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 	parent_inode->i_mtime = parent_inode->i_ctime = CURRENT_TIME;
 	ret = btrfs_update_inode(trans, parent_root, parent_inode);
 	if (ret)
-		goto abort_trans_dput;
+		goto abort_trans;
 
 	/*
 	 * pull in the delayed directory update
@@ -1038,10 +1035,8 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 	 * snapshot
 	 */
 	ret = btrfs_run_delayed_items(trans, root);
-	if (ret) { /* Transaction aborted */
-		dput(parent);
-		goto fail;
-	}
+	if (ret)	/* Transaction aborted */
+		goto abort_trans;
 
 	record_root_in_trans(trans, root);
 	btrfs_set_root_last_snapshot(&root->root_item, trans->transid);
@@ -1074,7 +1069,7 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 	if (ret) {
 		btrfs_tree_unlock(old);
 		free_extent_buffer(old);
-		goto abort_trans_dput;
+		goto abort_trans;
 	}
 
 	btrfs_set_lock_blocking(old);
@@ -1084,7 +1079,7 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 	btrfs_tree_unlock(old);
 	free_extent_buffer(old);
 	if (ret)
-		goto abort_trans_dput;
+		goto abort_trans;
 
 	/* see comments in should_cow_block() */
 	root->force_cow = 1;
@@ -1097,7 +1092,7 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 	btrfs_tree_unlock(tmp);
 	free_extent_buffer(tmp);
 	if (ret)
-		goto abort_trans_dput;
+		goto abort_trans;
 
 	/*
 	 * insert root back/forward references
@@ -1106,9 +1101,8 @@ static noinline int create_pending_snapshot(struct
btrfs_trans_handle *trans,
 				 parent_root->root_key.objectid,
 				 btrfs_ino(parent_inode), index,
 				 dentry->d_name.name, dentry->d_name.len);
-	dput(parent);
 	if (ret)
-		goto fail;
+		goto abort_trans;
 
 	key.offset = (u64)-1;
 	pending->snap = btrfs_read_fs_root_no_name(root->fs_info, &key);
@@ -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;
-- 
1.7.6.5
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On mon, 17 Sep 2012 18:56:27 +0200, David Sterba wrote:
> 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:
OK, I will send a separate patch to fix this problem.

Thanks for your review.
Miao
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html