[PATCH] btrfs: test for device flush-able should be after wait code

[PATCH] btrfs: test for device flush-able should be after wait code

[Anand Jain]

write_dev_flush() checks if the device is flush capable, however as
the device mode can change any time, this check should be after the
wait part of the code.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
 Fixes: c2a9c7ab475b btrfs: check if the device is flush capable
---
 fs/btrfs/disk-io.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9f2ffe2c6afb..603a7e32e708 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3496,13 +3496,10 @@ static void btrfs_end_empty_barrier(struct bio *bio)
  */
 static int write_dev_flush(struct btrfs_device *device, int wait)
 {
-	struct request_queue *q = bdev_get_queue(device->bdev);
+	struct request_queue *devq;
 	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)
@@ -3527,6 +3524,10 @@ static int write_dev_flush(struct btrfs_device *device, int wait)
 		return ret;
 	}
 
+	devq = bdev_get_queue(device->bdev);
+	if (!test_bit(QUEUE_FLAG_WC, &devq->queue_flags))
+		return 0;
+
 	/*
 	 * one reference for us, and we leave it for the
 	 * caller
-- 
2.7.0

Re: [PATCH] btrfs: test for device flush-able should be after wait code

[David Sterba]

On Wed, Jun 14, 2017 at 06:02:38PM +0800, Anand Jain wrote:
> write_dev_flush() checks if the device is flush capable, however as
> the device mode can change any time, this check should be after the
> wait part of the code.

This would apply to code before "btrfs: wait part of the
write_dev_flush() can be separated out", right?

The problematic case is when a flush bio is sent, queue status changes
and waiting is skipped. A bio would leak. Next time the flushing is
enabled, write_dev_flush allocaes a new bio and either it would leak or
waiting will happen as expected.

The bio would leak only if the barriers are switched between write and
wait. Not impossible, but I still think a race hard to win. The
consequences are not absolutely fatal.

Also, the cleanups in write_dev_flush fix the bug in another way, so we
don't need this separate patch as a potential stable backport. The patch
mentioned above can be considered a fix but would need some manual
adaptations to apply. Therefore I don't think we need the $subj patch.

Re: [PATCH] btrfs: test for device flush-able should be after wait code

[Anand Jain]

On 06/14/2017 08:26 PM, David Sterba wrote:
> On Wed, Jun 14, 2017 at 06:02:38PM +0800, Anand Jain wrote:
>> write_dev_flush() checks if the device is flush capable, however as
>> the device mode can change any time, this check should be after the
>> wait part of the code.
> 
> This would apply to code before "btrfs: wait part of the
> write_dev_flush() can be separated out", right?

  Yes.

> The problematic case is when a flush bio is sent, queue status changes
> and waiting is skipped. A bio would leak. Next time the flushing is
> enabled, write_dev_flush allocaes a new bio and either it would leak or
> waiting will happen as expected.
> 
> The bio would leak only if the barriers are switched between write and
> wait. Not impossible, but I still think a race hard to win. The
> consequences are not absolutely fatal.

  Yeah more of a theoretical problem and hard to reproduce unless there
  is a deliberate attempt.

> Also, the cleanups in write_dev_flush fix the bug in another way, so we
> don't need this separate patch as a potential stable backport. The patch
> mentioned above can be considered a fix but would need some manual
> adaptations to apply. Therefore I don't think we need the $subj patch.

  Agreed. Kindly ignore this and the V2.2 of
     [PATCH 3/3 v2.2] btrfs: wait part of the write_dev_flush()...
  patch as well.

Thanks, Anand