[PATCH v11 0/3] bio: control bio max size

[PATCH v11 0/3] bio: control bio max size

[Changheun Lee]

bio size can grow up to 4GB after muli-page bvec has been enabled.
But sometimes large size of bio would lead to inefficient behaviors.
Control of bio max size will be helpful to improve inefficiency.

blk_queue_max_bio_bytes() is added to enable be set the max_bio_bytes in
each driver layer. And max_bio_bytes sysfs is added to show current
max_bio_bytes for each request queue.
bio size can be controlled via max_bio_bytes.

Changheun Lee (3):
  bio: control bio max size
  blk-sysfs: add max_bio_bytes
  scsi: set max_bio_bytes with queue max sectors

 Documentation/ABI/testing/sysfs-block | 10 ++++++++++
 Documentation/block/queue-sysfs.rst   |  7 +++++++
 block/bio.c                           | 17 ++++++++++++++---
 block/blk-settings.c                  | 19 +++++++++++++++++++
 block/blk-sysfs.c                     |  7 +++++++
 drivers/scsi/scsi_lib.c               |  2 ++
 include/linux/bio.h                   |  4 +++-
 include/linux/blkdev.h                |  3 +++
 8 files changed, 65 insertions(+), 4 deletions(-)

-- 
2.29.0

[PATCH v11 1/3] bio: control bio max size

[Changheun Lee]

bio size can grow up to 4GB after muli-page bvec has been enabled.
But sometimes large size of bio would lead to inefficient behaviors.
Control of bio max size will be helpful to improve inefficiency.

Below is a example for inefficient behaviours.
In case of large chunk direct I/O, - 32MB chunk read in user space -
all pages for 32MB would be merged to a bio structure if the pages
physical addresses are contiguous. It makes some delay to submit
until merge complete. bio max size should be limited to a proper size.

When 32MB chunk read with direct I/O option is coming from userspace,
kernel behavior is below now in do_direct_IO() loop. It's timeline.

 | bio merge for 32MB. total 8,192 pages are merged.
 | total elapsed time is over 2ms.
 |------------------ ... ----------------------->|
                                                 | 8,192 pages merged a bio.
                                                 | at this time, first bio submit is done.
                                                 | 1 bio is split to 32 read request and issue.
                                                 |--------------->
                                                  |--------------->
                                                   |--------------->
                                                              ......
                                                                   |--------------->
                                                                    |--------------->|
                          total 19ms elapsed to complete 32MB read done from device. |

If bio max size is limited with 1MB, behavior is changed below.

 | bio merge for 1MB. 256 pages are merged for each bio.
 | total 32 bio will be made.
 | total elapsed time is over 2ms. it's same.
 | but, first bio submit timing is fast. about 100us.
 |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
      | 256 pages merged a bio.
      | at this time, first bio submit is done.
      | and 1 read request is issued for 1 bio.
      |--------------->
           |--------------->
                |--------------->
                                      ......
                                                 |--------------->
                                                  |--------------->|
        total 17ms elapsed to complete 32MB read done from device. |

As a result, read request issue timing is faster if bio max size is limited.
Current kernel behavior with multipage bvec, super large bio can be created.
And it lead to delay first I/O request issue.

Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
---
 block/bio.c            | 17 ++++++++++++++---
 block/blk-settings.c   | 19 +++++++++++++++++++
 include/linux/bio.h    |  4 +++-
 include/linux/blkdev.h |  3 +++
 4 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 44205dfb6b60..c52639bb80cd 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -255,6 +255,13 @@ void bio_init(struct bio *bio, struct bio_vec *table,
 }
 EXPORT_SYMBOL(bio_init);
 
+unsigned int bio_max_size(struct bio *bio)
+{
+	struct block_device *bdev = bio->bi_bdev;
+
+	return bdev ? bdev->bd_disk->queue->limits.max_bio_bytes : UINT_MAX;
+}
+
 /**
  * bio_reset - reinitialize a bio
  * @bio:	bio to reset
@@ -866,7 +873,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page,
 		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
 
 		if (page_is_mergeable(bv, page, len, off, same_page)) {
-			if (bio->bi_iter.bi_size > UINT_MAX - len) {
+			if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
 				*same_page = false;
 				return false;
 			}
@@ -995,6 +1002,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 {
 	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
 	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
+	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
 	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
 	struct page **pages = (struct page **)bv;
 	bool same_page = false;
@@ -1010,7 +1018,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
 	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
 	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
 
-	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
+	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
+				  &offset);
 	if (unlikely(size <= 0))
 		return size ? size : -EFAULT;
 
@@ -1038,6 +1047,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
 {
 	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
 	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
+	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
 	struct request_queue *q = bio->bi_bdev->bd_disk->queue;
 	unsigned int max_append_sectors = queue_max_zone_append_sectors(q);
 	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
@@ -1058,7 +1068,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
 	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
 	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
 
-	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
+	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
+				  &offset);
 	if (unlikely(size <= 0))
 		return size ? size : -EFAULT;
 
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 902c40d67120..e270e31519a1 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -32,6 +32,7 @@ EXPORT_SYMBOL_GPL(blk_queue_rq_timeout);
  */
 void blk_set_default_limits(struct queue_limits *lim)
 {
+	lim->max_bio_bytes = UINT_MAX;
 	lim->max_segments = BLK_MAX_SEGMENTS;
 	lim->max_discard_segments = 1;
 	lim->max_integrity_segments = 0;
@@ -100,6 +101,24 @@ void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce bounce)
 }
 EXPORT_SYMBOL(blk_queue_bounce_limit);
 
+/**
+ * blk_queue_max_bio_bytes - set bio max size for queue
+ * @q: the request queue for the device
+ * @bytes : bio max bytes to be set
+ *
+ * Description:
+ *    Set proper bio max size to optimize queue operating.
+ **/
+void blk_queue_max_bio_bytes(struct request_queue *q, unsigned int bytes)
+{
+	struct queue_limits *limits = &q->limits;
+	unsigned int max_bio_bytes = round_up(bytes, PAGE_SIZE);
+
+	limits->max_bio_bytes = max_t(unsigned int, max_bio_bytes,
+				      BIO_MAX_VECS * PAGE_SIZE);
+}
+EXPORT_SYMBOL(blk_queue_max_bio_bytes);
+
 /**
  * blk_queue_max_hw_sectors - set max sectors for a request for this queue
  * @q:  the request queue for the device
diff --git a/include/linux/bio.h b/include/linux/bio.h
index a0b4cfdf62a4..f1a99f0a240c 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -106,6 +106,8 @@ static inline void *bio_data(struct bio *bio)
 	return NULL;
 }
 
+extern unsigned int bio_max_size(struct bio *bio);
+
 /**
  * bio_full - check if the bio is full
  * @bio:	bio to check
@@ -119,7 +121,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
 	if (bio->bi_vcnt >= bio->bi_max_vecs)
 		return true;
 
-	if (bio->bi_iter.bi_size > UINT_MAX - len)
+	if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
 		return true;
 
 	return false;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1255823b2bc0..861888501fc0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -326,6 +326,8 @@ enum blk_bounce {
 };
 
 struct queue_limits {
+	unsigned int		max_bio_bytes;
+
 	enum blk_bounce		bounce;
 	unsigned long		seg_boundary_mask;
 	unsigned long		virt_boundary_mask;
@@ -1132,6 +1134,7 @@ extern void blk_abort_request(struct request *);
  * Access functions for manipulating queue properties
  */
 extern void blk_cleanup_queue(struct request_queue *);
+extern void blk_queue_max_bio_bytes(struct request_queue *, unsigned int);
 void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce limit);
 extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
 extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
-- 
2.29.0

[PATCH v11 2/3] blk-sysfs: add max_bio_bytes

[Changheun Lee]

Add max_bio_bytes block sysfs node to show current maximum bio size.

Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
---
 Documentation/ABI/testing/sysfs-block | 10 ++++++++++
 Documentation/block/queue-sysfs.rst   |  7 +++++++
 block/blk-sysfs.c                     |  7 +++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-block b/Documentation/ABI/testing/sysfs-block
index e34cdeeeb9d4..8c8a793c04b4 100644
--- a/Documentation/ABI/testing/sysfs-block
+++ b/Documentation/ABI/testing/sysfs-block
@@ -316,3 +316,13 @@ Description:
 		does not complete in this time then the block driver timeout
 		handler is invoked. That timeout handler can decide to retry
 		the request, to fail it or to start a device recovery strategy.
+
+What:		/sys/block/<disk>/queue/max_bio_bytes
+Date:		June 2021
+Contact:	Changheun Lee <nanich.lee@samsung.com>
+Description:
+		max_bio_bytes is the maximum bio size to be submitted in bytes.
+		It shows current maximum bio size, and bio size to be submitted
+		will be limited with this. Default value is UINT_MAX, and
+		the minimum value is 1MB. 1MB(=BIO_MAX_VECS * PAGE_SIZE) is
+		legacy maximum bio size.
diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst
index 4dc7f0d499a8..90af56899aa9 100644
--- a/Documentation/block/queue-sysfs.rst
+++ b/Documentation/block/queue-sysfs.rst
@@ -286,4 +286,11 @@ sequential zones of zoned block devices (devices with a zoned attributed
 that reports "host-managed" or "host-aware"). This value is always 0 for
 regular block devices.
 
+max_bio_bytes (RO)
+---------------------------
+This is the maximum number of bytes that bio size to be submitted will be
+limited. A value of 4,294,967,295(UINT_MAX) means no limit of bio size,
+and it's a default value. The minimum value is 1MB. It's legacy maximum
+bio size. (=BIO_MAX_VECS * PAGE_SIZE)
+
 Jens Axboe <jens.axboe@oracle.com>, February 2009
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e03bedf180ab..c4cae6bbcb3b 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -108,6 +108,11 @@ queue_ra_store(struct request_queue *q, const char *page, size_t count)
 	return ret;
 }
 
+static ssize_t queue_max_bio_bytes_show(struct request_queue *q, char *page)
+{
+	return queue_var_show(q->limits.max_bio_bytes, (page));
+}
+
 static ssize_t queue_max_sectors_show(struct request_queue *q, char *page)
 {
 	int max_sectors_kb = queue_max_sectors(q) >> 1;
@@ -577,6 +582,7 @@ static struct queue_sysfs_entry _prefix##_entry = {	\
 
 QUEUE_RW_ENTRY(queue_requests, "nr_requests");
 QUEUE_RW_ENTRY(queue_ra, "read_ahead_kb");
+QUEUE_RO_ENTRY(queue_max_bio_bytes, "max_bio_bytes");
 QUEUE_RW_ENTRY(queue_max_sectors, "max_sectors_kb");
 QUEUE_RO_ENTRY(queue_max_hw_sectors, "max_hw_sectors_kb");
 QUEUE_RO_ENTRY(queue_max_segments, "max_segments");
@@ -635,6 +641,7 @@ QUEUE_RW_ENTRY(queue_stable_writes, "stable_writes");
 static struct attribute *queue_attrs[] = {
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
+	&queue_max_bio_bytes_entry.attr,
 	&queue_max_hw_sectors_entry.attr,
 	&queue_max_sectors_entry.attr,
 	&queue_max_segments_entry.attr,
-- 
2.29.0

[PATCH v11 3/3] scsi: set max_bio_bytes with queue max sectors

[Changheun Lee]

Set max_bio_bytes same with queue max sectors. It will lead to fast bio
submit when bio size is over than queue max sectors. And it might be helpful
to align submit bio size in some case.

Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
---
 drivers/scsi/scsi_lib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 532304d42f00..f6269268b0e0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 	blk_queue_virt_boundary(q, shost->virt_boundary_mask);
 	dma_set_max_seg_size(dev, queue_max_segment_size(q));
 
+	blk_queue_max_bio_bytes(q, queue_max_sectors(q));
+
 	/*
 	 * Set a reasonable default alignment:  The larger of 32-byte (dword),
 	 * which is a common minimum for HBAs, and the minimum DMA alignment,
-- 
2.29.0

Re: [PATCH v11 1/3] bio: control bio max size

[Damien Le Moal]

On 2021/06/02 21:29, Changheun Lee wrote:
> bio size can grow up to 4GB after muli-page bvec has been enabled.
> But sometimes large size of bio would lead to inefficient behaviors.
> Control of bio max size will be helpful to improve inefficiency.
> 
> Below is a example for inefficient behaviours.
> In case of large chunk direct I/O, - 32MB chunk read in user space -
> all pages for 32MB would be merged to a bio structure if the pages
> physical addresses are contiguous. It makes some delay to submit
> until merge complete. bio max size should be limited to a proper size.
> 
> When 32MB chunk read with direct I/O option is coming from userspace,
> kernel behavior is below now in do_direct_IO() loop. It's timeline.
> 
>  | bio merge for 32MB. total 8,192 pages are merged.
>  | total elapsed time is over 2ms.
>  |------------------ ... ----------------------->|
>                                                  | 8,192 pages merged a bio.
>                                                  | at this time, first bio submit is done.
>                                                  | 1 bio is split to 32 read request and issue.
>                                                  |--------------->
>                                                   |--------------->
>                                                    |--------------->
>                                                               ......
>                                                                    |--------------->
>                                                                     |--------------->|
>                           total 19ms elapsed to complete 32MB read done from device. |
> 
> If bio max size is limited with 1MB, behavior is changed below.
> 
>  | bio merge for 1MB. 256 pages are merged for each bio.
>  | total 32 bio will be made.
>  | total elapsed time is over 2ms. it's same.
>  | but, first bio submit timing is fast. about 100us.
>  |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
>       | 256 pages merged a bio.
>       | at this time, first bio submit is done.
>       | and 1 read request is issued for 1 bio.
>       |--------------->
>            |--------------->
>                 |--------------->
>                                       ......
>                                                  |--------------->
>                                                   |--------------->|
>         total 17ms elapsed to complete 32MB read done from device. |
> 
> As a result, read request issue timing is faster if bio max size is limited.
> Current kernel behavior with multipage bvec, super large bio can be created.
> And it lead to delay first I/O request issue.
> 
> Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> ---
>  block/bio.c            | 17 ++++++++++++++---
>  block/blk-settings.c   | 19 +++++++++++++++++++
>  include/linux/bio.h    |  4 +++-
>  include/linux/blkdev.h |  3 +++
>  4 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 44205dfb6b60..c52639bb80cd 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -255,6 +255,13 @@ void bio_init(struct bio *bio, struct bio_vec *table,
>  }
>  EXPORT_SYMBOL(bio_init);
>  
> +unsigned int bio_max_size(struct bio *bio)

It would be nice to call this function the same as the limit name: bio_max_bytes().

> +{
> +	struct block_device *bdev = bio->bi_bdev;
> +
> +	return bdev ? bdev->bd_disk->queue->limits.max_bio_bytes : UINT_MAX;

My personal preference goes to a plain if() instead of this.

> +}
> +
>  /**
>   * bio_reset - reinitialize a bio
>   * @bio:	bio to reset
> @@ -866,7 +873,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page,
>  		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
>  
>  		if (page_is_mergeable(bv, page, len, off, same_page)) {
> -			if (bio->bi_iter.bi_size > UINT_MAX - len) {
> +			if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
>  				*same_page = false;
>  				return false;
>  			}
> @@ -995,6 +1002,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  {
>  	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
>  	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> +	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
>  	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
>  	struct page **pages = (struct page **)bv;
>  	bool same_page = false;
> @@ -1010,7 +1018,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
>  	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
>  
> -	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> +	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
> +				  &offset);
>  	if (unlikely(size <= 0))
>  		return size ? size : -EFAULT;
>  
> @@ -1038,6 +1047,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
>  {
>  	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
>  	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> +	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
>  	struct request_queue *q = bio->bi_bdev->bd_disk->queue;
>  	unsigned int max_append_sectors = queue_max_zone_append_sectors(q);
>  	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
> @@ -1058,7 +1068,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
>  	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
>  	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
>  
> -	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> +	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
> +				  &offset);
>  	if (unlikely(size <= 0))
>  		return size ? size : -EFAULT;
>  
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 902c40d67120..e270e31519a1 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -32,6 +32,7 @@ EXPORT_SYMBOL_GPL(blk_queue_rq_timeout);
>   */
>  void blk_set_default_limits(struct queue_limits *lim)
>  {
> +	lim->max_bio_bytes = UINT_MAX;
>  	lim->max_segments = BLK_MAX_SEGMENTS;
>  	lim->max_discard_segments = 1;
>  	lim->max_integrity_segments = 0;

What about the limit setup for stacked devices ? Leaving it to UINT_MAX is OK ?
If for your use case you add dm-linear on top of the device and rerun your test,
don't you get again slow performance ?

> @@ -100,6 +101,24 @@ void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce bounce)
>  }
>  EXPORT_SYMBOL(blk_queue_bounce_limit);
>  
> +/**
> + * blk_queue_max_bio_bytes - set bio max size for queue
> + * @q: the request queue for the device
> + * @bytes : bio max bytes to be set
> + *
> + * Description:
> + *    Set proper bio max size to optimize queue operating.
> + **/
> +void blk_queue_max_bio_bytes(struct request_queue *q, unsigned int bytes)
> +{
> +	struct queue_limits *limits = &q->limits;
> +	unsigned int max_bio_bytes = round_up(bytes, PAGE_SIZE);
> +
> +	limits->max_bio_bytes = max_t(unsigned int, max_bio_bytes,
> +				      BIO_MAX_VECS * PAGE_SIZE);
> +}
> +EXPORT_SYMBOL(blk_queue_max_bio_bytes);

Why does this need to be exported ?

> +
>  /**
>   * blk_queue_max_hw_sectors - set max sectors for a request for this queue
>   * @q:  the request queue for the device
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index a0b4cfdf62a4..f1a99f0a240c 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -106,6 +106,8 @@ static inline void *bio_data(struct bio *bio)
>  	return NULL;
>  }
>  
> +extern unsigned int bio_max_size(struct bio *bio);

No need for extern.

> +
>  /**
>   * bio_full - check if the bio is full
>   * @bio:	bio to check
> @@ -119,7 +121,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
>  	if (bio->bi_vcnt >= bio->bi_max_vecs)
>  		return true;
>  
> -	if (bio->bi_iter.bi_size > UINT_MAX - len)
> +	if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
>  		return true;
>  
>  	return false;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 1255823b2bc0..861888501fc0 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -326,6 +326,8 @@ enum blk_bounce {
>  };
>  
>  struct queue_limits {
> +	unsigned int		max_bio_bytes;
> +
>  	enum blk_bounce		bounce;
>  	unsigned long		seg_boundary_mask;
>  	unsigned long		virt_boundary_mask;
> @@ -1132,6 +1134,7 @@ extern void blk_abort_request(struct request *);
>   * Access functions for manipulating queue properties
>   */
>  extern void blk_cleanup_queue(struct request_queue *);
> +extern void blk_queue_max_bio_bytes(struct request_queue *, unsigned int);

No need for extern.

>  void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce limit);
>  extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
>  extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v11 3/3] scsi: set max_bio_bytes with queue max sectors

