* [PATCH 0/3] block: fix bi_vcnt misuse for cloned bio
@ 2025-12-18 9:31 Ming Lei
2025-12-18 9:31 ` [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way Ming Lei
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Ming Lei @ 2025-12-18 9:31 UTC (permalink / raw)
To: Jens Axboe, linux-block
Cc: io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty, Ming Lei
Hello Guys,
The 1st patch removes .bi_vcnt from bio_may_need_split() because the
incoming bio may be cloned.
The 2nd patch doesn't initialize cloned bio's bi_vcnt in
bio_iov_bvec_set().
The 3rd patch removes iov iter nr_segs re-calculation for io_import_kbuf().
Ming Lei (3):
block: fix bio_may_need_split() by using bvec iterator way
block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set()
io_uring: don't re-calculate iov_iter nr_segs in io_import_kbuf()
block/bio.c | 2 +-
block/blk.h | 13 ++++++++++---
io_uring/rsrc.c | 10 ----------
3 files changed, 11 insertions(+), 14 deletions(-)
--
2.47.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 9:31 [PATCH 0/3] block: fix bi_vcnt misuse for cloned bio Ming Lei @ 2025-12-18 9:31 ` Ming Lei 2025-12-18 9:37 ` Christoph Hellwig 2025-12-20 8:19 ` Nitesh Shetty 2025-12-18 9:31 ` [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() Ming Lei 2025-12-18 9:31 ` [PATCH 3/3] io_uring: don't re-calculate iov_iter nr_segs in io_import_kbuf() Ming Lei 2 siblings, 2 replies; 12+ messages in thread From: Ming Lei @ 2025-12-18 9:31 UTC (permalink / raw) To: Jens Axboe, linux-block Cc: io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty, Ming Lei ->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine passed to bio_may_need_split(). So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead checking with help from bio size and bvec->len. Meantime retrieving the 1st bvec via __bvec_iter_bvec(). Fixes: abd45c159df5 ("block: handle fast path of bio splitting inline") Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/block/blk.h b/block/blk.h index e4c433f62dfc..a0b9cecba8fa 100644 --- a/block/blk.h +++ b/block/blk.h @@ -371,12 +371,19 @@ struct bio *bio_split_zone_append(struct bio *bio, static inline bool bio_may_need_split(struct bio *bio, const struct queue_limits *lim) { + const struct bio_vec *bv; + if (lim->chunk_sectors) return true; - if (bio->bi_vcnt != 1) + + if ((bio_op(bio) != REQ_OP_READ && bio_op(bio) != REQ_OP_WRITE) || + !bio->bi_io_vec) + return true; + + bv = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter); + if (bio->bi_iter.bi_size > bv->bv_len) return true; - return bio->bi_io_vec->bv_len + bio->bi_io_vec->bv_offset > - lim->max_fast_segment_size; + return bv->bv_len + bv->bv_offset > lim->max_fast_segment_size; } /** -- 2.47.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 9:31 ` [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way Ming Lei @ 2025-12-18 9:37 ` Christoph Hellwig 2025-12-18 9:45 ` Ming Lei 2025-12-20 8:19 ` Nitesh Shetty 1 sibling, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2025-12-18 9:37 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty On Thu, Dec 18, 2025 at 05:31:42PM +0800, Ming Lei wrote: > ->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine > passed to bio_may_need_split(). > > So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead > checking with help from bio size and bvec->len. > > Meantime retrieving the 1st bvec via __bvec_iter_bvec(). That totally misses the point. The ->bi_vcnt is a fast and lose check to see if we need the fairly expensive iterators to do the real check. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 9:37 ` Christoph Hellwig @ 2025-12-18 9:45 ` Ming Lei 2025-12-18 15:16 ` Nitesh Shetty 0 siblings, 1 reply; 12+ messages in thread From: Ming Lei @ 2025-12-18 9:45 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty On Thu, Dec 18, 2025 at 01:37:37AM -0800, Christoph Hellwig wrote: > On Thu, Dec 18, 2025 at 05:31:42PM +0800, Ming Lei wrote: > > ->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine > > passed to bio_may_need_split(). > > > > So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead > > checking with help from bio size and bvec->len. > > > > Meantime retrieving the 1st bvec via __bvec_iter_bvec(). > > That totally misses the point. The ->bi_vcnt is a fast and lose > check to see if we need the fairly expensive iterators to do the > real check. It is just __bvec_iter_bvec(), whatever it should be in cache sooner or later. Thanks, Ming ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 9:45 ` Ming Lei @ 2025-12-18 15:16 ` Nitesh Shetty 2025-12-18 16:08 ` Ming Lei 0 siblings, 1 reply; 12+ messages in thread From: Nitesh Shetty @ 2025-12-18 15:16 UTC (permalink / raw) To: Ming Lei Cc: Christoph Hellwig, Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl [-- Attachment #1: Type: text/plain, Size: 3726 bytes --] On 18/12/25 05:45PM, Ming Lei wrote: >On Thu, Dec 18, 2025 at 01:37:37AM -0800, Christoph Hellwig wrote: >> On Thu, Dec 18, 2025 at 05:31:42PM +0800, Ming Lei wrote: >> > ->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine >> > passed to bio_may_need_split(). >> > >> > So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead >> > checking with help from bio size and bvec->len. >> > >> > Meantime retrieving the 1st bvec via __bvec_iter_bvec(). >> >> That totally misses the point. The ->bi_vcnt is a fast and lose >> check to see if we need the fairly expensive iterators to do the >> real check. > >It is just __bvec_iter_bvec(), whatever it should be in cache sooner or >later. > > Functionality wise overall patch looks fine to me, but as Christoph stated there is slight performance(IOPS) penalty. Here is my benchmarking numbers[1], I suspect Jens setup might show more regression. Regards, Nitesh [1] =============================== a. two optane nvme device setup: ---------- base case: ---------- sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nvme0n1 /dev/nvme1n1 submitter=0, tid=206586, file=/dev/nvme0n1, nfiles=1, node=-1 submitter=1, tid=206587, file=/dev/nvme1n1, nfiles=1, node=-1 polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=6.45M, BW=3.15GiB/s, IOS/call=32/31 IOPS=6.47M, BW=3.16GiB/s, IOS/call=32/32 IOPS=6.47M, BW=3.16GiB/s, IOS/call=32/32 Exiting on timeout Maximum IOPS=6.47M ---------------- with this patch: ---------------- sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nvme0n1 /dev/nvme1n1 submitter=0, tid=6352, file=/dev/nvme0n1, nfiles=1, node=-1 polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 submitter=1, tid=6353, file=/dev/nvme1n1, nfiles=1, node=-1 IOPS=6.30M, BW=3.08GiB/s, IOS/call=32/31 IOPS=6.35M, BW=3.10GiB/s, IOS/call=32/31 IOPS=6.37M, BW=3.11GiB/s, IOS/call=32/32 Exiting on timeout Maximum IOPS=6.37M ============================= b. two null-blk device setup: ------------------ null device setup: ------------------ sudo modprobe null_blk queue_mode=2 gb=10 bs=512 nr_devices=2 irqmode=2 \ completion_nsec=1000000 hw_queue_depth=256 memory_backed=0 discard=0 \ use_per_node_hctx=1 ---------- base case: ---------- sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nullb0 /dev/nullb1 submitter=0, tid=6743, file=/dev/nullb0, nfiles=1, node=-1 submitter=1, tid=6744, file=/dev/nullb1, nfiles=1, node=-1 polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=7.89M, BW=3.85GiB/s, IOS/call=32/31 IOPS=7.96M, BW=3.89GiB/s, IOS/call=32/32 IOPS=7.99M, BW=3.90GiB/s, IOS/call=32/32 Exiting on timeout Maximum IOPS=7.99M ------------------- with this patchset: ------------------- sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nullb0 /dev/nullb1 submitter=0, tid=35633, file=/dev/nullb0, nfiles=1, node=-1 submitter=1, tid=35634, file=/dev/nullb1, nfiles=1, node=-1 polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 Engine=io_uring, sq_ring=128, cq_ring=128 IOPS=7.79M, BW=3.80GiB/s, IOS/call=32/31 IOPS=7.86M, BW=3.84GiB/s, IOS/call=32/32 IOPS=7.89M, BW=3.85GiB/s, IOS/call=32/32 Exiting on timeout Maximum IOPS=7.89M [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 15:16 ` Nitesh Shetty @ 2025-12-18 16:08 ` Ming Lei 2025-12-20 8:16 ` Nitesh Shetty 0 siblings, 1 reply; 12+ messages in thread From: Ming Lei @ 2025-12-18 16:08 UTC (permalink / raw) To: Nitesh Shetty Cc: Christoph Hellwig, Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl On Thu, Dec 18, 2025 at 08:46:47PM +0530, Nitesh Shetty wrote: > On 18/12/25 05:45PM, Ming Lei wrote: > > On Thu, Dec 18, 2025 at 01:37:37AM -0800, Christoph Hellwig wrote: > > > On Thu, Dec 18, 2025 at 05:31:42PM +0800, Ming Lei wrote: > > > > ->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine > > > > passed to bio_may_need_split(). > > > > > > > > So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead > > > > checking with help from bio size and bvec->len. > > > > > > > > Meantime retrieving the 1st bvec via __bvec_iter_bvec(). > > > > > > That totally misses the point. The ->bi_vcnt is a fast and lose > > > check to see if we need the fairly expensive iterators to do the > > > real check. > > > > It is just __bvec_iter_bvec(), whatever it should be in cache sooner or > > later. > > > > > Functionality wise overall patch looks fine to me, but as Christoph > stated there is slight performance(IOPS) penalty. > Here is my benchmarking numbers[1], I suspect Jens setup might show > more regression. > > Regards, > Nitesh > > > [1] > =============================== > a. two optane nvme device setup: > ---------- > base case: > ---------- > sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ > -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nvme0n1 /dev/nvme1n1 > submitter=0, tid=206586, file=/dev/nvme0n1, nfiles=1, node=-1 > submitter=1, tid=206587, file=/dev/nvme1n1, nfiles=1, node=-1 > polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 > Engine=io_uring, sq_ring=128, cq_ring=128 > polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 > Engine=io_uring, sq_ring=128, cq_ring=128 > IOPS=6.45M, BW=3.15GiB/s, IOS/call=32/31 > IOPS=6.47M, BW=3.16GiB/s, IOS/call=32/32 > IOPS=6.47M, BW=3.16GiB/s, IOS/call=32/32 > Exiting on timeout > Maximum IOPS=6.47M > > ---------------- > with this patch: > ---------------- > sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ > -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nvme0n1 /dev/nvme1n1 > submitter=0, tid=6352, file=/dev/nvme0n1, nfiles=1, node=-1 > polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 > Engine=io_uring, sq_ring=128, cq_ring=128 > submitter=1, tid=6353, file=/dev/nvme1n1, nfiles=1, node=-1 > IOPS=6.30M, BW=3.08GiB/s, IOS/call=32/31 > IOPS=6.35M, BW=3.10GiB/s, IOS/call=32/31 > IOPS=6.37M, BW=3.11GiB/s, IOS/call=32/32 > Exiting on timeout > Maximum IOPS=6.37M > > ============================= > b. two null-blk device setup: > ------------------ > null device setup: > ------------------ > sudo modprobe null_blk queue_mode=2 gb=10 bs=512 nr_devices=2 irqmode=2 \ > completion_nsec=1000000 hw_queue_depth=256 memory_backed=0 discard=0 \ > use_per_node_hctx=1 > > ---------- > base case: > ---------- > sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ > -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nullb0 /dev/nullb1 > submitter=0, tid=6743, file=/dev/nullb0, nfiles=1, node=-1 > submitter=1, tid=6744, file=/dev/nullb1, nfiles=1, node=-1 > polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 > Engine=io_uring, sq_ring=128, cq_ring=128 > IOPS=7.89M, BW=3.85GiB/s, IOS/call=32/31 > IOPS=7.96M, BW=3.89GiB/s, IOS/call=32/32 > IOPS=7.99M, BW=3.90GiB/s, IOS/call=32/32 > Exiting on timeout > Maximum IOPS=7.99M > > ------------------- > with this patchset: > ------------------- > sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ > -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nullb0 /dev/nullb1 > submitter=0, tid=35633, file=/dev/nullb0, nfiles=1, node=-1 > submitter=1, tid=35634, file=/dev/nullb1, nfiles=1, node=-1 > polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 > Engine=io_uring, sq_ring=128, cq_ring=128 > polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 > Engine=io_uring, sq_ring=128, cq_ring=128 > IOPS=7.79M, BW=3.80GiB/s, IOS/call=32/31 > IOPS=7.86M, BW=3.84GiB/s, IOS/call=32/32 > IOPS=7.89M, BW=3.85GiB/s, IOS/call=32/32 > Exiting on timeout > Maximum IOPS=7.89M Thanks for the perf test! This patch only adds bio->bi_iter memory footprint, which is supposed to hit from L1, maybe because `bi_io_vec` is in the 2nd cacheline, can you see any difference with the following change? diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 5dc061d318a4..1c4570b37436 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -240,6 +240,7 @@ struct bio { /* for plugged zoned writes only: */ unsigned int __bi_nr_segments; }; + struct bio_vec *bi_io_vec; /* the actual vec list */ bio_end_io_t *bi_end_io; void *bi_private; #ifdef CONFIG_BLK_CGROUP @@ -275,8 +276,6 @@ struct bio { atomic_t __bi_cnt; /* pin count */ - struct bio_vec *bi_io_vec; /* the actual vec list */ - struct bio_set *bi_pool; }; Thanks, Ming ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 16:08 ` Ming Lei @ 2025-12-20 8:16 ` Nitesh Shetty 0 siblings, 0 replies; 12+ messages in thread From: Nitesh Shetty @ 2025-12-20 8:16 UTC (permalink / raw) To: Ming Lei Cc: Christoph Hellwig, Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl [-- Attachment #1: Type: text/plain, Size: 5205 bytes --] On 19/12/25 12:08AM, Ming Lei wrote: >On Thu, Dec 18, 2025 at 08:46:47PM +0530, Nitesh Shetty wrote: >> On 18/12/25 05:45PM, Ming Lei wrote: >> > On Thu, Dec 18, 2025 at 01:37:37AM -0800, Christoph Hellwig wrote: >> > > On Thu, Dec 18, 2025 at 05:31:42PM +0800, Ming Lei wrote: >> > > > ->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine >> > > > passed to bio_may_need_split(). >> > > > >> > > > So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead >> > > > checking with help from bio size and bvec->len. >> > > > >> > > > Meantime retrieving the 1st bvec via __bvec_iter_bvec(). >> > > >> > > That totally misses the point. The ->bi_vcnt is a fast and lose >> > > check to see if we need the fairly expensive iterators to do the >> > > real check. >> > >> > It is just __bvec_iter_bvec(), whatever it should be in cache sooner or >> > later. >> > >> > >> Functionality wise overall patch looks fine to me, but as Christoph >> stated there is slight performance(IOPS) penalty. >> Here is my benchmarking numbers[1], I suspect Jens setup might show >> more regression. >> >> Regards, >> Nitesh >> >> >> [1] >> =============================== >> a. two optane nvme device setup: >> ---------- >> base case: >> ---------- >> sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ >> -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nvme0n1 /dev/nvme1n1 >> submitter=0, tid=206586, file=/dev/nvme0n1, nfiles=1, node=-1 >> submitter=1, tid=206587, file=/dev/nvme1n1, nfiles=1, node=-1 >> polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 >> Engine=io_uring, sq_ring=128, cq_ring=128 >> polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 >> Engine=io_uring, sq_ring=128, cq_ring=128 >> IOPS=6.45M, BW=3.15GiB/s, IOS/call=32/31 >> IOPS=6.47M, BW=3.16GiB/s, IOS/call=32/32 >> IOPS=6.47M, BW=3.16GiB/s, IOS/call=32/32 >> Exiting on timeout >> Maximum IOPS=6.47M >> >> ---------------- >> with this patch: >> ---------------- >> sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ >> -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nvme0n1 /dev/nvme1n1 >> submitter=0, tid=6352, file=/dev/nvme0n1, nfiles=1, node=-1 >> polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 >> Engine=io_uring, sq_ring=128, cq_ring=128 >> submitter=1, tid=6353, file=/dev/nvme1n1, nfiles=1, node=-1 >> IOPS=6.30M, BW=3.08GiB/s, IOS/call=32/31 >> IOPS=6.35M, BW=3.10GiB/s, IOS/call=32/31 >> IOPS=6.37M, BW=3.11GiB/s, IOS/call=32/32 >> Exiting on timeout >> Maximum IOPS=6.37M >> >> ============================= >> b. two null-blk device setup: >> ------------------ >> null device setup: >> ------------------ >> sudo modprobe null_blk queue_mode=2 gb=10 bs=512 nr_devices=2 irqmode=2 \ >> completion_nsec=1000000 hw_queue_depth=256 memory_backed=0 discard=0 \ >> use_per_node_hctx=1 >> >> ---------- >> base case: >> ---------- >> sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ >> -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nullb0 /dev/nullb1 >> submitter=0, tid=6743, file=/dev/nullb0, nfiles=1, node=-1 >> submitter=1, tid=6744, file=/dev/nullb1, nfiles=1, node=-1 >> polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 >> Engine=io_uring, sq_ring=128, cq_ring=128 >> IOPS=7.89M, BW=3.85GiB/s, IOS/call=32/31 >> IOPS=7.96M, BW=3.89GiB/s, IOS/call=32/32 >> IOPS=7.99M, BW=3.90GiB/s, IOS/call=32/32 >> Exiting on timeout >> Maximum IOPS=7.99M >> >> ------------------- >> with this patchset: >> ------------------- >> sudo taskset -c 0,1 /home/nitesh/src/private/fio/t/io_uring -b512 \ >> -d128 -c32 -s32 -p1 -F1 -B1 -n2 -r4 /dev/nullb0 /dev/nullb1 >> submitter=0, tid=35633, file=/dev/nullb0, nfiles=1, node=-1 >> submitter=1, tid=35634, file=/dev/nullb1, nfiles=1, node=-1 >> polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 >> Engine=io_uring, sq_ring=128, cq_ring=128 >> polled=1, fixedbufs=1, register_files=1, buffered=0, QD=128 >> Engine=io_uring, sq_ring=128, cq_ring=128 >> IOPS=7.79M, BW=3.80GiB/s, IOS/call=32/31 >> IOPS=7.86M, BW=3.84GiB/s, IOS/call=32/32 >> IOPS=7.89M, BW=3.85GiB/s, IOS/call=32/32 >> Exiting on timeout >> Maximum IOPS=7.89M > >Thanks for the perf test! > >This patch only adds bio->bi_iter memory footprint, which is supposed >to hit from L1, maybe because `bi_io_vec` is in the 2nd cacheline, can >you see any difference with the following change? > > >diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h >index 5dc061d318a4..1c4570b37436 100644 >--- a/include/linux/blk_types.h >+++ b/include/linux/blk_types.h >@@ -240,6 +240,7 @@ struct bio { > /* for plugged zoned writes only: */ > unsigned int __bi_nr_segments; > }; >+ struct bio_vec *bi_io_vec; /* the actual vec list */ > bio_end_io_t *bi_end_io; > void *bi_private; > #ifdef CONFIG_BLK_CGROUP >@@ -275,8 +276,6 @@ struct bio { > > atomic_t __bi_cnt; /* pin count */ > >- struct bio_vec *bi_io_vec; /* the actual vec list */ >- > struct bio_set *bi_pool; > }; > With above patch perf numbers match the base case. Thanks, Nitesh Shetty [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way 2025-12-18 9:31 ` [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way Ming Lei 2025-12-18 9:37 ` Christoph Hellwig @ 2025-12-20 8:19 ` Nitesh Shetty 1 sibling, 0 replies; 12+ messages in thread From: Nitesh Shetty @ 2025-12-20 8:19 UTC (permalink / raw) To: Ming Lei; +Cc: Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl [-- Attachment #1: Type: text/plain, Size: 1146 bytes --] On 18/12/25 05:31PM, Ming Lei wrote: >->bi_vcnt doesn't make sense for cloned bio, which is perfectly fine >passed to bio_may_need_split(). > >So fix bio_may_need_split() by not taking ->bi_vcnt directly, instead >checking with help from bio size and bvec->len. > >Meantime retrieving the 1st bvec via __bvec_iter_bvec(). > >Fixes: abd45c159df5 ("block: handle fast path of bio splitting inline") >Signed-off-by: Ming Lei <ming.lei@redhat.com> >--- > block/blk.h | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > >diff --git a/block/blk.h b/block/blk.h >index e4c433f62dfc..a0b9cecba8fa 100644 >--- a/block/blk.h >+++ b/block/blk.h >@@ -371,12 +371,19 @@ struct bio *bio_split_zone_append(struct bio *bio, > static inline bool bio_may_need_split(struct bio *bio, > const struct queue_limits *lim) > { >+ const struct bio_vec *bv; >+ > if (lim->chunk_sectors) > return true; >- if (bio->bi_vcnt != 1) >+ >+ if ((bio_op(bio) != REQ_OP_READ && bio_op(bio) != REQ_OP_WRITE) || >+ !bio->bi_io_vec) REQ_OP_READ, REQ_OP_WRITE check is not necessary, since bio_may_need_split is always called for READ/WRITE. Thanks, Nitesh [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() 2025-12-18 9:31 [PATCH 0/3] block: fix bi_vcnt misuse for cloned bio Ming Lei 2025-12-18 9:31 ` [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way Ming Lei @ 2025-12-18 9:31 ` Ming Lei 2025-12-18 9:38 ` Christoph Hellwig 2025-12-18 9:31 ` [PATCH 3/3] io_uring: don't re-calculate iov_iter nr_segs in io_import_kbuf() Ming Lei 2 siblings, 1 reply; 12+ messages in thread From: Ming Lei @ 2025-12-18 9:31 UTC (permalink / raw) To: Jens Axboe, linux-block Cc: io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty, Ming Lei For a cloned bio, bi_vcnt should not be initialized since the bio_vec array is shared and owned by the original bio. Instead, initialize bi_iter.bi_idx to 0 to properly start iteration from the beginning of the shared bio_vec array. This also avoids to touch iov_iter.nr_segs, which belongs to iov_iter implementation detail. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/bio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/bio.c b/block/bio.c index e726c0e280a8..79d1fef8ad0f 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1162,8 +1162,8 @@ void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter) { WARN_ON_ONCE(bio->bi_max_vecs); - bio->bi_vcnt = iter->nr_segs; bio->bi_io_vec = (struct bio_vec *)iter->bvec; + bio->bi_iter.bi_idx = 0; bio->bi_iter.bi_bvec_done = iter->iov_offset; bio->bi_iter.bi_size = iov_iter_count(iter); bio_set_flag(bio, BIO_CLONED); -- 2.47.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() 2025-12-18 9:31 ` [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() Ming Lei @ 2025-12-18 9:38 ` Christoph Hellwig 2025-12-18 9:48 ` Ming Lei 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2025-12-18 9:38 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty On Thu, Dec 18, 2025 at 05:31:43PM +0800, Ming Lei wrote: > For a cloned bio, bi_vcnt should not be initialized since the bio_vec > array is shared and owned by the original bio. Maybe, maybe not. What is the rational for that "should" ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() 2025-12-18 9:38 ` Christoph Hellwig @ 2025-12-18 9:48 ` Ming Lei 0 siblings, 0 replies; 12+ messages in thread From: Ming Lei @ 2025-12-18 9:48 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-block, io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty On Thu, Dec 18, 2025 at 01:38:55AM -0800, Christoph Hellwig wrote: > On Thu, Dec 18, 2025 at 05:31:43PM +0800, Ming Lei wrote: > > For a cloned bio, bi_vcnt should not be initialized since the bio_vec > > array is shared and owned by the original bio. > > Maybe, maybe not. What is the rational for that "should" ? ->bi_vcnt is never set for bio allocated from bio_alloc_clone(). Thanks, Ming ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] io_uring: don't re-calculate iov_iter nr_segs in io_import_kbuf() 2025-12-18 9:31 [PATCH 0/3] block: fix bi_vcnt misuse for cloned bio Ming Lei 2025-12-18 9:31 ` [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way Ming Lei 2025-12-18 9:31 ` [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() Ming Lei @ 2025-12-18 9:31 ` Ming Lei 2 siblings, 0 replies; 12+ messages in thread From: Ming Lei @ 2025-12-18 9:31 UTC (permalink / raw) To: Jens Axboe, linux-block Cc: io-uring, Caleb Sander Mateos, huang-jl, Nitesh Shetty, Ming Lei We have provide correct byte counts to iov_iter, and it is enough to cap the iteration, not necessary to re-calculate exact nr_segs. Especially the previous two patches avoid to use bio->bi_vcnt as split hint, and don't use iov_iter->nr_segs to initialize bio->bi_vcnt. The iov_iter nr_segs re-calculation[1] is added for avoiding unnecessary bio split, which is fixed now by the previous two patches. [1] https://lkml.org/lkml/2025/4/16/351 Signed-off-by: Ming Lei <ming.lei@redhat.com> --- io_uring/rsrc.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index a63474b331bf..ee6283676ba7 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -1055,16 +1055,6 @@ static int io_import_kbuf(int ddir, struct iov_iter *iter, iov_iter_bvec(iter, ddir, imu->bvec, imu->nr_bvecs, count); iov_iter_advance(iter, offset); - - if (count < imu->len) { - const struct bio_vec *bvec = iter->bvec; - - while (len > bvec->bv_len) { - len -= bvec->bv_len; - bvec++; - } - iter->nr_segs = 1 + bvec - iter->bvec; - } return 0; } -- 2.47.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-12-20 8:20 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-18 9:31 [PATCH 0/3] block: fix bi_vcnt misuse for cloned bio Ming Lei 2025-12-18 9:31 ` [PATCH 1/3] block: fix bio_may_need_split() by using bvec iterator way Ming Lei 2025-12-18 9:37 ` Christoph Hellwig 2025-12-18 9:45 ` Ming Lei 2025-12-18 15:16 ` Nitesh Shetty 2025-12-18 16:08 ` Ming Lei 2025-12-20 8:16 ` Nitesh Shetty 2025-12-20 8:19 ` Nitesh Shetty 2025-12-18 9:31 ` [PATCH 2/3] block: don't initialize bi_vcnt for cloned bio in bio_iov_bvec_set() Ming Lei 2025-12-18 9:38 ` Christoph Hellwig 2025-12-18 9:48 ` Ming Lei 2025-12-18 9:31 ` [PATCH 3/3] io_uring: don't re-calculate iov_iter nr_segs in io_import_kbuf() Ming Lei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox