| Message ID | 20240702100753.2168-1-anuj20.g@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: reuse original bio_vec array for integrity during clone | expand |
This looks good on it's own:
Signed-off-by: Christoph Hellwig <hch@lst.de>
but while looking for potentially issues with it I noticed the
integrity_req_gap_back_merge / integrity_req_gap_front_merge helpers
that directly access bip->bip_vec. Given how even the old clone
implementation always clones the entire bvec array this patch
isn't even making things worse on it's own, but if we get more uses
of biovec clones we'll probably run into issues there.
For nvme there current is only a single integrity segments, so we
can't ever hits this code (at least until we implement SGL support
for metadata, which would be really nice - any takers?).
For SCSI mpi3mr can set a virt_mask and supports multiple integrity
segments. But I don't know if the two can be combined as the
virt_mask is only set when it is fronting nvme and it might or
might not support integrity data for that use case.
Anuj, > Modify bio_integrity_clone to reuse the original bvec array instead of > allocating and copying it, similar to how bio data path is cloned. Pointing to the original bvec array when cloning was the original approach. I'm not sure how we ended up copying, presumably that was done as part of the iter conversion efforts. In any case this change is fine with me. Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
On Tue, Jul 2, 2024 at 7:38 PM Anuj Gupta <anuj20.g@samsung.com> wrote: > > Modify bio_integrity_clone to reuse the original bvec array instead of > allocating and copying it, similar to how bio data path is cloned. > > Suggested-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> > --- > block/bio-integrity.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/block/bio-integrity.c b/block/bio-integrity.c > index c4aed1dfa497..b78c145eb026 100644 > --- a/block/bio-integrity.c > +++ b/block/bio-integrity.c > @@ -76,7 +76,7 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio, > &bip->bip_max_vcnt, gfp_mask); > if (!bip->bip_vec) > goto err; > - } else { > + } else if (nr_vecs) { > bip->bip_vec = bip->bip_inline_vecs; > } > > @@ -584,14 +584,11 @@ int bio_integrity_clone(struct bio *bio, struct bio *bio_src, > > BUG_ON(bip_src == NULL); > > - bip = bio_integrity_alloc(bio, gfp_mask, bip_src->bip_vcnt); > + bip = bio_integrity_alloc(bio, gfp_mask, 0); > if (IS_ERR(bip)) > return PTR_ERR(bip); > > - memcpy(bip->bip_vec, bip_src->bip_vec, > - bip_src->bip_vcnt * sizeof(struct bio_vec)); > - > - bip->bip_vcnt = bip_src->bip_vcnt; > + bip->bip_vec = bip_src->bip_vec; > bip->bip_iter = bip_src->bip_iter; > bip->bip_flags = bip_src->bip_flags & ~BIP_BLOCK_INTEGRITY; I am curious how the patch avoids double free? Given nothing changes in bip free code path and source bip_vec is associated with this bip. Thanks,
On Wed, Jul 03, 2024 at 11:08:49AM +0800, Ming Lei wrote: > > - bip = bio_integrity_alloc(bio, gfp_mask, bip_src->bip_vcnt); > > + bip = bio_integrity_alloc(bio, gfp_mask, 0); > > if (IS_ERR(bip)) > > return PTR_ERR(bip); > > > > - memcpy(bip->bip_vec, bip_src->bip_vec, > > - bip_src->bip_vcnt * sizeof(struct bio_vec)); > > - > > - bip->bip_vcnt = bip_src->bip_vcnt; > > + bip->bip_vec = bip_src->bip_vec; > > bip->bip_iter = bip_src->bip_iter; > > bip->bip_flags = bip_src->bip_flags & ~BIP_BLOCK_INTEGRITY; > > I am curious how the patch avoids double free? Given nothing changes > in bip free code path and source bip_vec is associated with this bip. bvec_free only frees the bvec array if nr_vecs is > BIO_INLINE_VECS. bio_integrity_clone now passes 0 as nr_vecs to bio_integrity_alloc so it won't ever free the bvec array. This matches what we are doing for the data bvec array in the bio.
On Wed, Jul 3, 2024 at 12:46 PM Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Jul 03, 2024 at 11:08:49AM +0800, Ming Lei wrote: > > > - bip = bio_integrity_alloc(bio, gfp_mask, bip_src->bip_vcnt); > > > + bip = bio_integrity_alloc(bio, gfp_mask, 0); > > > if (IS_ERR(bip)) > > > return PTR_ERR(bip); > > > > > > - memcpy(bip->bip_vec, bip_src->bip_vec, > > > - bip_src->bip_vcnt * sizeof(struct bio_vec)); > > > - > > > - bip->bip_vcnt = bip_src->bip_vcnt; > > > + bip->bip_vec = bip_src->bip_vec; > > > bip->bip_iter = bip_src->bip_iter; > > > bip->bip_flags = bip_src->bip_flags & ~BIP_BLOCK_INTEGRITY; > > > > I am curious how the patch avoids double free? Given nothing changes > > in bip free code path and source bip_vec is associated with this bip. > > bvec_free only frees the bvec array if nr_vecs is > BIO_INLINE_VECS. > bio_integrity_clone now passes 0 as nr_vecs to bio_integrity_alloc > so it won't ever free the bvec array. This matches what we are > doing for the data bvec array in the bio. OK, thanks for the clarification! Reviewed-by: Ming Lei <ming.lei@redhat.com>
On Tue, 02 Jul 2024 15:37:53 +0530, Anuj Gupta wrote: > Modify bio_integrity_clone to reuse the original bvec array instead of > allocating and copying it, similar to how bio data path is cloned. > > Applied, thanks! [1/1] block: reuse original bio_vec array for integrity during clone commit: ba942238056584efd3adc278a76592258d500918 Best regards,
diff --git a/block/bio-integrity.c b/block/bio-integrity.c index c4aed1dfa497..b78c145eb026 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -76,7 +76,7 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio, &bip->bip_max_vcnt, gfp_mask); if (!bip->bip_vec) goto err; - } else { + } else if (nr_vecs) { bip->bip_vec = bip->bip_inline_vecs; } @@ -584,14 +584,11 @@ int bio_integrity_clone(struct bio *bio, struct bio *bio_src, BUG_ON(bip_src == NULL); - bip = bio_integrity_alloc(bio, gfp_mask, bip_src->bip_vcnt); + bip = bio_integrity_alloc(bio, gfp_mask, 0); if (IS_ERR(bip)) return PTR_ERR(bip); - memcpy(bip->bip_vec, bip_src->bip_vec, - bip_src->bip_vcnt * sizeof(struct bio_vec)); - - bip->bip_vcnt = bip_src->bip_vcnt; + bip->bip_vec = bip_src->bip_vec; bip->bip_iter = bip_src->bip_iter; bip->bip_flags = bip_src->bip_flags & ~BIP_BLOCK_INTEGRITY;
Modify bio_integrity_clone to reuse the original bvec array instead of allocating and copying it, similar to how bio data path is cloned. Suggested-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> --- block/bio-integrity.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)