| Message ID | 20240425183943.6319-5-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] block: set bip_vcnt correctly | expand |
On Fri, Apr 26, 2024 at 12:09:37AM +0530, Kanchan Joshi wrote: > From: Anuj Gupta <anuj20.g@samsung.com> > > Do it only once when the parent bio completes. > > Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > --- > block/bio-integrity.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/block/bio-integrity.c b/block/bio-integrity.c > index b4042414a08f..b698eb77515d 100644 > --- a/block/bio-integrity.c > +++ b/block/bio-integrity.c > @@ -119,7 +119,8 @@ static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip) > ret = copy_to_iter(bvec_virt(&src_bvec), bytes, &iter); > WARN_ON_ONCE(ret != bytes); > > - bio_integrity_unpin_bvec(copy, nr_vecs, true); > + if (!bio_flagged((bip->bip_bio), BIO_CLONED)) > + bio_integrity_unpin_bvec(copy, nr_vecs, true); > } This feels wrong. I suspect the problem is that BIP_COPY_USER is inherited for clone bios while it shouldn't.
On 4/27/2024 12:35 PM, Christoph Hellwig wrote: > On Fri, Apr 26, 2024 at 12:09:37AM +0530, Kanchan Joshi wrote: >> From: Anuj Gupta <anuj20.g@samsung.com> >> >> Do it only once when the parent bio completes. >> >> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> >> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> >> --- >> block/bio-integrity.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/block/bio-integrity.c b/block/bio-integrity.c >> index b4042414a08f..b698eb77515d 100644 >> --- a/block/bio-integrity.c >> +++ b/block/bio-integrity.c >> @@ -119,7 +119,8 @@ static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip) >> ret = copy_to_iter(bvec_virt(&src_bvec), bytes, &iter); >> WARN_ON_ONCE(ret != bytes); >> >> - bio_integrity_unpin_bvec(copy, nr_vecs, true); >> + if (!bio_flagged((bip->bip_bio), BIO_CLONED)) >> + bio_integrity_unpin_bvec(copy, nr_vecs, true); >> } > > This feels wrong. I suspect the problem is that BIP_COPY_USER is > inherited for clone bios while it shouldn't. > But BIP_COPY_USER flag is really required in the clone bio. So that we can copy the subset of the metadata back (from kernel bounce buffer to user space pinned buffer) in case of read io. Overall, copy-back will happen in installments (for each cloned bio), while the unpin will happen in one shot (for the source bio).
On Mon, Apr 29, 2024 at 05:10:59PM +0530, Kanchan Joshi wrote: > > This feels wrong. I suspect the problem is that BIP_COPY_USER is > > inherited for clone bios while it shouldn't. > > > > But BIP_COPY_USER flag is really required in the clone bio. So that we > can copy the subset of the metadata back (from kernel bounce buffer to > user space pinned buffer) in case of read io. > > Overall, copy-back will happen in installments (for each cloned bio), > while the unpin will happen in one shot (for the source bio). That seems a bit odd compared to the bio data path. If you think this is better than the version used in the data path let's convert the data path to this scheme first to make sure we don't diverge and get the far better testing on the main data map side.
On 4/29/2024 10:39 PM, Christoph Hellwig wrote: > On Mon, Apr 29, 2024 at 05:10:59PM +0530, Kanchan Joshi wrote: >>> This feels wrong. I suspect the problem is that BIP_COPY_USER is >>> inherited for clone bios while it shouldn't. >>> >> >> But BIP_COPY_USER flag is really required in the clone bio. So that we >> can copy the subset of the metadata back (from kernel bounce buffer to >> user space pinned buffer) in case of read io. >> >> Overall, copy-back will happen in installments (for each cloned bio), >> while the unpin will happen in one shot (for the source bio). > > That seems a bit odd compared to the bio data path. If you think this > is better than the version used in the data path let's convert the > data path to this scheme first to make sure we don't diverge and get > the far better testing on the main data map side. > Can you please tell what function(s) in bio data path that need this conversion? To me data path handling seems similar. Each cloned bio will lead to some amount of data transfer to pinned user-memory. The same is happening for meta transfer here.
On Wed, May 01, 2024 at 06:32:45PM +0530, Kanchan Joshi wrote: > Can you please tell what function(s) in bio data path that need this > conversion? > To me data path handling seems similar. Each cloned bio will lead to > some amount of data transfer to pinned user-memory. The same is > happening for meta transfer here. Well, everywhere. e.g. for direct I/O everything is just driven from the fs/direct-io.c and and fs/iomap/direct-io.c code without any knowledge in the underlying driver if data has been pinned (no bounce buffering in this case). Or for passthrough I/O none of the underlying logic knows about the pinning or bounce buffering, everything is handled in block/blk-map.c.
On 5/2/2024 12:42 PM, Christoph Hellwig wrote: > On Wed, May 01, 2024 at 06:32:45PM +0530, Kanchan Joshi wrote: >> Can you please tell what function(s) in bio data path that need this >> conversion? >> To me data path handling seems similar. Each cloned bio will lead to >> some amount of data transfer to pinned user-memory. The same is >> happening for meta transfer here. > Well, everywhere. Next version will not inherit BIP_COPY_USER for clone bios.
diff --git a/block/bio-integrity.c b/block/bio-integrity.c index b4042414a08f..b698eb77515d 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -119,7 +119,8 @@ static void bio_integrity_uncopy_user(struct bio_integrity_payload *bip) ret = copy_to_iter(bvec_virt(&src_bvec), bytes, &iter); WARN_ON_ONCE(ret != bytes); - bio_integrity_unpin_bvec(copy, nr_vecs, true); + if (!bio_flagged((bip->bip_bio), BIO_CLONED)) + bio_integrity_unpin_bvec(copy, nr_vecs, true); } static void bio_integrity_unmap_user(struct bio_integrity_payload *bip) @@ -129,11 +130,14 @@ static void bio_integrity_unmap_user(struct bio_integrity_payload *bip) if (bip->bip_flags & BIP_COPY_USER) { if (dirty) bio_integrity_uncopy_user(bip); - kfree(bvec_virt(bip->bip_vec)); + if (!bio_flagged((bip->bip_bio), BIO_CLONED)) + kfree(bvec_virt(bip->bip_vec)); return; } - bio_integrity_unpin_bvec(bip->bip_vec, bip->bip_max_vcnt, dirty); + if (!bio_flagged((bip->bip_bio), BIO_CLONED)) + bio_integrity_unpin_bvec(bip->bip_vec, bip->bip_max_vcnt, + dirty); } /**