[PATCH 0/4] btrfs: optimize disks flush code

[PATCH 0/4] btrfs: optimize disks flush code

[Anand Jain]

A few items were identified that could be optimized and simplified in
wait_dev_flush() and wait_dev_flush().

Anand Jain (4):
  btrfs: move last_flush_error to write_dev_flush and wait_dev_flush
  btrfs: opencode check_barrier_error()
  Btrfs: change wait_dev_flush() return type to bool
  btrfs: use test_and_clear_bit() in wait_dev_flush()

 fs/btrfs/disk-io.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

-- 
2.39.2

[PATCH 1/4] btrfs: move last_flush_error to write_dev_flush and wait_dev_flush

[Anand Jain]

We parallelize the flush command across devices using our own code,
write_dev_flush() sends the flush command to each device and
wait_dev_flush() waits for the flush to complete on all devices. Errors
from each device are recorded at device->last_flush_error and reset to
BLK_STS_OK in write_dev_flush() and to the error, if any, in
wait_dev_flush(). These functions are called from barrier_all_devices().

This patch consolidates the use of device->last_flush_error in
write_dev_flush() and wait_dev_flush() to remove it from
barrier_all_devices().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b638e27468a7..7f3fa5e2253d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4072,6 +4072,8 @@ static void write_dev_flush(struct btrfs_device *device)
 {
 	struct bio *bio = &device->flush_bio;
 
+	device->last_flush_error = BLK_STS_OK;
+
 #ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 	/*
 	 * When a disk has write caching disabled, we skip submission of a bio
@@ -4111,6 +4113,11 @@ static blk_status_t wait_dev_flush(struct btrfs_device *device)
 	clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state);
 	wait_for_completion_io(&device->flush_wait);
 
+	if (bio->bi_status) {
+		device->last_flush_error = bio->bi_status;
+		btrfs_dev_stat_inc_and_print(device, BTRFS_DEV_STAT_FLUSH_ERRS);
+	}
+
 	return bio->bi_status;
 }
 
@@ -4145,7 +4152,6 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 			continue;
 
 		write_dev_flush(dev);
-		dev->last_flush_error = BLK_STS_OK;
 	}
 
 	/* wait for all the barriers */
@@ -4161,12 +4167,8 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 			continue;
 
 		ret = wait_dev_flush(dev);
-		if (ret) {
-			dev->last_flush_error = ret;
-			btrfs_dev_stat_inc_and_print(dev,
-					BTRFS_DEV_STAT_FLUSH_ERRS);
+		if (ret)
 			errors_wait++;
-		}
 	}
 
 	if (errors_wait) {
-- 
2.39.2

[PATCH 2/4] btrfs: opencode check_barrier_error()

[Anand Jain]

check_barrier_error() is almost a single line function, and just calls
btrfs_check_rw_degradable(). Instead, opencode it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7f3fa5e2253d..745be1f4ab6d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4121,13 +4121,6 @@ static blk_status_t wait_dev_flush(struct btrfs_device *device)
 	return bio->bi_status;
 }
 
-static int check_barrier_error(struct btrfs_fs_info *fs_info)
-{
-	if (!btrfs_check_rw_degradable(fs_info, NULL))
-		return -EIO;
-	return 0;
-}
-
 /*
  * send an empty flush down to each device in parallel,
  * then wait for them
@@ -4171,14 +4164,13 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 			errors_wait++;
 	}
 
-	if (errors_wait) {
-		/*
-		 * At some point we need the status of all disks
-		 * to arrive at the volume status. So error checking
-		 * is being pushed to a separate loop.
-		 */
-		return check_barrier_error(info);
-	}
+	/*
+	 * Checks last_flush_error of disks in order to determine the
+	 * volume state.
+	 */
+	if (errors_wait && !btrfs_check_rw_degradable(info, NULL))
+		return -EIO;
+
 	return 0;
 }
 
-- 
2.39.2

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

[Anand Jain]

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.
  */
