[v2] block: limit the extract size to align queue limit

  From: Ed Tsai <ed.tsai@mediatek.com>

When an application performs a large IO, it fills and submits multiple
full bios to the block layer. Referring to commit 07173c3ec276
("block: enable multipage bvecs"), the full bio size is no longer fixed
at 1MB but can vary based on the physical memory layout.

The size of the full bio no longer aligns with the maximum IO size of
the queue. Therefore, in a 64MB read, you may see many unaligned bios
being submitted.

Executing the command to perform a 64MB read:

	dd if=/data/test_file of=/dev/null bs=64m count=1 iflag=direct

It demonstrates the submission of numerous unaligned bios:

	block_bio_queue: 254,52 R 2933336 + 2136
	block_bio_queue: 254,52 R 2935472 + 2152
	block_bio_queue: 254,52 R 2937624 + 2128
	block_bio_queue: 254,52 R 2939752 + 2160

This patch limits the number of extract pages to ensure that we submit
the bio once we fill enough pages, preventing the block layer from
spliting small I/Os in between.

I performed the Antutu V10 Storage Test on a UFS 4.0 device, which
resulted in a significant improvement in the Sequential test:

Sequential Read (average of 5 rounds):
Original: 3033.7 MB/sec
Patched: 3520.9 MB/sec

Sequential Write (average of 5 rounds):
Original: 2225.4 MB/sec
Patched: 2800.3 MB/sec

Link: https://lore.kernel.org/linux-arm-kernel/20231025092255.27930-1-ed.tsai@mediatek.com/
Signed-off-by: Ed Tsai <ed.tsai@mediatek.com>

---
 block/bio.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
  

On Fri, Nov 10, 2023 at 01:19:49PM +0800, ed.tsai@mediatek.com wrote:
> +	if (bdev && blk_queue_pci_p2pdma(bdev->bd_disk->queue))
>  		extraction_flags |= ITER_ALLOW_P2PDMA;

As pointed out in reply to Ming, you really need to first figure out
if we can assume we have a valid bdev or not, and if not pass all the
relevant information separately.

