| Message ID | 20240823103811.2421-4-anuj20.g@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/10] block: define set of integrity flags to be inherited by cloned bip | expand |
On Fri, Aug 23, 2024 at 04:08:03PM +0530, Anuj Gupta wrote: > Copy back the bounce buffer to user-space in entirety when the parent > bio completes. This looks odd to me. The usual way to handle iterating the entire submitter controlled data is to just iterate over the bvec array, as done by bio_for_each_segment_all/bio_for_each_bvec_all for the bio data. I think you want to do the same here, probably with a similar bip_for_each_bvec_all or similar helper. That way you don't need to stash away the iter. Currently we have the field for that, but I really want to split up struct bio_integrity_payload into what is actually needed for the payload and stuff only needed for the block layer autogenerated PI (bip_bio/bio_iter/bip_work).
On Sat, Aug 24, 2024 at 10:31:16AM +0200, Christoph Hellwig wrote: > On Fri, Aug 23, 2024 at 04:08:03PM +0530, Anuj Gupta wrote: > > Copy back the bounce buffer to user-space in entirety when the parent > > bio completes. > > This looks odd to me. The usual way to handle iterating the entire > submitter controlled data is to just iterate over the bvec array, as > done by bio_for_each_segment_all/bio_for_each_bvec_all for the bio > data. I think you want to do the same here, probably with a > similar bip_for_each_bvec_all or similar helper. That way you don't > need to stash away the iter. Currently we have the field for that, > but I really want to split up struct bio_integrity_payload into > what is actually needed for the payload and stuff only needed for > the block layer autogenerated PI (bip_bio/bio_iter/bip_work). I can add it [*], to iterate over the entire bvec array. But the original bio_iter still needs to be stored during submission, to calculate the number of bytes in the original integrity/metadata iter (as it could have gotten split, and I don't have original integrity iter stored anywhere). Do you have a different opinion? [*] diff --git a/block/bio-integrity.c b/block/bio-integrity.c index ff7de4fe74c4..f1690c644e70 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -125,11 +125,23 @@ static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip) struct bio_vec *copy = &bip->bip_vec[1]; size_t bytes = bio_iter_integrity_bytes(bi, bip->bio_iter); struct iov_iter iter; - int ret; + struct bio_vec *bvec; + struct bvec_iter_all iter_all; iov_iter_bvec(&iter, ITER_DEST, copy, nr_vecs, bytes); - ret = copy_to_iter(bvec_virt(bip->bip_vec), bytes, &iter); - WARN_ON_ONCE(ret != bytes); + bip_for_each_segment_all(bvec, bip, iter_all) { + ssize_t ret; + + ret = copy_page_to_iter(bvec->bv_page, + bvec->bv_offset, + bvec->bv_len, + &iter); + + if (!iov_iter_count(&iter)) + break; + + WARN_ON_ONCE(ret < bvec->bv_len); + } bio_integrity_unpin_bvec(copy, nr_vecs, true); } diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h index 22ff2ae16444..3132ef6f27e0 100644 --- a/include/linux/bio-integrity.h +++ b/include/linux/bio-integrity.h @@ -46,6 +46,19 @@ struct uio_meta { struct iov_iter iter; }; +static inline bool bip_next_segment(const struct bio_integrity_payload *bip, + struct bvec_iter_all *iter) +{ + if (iter->idx >= bip->bip_vcnt) + return false; + + bvec_advance(&bip->bip_vec[iter->idx], iter); + return true; +} + +#define bip_for_each_segment_all(bvl, bip, iter) \ + for (bvl = bvec_init_iter_all(&iter); bip_next_segment((bip), &iter); ) + #define BIP_CLONE_FLAGS (BIP_MAPPED_INTEGRITY | BIP_CTRL_NOCHECK | \ BIP_DISK_NOCHECK | BIP_IP_CHECKSUM | \ BIP_CHECK_GUARD | BIP_CHECK_REFTAG | \
On Wed, Aug 28, 2024 at 04:48:06PM +0530, Anuj Gupta wrote: > I can add it [*], to iterate over the entire bvec array. But the original > bio_iter still needs to be stored during submission, to calculate the > number of bytes in the original integrity/metadata iter (as it could have > gotten split, and I don't have original integrity iter stored anywhere). > Do you have a different opinion? Just like for the bio data, the original submitter should never use the iter, but the _all iter.
diff --git a/block/bio-integrity.c b/block/bio-integrity.c index 0b69f2b003c3..d8b810a2b4bf 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -118,9 +118,11 @@ static void bio_integrity_unpin_bvec(struct bio_vec *bv, int nr_vecs, static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip) { + struct bio *bio = bip->bip_bio; + struct blk_integrity *bi = blk_get_integrity(bio->bi_bdev->bd_disk); unsigned short nr_vecs = bip->bip_max_vcnt - 1; struct bio_vec *copy = &bip->bip_vec[1]; - size_t bytes = bip->bip_iter.bi_size; + size_t bytes = bio_iter_integrity_bytes(bi, bip->bio_iter); struct iov_iter iter; int ret; @@ -253,6 +255,7 @@ static int bio_integrity_copy_user(struct bio *bio, struct bio_vec *bvec, bip->bip_flags |= BIP_COPY_USER; bip->bip_iter.bi_sector = seed; bip->bip_vcnt = nr_vecs; + bip->bio_iter = bio->bi_iter; return 0; free_bip: bio_integrity_free(bio);
Copy back the bounce buffer to user-space in entirety when the parent bio completes. Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> --- block/bio-integrity.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)