[PATCH 1/3 v2] btrfs: write_dev_flush does not return ENOMEM anymore

[PATCH 1/3 v2] btrfs: write_dev_flush does not return ENOMEM anymore

[Anand Jain]

As commit
 9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling

removed the -ENOMEM return from write_dev_flush()

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: . Add the removal of submit_flush_error in check_barrier_error()
    from 2/2 before to 1/3 here, as its appropriate to be here.
    . Did not split the write and wait part here in this patch.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9f2ffe2c6afb..9fbcfa9ccab7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3505,12 +3505,6 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 
 	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);
 
@@ -3548,25 +3542,16 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 
 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;
-		}
-		if (dev->last_flush_error == -ENOMEM)
-			submit_flush_error++;
-		if (dev->last_flush_error && dev->last_flush_error != -ENOMEM)
+		if (!dev->bdev || dev->last_flush_error)
 			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;
@@ -3596,10 +3581,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, 0);
+		dev->last_flush_error = 0;
 	}
 
 	/* wait for all the barriers */
@@ -3620,16 +3603,6 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 		}
 	}
 
-	/*
-	 * 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.7.0

[PATCH 2/3 v2] btrfs: remove redundant null bdev counting during flush submit

[Anand Jain]

There is no extra benefit to count null bdev during submit loop,
as these null devices will be anyway checked during command
completion device loop just after the submit loop, and as we are
holding the device_list_mutex, the device->bdev status won't
change in between.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: . The tile of this patch is changed from 
      [PATCH 2/2] btrfs: flush error also accounts for its send error
      also added commit log.
    . The submit_flush_error is removed in patch 1/3 but it was here
      before.
    . Dropped the comments.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9fbcfa9ccab7..a9f1c48519dd 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3565,7 +3565,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;
 
@@ -3574,10 +3573,8 @@ 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;
 
@@ -3603,7 +3600,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 		}
 	}
 
-	if (errors_send || errors_wait) {
+	if (errors_wait) {
 		/*
 		 * At some point we need the status of all disks
 		 * to arrive at the volume status. So error checking
-- 
2.7.0

[PATCH 3/3 v2] btrfs: wait part of the write_dev_flush() can be separated out

[Anand Jain]

Submit and wait parts of write_dev_flush() can be split into two
separate functions for better readability.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 v2: new in v2

 fs/btrfs/disk-io.c | 60 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a9f1c48519dd..0d92a7c2f550 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3488,38 +3488,16 @@ 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
+ * 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)
 {
 	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;
-
-		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
@@ -3536,8 +3514,32 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 
 	bio_get(bio);
 	btrfsic_submit_bio(bio);
+}
 
-	return 0;
+/*
+ * If the flush bio has been submitted by write_dev_flush, wait for it.
+ */
+static int wait_dev_flush(struct btrfs_device *device)
+{
+	int ret = 0;
+	struct bio *bio = device->flush_bio;
+
+	if (!bio)
+		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;
 }
 
 static int check_barrier_error(struct btrfs_fs_devices *fsdevs)
@@ -3578,7 +3580,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 		if (!dev->in_fs_metadata || !dev->writeable)
 			continue;
 
-		write_dev_flush(dev, 0);
+		write_dev_flush(dev);
 		dev->last_flush_error = 0;
 	}
 
@@ -3593,7 +3595,7 @@ 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++;
-- 
2.7.0

[PATCH 1/3 v2.1] btrfs: write_dev_flush does not return ENOMEM anymore

[Anand Jain]

As commit
 9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling

removed the -ENOMEM return from write_dev_flush()/send-part so no need to
check for the -ENOMEM during send.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2.1: Part of commit log got missed out during send. fix it.
v2: . Add the removal of submit_flush_error in check_barrier_error()
    from 2/2 before to 1/3 here, as its appropriate to be here.
    . Did not split the write and wait part here in this patch.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9f2ffe2c6afb..9fbcfa9ccab7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3505,12 +3505,6 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 
 	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);
 
@@ -3548,25 +3542,16 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 
 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;
-		}
-		if (dev->last_flush_error == -ENOMEM)
-			submit_flush_error++;
-		if (dev->last_flush_error && dev->last_flush_error != -ENOMEM)
+		if (!dev->bdev || dev->last_flush_error)
 			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;
@@ -3596,10 +3581,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, 0);
+		dev->last_flush_error = 0;
 	}
 
 	/* wait for all the barriers */
@@ -3620,16 +3603,6 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 		}
 	}
 
