Re: [PATCH 3/4] Btrfs: change wait_dev_flush() return type to bool

From: Anand Jain <anand.jain@oracle.com>
To: dsterba@suse.cz
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 3/4] Btrfs: change wait_dev_flush() return type to bool
Date: Tue, 28 Mar 2023 10:58:53 +0800	[thread overview]
Message-ID: <cc29cc5b-b556-425b-ec3c-3f88fc3cc51c@oracle.com> (raw)
In-Reply-To: <20230327171144.GH10580@twin.jikos.cz>

On 3/28/23 01:11, David Sterba wrote:
> On Mon, Mar 27, 2023 at 05:53:09PM +0800, Anand Jain wrote:
>> The flush error code is maintained in btrfs_device::last_flush_error, so
>> there is no point in returning it in wait_dev_flush() when it is not being
>> used. Instead, we can return a boolean value.
>>
>> Note that even though btrfs_device::last_flush_error may not be used, we
>> will keep it for now.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/disk-io.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 745be1f4ab6d..040142f2e76c 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -4102,13 +4102,14 @@ static void write_dev_flush(struct btrfs_device *device)
>>   
>>   /*
>>    * If the flush bio has been submitted by write_dev_flush, wait for it.
>> + * Return false for any error, and true otherwise.
> 
> This does not match how the function is used, originally a zero value
> means no error, now zero (false) means an error.
> 
> 4152         list_for_each_entry(dev, head, dev_list) {
> 4153                 if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
> 4154                         continue;
> 4155                 if (!dev->bdev) {
> 4156                         errors_wait++;
> 4157                         continue;
> 4158                 }
> 4159                 if (!test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &dev->dev_state) ||
> 4160                     !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
> 4161                         continue;
> 4162
> 4163                 ret = wait_dev_flush(dev);
> 4164                 if (ret)
> 4165                         errors_wait++;
> 4166         }
> 
> So here it's reversed (with all patches applied). You could keep the
> meaning of the retrun value to be true=ok, false=error, it's still
> understandable if there conditions looks like
> 
> 	ret = wait_dev_flush()
> 	if (!ret)
> 		errors++;
> 

  My bad. Though I knew, it slipped. Thanks for pointing it out.

> Another pattern is to return true on errors (typically functions that
> check some condition), so it's the conditions are structured as:
> 
> 	if (error)
> 		handle();
> 

  Sure. I'll fix it.

>>    */
>> -static blk_status_t wait_dev_flush(struct btrfs_device *device)
>> +static bool wait_dev_flush(struct btrfs_device *device)
>>   {
>>   	struct bio *bio = &device->flush_bio;
>>   
>>   	if (!test_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state))
>> -		return BLK_STS_OK;
>> +		return true;
> 
> This should be 'false'
Thanks.

>>   
>>   	clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state);
>>   	wait_for_completion_io(&device->flush_wait);
>> @@ -4116,9 +4117,10 @@ static blk_status_t wait_dev_flush(struct btrfs_device *device)
>>   	if (bio->bi_status) {
>>   		device->last_flush_error = bio->bi_status;
>>   		btrfs_dev_stat_inc_and_print(device, BTRFS_DEV_STAT_FLUSH_ERRS);
>> +		return false;
>>   	}
>>   
>> -	return bio->bi_status;
>> +	return true;
>>   }