* [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
* [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 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 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
* 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
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).