| Message ID | 20250215-clone-bi_vcnt-v1-1-5d00c95fd53a@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: set bi_vcnt when cloning bio | expand |
On 15/02/2025 10:58, Andreas Hindborg wrote: > When cloning a bio, the `bio.bi_vcnt` field is not cloned. This is a > problem if users want to perform bounds checks on the `bio.bi_io_vec` > field. Is this fixing a potential problem? Or fixing a real issue? Thanks, John > > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org> > --- > block/bio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/bio.c b/block/bio.c > index f0c416e5931d9..334eedf312803 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -870,6 +870,7 @@ struct bio *bio_alloc_clone(struct block_device *bdev, struct bio *bio_src, > return NULL; > } > bio->bi_io_vec = bio_src->bi_io_vec; > + bio->bi_vcnt = bio_src->bi_vcnt; > > return bio; > } > > --- > base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 > change-id: 20250215-clone-bi_vcnt-f3f770988894 > > Best regards,
"John Garry" <john.g.garry@oracle.com> writes: > On 15/02/2025 10:58, Andreas Hindborg wrote: >> When cloning a bio, the `bio.bi_vcnt` field is not cloned. This is a >> problem if users want to perform bounds checks on the `bio.bi_io_vec` >> field. > > Is this fixing a potential problem? Or fixing a real issue? It is fixing a problem I ran into in rnull, the rust null block implementation. When running with debug assertions enabled, a bound check on `bi_io_vec` fails for split bio, because `bio_vcnt` becomes zero in the cloned bio. I can work around this by not using a slice type to represent `bi_io_vec` in rust, not a big deal. But I am genuinely curious if there is a reason for not setting `bi_vcnt` during a clone. As far as I can tell, it should be safe to set. `bi_vcnt` being zero does not seem to have any effect other than to puzzle developers debugging the code. Maybe I missed something? Best regards, Andreas Hindborg
On 18/02/2025 11:40, Andreas Hindborg wrote: > "John Garry" <john.g.garry@oracle.com> writes: > >> On 15/02/2025 10:58, Andreas Hindborg wrote: >>> When cloning a bio, the `bio.bi_vcnt` field is not cloned. This is a >>> problem if users want to perform bounds checks on the `bio.bi_io_vec` >>> field. >> >> Is this fixing a potential problem? Or fixing a real issue? > > It is fixing a problem I ran into in rnull, the rust null block > implementation. When running with debug assertions enabled, a bound > check on `bi_io_vec` fails for split bio, because `bio_vcnt` becomes > zero in the cloned bio. > > I can work around this by not using a slice type to represent > `bi_io_vec` in rust, not a big deal. > > But I am genuinely curious if there is a reason for not setting > `bi_vcnt` during a clone. I think that it came from commit 59d276fe0 (with the addition of bio_clone_fast()), where we assume that the cloned bio is not having the bio_vec touched and so does not need to know bi_vcnt (or bi_max_vecs). And it is inefficient to needlessly set bi_vcnt then. > As far as I can tell, it should be safe to > set. `bi_vcnt` being zero does not seem to have any effect other than to > puzzle developers debugging the code. > > Maybe I missed something? > > Thanks, John
"John Garry" <john.g.garry@oracle.com> writes: > On 18/02/2025 11:40, Andreas Hindborg wrote: >> "John Garry" <john.g.garry@oracle.com> writes: >> >>> On 15/02/2025 10:58, Andreas Hindborg wrote: >>>> When cloning a bio, the `bio.bi_vcnt` field is not cloned. This is a >>>> problem if users want to perform bounds checks on the `bio.bi_io_vec` >>>> field. >>> >>> Is this fixing a potential problem? Or fixing a real issue? >> >> It is fixing a problem I ran into in rnull, the rust null block >> implementation. When running with debug assertions enabled, a bound >> check on `bi_io_vec` fails for split bio, because `bio_vcnt` becomes >> zero in the cloned bio. >> >> I can work around this by not using a slice type to represent >> `bi_io_vec` in rust, not a big deal. >> >> But I am genuinely curious if there is a reason for not setting >> `bi_vcnt` during a clone. > > I think that it came from commit 59d276fe0 (with the addition of > bio_clone_fast()), where we assume that the cloned bio is not having the > bio_vec touched and so does not need to know bi_vcnt (or bi_max_vecs). > And it is inefficient to needlessly set bi_vcnt then. I see. That is a few days ago. I am quite confident that for modern hardware and workloads, this assignment will not have any measurable impact on performance. Can we add it back? I understand if you would prefer not to, since it is not strictly necessary. But in that case, I would suggest patching the documentation of `struct bio` something like this: --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -255,7 +255,8 @@ struct bio { struct bio_integrity_payload *bi_integrity; /* data integrity */ #endif - unsigned short bi_vcnt; /* how many bio_vec's */ + unsigned short bi_vcnt; /* how many bio_vec's. Not valid if this bio is + a clone (flagged BIO_CLONED). */ /* * Everything starting with bi_max_vecs will be preserved by bio_reset() Best regards, Andreas Hindborg
On 2/18/25 9:12 AM, John Garry wrote: > On 18/02/2025 11:40, Andreas Hindborg wrote: >> But I am genuinely curious if there is a reason for not setting >> `bi_vcnt` during a clone. > > I think that it came from commit 59d276fe0 (with the addition of > bio_clone_fast()), where we assume that the cloned bio is not having the > bio_vec touched and so does not need to know bi_vcnt (or bi_max_vecs). > And it is inefficient to needlessly set bi_vcnt then. Hmm ... I prefer paying the very small performance hit caused by copying bi_vcnt rather than having to deal with the inconsistency caused by not copying that data structure member. Thanks, Bart.
On 18/02/2025 22:21, Bart Van Assche wrote: > On 2/18/25 9:12 AM, John Garry wrote: >> On 18/02/2025 11:40, Andreas Hindborg wrote: >>> But I am genuinely curious if there is a reason for not setting >>> `bi_vcnt` during a clone. >> >> I think that it came from commit 59d276fe0 (with the addition of >> bio_clone_fast()), where we assume that the cloned bio is not having >> the bio_vec touched and so does not need to know bi_vcnt (or >> bi_max_vecs). And it is inefficient to needlessly set bi_vcnt then. > > Hmm ... I prefer paying the very small performance hit caused by copying > bi_vcnt rather than having to deal with the inconsistency caused by not > copying that data structure member. From my experience, setting anything which is not strictly necessary in the fastpath code is generally not wanted. Thanks, John
diff --git a/block/bio.c b/block/bio.c index f0c416e5931d9..334eedf312803 100644 --- a/block/bio.c +++ b/block/bio.c @@ -870,6 +870,7 @@ struct bio *bio_alloc_clone(struct block_device *bdev, struct bio *bio_src, return NULL; } bio->bi_io_vec = bio_src->bi_io_vec; + bio->bi_vcnt = bio_src->bi_vcnt; return bio; }
When cloning a bio, the `bio.bi_vcnt` field is not cloned. This is a problem if users want to perform bounds checks on the `bio.bi_io_vec` field. Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org> --- block/bio.c | 1 + 1 file changed, 1 insertion(+) --- base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 change-id: 20250215-clone-bi_vcnt-f3f770988894 Best regards,