[Damien Le Moal]

On 2021/06/02 21:29, Changheun Lee wrote:
> Set max_bio_bytes same with queue max sectors. It will lead to fast bio
> submit when bio size is over than queue max sectors. And it might be helpful
> to align submit bio size in some case.
> 
> Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> ---
>  drivers/scsi/scsi_lib.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 532304d42f00..f6269268b0e0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
>  	blk_queue_virt_boundary(q, shost->virt_boundary_mask);
>  	dma_set_max_seg_size(dev, queue_max_segment_size(q));
>  
> +	blk_queue_max_bio_bytes(q, queue_max_sectors(q));

Doing this unconditionally for all scsi block devices is probably not a good
idea. Cannot this be moved to the LLD handling the devices that actually need it ?

> +
>  	/*
>  	 * Set a reasonable default alignment:  The larger of 32-byte (dword),
>  	 * which is a common minimum for HBAs, and the minimum DMA alignment,
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v11 3/3] scsi: set max_bio_bytes with queue max sectors

[Bart Van Assche]

On 6/2/21 5:10 AM, Changheun Lee wrote:
> Set max_bio_bytes same with queue max sectors. It will lead to fast bio
> submit when bio size is over than queue max sectors. And it might be helpful
> to align submit bio size in some case.
> 
> Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> ---
>  drivers/scsi/scsi_lib.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 532304d42f00..f6269268b0e0 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
>  	blk_queue_virt_boundary(q, shost->virt_boundary_mask);
>  	dma_set_max_seg_size(dev, queue_max_segment_size(q));
>  
> +	blk_queue_max_bio_bytes(q, queue_max_sectors(q));
> +
>  	/*
>  	 * Set a reasonable default alignment:  The larger of 32-byte (dword),
>  	 * which is a common minimum for HBAs, and the minimum DMA alignment,

Has this patch been tested with dm-crypt on top of a SCSI device? I'm
concerned that this patch will trigger data corruption with dm-crypt on
top because the above change will make the following dm-crypt code fail
for a sufficiently large bio:

		bio_add_page(clone, page, len, 0);

When testing dm-crypt on top of this patch series, please change the
above dm-crypt code into the following before running any tests:

		WARN_ON(bio_add_page(clone, page, len, 0) < len);

Thanks,

Bart.

Re: [PATCH v11 1/3] bio: control bio max size

[Changheun Lee]

> On 2021/06/02 21:29, Changheun Lee wrote:
> > bio size can grow up to 4GB after muli-page bvec has been enabled.
> > But sometimes large size of bio would lead to inefficient behaviors.
> > Control of bio max size will be helpful to improve inefficiency.
> > 
> > Below is a example for inefficient behaviours.
> > In case of large chunk direct I/O, - 32MB chunk read in user space -
> > all pages for 32MB would be merged to a bio structure if the pages
> > physical addresses are contiguous. It makes some delay to submit
> > until merge complete. bio max size should be limited to a proper size.
> > 
> > When 32MB chunk read with direct I/O option is coming from userspace,
> > kernel behavior is below now in do_direct_IO() loop. It's timeline.
> > 
> >  | bio merge for 32MB. total 8,192 pages are merged.
> >  | total elapsed time is over 2ms.
> >  |------------------ ... ----------------------->|
> >                                                  | 8,192 pages merged a bio.
> >                                                  | at this time, first bio submit is done.
> >                                                  | 1 bio is split to 32 read request and issue.
> >                                                  |--------------->
> >                                                   |--------------->
> >                                                    |--------------->
> >                                                               ......
> >                                                                    |--------------->
> >                                                                     |--------------->|
> >                           total 19ms elapsed to complete 32MB read done from device. |
> > 
> > If bio max size is limited with 1MB, behavior is changed below.
> > 
> >  | bio merge for 1MB. 256 pages are merged for each bio.
> >  | total 32 bio will be made.
> >  | total elapsed time is over 2ms. it's same.
> >  | but, first bio submit timing is fast. about 100us.
> >  |--->|--->|--->|---> ... -->|--->|--->|--->|--->|
> >       | 256 pages merged a bio.
> >       | at this time, first bio submit is done.
> >       | and 1 read request is issued for 1 bio.
> >       |--------------->
> >            |--------------->
> >                 |--------------->
> >                                       ......
> >                                                  |--------------->
> >                                                   |--------------->|
> >         total 17ms elapsed to complete 32MB read done from device. |
> > 
> > As a result, read request issue timing is faster if bio max size is limited.
> > Current kernel behavior with multipage bvec, super large bio can be created.
> > And it lead to delay first I/O request issue.
> > 
> > Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> > ---
> >  block/bio.c            | 17 ++++++++++++++---
> >  block/blk-settings.c   | 19 +++++++++++++++++++
> >  include/linux/bio.h    |  4 +++-
> >  include/linux/blkdev.h |  3 +++
> >  4 files changed, 39 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/bio.c b/block/bio.c
> > index 44205dfb6b60..c52639bb80cd 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -255,6 +255,13 @@ void bio_init(struct bio *bio, struct bio_vec *table,
> >  }
> >  EXPORT_SYMBOL(bio_init);
> >  
> > +unsigned int bio_max_size(struct bio *bio)
> 
> It would be nice to call this function the same as the limit name: bio_max_bytes().

I'll do in next version.

> 
> > +{
> > +	struct block_device *bdev = bio->bi_bdev;
> > +
> > +	return bdev ? bdev->bd_disk->queue->limits.max_bio_bytes : UINT_MAX;
> 
> My personal preference goes to a plain if() instead of this.
> 
> > +}
> > +
> >  /**
> >   * bio_reset - reinitialize a bio
> >   * @bio:	bio to reset
> > @@ -866,7 +873,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page *page,
> >  		struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1];
> >  
> >  		if (page_is_mergeable(bv, page, len, off, same_page)) {
> > -			if (bio->bi_iter.bi_size > UINT_MAX - len) {
> > +			if (bio->bi_iter.bi_size > bio_max_size(bio) - len) {
> >  				*same_page = false;
> >  				return false;
> >  			}
> > @@ -995,6 +1002,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> >  {
> >  	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
> >  	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> > +	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
> >  	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
> >  	struct page **pages = (struct page **)bv;
> >  	bool same_page = false;
> > @@ -1010,7 +1018,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> >  	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
> >  	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
> >  
> > -	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> > +	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
> > +				  &offset);
> >  	if (unlikely(size <= 0))
> >  		return size ? size : -EFAULT;
> >  
> > @@ -1038,6 +1047,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
> >  {
> >  	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
> >  	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> > +	unsigned int bytes_left = bio_max_size(bio) - bio->bi_iter.bi_size;
> >  	struct request_queue *q = bio->bi_bdev->bd_disk->queue;
> >  	unsigned int max_append_sectors = queue_max_zone_append_sectors(q);
> >  	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
> > @@ -1058,7 +1068,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter)
> >  	BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
> >  	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
> >  
> > -	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> > +	size = iov_iter_get_pages(iter, pages, bytes_left, nr_pages,
> > +				  &offset);
> >  	if (unlikely(size <= 0))
> >  		return size ? size : -EFAULT;
> >  
> > diff --git a/block/blk-settings.c b/block/blk-settings.c
> > index 902c40d67120..e270e31519a1 100644
> > --- a/block/blk-settings.c
> > +++ b/block/blk-settings.c
> > @@ -32,6 +32,7 @@ EXPORT_SYMBOL_GPL(blk_queue_rq_timeout);
> >   */
> >  void blk_set_default_limits(struct queue_limits *lim)
> >  {
> > +	lim->max_bio_bytes = UINT_MAX;
> >  	lim->max_segments = BLK_MAX_SEGMENTS;
> >  	lim->max_discard_segments = 1;
> >  	lim->max_integrity_segments = 0;
> 
> What about the limit setup for stacked devices ? Leaving it to UINT_MAX is OK ?
> If for your use case you add dm-linear on top of the device and rerun your test,
> don't you get again slow performance ?

After applying of multipage bvec, max bio size is UINT_MAX. So set to UINT_MAX
as a default. As you comment, performance is not improved if limitation of bio
size is not in stacked device. Limit of bio size is needed in both of physical
device and statcked device to improve performance.
Currently I want to focus on physical devices only. After this, I'll prepare
patch for stacked devices.

> 
> > @@ -100,6 +101,24 @@ void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce bounce)
> >  }
> >  EXPORT_SYMBOL(blk_queue_bounce_limit);
> >  
> > +/**
> > + * blk_queue_max_bio_bytes - set bio max size for queue
> > + * @q: the request queue for the device
> > + * @bytes : bio max bytes to be set
> > + *
> > + * Description:
> > + *    Set proper bio max size to optimize queue operating.
> > + **/
> > +void blk_queue_max_bio_bytes(struct request_queue *q, unsigned int bytes)
> > +{
> > +	struct queue_limits *limits = &q->limits;
> > +	unsigned int max_bio_bytes = round_up(bytes, PAGE_SIZE);
> > +
> > +	limits->max_bio_bytes = max_t(unsigned int, max_bio_bytes,
> > +				      BIO_MAX_VECS * PAGE_SIZE);
> > +}
> > +EXPORT_SYMBOL(blk_queue_max_bio_bytes);
> 
> Why does this need to be exported ?