-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;
 
 	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;
 }
 
 /*
-- 
2.39.2

[PATCH 4/4] btrfs: use test_and_clear_bit() in wait_dev_flush()

[Anand Jain]

The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT
bit and then clears it separately. Instead, use test_and_clear_bit().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 040142f2e76c..1f9e2a2a8267 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4108,10 +4108,9 @@ 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))
+	if (!test_and_clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state))
 		return true;
 
-	clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state);
 	wait_for_completion_io(&device->flush_wait);
 
 	if (bio->bi_status) {
-- 
2.39.2

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

[David Sterba]

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++;

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();

>   */
> -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'

>  
>  	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;
>  }

Re: [PATCH 4/4] btrfs: use test_and_clear_bit() in wait_dev_flush()

[David Sterba]

On Mon, Mar 27, 2023 at 05:53:10PM +0800, Anand Jain wrote:
> The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT
> bit and then clears it separately. Instead, use test_and_clear_bit().

But why would we need to do it like that? The write and wait are
executed in one thread so we don't need atomic change to the status and
thus a separate set/test/clear bit are fine. If not, then please
explain. Thanks.

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

[kernel test robot]

Hi Anand,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on linus/master v6.3-rc4 next-20230327]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Anand-Jain/btrfs-move-last_flush_error-to-write_dev_flush-and-wait_dev_flush/20230327-180139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/3e067c8b0956f0134501c8eea2e19c8eb5adcedc.1679910088.git.anand.jain%40oracle.com
patch subject: [PATCH 3/4] Btrfs: change wait_dev_flush() return type to bool
config: i386-randconfig-s002 (https://download.01.org/0day-ci/archive/20230328/202303280731.3zPschfL-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d26d540e470da0010fed61401cf0b7147f175aa1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Anand-Jain/btrfs-move-last_flush_error-to-write_dev_flush-and-wait_dev_flush/20230327-180139
        git checkout d26d540e470da0010fed61401cf0b7147f175aa1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303280731.3zPschfL-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> fs/btrfs/disk-io.c:4172:21: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted blk_status_t [usertype] ret @@     got bool @@
   fs/btrfs/disk-io.c:4172:21: sparse:     expected restricted blk_status_t [usertype] ret
   fs/btrfs/disk-io.c:4172:21: sparse:     got bool

vim +4172 fs/btrfs/disk-io.c

387125fc722a8e Chris Mason       2011-11-18  4133  
387125fc722a8e Chris Mason       2011-11-18  4134  /*
387125fc722a8e Chris Mason       2011-11-18  4135   * send an empty flush down to each device in parallel,
387125fc722a8e Chris Mason       2011-11-18  4136   * then wait for them
387125fc722a8e Chris Mason       2011-11-18  4137   */
387125fc722a8e Chris Mason       2011-11-18  4138  static int barrier_all_devices(struct btrfs_fs_info *info)
387125fc722a8e Chris Mason       2011-11-18  4139  {
387125fc722a8e Chris Mason       2011-11-18  4140  	struct list_head *head;
387125fc722a8e Chris Mason       2011-11-18  4141  	struct btrfs_device *dev;
5af3e8cce8b7ba Stefan Behrens    2012-08-01  4142  	int errors_wait = 0;
4e4cbee93d5613 Christoph Hellwig 2017-06-03  4143  	blk_status_t ret;
387125fc722a8e Chris Mason       2011-11-18  4144  
1538e6c52e1917 David Sterba      2017-06-16  4145  	lockdep_assert_held(&info->fs_devices->device_list_mutex);
387125fc722a8e Chris Mason       2011-11-18  4146  	/* send down all the barriers */
387125fc722a8e Chris Mason       2011-11-18  4147  	head = &info->fs_devices->devices;
1538e6c52e1917 David Sterba      2017-06-16  4148  	list_for_each_entry(dev, head, dev_list) {
e6e674bd4d54fe Anand Jain        2017-12-04  4149  		if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
f88ba6a2a44ee9 Hidetoshi Seto    2014-02-05  4150  			continue;
cea7c8bf77209b Anand Jain        2017-06-13  4151  		if (!dev->bdev)
387125fc722a8e Chris Mason       2011-11-18  4152  			continue;
e12c96214d28f9 Anand Jain        2017-12-04  4153  		if (!test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &dev->dev_state) ||
ebbede42d47dc7 Anand Jain        2017-12-04  4154  		    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
387125fc722a8e Chris Mason       2011-11-18  4155  			continue;
387125fc722a8e Chris Mason       2011-11-18  4156  
4fc6441aac7589 Anand Jain        2017-06-13  4157  		write_dev_flush(dev);
387125fc722a8e Chris Mason       2011-11-18  4158  	}
387125fc722a8e Chris Mason       2011-11-18  4159  
387125fc722a8e Chris Mason       2011-11-18  4160  	/* wait for all the barriers */
1538e6c52e1917 David Sterba      2017-06-16  4161  	list_for_each_entry(dev, head, dev_list) {
e6e674bd4d54fe Anand Jain        2017-12-04  4162  		if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state))
f88ba6a2a44ee9 Hidetoshi Seto    2014-02-05  4163  			continue;
387125fc722a8e Chris Mason       2011-11-18  4164  		if (!dev->bdev) {
5af3e8cce8b7ba Stefan Behrens    2012-08-01  4165  			errors_wait++;
387125fc722a8e Chris Mason       2011-11-18  4166  			continue;
387125fc722a8e Chris Mason       2011-11-18  4167  		}
e12c96214d28f9 Anand Jain        2017-12-04  4168  		if (!test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &dev->dev_state) ||
ebbede42d47dc7 Anand Jain        2017-12-04  4169  		    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
387125fc722a8e Chris Mason       2011-11-18  4170  			continue;
387125fc722a8e Chris Mason       2011-11-18  4171  
4fc6441aac7589 Anand Jain        2017-06-13 @4172  		ret = wait_dev_flush(dev);
7b3115dae5a0a2 Anand Jain        2023-03-27  4173  		if (ret)
5af3e8cce8b7ba Stefan Behrens    2012-08-01  4174  			errors_wait++;
387125fc722a8e Chris Mason       2011-11-18  4175  	}
401b41e5a85a63 Anand Jain        2017-05-06  4176  
401b41e5a85a63 Anand Jain        2017-05-06  4177  	/*
a112dad7e3abca Anand Jain        2023-03-27  4178  	 * Checks last_flush_error of disks in order to determine the
a112dad7e3abca Anand Jain        2023-03-27  4179  	 * volume state.
401b41e5a85a63 Anand Jain        2017-05-06  4180  	 */
a112dad7e3abca Anand Jain        2023-03-27  4181  	if (errors_wait && !btrfs_check_rw_degradable(info, NULL))
a112dad7e3abca Anand Jain        2023-03-27  4182  		return -EIO;
a112dad7e3abca Anand Jain        2023-03-27  4183  
387125fc722a8e Chris Mason       2011-11-18  4184  	return 0;
387125fc722a8e Chris Mason       2011-11-18  4185  }
387125fc722a8e Chris Mason       2011-11-18  4186  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Anand Jain]

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;
>>   }

