* [PATCH 0/2] cleanup write_dev_flush() @ 2017-06-09 6:52 Anand Jain 2017-06-09 6:52 ` [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore Anand Jain 2017-06-09 6:52 ` [PATCH 2/2] btrfs: flush error also accounts for its send error Anand Jain 0 siblings, 2 replies; 6+ messages in thread From: Anand Jain @ 2017-06-09 6:52 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba Commit 9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling has removed ENOMEM in write_dev_flush() so the below two patches adds its related cleanups. This patch is on top of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next But without the namelen-check patches, the last commit is 30426258414b btrfs: pass bytes to btrfs_bio_alloc Anand Jain (2): btrfs: write_dev_flush does not return ENOMEM anymore btrfs: flush error also accounts for its send error fs/btrfs/disk-io.c | 99 +++++++++++++++++++----------------------------------- 1 file changed, 35 insertions(+), 64 deletions(-) -- 2.8.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore 2017-06-09 6:52 [PATCH 0/2] cleanup write_dev_flush() Anand Jain @ 2017-06-09 6:52 ` Anand Jain 2017-06-12 16:14 ` David Sterba 2017-06-09 6:52 ` [PATCH 2/2] btrfs: flush error also accounts for its send error Anand Jain 1 sibling, 1 reply; 6+ messages in thread From: Anand Jain @ 2017-06-09 6:52 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba Commit 9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling removed the -ENOMEM return from write_dev_flush() so no need to check for the -ENOMEM during send. This patch also peals write_dev_flush's wait part of the code, and creates a new function wait_dev_flush(). Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/disk-io.c | 84 ++++++++++++++++++------------------------------------ 1 file changed, 28 insertions(+), 56 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 9f2ffe2c6afb..d8151839bb3d 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3488,62 +3488,48 @@ static void btrfs_end_empty_barrier(struct bio *bio) } /* - * trigger flushes for one the devices. If you pass wait == 0, the flushes are - * sent down. With wait == 1, it waits for the previous flush. - * - * any device where the flush fails with eopnotsupp are flagged as not-barrier - * capable + * trigger flushes for one the devices. */ -static int write_dev_flush(struct btrfs_device *device, int wait) +static void write_dev_flush(struct btrfs_device *device) { struct request_queue *q = bdev_get_queue(device->bdev); struct bio *bio; - int ret = 0; if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) - return 0; - - if (wait) { - bio = device->flush_bio; - if (!bio) - /* - * This means the alloc has failed with ENOMEM, however - * here we return 0, as its not a device error. - */ - return 0; - - wait_for_completion(&device->flush_wait); - - if (bio->bi_error) { - ret = bio->bi_error; - btrfs_dev_stat_inc_and_print(device, - BTRFS_DEV_STAT_FLUSH_ERRS); - } - - /* drop the reference from the wait == 0 run */ - bio_put(bio); - device->flush_bio = NULL; - - return ret; - } + return; - /* - * one reference for us, and we leave it for the - * caller - */ - device->flush_bio = NULL; bio = btrfs_io_bio_alloc(GFP_NOFS, 0); bio->bi_end_io = btrfs_end_empty_barrier; bio->bi_bdev = device->bdev; bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH; init_completion(&device->flush_wait); bio->bi_private = &device->flush_wait; + device->flush_bio = bio; bio_get(bio); btrfsic_submit_bio(bio); +} - return 0; +static int wait_dev_flush(struct btrfs_device *device) +{ + int ret; + struct bio *bio = device->flush_bio; + + if (!bio) + return 0; + + wait_for_completion(&device->flush_wait); + + ret = bio->bi_error; + if (ret) + btrfs_dev_stat_inc_and_print(device, + BTRFS_DEV_STAT_FLUSH_ERRS); + + bio_put(bio); + device->flush_bio = NULL; + + return ret; } static int check_barrier_error(struct btrfs_fs_devices *fsdevs) @@ -3559,9 +3545,7 @@ static int check_barrier_error(struct btrfs_fs_devices *fsdevs) dev_flush_error++; continue; } - if (dev->last_flush_error == -ENOMEM) - submit_flush_error++; - if (dev->last_flush_error && dev->last_flush_error != -ENOMEM) + if (dev->last_flush_error) dev_flush_error++; } @@ -3596,10 +3580,8 @@ static int barrier_all_devices(struct btrfs_fs_info *info) if (!dev->in_fs_metadata || !dev->writeable) continue; - ret = write_dev_flush(dev, 0); - if (ret) - errors_send++; - dev->last_flush_error = ret; + write_dev_flush(dev); + dev->last_flush_error = 0; } /* wait for all the barriers */ @@ -3613,23 +3595,13 @@ static int barrier_all_devices(struct btrfs_fs_info *info) if (!dev->in_fs_metadata || !dev->writeable) continue; - ret = write_dev_flush(dev, 1); + ret = wait_dev_flush(dev); if (ret) { dev->last_flush_error = ret; errors_wait++; } } - /* - * Try hard in case of flush. Lets say, in RAID1 we have - * the following situation - * dev1: EIO dev2: ENOMEM - * this is not a fatal error as we hope to recover from - * ENOMEM in the next attempt to flush. - * But the following is considered as fatal - * dev1: ENOMEM dev2: ENOMEM - * dev1: bdev == NULL dev2: ENOMEM - */ if (errors_send || errors_wait) { /* * At some point we need the status of all disks -- 2.8.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore 2017-06-09 6:52 ` [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore Anand Jain @ 2017-06-12 16:14 ` David Sterba 2017-06-13 9:07 ` Anand Jain 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2017-06-12 16:14 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Fri, Jun 09, 2017 at 02:52:28PM +0800, Anand Jain wrote: > Commit > 9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling > > removed the -ENOMEM return from write_dev_flush() so no need to > check for the -ENOMEM during send. > > This patch also peals write_dev_flush's wait part of the code, > and creates a new function wait_dev_flush(). "This patch also" usually means that the patch should be split. > /* > - * trigger flushes for one the devices. If you pass wait == 0, the flushes are > - * sent down. With wait == 1, it waits for the previous flush. > - * > - * any device where the flush fails with eopnotsupp are flagged as not-barrier > - * capable > + * trigger flushes for one the devices. In the patch that splits write_dev_flush, please use the following comment wording: * Submit a flush request to the device if it supports it. Error handling is * done in the waiting counterpart. > */ > -static int write_dev_flush(struct btrfs_device *device, int wait) > +static void write_dev_flush(struct btrfs_device *device) and /* * If the flush bio has been submitted by write_dev_flush, wait for it. */ > +static int wait_dev_flush(struct btrfs_device *device) > +{ But otherwise the cleanup is good. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore 2017-06-12 16:14 ` David Sterba @ 2017-06-13 9:07 ` Anand Jain 0 siblings, 0 replies; 6+ messages in thread From: Anand Jain @ 2017-06-13 9:07 UTC (permalink / raw) To: dsterba, linux-btrfs On 06/13/2017 12:14 AM, David Sterba wrote: > On Fri, Jun 09, 2017 at 02:52:28PM +0800, Anand Jain wrote: >> Commit >> 9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling >> >> removed the -ENOMEM return from write_dev_flush() so no need to >> check for the -ENOMEM during send. >> >> This patch also peals write_dev_flush's wait part of the code, >> and creates a new function wait_dev_flush(). > > "This patch also" usually means that the patch should be split. > >> /* >> - * trigger flushes for one the devices. If you pass wait == 0, the flushes are >> - * sent down. With wait == 1, it waits for the previous flush. >> - * >> - * any device where the flush fails with eopnotsupp are flagged as not-barrier >> - * capable >> + * trigger flushes for one the devices. > > In the patch that splits write_dev_flush, please use the following > comment wording: > > * Submit a flush request to the device if it supports it. Error handling is > * done in the waiting counterpart. > >> */ >> -static int write_dev_flush(struct btrfs_device *device, int wait) >> +static void write_dev_flush(struct btrfs_device *device) > > and > > /* > * If the flush bio has been submitted by write_dev_flush, wait for it. > */ > >> +static int wait_dev_flush(struct btrfs_device *device) >> +{ > > But otherwise the cleanup is good. Done. V2 has been sent out. Thanks for the review. -Anand ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] btrfs: flush error also accounts for its send error 2017-06-09 6:52 [PATCH 0/2] cleanup write_dev_flush() Anand Jain 2017-06-09 6:52 ` [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore Anand Jain @ 2017-06-09 6:52 ` Anand Jain 2017-06-12 16:22 ` David Sterba 1 sibling, 1 reply; 6+ messages in thread From: Anand Jain @ 2017-06-09 6:52 UTC (permalink / raw) To: linux-btrfs; +Cc: dsterba So remove the static check of send error Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/disk-io.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index d8151839bb3d..682c494dbc7f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -3534,14 +3534,11 @@ static int wait_dev_flush(struct btrfs_device *device) static int check_barrier_error(struct btrfs_fs_devices *fsdevs) { - int submit_flush_error = 0; int dev_flush_error = 0; struct btrfs_device *dev; - int tolerance; list_for_each_entry_rcu(dev, &fsdevs->devices, dev_list) { if (!dev->bdev) { - submit_flush_error++; dev_flush_error++; continue; } @@ -3549,8 +3546,8 @@ static int check_barrier_error(struct btrfs_fs_devices *fsdevs) dev_flush_error++; } - tolerance = fsdevs->fs_info->num_tolerated_disk_barrier_failures; - if (submit_flush_error > tolerance || dev_flush_error > tolerance) + if (dev_flush_error > + fsdevs->fs_info->num_tolerated_disk_barrier_failures) return -EIO; return 0; @@ -3564,7 +3561,6 @@ static int barrier_all_devices(struct btrfs_fs_info *info) { struct list_head *head; struct btrfs_device *dev; - int errors_send = 0; int errors_wait = 0; int ret; @@ -3573,10 +3569,9 @@ static int barrier_all_devices(struct btrfs_fs_info *info) list_for_each_entry_rcu(dev, head, dev_list) { if (dev->missing) continue; - if (!dev->bdev) { - errors_send++; + if (!dev->bdev) continue; - } + if (!dev->in_fs_metadata || !dev->writeable) continue; @@ -3602,7 +3597,11 @@ static int barrier_all_devices(struct btrfs_fs_info *info) } } - if (errors_send || errors_wait) { + /* + * By checking errors_wait here it avoids traverse through + * the device loop for normal healthy case + */ + if (errors_wait) { /* * At some point we need the status of all disks * to arrive at the volume status. So error checking -- 2.8.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] btrfs: flush error also accounts for its send error 2017-06-09 6:52 ` [PATCH 2/2] btrfs: flush error also accounts for its send error Anand Jain @ 2017-06-12 16:22 ` David Sterba 0 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2017-06-12 16:22 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Fri, Jun 09, 2017 at 02:52:29PM +0800, Anand Jain wrote: > So remove the static check of send error Please improve the changelog text. > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/disk-io.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index d8151839bb3d..682c494dbc7f 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -3534,14 +3534,11 @@ static int wait_dev_flush(struct btrfs_device *device) > > static int check_barrier_error(struct btrfs_fs_devices *fsdevs) > { > - int submit_flush_error = 0; > int dev_flush_error = 0; > struct btrfs_device *dev; > - int tolerance; > > list_for_each_entry_rcu(dev, &fsdevs->devices, dev_list) { > if (!dev->bdev) { > - submit_flush_error++; > dev_flush_error++; > continue; > } > @@ -3549,8 +3546,8 @@ static int check_barrier_error(struct btrfs_fs_devices *fsdevs) > dev_flush_error++; > } > > - tolerance = fsdevs->fs_info->num_tolerated_disk_barrier_failures; > - if (submit_flush_error > tolerance || dev_flush_error > tolerance) > + if (dev_flush_error > > + fsdevs->fs_info->num_tolerated_disk_barrier_failures) > return -EIO; > > return 0; > @@ -3564,7 +3561,6 @@ static int barrier_all_devices(struct btrfs_fs_info *info) > { > struct list_head *head; > struct btrfs_device *dev; > - int errors_send = 0; > int errors_wait = 0; > int ret; > > @@ -3573,10 +3569,9 @@ static int barrier_all_devices(struct btrfs_fs_info *info) > list_for_each_entry_rcu(dev, head, dev_list) { > if (dev->missing) > continue; > - if (!dev->bdev) { > - errors_send++; > + if (!dev->bdev) > continue; > - } > + > if (!dev->in_fs_metadata || !dev->writeable) > continue; > > @@ -3602,7 +3597,11 @@ static int barrier_all_devices(struct btrfs_fs_info *info) > } > } > > - if (errors_send || errors_wait) { > + /* > + * By checking errors_wait here it avoids traverse through > + * the device loop for normal healthy case > + */ I think the code makes it obvious that we skip the barrier check, the comment does not bring any new information, please drop it. > + if (errors_wait) { > /* > * At some point we need the status of all disks > * to arrive at the volume status. So error checking ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-13 9:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-06-09 6:52 [PATCH 0/2] cleanup write_dev_flush() Anand Jain 2017-06-09 6:52 ` [PATCH 1/2] btrfs: write_dev_flush does not return ENOMEM anymore Anand Jain 2017-06-12 16:14 ` David Sterba 2017-06-13 9:07 ` Anand Jain 2017-06-09 6:52 ` [PATCH 2/2] btrfs: flush error also accounts for its send error Anand Jain 2017-06-12 16:22 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).