Re: [PATCH 2/3] btrfs: use btrfs_find_inode in btrfs_prune_dentries

[Nikolay Borisov <nborisov@suse.com>]

On 4.08.22 г. 18:41 ч., David Sterba wrote:
> On Thu, Jul 21, 2022 at 04:50:05PM +0300, Nikolay Borisov wrote:
>> Now that we have a standalone function which encapsulates the logic of
>> searching the root's ino rb tree use that. It results in massive
>> simplification of the code.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>   fs/btrfs/inode.c | 47 +++++++++++++++++------------------------------
>>   1 file changed, 17 insertions(+), 30 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index c11169ba28b2..06724925a3d3 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4635,44 +4635,27 @@ struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid)
>>   static void btrfs_prune_dentries(struct btrfs_root *root)
>>   {
>>   	struct btrfs_fs_info *fs_info = root->fs_info;
>> -	struct rb_node *node;
>> -	struct rb_node *prev;
>> -	struct btrfs_inode *entry;
>> -	struct inode *inode;
>> +	struct rb_node *node = NULL;
>>   	u64 objectid = 0;
>>   
>>   	if (!BTRFS_FS_ERROR(fs_info))
>>   		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
>>   
>>   	spin_lock(&root->inode_lock);
>> -again:
>> -	node = root->inode_tree.rb_node;
>> -	prev = NULL;
>> -	while (node) {
>> -		prev = node;
>> -		entry = rb_entry(node, struct btrfs_inode, rb_node);
>> +	do {
>> +		struct btrfs_inode *entry;
>> +		struct inode *inode;
>>   
>> -		if (objectid < btrfs_ino(entry))
>> -			node = node->rb_left;
>> -		else if (objectid > btrfs_ino(entry))
>> -			node = node->rb_right;
>> -		else
>> -			break;
>> -	}
>> -	if (!node) {
>> -		while (prev) {
>> -			entry = rb_entry(prev, struct btrfs_inode, rb_node);
>> -			if (objectid <= btrfs_ino(entry)) {
>> -				node = prev;
>> +		if (!node) {
>> +			node = btrfs_find_inode(root, objectid);
>> +			if (!node)
>>   				break;
>> -			}
>> -			prev = rb_next(prev);
>>   		}
>> -	}
>> -	while (node) {
>> +
>>   		entry = rb_entry(node, struct btrfs_inode, rb_node);
>>   		objectid = btrfs_ino(entry) + 1;
>>   		inode = igrab(&entry->vfs_inode);
>> +
>>   		if (inode) {
>>   			spin_unlock(&root->inode_lock);
>>   			if (atomic_read(&inode->i_count) > 1)
>> @@ -4684,14 +4667,18 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
>>   			iput(inode);
>>   			cond_resched();
>>   			spin_lock(&root->inode_lock);
>> -			goto again;
>> +			node = NULL;
> 
> This sets node to NULL and continues, which sends it down to while
> (node), which makes it effectively a break and it's not equivalent to
> the original behaviour that jumps back to again: or "do {" , or I'm
> missing something.

You are right, this is buggy, I'll rework it.

> 
>> +			continue;
>>   		}
>>   
>> -		if (cond_resched_lock(&root->inode_lock))
>> -			goto again;
>> +		if (cond_resched_lock(&root->inode_lock)) {
>> +			node = NULL;
>> +			continue;
>> +		}
>>   
>>   		node = rb_next(node);
>> -	}
>> +	} while(node);
>> +
>>   	spin_unlock(&root->inode_lock);
>>   }
>>   
>> -- 
>> 2.25.1