Re: [PATCH 4/4] btrfs: use test_and_clear_bit() in wait_dev_flush()

[Anand Jain]

On 3/28/23 01:14, David Sterba wrote:
> On Mon, Mar 27, 2023 at 05:53:10PM +0800, Anand Jain wrote:
>> The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT
>> bit and then clears it separately. Instead, use test_and_clear_bit().
> 
> But why would we need to do it like that? The write and wait are
> executed in one thread so we don't need atomic change to the status and
> thus a separate set/test/clear bit are fine. If not, then please
> explain. Thanks.

It's true that atomic test_and_clear_bit() isn't necessary in this case.
Nonetheless, using it have benefits such as cleaner code and improved
efficiency[1].

  [1]. I was curious, so I made wait_dev_flush() non-inline and checked
  the ASM code for wait_dev_flush(). After the patch, there were 8 fewer
  instructions.

I'm okay with dropping this patch if you prefer to maintain the correct
usage of test_and_clear_bit().

Thanks, Anand

[PATCH] fixup: Btrfs: change wait_dev_flush() return type to bool v2

[Anand Jain]

A fixup for the patch:
 Btrfs: change wait_dev_flush() return type to bool v2

In v2:
Fixes:
 Update write_dev_flush() to return false upon success and true upon errors.
 Remove the local variable ret in barrier_all_devices().
 Correct the bug where errors_wait was incremented upon success.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Dave,