I think it might be good for usability in the other module, like as
the other exported functions in blk-settings.c

> 
> > +
> >  /**
> >   * blk_queue_max_hw_sectors - set max sectors for a request for this queue
> >   * @q:  the request queue for the device
> > diff --git a/include/linux/bio.h b/include/linux/bio.h
> > index a0b4cfdf62a4..f1a99f0a240c 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -106,6 +106,8 @@ static inline void *bio_data(struct bio *bio)
> >  	return NULL;
> >  }
> >  
> > +extern unsigned int bio_max_size(struct bio *bio);
> 
> No need for extern.

If not, compile error - implicit declaration of function -  is occured
in some environment(-Werror=implicit-function-declaration).

> 
> > +
> >  /**
> >   * bio_full - check if the bio is full
> >   * @bio:	bio to check
> > @@ -119,7 +121,7 @@ static inline bool bio_full(struct bio *bio, unsigned len)
> >  	if (bio->bi_vcnt >= bio->bi_max_vecs)
> >  		return true;
> >  
> > -	if (bio->bi_iter.bi_size > UINT_MAX - len)
> > +	if (bio->bi_iter.bi_size > bio_max_size(bio) - len)
> >  		return true;
> >  
> >  	return false;
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 1255823b2bc0..861888501fc0 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -326,6 +326,8 @@ enum blk_bounce {
> >  };
> >  
> >  struct queue_limits {
> > +	unsigned int		max_bio_bytes;
> > +
> >  	enum blk_bounce		bounce;
> >  	unsigned long		seg_boundary_mask;
> >  	unsigned long		virt_boundary_mask;
> > @@ -1132,6 +1134,7 @@ extern void blk_abort_request(struct request *);
> >   * Access functions for manipulating queue properties
> >   */
> >  extern void blk_cleanup_queue(struct request_queue *);
> > +extern void blk_queue_max_bio_bytes(struct request_queue *, unsigned int);
> 
> No need for extern.
> 
> >  void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce limit);
> >  extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
> >  extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
> > 