> +	if (bdev && bio_op(bio) != REQ_OP_ZONE_APPEND) {
> +		unsigned int max = queue_max_bytes(bdev_get_queue(bdev));

The higher level code must not look at queue_max_bytes, that is only
used for splitting and might not even be initialized.
  

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on hch-configfs/for-next linus/master v6.6 next-20231110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/ed-tsai-mediatek-com/block-limit-the-extract-size-to-align-queue-limit/20231110-142205
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20231110051950.21972-1-ed.tsai%40mediatek.com
patch subject: [PATCH v2] block: limit the extract size to align queue limit
config: arc-randconfig-002-20231110 (https://download.01.org/0day-ci/archive/20231110/202311101853.9N398fyj-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311101853.9N398fyj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311101853.9N398fyj-lkp@intel.com/

All warnings (new ones prefixed by >>):

   block/bio.c: In function '__bio_iov_iter_get_pages':
>> block/bio.c:1261:29: warning: suggest parentheses around '-' in operand of '&' [-Wparentheses]
    1261 |                         max - bio->bi_iter.bi_size & (max - 1) : max;
         |                         ~~~~^~~~~~~~~~~~~~~~~~~~~~


vim +1261 block/bio.c

  1214	
  1215	/**
  1216	 * __bio_iov_iter_get_pages - pin user or kernel pages and add them to a bio
  1217	 * @bio: bio to add pages to
  1218	 * @iter: iov iterator describing the region to be mapped
  1219	 *
  1220	 * Extracts pages from *iter and appends them to @bio's bvec array.  The pages
  1221	 * will have to be cleaned up in the way indicated by the BIO_PAGE_PINNED flag.
  1222	 * For a multi-segment *iter, this function only adds pages from the next
  1223	 * non-empty segment of the iov iterator.
  1224	 */
  1225	static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
  1226	{
  1227		iov_iter_extraction_t extraction_flags = 0;
  1228		unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
  1229		unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
  1230		struct block_device *bdev = bio->bi_bdev;
  1231		struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
  1232		struct page **pages = (struct page **)bv;
  1233		ssize_t max_extract = UINT_MAX - bio->bi_iter.bi_size;
  1234		ssize_t size, left;
  1235		unsigned len, i = 0;
  1236		size_t offset;
  1237		int ret = 0;
  1238	
  1239		/*
  1240		 * Move page array up in the allocated memory for the bio vecs as far as
  1241		 * possible so that we can start filling biovecs from the beginning
  1242		 * without overwriting the temporary page array.
  1243		 */
  1244		BUILD_BUG_ON(PAGE_PTRS_PER_BVEC < 2);
  1245		pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
  1246	
  1247		if (bdev && blk_queue_pci_p2pdma(bdev->bd_disk->queue))
  1248			extraction_flags |= ITER_ALLOW_P2PDMA;
  1249	
  1250		/*
  1251		 * Each segment in the iov is required to be a block size multiple.
  1252		 * However, we may not be able to get the entire segment if it spans
  1253		 * more pages than bi_max_vecs allows, so we have to ALIGN_DOWN the
  1254		 * result to ensure the bio's total size is correct. The remainder of
  1255		 * the iov data will be picked up in the next bio iteration.
  1256		 */
  1257		if (bdev && bio_op(bio) != REQ_OP_ZONE_APPEND) {
  1258			unsigned int max = queue_max_bytes(bdev_get_queue(bdev));
  1259	
  1260			max_extract = bio->bi_iter.bi_size ?
> 1261				max - bio->bi_iter.bi_size & (max - 1) : max;
  1262		}
  1263		size = iov_iter_extract_pages(iter, &pages, max_extract,
  1264					      nr_pages, extraction_flags, &offset);
  1265		if (unlikely(size <= 0))
  1266			return size ? size : -EFAULT;
  1267	
  1268		nr_pages = DIV_ROUND_UP(offset + size, PAGE_SIZE);
  1269	
  1270		if (bdev) {
  1271			size_t trim = size & (bdev_logical_block_size(bdev) - 1);
  1272			iov_iter_revert(iter, trim);
  1273			size -= trim;
  1274		}
  1275	
  1276		if (unlikely(!size)) {
  1277			ret = -EFAULT;
  1278			goto out;
  1279		}
  1280	
  1281		for (left = size, i = 0; left > 0; left -= len, i++) {
  1282			struct page *page = pages[i];
  1283	
  1284			len = min_t(size_t, PAGE_SIZE - offset, left);
  1285			if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
  1286				ret = bio_iov_add_zone_append_page(bio, page, len,
  1287						offset);
  1288				if (ret)
  1289					break;
  1290			} else
  1291				bio_iov_add_page(bio, page, len, offset);
  1292	
  1293			offset = 0;
  1294		}
  1295	
  1296		iov_iter_revert(iter, left);
  1297	out:
  1298		while (i < nr_pages)
  1299			bio_release_page(bio, pages[i++]);
  1300	
  1301		return ret;
  1302	}
  1303
  

On Fri, Nov 10, 2023 at 01:19:49PM +0800, ed.tsai@mediatek.com wrote:
> From: Ed Tsai <ed.tsai@mediatek.com>
> 
> When an application performs a large IO, it fills and submits multiple
> full bios to the block layer. Referring to commit 07173c3ec276
> ("block: enable multipage bvecs"), the full bio size is no longer fixed
> at 1MB but can vary based on the physical memory layout.
> 
> The size of the full bio no longer aligns with the maximum IO size of
> the queue. Therefore, in a 64MB read, you may see many unaligned bios
> being submitted.
> 
> Executing the command to perform a 64MB read:
> 
> 	dd if=/data/test_file of=/dev/null bs=64m count=1 iflag=direct
> 
> It demonstrates the submission of numerous unaligned bios:
> 
> 	block_bio_queue: 254,52 R 2933336 + 2136
> 	block_bio_queue: 254,52 R 2935472 + 2152
> 	block_bio_queue: 254,52 R 2937624 + 2128
> 	block_bio_queue: 254,52 R 2939752 + 2160
> 
> This patch limits the number of extract pages to ensure that we submit
> the bio once we fill enough pages, preventing the block layer from
> spliting small I/Os in between.
> 
> I performed the Antutu V10 Storage Test on a UFS 4.0 device, which
> resulted in a significant improvement in the Sequential test:
> 
> Sequential Read (average of 5 rounds):
> Original: 3033.7 MB/sec
> Patched: 3520.9 MB/sec
> 
> Sequential Write (average of 5 rounds):
> Original: 2225.4 MB/sec
> Patched: 2800.3 MB/sec
> 
> Link: https://lore.kernel.org/linux-arm-kernel/20231025092255.27930-1-ed.tsai@mediatek.com/
> Signed-off-by: Ed Tsai <ed.tsai@mediatek.com>
> 
> ---
>  block/bio.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 816d412c06e9..8d3a112e68da 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1227,8 +1227,10 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  	iov_iter_extraction_t extraction_flags = 0;
>  	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
>  	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> +	struct block_device *bdev = bio->bi_bdev;
>  	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
>  	struct page **pages = (struct page **)bv;
> +	ssize_t max_extract = UINT_MAX - bio->bi_iter.bi_size;
>  	ssize_t size, left;
>  	unsigned len, i = 0;
>  	size_t offset;
> @@ -1242,7 +1244,7 @@ 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);
>  
> -	if (bio->bi_bdev && blk_queue_pci_p2pdma(bio->bi_bdev->bd_disk->queue))
> +	if (bdev && blk_queue_pci_p2pdma(bdev->bd_disk->queue))
>  		extraction_flags |= ITER_ALLOW_P2PDMA;
>  
>  	/*
> @@ -1252,16 +1254,21 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
>  	 * result to ensure the bio's total size is correct. The remainder of
>  	 * the iov data will be picked up in the next bio iteration.
>  	 */
> -	size = iov_iter_extract_pages(iter, &pages,
> -				      UINT_MAX - bio->bi_iter.bi_size,
> +	if (bdev && bio_op(bio) != REQ_OP_ZONE_APPEND) {
> +		unsigned int max = queue_max_bytes(bdev_get_queue(bdev));
> +
> +		max_extract = bio->bi_iter.bi_size ?
> +			max - bio->bi_iter.bi_size & (max - 1) : max;
> +	}
> +	size = iov_iter_extract_pages(iter, &pages, max_extract,
>  				      nr_pages, extraction_flags, &offset);

The above is just what I did in the 'slow path' of patch v2[1], and it can't work
well for every extracting pages which is usually slow, and batching
extracting pages should be done always, such as:

1) build one ublk disk(suppose it is /dev/ublkb0) with max sectors of 32k:

- rublk add null --io-buf-size=16384 -q 2	[2]

2) run 64KB IO

fio --direct=1 --size=230G --bsrange=64k-64k --runtime=20 --numjobs=2 --ioengine=libaio \
	--iodepth=64 --iodepth_batch_submit=64 --iodepth_batch_complete_min=64 --group_reporting=1 \
	--filename=/dev/ublkb0 --name=/dev/ublkb0-test-randread --rw=randread

In my local VM, read BW is dropped to 3709MB/s from 20GB/s in the above
fio test with this patch.

The point is that:

1) bio size alignment is only needed in case of multiple bios

2) bio size alignment is needed only when the current bio is approaching
to become FULL

3) with multiple bvec, it is hard to know how many pages can be held
in bvecs beforehand

In short, running every alignment is much less efficient.


[1] https://lore.kernel.org/linux-block/202311100354.HYfqOQ7o-lkp@intel.com/T/#u
[2] install rublk via `cargo install --version=^0.1 rublk` and
CONFIG_BLK_DEV_UBLK is required

Thanks, 
Ming