| Message ID | 3e067c8b0956f0134501c8eea2e19c8eb5adcedc.1679910088.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: optimize disks flush code | expand |
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; > }
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
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; >> }
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; } /*
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(-)