[RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[Anand Jain]

BTRFS wanted a block device flush function which does not wait for
its completion, so that the flush for the next device can be called
in the same thread.

Here is a RFC patch to provide the function
'blkdev_issue_flush_no_wait()', which is based on the current device
flush function 'blkdev_issue_flush()', however it uses submit_bio()
instead of submit_bio_wait().

This patch is for review comments, will send out a final patch based
on the comments received.

Thanks, Anand

Anand Jain (2):
  block: Introduce blkdev_issue_flush_no_wait()
  btrfs: Use blkdev_issue_flush_no_wait()

 block/blk-flush.c      |  47 +++++++++++++++++++++
 fs/btrfs/disk-io.c     | 108 +++++++++++++++----------------------------------
 fs/btrfs/volumes.h     |   2 +-
 include/linux/blkdev.h |   8 ++++
 4 files changed, 88 insertions(+), 77 deletions(-)

-- 
2.10.0

[PATCH 1/2] block: Introduce blkdev_issue_flush_no_wait()

[Anand Jain]

blkdev_issue_flush() is a blocking function and returns only after
the flush bio is completed, so a module handling more than one
device can't issue flush for all the devices unless it uses worker
thread.

This patch adds a new function blkdev_issue_flush_no_wait(), which
uses submit_bio() instead of submit_bio_wait(), and accepts the
completion function and data from the caller.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 block/blk-flush.c      | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |  8 ++++++++
 2 files changed, 55 insertions(+)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index c4e0880b54bb..16b9c61f4cd3 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -488,6 +488,53 @@ void blk_insert_flush(struct request *rq)
 	blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
 }
 
+/*
+ * blkdev_issue_flush_no_wait - Issue a flush for the given block device
+ * @bdev:       blockdev to issue flush for
+ * @gfp_mask:   memory allocation flags (for bio_alloc)
+ * @endio_fn:   completion function for the flush bio
+ * @data:       flush bio private data
+ *
+ * Description:
+ *    Issue a flush for the block device in question. Caller must provide
+ *    the completion function and the private data, to be called when the
+ *    issued flush bio completes.
+ */
+int blkdev_issue_flush_no_wait(struct block_device *bdev, gfp_t gfp_mask,
+		bio_end_io_t *endio_fn, void *data)
+{
+	struct request_queue *q;
+	struct bio *bio;
+
+	if (bdev->bd_disk == NULL)
+		return -ENXIO;
+
+	q = bdev_get_queue(bdev);
+	if (!q)
+		return -ENXIO;
+
+	/*
+	 * some block devices may not have their queue correctly set up here
+	 * (e.g. loop device without a backing file) and so issuing a flush
+	 * here will panic. Ensure there is a request function before issuing
+	 * the flush.
+	 */
+	if (!q->make_request_fn)
+		return -ENXIO;
+
+	bio = bio_alloc(gfp_mask, 0);
+	bio->bi_bdev = bdev;
+	bio->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC;
+	bio->bi_end_io = endio_fn;
+	bio->bi_private = data;
+
+	init_completion(data);
+	submit_bio(bio);
+
+	return 0;
+}
+EXPORT_SYMBOL(blkdev_issue_flush_no_wait);
+
 /**
  * blkdev_issue_flush - queue a flush
  * @bdev:	blockdev to issue flush for
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b5d1e27631ee..5d2b62239251 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1322,6 +1322,8 @@ static inline struct request *blk_map_queue_find_tag(struct blk_queue_tag *bqt,
 }
 
 extern int blkdev_issue_flush(struct block_device *, gfp_t, sector_t *);
+extern int blkdev_issue_flush_no_wait(struct block_device *bdev,
+			gfp_t gfp_mask, bio_end_io_t *endio_fn, void *data);
 extern int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct page *page);
 
@@ -1994,6 +1996,12 @@ static inline int blkdev_issue_flush(struct block_device *bdev, gfp_t gfp_mask,
 	return 0;
 }
 
+static inline int blkdev_issue_flush_no_wait(struct block_device *bdev,
+			gfp_t gfp_mask, bio_end_io_t *endio_fn, void *data)
+{
+	return 0;
+}
+
 #endif /* CONFIG_BLOCK */
 
 #endif
