Re: [PATCH v3 1/6] btrfs: btrfs_cleanup_fs_roots handle ret variable

[Anand Jain <anand.jain@oracle.com>]

On 5/21/24 23:10, David Sterba wrote:
> On Thu, May 16, 2024 at 07:12:10PM +0800, Anand Jain wrote:
>> Since err represents the function return value, rename it as ret,
>> and rename the original ret, which serves as a helper return value,
>> to found. Also, optimize the code to continue call btrfs_put_root()
>> for the rest of the root if even after btrfs_orphan_cleanup() returns
>> error.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v3: Add a code comment.
>> v2: Rename to 'found' instead of 'ret2' (Josef).
>>      Call btrfs_put_root() in the while-loop, avoids use of the variable
>> 	'found' outside of the while loop (Qu).
>>      Use 'unsigned int i' instead of 'int' (Goffredo).
>>
>>   fs/btrfs/disk-io.c | 38 ++++++++++++++++++++------------------
>>   1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index a91a8056758a..d38cf973b02a 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2925,22 +2925,23 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>   {
>>   	u64 root_objectid = 0;
>>   	struct btrfs_root *gang[8];
>> -	int i = 0;
>> -	int err = 0;
>> -	unsigned int ret = 0;
>> +	int ret = 0;
>>   
>>   	while (1) {
>> +		unsigned int i;
>> +		unsigned int found;
>> +
>>   		spin_lock(&fs_info->fs_roots_radix_lock);
>> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>> +		found = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
>>   					     (void **)gang, root_objectid,
>>   					     ARRAY_SIZE(gang));
>> -		if (!ret) {
>> +		if (!found) {
>>   			spin_unlock(&fs_info->fs_roots_radix_lock);
>>   			break;
>>   		}
>> -		root_objectid = btrfs_root_id(gang[ret - 1]) + 1;
>> +		root_objectid = btrfs_root_id(gang[found - 1]) + 1;
>>   
>> -		for (i = 0; i < ret; i++) {
>> +		for (i = 0; i < found; i++) {
> 
> You could also move the declaration of 'i' to the for loop as you move
> the other definition anyway.

Yep. Done in v4.

Thanks.

> 
>>   			/* Avoid to grab roots in dead_roots. */
>>   			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
>>   				gang[i] = NULL;
>> @@ -2951,24 +2952,25 @@ static int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>   		}
>>   		spin_unlock(&fs_info->fs_roots_radix_lock);
>>   
>> -		for (i = 0; i < ret; i++) {
>> +		for (i = 0; i < found; i++) {
>>   			if (!gang[i])
>>   				continue;
>>   			root_objectid = btrfs_root_id(gang[i]);
>> -			err = btrfs_orphan_cleanup(gang[i]);
>> -			if (err)
>> -				goto out;
>> +			/*
>> +			 * Continue to release the remaining roots after the first
>> +			 * error without cleanup and preserve the first error
>> +			 * for the return.
>> +			 */
>> +			if (!ret)
>> +				ret = btrfs_orphan_cleanup(gang[i]);
>>   			btrfs_put_root(gang[i]);
>>   		}
>> +		if (ret)
>> +			break;
>> +
>>   		root_objectid++;
>>   	}
>> -out:
>> -	/* Release the uncleaned roots due to error. */
>> -	for (; i < ret; i++) {
>> -		if (gang[i])
>> -			btrfs_put_root(gang[i]);
>> -	}
>> -	return err;
>> +	return ret;
>>   }
>>   
>>   /*
>> -- 
>> 2.38.1
>>