Re: [PATCH] Btrfs: eliminate the exceptional root_tree refs=0

[Stefan Behrens <sbehrens@giantdisaster.de>]

On Fri, 06 Sep 2013 11:08:16 +0800, Miao Xie wrote:
> On 	thu, 5 Sep 2013 16:58:43 +0200, Stefan Behrens wrote:
>> The fact that btrfs_root_refs() returned 0 for the tree_root caused
>> bugs in the past, therefore it is set to 1 with this patch and
>> (hopefully) all affected code is adapted to this change.
>>
>> I verified this change by temporarily adding WARN_ON() checks
>> everywhere where btrfs_root_refs() is used, checking whether the
>> logic of the code is changed by btrfs_root_refs() returning 1
>> instead of 0 for root->root_key.objectid == BTRFS_ROOT_TREE_OBJECTID.
>> With these added checks, I ran the xfstests './check -g auto'.
>>
>> The two roots chunk_root and log_root_tree that are only referenced
>> by the superblock and the log_roots below the log_root_tree still
>> have btrfs_root_refs() == 0, only the tree_root is changed.
>>
>> Signed-off-by: Stefan Behrens <sbehrens@giantdisaster.de>
>> ---
>>  fs/btrfs/disk-io.c   |  1 +
>>  fs/btrfs/inode-map.c |  3 +--
>>  fs/btrfs/inode.c     | 21 ++++++++-------------
>>  3 files changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index fa8b2c6..ffc3e43 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2667,6 +2667,7 @@ retry_root_backup:
>>  
>>  	btrfs_set_root_node(&tree_root->root_item, tree_root->node);
>>  	tree_root->commit_root = btrfs_root_node(tree_root);
>> +	btrfs_set_root_refs(&tree_root->root_item, 1);
>>  
>>  	location.objectid = BTRFS_EXTENT_TREE_OBJECTID;
>>  	location.type = BTRFS_ROOT_ITEM_KEY;
>> diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
>> index 2c66ddb..d11e1c6 100644
>> --- a/fs/btrfs/inode-map.c
>> +++ b/fs/btrfs/inode-map.c
>> @@ -412,8 +412,7 @@ int btrfs_save_ino_cache(struct btrfs_root *root,
>>  		return 0;
>>  
>>  	/* Don't save inode cache if we are deleting this root */
>> -	if (btrfs_root_refs(&root->root_item) == 0 &&
>> -	    root != root->fs_info->tree_root)
>> +	if (btrfs_root_refs(&root->root_item) == 0)
>>  		return 0;
>>  
>>  	if (!btrfs_test_opt(root, INODE_MAP_CACHE))
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 26992ee..7d0ef55 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4472,8 +4472,10 @@ void btrfs_evict_inode(struct inode *inode)
>>  	trace_btrfs_inode_evict(inode);
>>  
>>  	truncate_inode_pages(&inode->i_data, 0);
>> -	if (inode->i_nlink && (btrfs_root_refs(&root->root_item) != 0 ||
>> -			       btrfs_is_free_space_inode(inode)))
>> +	if (inode->i_nlink &&
>> +	    ((btrfs_root_refs(&root->root_item) != 0 &&
>> +	      root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) ||
>> +	     btrfs_is_free_space_inode(inode)))
> 
> if (inode->i_nlink && btrfs_root_refs(&root->root_item)) {
> }
> 
> is OK, because with this patch, the refs of the free space inodes' root(tree root)
> should be 1. For the ino cache inodes' root(fs/file root), if the root is dead,
> we can drop the ino cache inode safely. And if the root is not dead, the refs
> should be > 0, we will skip the drop.

Thank you for your comments!

It needs to be like this or it doesn't work. The code that you propose
changes the logic when close_ctree() calls iput(fs_info->btree_inode).
The if-condition in btrfs_evict_inode() used to be false for
fs_info->btree_inode before this patch and would become true. And the
system locks up in the middle of a './check -g auto' run with page cache
issues and I have also seen the OOM killer killing the entire test box.

But maybe you can propose how to make this if-statement more explicit
and readable, maybe by explicitly checking for inode ==
fs_info->btree_inode?


>>  		goto no_delete;
>>  
>>  	if (is_bad_inode(inode)) {
>> @@ -4490,7 +4492,8 @@ void btrfs_evict_inode(struct inode *inode)
>>  	}
>>  
>>  	if (inode->i_nlink > 0) {
>> -		BUG_ON(btrfs_root_refs(&root->root_item) != 0);
>> +		BUG_ON(btrfs_root_refs(&root->root_item) != 0 &&
>> +		       root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID);
> 
> This change is unnecessary because the refs of the root tree is 1 now.

Because of the thing above, this is needed.


> 
>>  		goto no_delete;
>>  	}
>>  
>> @@ -4731,14 +4734,7 @@ static void inode_tree_del(struct inode *inode)
>>  	}
>>  	spin_unlock(&root->inode_lock);
>>  
>> -	/*
>> -	 * Free space cache has inodes in the tree root, but the tree root has a
>> -	 * root_refs of 0, so this could end up dropping the tree root as a
>> -	 * snapshot, so we need the extra !root->fs_info->tree_root check to
>> -	 * make sure we don't drop it.
>> -	 */
>> -	if (empty && btrfs_root_refs(&root->root_item) == 0 &&
>> -	    root != root->fs_info->tree_root) {
>> +	if (empty && btrfs_root_refs(&root->root_item) == 0) {
>>  		synchronize_srcu(&root->fs_info->subvol_srcu);
>>  		spin_lock(&root->inode_lock);
>>  		empty = RB_EMPTY_ROOT(&root->inode_tree);
>> @@ -7872,8 +7868,7 @@ int btrfs_drop_inode(struct inode *inode)
>>  		return 1;
>>  
>>  	/* the snap/subvol tree is on deleting */
>> -	if (btrfs_root_refs(&root->root_item) == 0 &&
>> -	    root != root->fs_info->tree_root)
>> +	if (btrfs_root_refs(&root->root_item) == 0)
>>  		return 1;
>>  	else
>>  		return generic_drop_inode(inode);
> 
> The others is OK.