Re: [PATCH v11 3/3] scsi: set max_bio_bytes with queue max sectors

[Changheun Lee]

> On 2021/06/02 21:29, Changheun Lee wrote:
> > Set max_bio_bytes same with queue max sectors. It will lead to fast bio
> > submit when bio size is over than queue max sectors. And it might be helpful
> > to align submit bio size in some case.
> > 
> > Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> > ---
> >  drivers/scsi/scsi_lib.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 532304d42f00..f6269268b0e0 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
> >  	blk_queue_virt_boundary(q, shost->virt_boundary_mask);
> >  	dma_set_max_seg_size(dev, queue_max_segment_size(q));
> >  
> > +	blk_queue_max_bio_bytes(q, queue_max_sectors(q));
> 
> Doing this unconditionally for all scsi block devices is probably not a good
> idea. Cannot this be moved to the LLD handling the devices that actually need it ?

OK, I'll try to check more nice location in LLD.

> 
> > +
> >  	/*
> >  	 * Set a reasonable default alignment:  The larger of 32-byte (dword),
> >  	 * which is a common minimum for HBAs, and the minimum DMA alignment,
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research

Thank you,
Changheun Lee

Re: [PATCH v11 3/3] scsi: set max_bio_bytes with queue max sectors

[Changheun Lee]

> On 6/2/21 5:10 AM, Changheun Lee wrote:
> > Set max_bio_bytes same with queue max sectors. It will lead to fast bio
> > submit when bio size is over than queue max sectors. And it might be helpful
> > to align submit bio size in some case.
> > 
> > Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> > ---
> >  drivers/scsi/scsi_lib.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 532304d42f00..f6269268b0e0 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
> >  	blk_queue_virt_boundary(q, shost->virt_boundary_mask);
> >  	dma_set_max_seg_size(dev, queue_max_segment_size(q));
> >  
> > +	blk_queue_max_bio_bytes(q, queue_max_sectors(q));
> > +
> >  	/*
> >  	 * Set a reasonable default alignment:  The larger of 32-byte (dword),
> >  	 * which is a common minimum for HBAs, and the minimum DMA alignment,
> 
> Has this patch been tested with dm-crypt on top of a SCSI device? I'm
> concerned that this patch will trigger data corruption with dm-crypt on
> top because the above change will make the following dm-crypt code fail
> for a sufficiently large bio:
> 
> bio_add_page(clone, page, len, 0);
> 
> When testing dm-crypt on top of this patch series, please change the
> above dm-crypt code into the following before running any tests:
> 
> WARN_ON(bio_add_page(clone, page, len, 0) < len);
> 
> Thanks,
> 
> Bart.

I think it will be OK if nr_iovecs is not over BIO_MAX_VECS in
crypt_alloc_buffer(). Because page is added to bio in PAGE_SIZE as maximum,
and the minimum of max_bio_bytes is "BIO_MAX_VECS * PAGE_SIZE".

for (i = 0; i < nr_iovecs; i++) {
	page = mempool_alloc(&cc->page_pool, gfp_mask);
	if (!page) {
		crypt_free_buffer_pages(cc, clone);
		bio_put(clone);
		gfp_mask |= __GFP_DIRECT_RECLAIM;
		goto retry;
	}

	len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size;

	bio_add_page(clone, page, len, 0);

	remaining_size -= len;
}

Anyway as your advise, I'm checking it in my test environment. There are no
problem by now yet. I'm testing with 1MB max_bio_bytes of SCSI, and UINT_MAX
max_bio_bytes of dm-crypt.

Thank you,
Changheun Lee.

Re: [PATCH v11 3/3] scsi: set max_bio_bytes with queue max sectors

[Changheun Lee]

> > On 2021/06/02 21:29, Changheun Lee wrote:
> > > Set max_bio_bytes same with queue max sectors. It will lead to fast bio
> > > submit when bio size is over than queue max sectors. And it might be helpful
> > > to align submit bio size in some case.
> > > 
> > > Signed-off-by: Changheun Lee <nanich.lee@samsung.com>
> > > ---
> > >  drivers/scsi/scsi_lib.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 532304d42f00..f6269268b0e0 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -1837,6 +1837,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
> > >  	blk_queue_virt_boundary(q, shost->virt_boundary_mask);
> > >  	dma_set_max_seg_size(dev, queue_max_segment_size(q));
> > >  
> > > +	blk_queue_max_bio_bytes(q, queue_max_sectors(q));
> > 
> > Doing this unconditionally for all scsi block devices is probably not a good
> > idea. Cannot this be moved to the LLD handling the devices that actually need it ?
> 
> OK, I'll try to check more nice location in LLD.
> 
> > 
> > > +
> > >  	/*
> > >  	 * Set a reasonable default alignment:  The larger of 32-byte (dword),
> > >  	 * which is a common minimum for HBAs, and the minimum DMA alignment,
> > > 
> > 
> > 
> > -- 
> > Damien Le Moal
> > Western Digital Research
> 
> Thank you,
> Changheun Lee

I think below location might be good. feedback to me please.

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3eb54937f1d8..0f97b7d275ee 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4831,6 +4831,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
 
 	ufshcd_get_lu_power_on_wp_status(hba, sdev);
 
+	blk_queue_max_bio_bytes(sdev->request_queue,
+				queue_max_sectors(sdev->request_queue));
+
 	return 0;
 }

Thank you,
Changheun Lee