-	/*
-	 * 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.7.0

Re: [PATCH 1/3 v2.1] btrfs: write_dev_flush does not return ENOMEM anymore

[David Sterba]

On Tue, Jun 13, 2017 at 05:32:29PM +0800, Anand Jain wrote:
> As commit
>  9035b5dbc576 btrfs: btrfs_io_bio_alloc never fails, skip error handling
> 
> removed the -ENOMEM return from write_dev_flush()/send-part so no need to
> check for the -ENOMEM during send.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2.1: Part of commit log got missed out during send. fix it.
> v2: . Add the removal of submit_flush_error in check_barrier_error()
>     from 2/2 before to 1/3 here, as its appropriate to be here.
>     . Did not split the write and wait part here in this patch.

All 3 reviewed and queued, thanks. I have a prototype for the
preallocated flush bio but was waiting until these cleanups are in
before snding it.

I've noticed that threre are two more "if (!device->bdev)" checks under
the device lock in write_all_supers, that might be worth removing.

However, a NULL bdev and device->missing are related and I think there
are some dark corners in dev replace where the invariant can be
temporarily broken. Given that we probably don't have great testing
coverage of devices with the flush capabilities, we should make it
possible to override that for debugging purposes. Eg. add a
"/sys/fs/btrfs/UUID/force_dev_flush" if CONFIG_BTRFS_DEBUG is enabled.

Anyway, that can also wait for later. We've made some changes this time
so I'd not add any more intrusive ones until the current devel queue is
on the way to master.

Re: [PATCH 1/3 v2.1] btrfs: write_dev_flush does not return ENOMEM anymore

[Anand Jain]

> All 3 reviewed and queued, thanks. 

  Thanks indeed.

> I have a prototype for the
> preallocated flush bio but was waiting until these cleanups are in
> before snding it.

  oh. sorry for the delay if any. -ENOMEM was only purpose I had/have
  in mind for peralloc, but looks like there is another purpose which
  I am not aware of.

> I've noticed that threre are two more "if (!device->bdev)" checks under
> the device lock in write_all_supers, that might be worth removing.

> However, a NULL bdev and device->missing are related and I think there
> are some dark corners in dev replace where the invariant can be
> temporarily broken.

  Ok. Thanks for the hints. Trying to dig.

> Given that we probably don't have great testing
> coverage of devices with the flush capabilities,

  Any hints on what type of test coverage ?

Thanks, Anand

[PATCH 3/3 v2.2] btrfs: wait part of the write_dev_flush() can be separated out

[Anand Jain]

Submit and wait parts of write_dev_flush() can be split into two
separate functions for better readability.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2.2: Make it conflict free patch on top of
       [PATCH] btrfs: test for device flush-able should be after wait code
      There is no other changes. Sorry for another version. Thxs.
v2.1: Part of commit log got missed out during send. fix it.
v2: . Add the removal of submit_flush_error in check_barrier_error()
    from 2/2 before to 1/3 here, as its appropriate to be here.
    . Did not split the write and wait part here in this patch.

 fs/btrfs/disk-io.c | 60 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 42bb674028f6..9b9375469134 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3488,39 +3488,17 @@ 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
+ * 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)
 {
 	struct request_queue *devq;
 	struct bio *bio;
-	int ret = 0;
-
-	if (wait) {
-		bio = device->flush_bio;
-
-		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;
-	}
 
 	devq = bdev_get_queue(device->bdev);
 	if (!test_bit(QUEUE_FLAG_WC, &devq->queue_flags))
-		return 0;
+		return;
 
 	/*
 	 * one reference for us, and we leave it for the
@@ -3537,8 +3515,32 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 
 	bio_get(bio);
 	btrfsic_submit_bio(bio);
+}
 
-	return 0;
+/*
+ * If the flush bio has been submitted by write_dev_flush, wait for it.
+ */
+static int wait_dev_flush(struct btrfs_device *device)
+{
+	int ret = 0;
+	struct bio *bio = device->flush_bio;
+
+	if (!bio)
+		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;
 }
 
 static int check_barrier_error(struct btrfs_fs_devices *fsdevs)
@@ -3579,7 +3581,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 		if (!dev->in_fs_metadata || !dev->writeable)
 			continue;
 
-		write_dev_flush(dev, 0);
+		write_dev_flush(dev);
 		dev->last_flush_error = 0;
 	}
 
@@ -3594,7 +3596,7 @@ 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++;
-- 
2.7.0

Re: [PATCH 1/3 v2.1] btrfs: write_dev_flush does not return ENOMEM anymore

[David Sterba]

On Wed, Jun 14, 2017 at 05:01:11PM +0800, Anand Jain wrote:
> > I have a prototype for the
> > preallocated flush bio but was waiting until these cleanups are in
> > before snding it.
> 
>   oh. sorry for the delay if any. -ENOMEM was only purpose I had/have
>   in mind for peralloc, but looks like there is another purpose which
>   I am not aware of.

The bio preallocation is a slight optimization, we don't have to
allocate and free the bio all the time.

> > I've noticed that threre are two more "if (!device->bdev)" checks under
> > the device lock in write_all_supers, that might be worth removing.
> 
> > However, a NULL bdev and device->missing are related and I think there
> > are some dark corners in dev replace where the invariant can be
> > temporarily broken.
> 
>   Ok. Thanks for the hints. Trying to dig.
> 
> > Given that we probably don't have great testing
> > coverage of devices with the flush capabilities,
> 
>   Any hints on what type of test coverage ?

Devices used for normal testing, any sort of test basically, that do not
have the flush capabilities will never exercise the in write_dev_flush.
So any reference counts or failures will not get covered.

Re: [PATCH 1/3 v2.1] btrfs: write_dev_flush does not return ENOMEM anymore

[Anand Jain]

>>> Given that we probably don't have great testing
>>> coverage of devices with the flush capabilities,
>>
>>    Any hints on what type of test coverage ?
> 
> Devices used for normal testing, any sort of test basically, that do not
> have the flush capabilities will never exercise the in write_dev_flush.

  Right. Same with -o nobarrier as well.

> So any reference counts or failures will not get covered.

  hm. Still I didn't understand your point of view, sorry.
  But the contrary is also true that when device cache is enabled,
  the write_dev_flush() gets exercised, so we are fine.

  I toggled the device write cache manually for testing this set.
     # echo "write through" > /sys/block/sdd/queue/write_cache
     # echo "write back" > /sys/block/sdd/queue/write_cache


Thanks, Anand