-- 
2.10.0

[PATCH 2/2] btrfs: Use blkdev_issue_flush_no_wait()

[Anand Jain]

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/disk-io.c | 108 ++++++++++++++++-------------------------------------
 fs/btrfs/volumes.h |   2 +-
 2 files changed, 33 insertions(+), 77 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8685d67185d0..f059f9bdbbd7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3478,121 +3478,77 @@ static int write_dev_supers(struct btrfs_device *device,
 }
 
 /*
- * endio for the write_dev_flush, this will wake anyone waiting
+ * endio for blkdev_issues_flush_no_wait, this will wake anyone waiting
  * for the barrier when it is done
  */
 static void btrfs_end_empty_barrier(struct bio *bio)
 {
-	if (bio->bi_private)
-		complete(bio->bi_private);
+	struct btrfs_device *device;
+
+	device = container_of(bio->bi_private, struct btrfs_device,
+						flush_wait);
+
+	device->last_flush_error = bio->bi_error;
+	complete(&device->flush_wait);
+
 	bio_put(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
- */
-static int write_dev_flush(struct btrfs_device *device, int wait)
+static void write_dev_flush(struct btrfs_device *device)
 {
+	int ret;
 	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)
-			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;
-	}
-
-	/*
-	 * one reference for us, and we leave it for the
-	 * caller
-	 */
-	device->flush_bio = NULL;
-	bio = btrfs_io_bio_alloc(GFP_NOFS, 0);
-	if (!bio)
-		return -ENOMEM;
-
-	bio->bi_end_io = btrfs_end_empty_barrier;
-	bio->bi_bdev = device->bdev;
-	bio->bi_opf = REQ_OP_WRITE | 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;
 
-	return 0;
+	ret = blkdev_issue_flush_no_wait(device->bdev, GFP_NOFS,
+		btrfs_end_empty_barrier, &device->flush_wait);
+	if (ret)
+		device->last_flush_error = ret;
+	else
+		device->last_flush_error = -1;
 }
 
 /*
- * send an empty flush down to each device in parallel,
- * then wait for them
+ * send flush down to each device in parallel, then wait for them.
  */
 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;
 
 	/* send down all the barriers */
 	head = &info->fs_devices->devices;
 	list_for_each_entry_rcu(dev, head, dev_list) {
+		dev->last_flush_error = 0;
 		if (dev->missing)
 			continue;
+
+		if (!dev->in_fs_metadata || !dev->writeable)
+			continue;
+
 		if (!dev->bdev) {
-			errors_send++;
+			dev->last_flush_error = -ENXIO;
 			continue;
 		}
-		if (!dev->in_fs_metadata || !dev->writeable)
-			continue;
 
-		ret = write_dev_flush(dev, 0);
-		if (ret)
-			errors_send++;
+		write_dev_flush(dev);
 	}
 
 	/* wait for all the barriers */
 	list_for_each_entry_rcu(dev, head, dev_list) {
-		if (dev->missing)
-			continue;
-		if (!dev->bdev) {
-			errors_wait++;
-			continue;
-		}
-		if (!dev->in_fs_metadata || !dev->writeable)
-			continue;
+		if (dev->last_flush_error == -1)
+			wait_for_completion(&dev->flush_wait);
 
-		ret = write_dev_flush(dev, 1);
-		if (ret)
+		if (dev->last_flush_error)
 			errors_wait++;
 	}
-	if (errors_send > info->num_tolerated_disk_barrier_failures ||
-	    errors_wait > info->num_tolerated_disk_barrier_failures)
+
+	if (errors_wait > info->num_tolerated_disk_barrier_failures)
 		return -EIO;
