Re: [PATCH 2/6] Btrfs: fix a tree mod logging issue for root replacement operations

[Jan Schmidt <list.btrfs@jan-o-sch.net>]

On Tue, October 23, 2012 at 17:28 (+0200), Liu Bo wrote:
> On 10/23/2012 09:55 PM, Jan Schmidt wrote:
>> Avoid the implicit free by tree_mod_log_set_root_pointer, which is wrong in
>> two places. Where needed, we call tree_mod_log_free_eb explicitly now.
>>
>> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
>> ---
>>  fs/btrfs/ctree.c |   10 ++--------
>>  1 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 44a7e25..e6b75cc 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -647,8 +647,6 @@ tree_mod_log_insert_root(struct btrfs_fs_info *fs_info,
>>  	if (tree_mod_dont_log(fs_info, NULL))
>>  		return 0;
>>  
>> -	__tree_mod_log_free_eb(fs_info, old_root);
>> -
>>  	ret = tree_mod_alloc(fs_info, flags, &tm);
>>  	if (ret < 0)
>>  		goto out;
>> @@ -926,12 +924,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
>>  			ret = btrfs_dec_ref(trans, root, buf, 1, 1);
>>  			BUG_ON(ret); /* -ENOMEM */
>>  		}
>> -		/*
>> -		 * don't log freeing in case we're freeing the root node, this
>> -		 * is done by tree_mod_log_set_root_pointer later
>> -		 */
>> -		if (buf != root->node && btrfs_header_level(buf) != 0)
>> -			tree_mod_log_free_eb(root->fs_info, buf);
>> +		tree_mod_log_free_eb(root->fs_info, buf);
> 
> Since we've owned a check for if eb's level is 0 in tree_mod_dont_log(),
> we can also get rid of these level checks in other places, can't we?

We can, yes. There may be hot paths where its worth an extra check to save the
overhead. That would require looking closer at the individual sites. Here it
definitely wasn't worth any extra check, so I dropped it en passant.

Thanks for reviewing and testing!
-Jan

>>  		clean_tree_block(trans, root, buf);
>>  		*last_ref = 1;
>>  	}
>> @@ -1728,6 +1721,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>>  			goto enospc;
>>  		}
>>  
>> +		tree_mod_log_free_eb(root->fs_info, root->node);
>>  		tree_mod_log_set_root_pointer(root, child);
>>  		rcu_assign_pointer(root->node, child);
>>  
>>
> 
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> Tested-by: Liu Bo <bo.li.liu@oracle.com>
> 
> thanks,
> liubo