diff mbox series

[v3,03/10] block: handle split correctly for user meta bounce buffer

Message ID 20240823103811.2421-4-anuj20.g@samsung.com (mailing list archive)
State New
Headers show
Series Read/Write with meta/integrity | expand

Commit Message

Anuj Gupta Aug. 23, 2024, 10:38 a.m. UTC
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(-)

Comments

Christoph Hellwig Aug. 24, 2024, 8:31 a.m. UTC | #1
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).
Anuj Gupta Aug. 28, 2024, 11:18 a.m. UTC | #2
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 | \
Christoph Hellwig Aug. 29, 2024, 4:04 a.m. UTC | #3
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 mbox series

Patch

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);