I am sending this patch as a fix-up while I am still waiting to hear
whether patch 4/4 will be dropped. If you would prefer to have this
series sent as v2 with patch 4/4 removed, I can do that.

 fs/btrfs/disk-io.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1f9e2a2a8267..a240e77448e0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4102,24 +4102,24 @@ 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.
+ * Return true for any error, and false otherwise.
  */
 static bool wait_dev_flush(struct btrfs_device *device)
 {
 	struct bio *bio = &device->flush_bio;
 
 	if (!test_and_clear_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state))
-		return true;
+		return false;
 
 	wait_for_completion_io(&device->flush_wait);
 
 	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 true;
 	}
 
-	return true;
+	return false;
 }
 
 /*
@@ -4131,7 +4131,6 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 	struct list_head *head;
 	struct btrfs_device *dev;
 	int errors_wait = 0;
-	blk_status_t ret;
 
 	lockdep_assert_held(&info->fs_devices->device_list_mutex);
 	/* send down all the barriers */
@@ -4160,8 +4159,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 		    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))
 			continue;
 
-		ret = wait_dev_flush(dev);
-		if (ret)
+		if (wait_dev_flush(dev))
 			errors_wait++;
 	}
 
-- 
2.39.2

Re: [PATCH] fixup: Btrfs: change wait_dev_flush() return type to bool v2

[David Sterba]

On Tue, Mar 28, 2023 at 01:31:27PM +0800, Anand Jain wrote:
> A fixup for the patch:
>  Btrfs: change wait_dev_flush() return type to bool v2
> 
> In v2:
> Fixes:
>  Update write_dev_flush() to return false upon success and true upon errors.
>  Remove the local variable ret in barrier_all_devices().
>  Correct the bug where errors_wait was incremented upon success.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> Dave,
> 
> I am sending this patch as a fix-up while I am still waiting to hear
> whether patch 4/4 will be dropped. If you would prefer to have this
> series sent as v2 with patch 4/4 removed, I can do that.

Ok let's do test_and_clear(), I'll fold the fixup and add the series to
misc-next. Thanks.

Re: [PATCH 4/4] btrfs: use test_and_clear_bit() in wait_dev_flush()

[David Sterba]

On Tue, Mar 28, 2023 at 01:05:12PM +0800, Anand Jain wrote:
> 
> 
> On 3/28/23 01:14, David Sterba wrote:
> > On Mon, Mar 27, 2023 at 05:53:10PM +0800, Anand Jain wrote:
> >> The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT
> >> bit and then clears it separately. Instead, use test_and_clear_bit().
> > 
> > But why would we need to do it like that? The write and wait are
> > executed in one thread so we don't need atomic change to the status and
> > thus a separate set/test/clear bit are fine. If not, then please
> > explain. Thanks.
> 
> It's true that atomic test_and_clear_bit() isn't necessary in this case.
> Nonetheless, using it have benefits such as cleaner code and improved
> efficiency[1].
> 
>   [1]. I was curious, so I made wait_dev_flush() non-inline and checked
>   the ASM code for wait_dev_flush(). After the patch, there were 8 fewer
>   instructions.
> 
> I'm okay with dropping this patch if you prefer to maintain the correct
> usage of test_and_clear_bit().

Fewer instructions is a bonus here, but from the logic POV a test_bit in
a condition immediately followed by a clear_bit is not a common
pattern.  So even if we don't need the atomic semantics it's following a
common pattern which is good.