+
 	return 0;
 }
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index c7d0fbc915ca..27af3a227bfb 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -123,8 +123,8 @@ struct btrfs_device {
 	struct list_head resized_list;
 
 	/* for sending down flush barriers */
-	struct bio *flush_bio;
 	struct completion flush_wait;
+	int last_flush_error;
 
 	/* per-device scrub information */
 	struct scrub_ctx *scrub_device;
-- 
2.10.0

Re: [PATCH 1/2] block: Introduce blkdev_issue_flush_no_wait()

[Christoph Hellwig]

On Tue, May 16, 2017 at 05:39:13PM +0800, Anand Jain wrote:
> blkdev_issue_flush() is a blocking function and returns only after
> the flush bio is completed, so a module handling more than one
> device can't issue flush for all the devices unless it uses worker
> thread.
> 
> This patch adds a new function blkdev_issue_flush_no_wait(), which
> uses submit_bio() instead of submit_bio_wait(), and accepts the
> completion function and data from the caller.

Just open code the damn thing, and drop the various superflous checks
while you're at it.

Re: [PATCH 2/2] btrfs: Use blkdev_issue_flush_no_wait()

[Christoph Hellwig]

On Tue, May 16, 2017 at 05:39:14PM +0800, Anand Jain wrote:
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

An explanation on why things are changed is entirely missing here..

> ---
>  fs/btrfs/disk-io.c | 108 ++++++++++++++++-------------------------------------
>  fs/btrfs/volumes.h |   2 +-
>  2 files changed, 33 insertions(+), 77 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 8685d67185d0..f059f9bdbbd7 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3478,121 +3478,77 @@ static int write_dev_supers(struct btrfs_device *device,
>  }
>  
>  /*
> - * endio for the write_dev_flush, this will wake anyone waiting
> + * endio for blkdev_issues_flush_no_wait, this will wake anyone waiting
>   * for the barrier when it is done
>   */
>  static void btrfs_end_empty_barrier(struct bio *bio)
>  {
> -	if (bio->bi_private)
> -		complete(bio->bi_private);
> +	struct btrfs_device *device;
> +
> +	device = container_of(bio->bi_private, struct btrfs_device,
> +						flush_wait);
> +
> +	device->last_flush_error = bio->bi_error;
> +	complete(&device->flush_wait);
> +
>  	bio_put(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
> - */
> -static int write_dev_flush(struct btrfs_device *device, int wait)
> +static void write_dev_flush(struct btrfs_device *device)
>  {
> +	int ret;
>  	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)
> -			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;
> -	}
> -
> -	/*
> -	 * one reference for us, and we leave it for the
> -	 * caller
> -	 */
> -	device->flush_bio = NULL;
> -	bio = btrfs_io_bio_alloc(GFP_NOFS, 0);
> -	if (!bio)
> -		return -ENOMEM;
> -
> -	bio->bi_end_io = btrfs_end_empty_barrier;
> -	bio->bi_bdev = device->bdev;
> -	bio->bi_opf = REQ_OP_WRITE | 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;
>  
> -	return 0;
> +	ret = blkdev_issue_flush_no_wait(device->bdev, GFP_NOFS,
> +		btrfs_end_empty_barrier, &device->flush_wait);
> +	if (ret)
> +		device->last_flush_error = ret;
> +	else
> +		device->last_flush_error = -1;
>  }
>  
>  /*
> - * send an empty flush down to each device in parallel,
> - * then wait for them
> + * send flush down to each device in parallel, then wait for them.
>   */
>  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;
>  
>  	/* send down all the barriers */
>  	head = &info->fs_devices->devices;
>  	list_for_each_entry_rcu(dev, head, dev_list) {
> +		dev->last_flush_error = 0;
>  		if (dev->missing)
>  			continue;
> +
> +		if (!dev->in_fs_metadata || !dev->writeable)
> +			continue;
> +
>  		if (!dev->bdev) {
> -			errors_send++;
> +			dev->last_flush_error = -ENXIO;
>  			continue;
>  		}
> -		if (!dev->in_fs_metadata || !dev->writeable)
> -			continue;
>  
> -		ret = write_dev_flush(dev, 0);
> -		if (ret)
> -			errors_send++;
> +		write_dev_flush(dev);
>  	}
>  
>  	/* wait for all the barriers */
>  	list_for_each_entry_rcu(dev, head, dev_list) {
> -		if (dev->missing)
> -			continue;
> -		if (!dev->bdev) {
> -			errors_wait++;
> -			continue;
> -		}
> -		if (!dev->in_fs_metadata || !dev->writeable)
> -			continue;
> +		if (dev->last_flush_error == -1)
> +			wait_for_completion(&dev->flush_wait);
>  
> -		ret = write_dev_flush(dev, 1);
> -		if (ret)
> +		if (dev->last_flush_error)
>  			errors_wait++;
>  	}
> -	if (errors_send > info->num_tolerated_disk_barrier_failures ||
> -	    errors_wait > info->num_tolerated_disk_barrier_failures)
> +
> +	if (errors_wait > info->num_tolerated_disk_barrier_failures)
>  		return -EIO;
> +
>  	return 0;
>  }
>  
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index c7d0fbc915ca..27af3a227bfb 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -123,8 +123,8 @@ struct btrfs_device {
>  	struct list_head resized_list;
>  
>  	/* for sending down flush barriers */
> -	struct bio *flush_bio;
>  	struct completion flush_wait;
> +	int last_flush_error;
>  
>  	/* per-device scrub information */
>  	struct scrub_ctx *scrub_device;
> -- 
> 2.10.0
> 
---end quoted text---

Re: [RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[Bart Van Assche]

T24gVHVlLCAyMDE3LTA1LTE2IGF0IDE3OjM5ICswODAwLCBBbmFuZCBKYWluIHdyb3RlOg0KPiBC
VFJGUyB3YW50ZWQgYSBibG9jayBkZXZpY2UgZmx1c2ggZnVuY3Rpb24gd2hpY2ggZG9lcyBub3Qg
d2FpdCBmb3INCj4gaXRzIGNvbXBsZXRpb24sIHNvIHRoYXQgdGhlIGZsdXNoIGZvciB0aGUgbmV4
dCBkZXZpY2UgY2FuIGJlIGNhbGxlZA0KPiBpbiB0aGUgc2FtZSB0aHJlYWQuDQo+IA0KPiBIZXJl
IGlzIGEgUkZDIHBhdGNoIHRvIHByb3ZpZGUgdGhlIGZ1bmN0aW9uDQo+ICdibGtkZXZfaXNzdWVf
Zmx1c2hfbm9fd2FpdCgpJywgd2hpY2ggaXMgYmFzZWQgb24gdGhlIGN1cnJlbnQgZGV2aWNlDQo+
IGZsdXNoIGZ1bmN0aW9uICdibGtkZXZfaXNzdWVfZmx1c2goKScsIGhvd2V2ZXIgaXQgdXNlcyBz
dWJtaXRfYmlvKCkNCj4gaW5zdGVhZCBvZiBzdWJtaXRfYmlvX3dhaXQoKS4NCj4gDQo+IFRoaXMg
cGF0Y2ggaXMgZm9yIHJldmlldyBjb21tZW50cywgd2lsbCBzZW5kIG91dCBhIGZpbmFsIHBhdGNo
IGJhc2VkDQo+IG9uIHRoZSBjb21tZW50cyByZWNlaXZlZC4NCg0KSGVsbG8gQW5hbmQsDQoNClNp
bmNlIHRoZSBibG9jayBsYXllciBjYW4gcmVvcmRlciByZXF1ZXN0cywgSSB0aGluayB1c2luZw0K
YmxrZGV2X2lzc3VlX2ZsdXNoX25vX3dhaXQoKSB3aWxsIG9ubHkgeWllbGQgdGhlIGludGVuZGVk
IHJlc3VsdCBpZg0KdGhlIGNhbGxlciB3YWl0cyB1bnRpbCB0aGUgcmVxdWVzdHMgdGhhdCBoYXZl
IHRvIGJlIGZsdXNoZWQgaGF2ZSBjb21wbGV0ZWQuDQpJcyB0aGF0IGhvdyB5b3UgaW50ZW5kIHRv
IHVzZSB0aGlzIGZ1bmN0aW9uPw0KDQpUaGFua3MsDQoNCkJhcnQu

Re: [RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 942 bytes --]

On Tue, 2017-05-16 at 17:39 +0800, Anand Jain wrote:
> BTRFS wanted a block device flush function which does not wait for
> its completion, so that the flush for the next device can be called
> in the same thread.
> 
> Here is a RFC patch to provide the function
> 'blkdev_issue_flush_no_wait()', which is based on the current device
> flush function 'blkdev_issue_flush()', however it uses submit_bio()
> instead of submit_bio_wait().
> 
> This patch is for review comments, will send out a final patch based
> on the comments received.

Hello Anand,

Since the block layer can reorder requests, I think using
blkdev_issue_flush_no_wait() will only yield the intended result if
the caller waits until the requests that have to be flushed have completed.
Is that how you intend to use this function?

Thanks,

Bart.ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±ý»k~ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[David Sterba]

On Tue, May 16, 2017 at 02:07:23PM +0000, Bart Van Assche wrote:
> On Tue, 2017-05-16 at 17:39 +0800, Anand Jain wrote:
> > BTRFS wanted a block device flush function which does not wait for
> > its completion, so that the flush for the next device can be called
> > in the same thread.
> > 
> > Here is a RFC patch to provide the function
> > 'blkdev_issue_flush_no_wait()', which is based on the current device
> > flush function 'blkdev_issue_flush()', however it uses submit_bio()
> > instead of submit_bio_wait().
> > 
> > This patch is for review comments, will send out a final patch based
> > on the comments received.
> 
> Since the block layer can reorder requests, I think using
> blkdev_issue_flush_no_wait() will only yield the intended result if
> the caller waits until the requests that have to be flushed have completed.
> Is that how you intend to use this function?

Yes, this is intended. Regarding the two patches, I don't think we need
them. A more detailed explanation below.

The function blkdev_issue_flush_no_wait would be used in multi-device
btrfs to submit the barriers in parallel.

fs/btrfs/disk-io.c:barrier_all_devices

pseudocode:

foreach device
	write_dev_flush(wait=0)
		submit_bio(device->bio)

(would newly use blkdev_issue_flush_no_wait)


foreach device
	write_dev_flush(wiat=1)
		wait_for_completion(device->bio)

The submission path of write_dev_flush mimics the structure of
blkdev_issue_flush, so Anand supposedly wants to move that to API.

I personally don't think this is necessary and am fine with opencoding
it, btrfs would likely be the only user of the new function anyway.
Other reason is that we want to preallocate the bio used for flushing so
we can avoid ENOMEM when submitting disk barriers. This would not be
possible. In summary, I think we can address all the problems inside
btrfs without extending block layer as for now.

Re: [RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[Anand Jain]

On 05/16/2017 10:07 PM, Bart Van Assche wrote:
> On Tue, 2017-05-16 at 17:39 +0800, Anand Jain wrote:
>> BTRFS wanted a block device flush function which does not wait for
>> its completion, so that the flush for the next device can be called
>> in the same thread.
>>
>> Here is a RFC patch to provide the function
>> 'blkdev_issue_flush_no_wait()', which is based on the current device
>> flush function 'blkdev_issue_flush()', however it uses submit_bio()
>> instead of submit_bio_wait().
>>
>> This patch is for review comments, will send out a final patch based
>> on the comments received.
>
> Hello Anand,
>
> Since the block layer can reorder requests, I think using
> blkdev_issue_flush_no_wait() will only yield the intended result if
> the caller waits until the requests that have to be flushed have completed.
> Is that how you intend to use this function?

  Yes. The patch 2/2 did that. Sorry for no explanation though.

Thanks, Anand


> Thanks,
>
> Bart.N�����r��y���b�X��ǧv�^�)޺{.n�+����{�n�߲)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
>

Re: [PATCH 1/2] block: Introduce blkdev_issue_flush_no_wait()

[Anand Jain]

On 05/16/2017 07:56 PM, Christoph Hellwig wrote:
> On Tue, May 16, 2017 at 05:39:13PM +0800, Anand Jain wrote:
>> blkdev_issue_flush() is a blocking function and returns only after
>> the flush bio is completed, so a module handling more than one
>> device can't issue flush for all the devices unless it uses worker
>> thread.
>>
>> This patch adds a new function blkdev_issue_flush_no_wait(), which
>> uses submit_bio() instead of submit_bio_wait(), and accepts the
>> completion function and data from the caller.
>
> Just open code the damn thing,

  Ok. Thanks for the comments.

> and drop the various superflous checks
> while you're at it.

  You mean at btrfs: write_dev_flush()
    OR
  block: blkdev_issue_flush() ?
  Where I find
         q = bdev_get_queue(bdev);
         if (!q)
                 return -ENXIO
  isn't needed as anyway generic_make_request_checks() will
  check that down below.
  Not too sure about the other two checks though.

Thanks, Anand


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC PATCH 0/2] Introduce blkdev_issue_flush_no_wait()

[Anand Jain]

On 05/18/2017 01:14 AM, David Sterba wrote:
> On Tue, May 16, 2017 at 02:07:23PM +0000, Bart Van Assche wrote:
>> On Tue, 2017-05-16 at 17:39 +0800, Anand Jain wrote:
>>> BTRFS wanted a block device flush function which does not wait for
>>> its completion, so that the flush for the next device can be called
>>> in the same thread.
>>>
>>> Here is a RFC patch to provide the function
>>> 'blkdev_issue_flush_no_wait()', which is based on the current device
>>> flush function 'blkdev_issue_flush()', however it uses submit_bio()
>>> instead of submit_bio_wait().
>>>
>>> This patch is for review comments, will send out a final patch based
>>> on the comments received.
>>
>> Since the block layer can reorder requests, I think using
>> blkdev_issue_flush_no_wait() will only yield the intended result if
>> the caller waits until the requests that have to be flushed have completed.
>> Is that how you intend to use this function?
>
> Yes, this is intended. Regarding the two patches, I don't think we need
> them. A more detailed explanation below.
>
> The function blkdev_issue_flush_no_wait would be used in multi-device
> btrfs to submit the barriers in parallel.
>
> fs/btrfs/disk-io.c:barrier_all_devices
>
> pseudocode:
>
> foreach device
> 	write_dev_flush(wait=0)
> 		submit_bio(device->bio)
>
> (would newly use blkdev_issue_flush_no_wait)
>
>
> foreach device
> 	write_dev_flush(wiat=1)
> 		wait_for_completion(device->bio)
>
> The submission path of write_dev_flush mimics the structure of
> blkdev_issue_flush, so Anand supposedly wants to move that to API.
>
> I personally don't think this is necessary and am fine with opencoding
> it, btrfs would likely be the only user of the new function anyway.
> Other reason is that we want to preallocate the bio used for flushing so
> we can avoid ENOMEM when submitting disk barriers. This would not be
> possible. In summary, I think we can address all the problems inside
> btrfs without extending block layer as for now.


   Thanks David.

   Pls. Ignore this set.



Thanks, Anand

Re: [PATCH 1/2] block: Introduce blkdev_issue_flush_no_wait()

[Christoph Hellwig]

On Thu, May 18, 2017 at 05:31:32PM +0800, Anand Jain wrote:
>  You mean at btrfs: write_dev_flush()
>    OR
>  block: blkdev_issue_flush() ?
>  Where I find
>         q = bdev_get_queue(bdev);
>         if (!q)
>                 return -ENXIO
>  isn't needed as anyway generic_make_request_checks() will
>  check that down below.
>  Not too sure about the other two checks though.

I was looking at your new function, but indeed it seems like the
function is mostly copy & pasted from blkdev_issue_flush.

The bdev->bd_disk, !bdev_get_queue and q->make_request_fn checks
are all things you don't need, any blkdev_issue_flush should not
either, although I'll need to look into the weird loop workaround
again, which doesn't make much sense to me.

Re: [PATCH 1/2] block: Introduce blkdev_issue_flush_no_wait()

[Anand Jain]

> The bdev->bd_disk, !bdev_get_queue and q->make_request_fn checks
> are all things you don't need, any blkdev_issue_flush should not
> either, although I'll need to look into the weird loop workaround
> again, which doesn't make much sense to me.

  I tried to confirm q->make_request_fn and got lost, I am going to
  try again.

Thanks, Anand