[01/10] block: set bip_vcnt correctly

Commit Message

Kanchan Joshi April 25, 2024, 6:39 p.m. UTC

From: Anuj Gupta <[e-mail]>

Set the bip_vcnt correctly in bio_integrity_init_user and
bio_integrity_copy_user. If the bio gets split at a later point,
this value is required to set the right bip_vcnt in the cloned bio.

Signed-off-by: Anuj Gupta <[e-mail]>
Signed-off-by: Kanchan Joshi <[e-mail]>
---
 block/bio-integrity.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Christoph Hellwig April 27, 2024, 7:02 a.m. UTC | #1

On Fri, Apr 26, 2024 at 12:09:34AM +0530, Kanchan Joshi wrote:
> From: Anuj Gupta <[e-mail]>
> 
> Set the bip_vcnt correctly in bio_integrity_init_user and
> bio_integrity_copy_user. If the bio gets split at a later point,
> this value is required to set the right bip_vcnt in the cloned bio.
> 
> Signed-off-by: Anuj Gupta <[e-mail]>
> Signed-off-by: Kanchan Joshi <[e-mail]>

Looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

Please add a Fixes tag and submit it separately from the features.

I'm actually kinda surprised the direct user mapping of integrity data
survived so far without this.

Keith Busch April 27, 2024, 2:16 p.m. UTC | #2

On Sat, Apr 27, 2024 at 09:02:14AM +0200, Christoph Hellwig wrote:
> On Fri, Apr 26, 2024 at 12:09:34AM +0530, Kanchan Joshi wrote:
> > From: Anuj Gupta <[e-mail]>
> > 
> > Set the bip_vcnt correctly in bio_integrity_init_user and
> > bio_integrity_copy_user. If the bio gets split at a later point,
> > this value is required to set the right bip_vcnt in the cloned bio.
> > 
> > Signed-off-by: Anuj Gupta <[e-mail]>
> > Signed-off-by: Kanchan Joshi <[e-mail]>
> 
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <[e-mail]>
> 
> Please add a Fixes tag and submit it separately from the features.
> 
> I'm actually kinda surprised the direct user mapping of integrity data
> survived so far without this.

The only existing use case for user metadata is REQ_OP_DRV_IN/OUT, which
never splits, so these initial fixes only really matter after this
series adds new usage for generic READ/WRITE.

Kanchan Joshi April 29, 2024, 10:59 a.m. UTC | #3

On 4/27/2024 7:46 PM, Keith Busch wrote:
>> Looks good:
>>
>> Reviewed-by: Christoph Hellwig<[e-mail]>
>>
>> Please add a Fixes tag and submit it separately from the features.
>>
>> I'm actually kinda surprised the direct user mapping of integrity data
>> survived so far without this.
> The only existing use case for user metadata is REQ_OP_DRV_IN/OUT, which
> never splits, so these initial fixes only really matter after this
> series adds new usage for generic READ/WRITE.
> 

Yes. It did not seem that there is any harm due to these missing pieces 
(first 6 patches).
Therefore, "Fixes" tag is not there, and patches were not sent 
separately from the features.

Christoph Hellwig May 1, 2024, 7:45 a.m. UTC | #4

On Sat, Apr 27, 2024 at 08:16:52AM -0600, Keith Busch wrote:
> > Please add a Fixes tag and submit it separately from the features.
> > 
> > I'm actually kinda surprised the direct user mapping of integrity data
> > survived so far without this.
> 
> The only existing use case for user metadata is REQ_OP_DRV_IN/OUT, which
> never splits, so these initial fixes only really matter after this
> series adds new usage for generic READ/WRITE.

Well, it matters to keep our contract up, even if we're not hitting it.

And apparently another user just came out of the woods in dm land..

Keith Busch May 1, 2024, 8:03 a.m. UTC | #5

On Wed, May 01, 2024 at 09:45:44AM +0200, Christoph Hellwig wrote:
> On Sat, Apr 27, 2024 at 08:16:52AM -0600, Keith Busch wrote:
> > > Please add a Fixes tag and submit it separately from the features.
> > > 
> > > I'm actually kinda surprised the direct user mapping of integrity data
> > > survived so far without this.
> > 
> > The only existing use case for user metadata is REQ_OP_DRV_IN/OUT, which
> > never splits, so these initial fixes only really matter after this
> > series adds new usage for generic READ/WRITE.
> 
> Well, it matters to keep our contract up, even if we're not hitting it.
> 
> And apparently another user just came out of the woods in dm land..

But the bug report from dm has nothing to do with user mapped metadata.
That bug existed before that was added, so yeah, patch 5 from this
series (or something like it) should be applied on its own.

Patch

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 2e3e8e04961e..e3390424e6b5 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -254,6 +254,7 @@  static int bio_integrity_copy_user(struct bio *bio, struct bio_vec *bvec,
 
 	bip->bip_flags |= BIP_INTEGRITY_USER | BIP_COPY_USER;
 	bip->bip_iter.bi_sector = seed;
+	bip->bip_vcnt = nr_vecs;
 	return 0;
 free_bip:
 	bio_integrity_free(bio);
@@ -275,6 +276,7 @@  static int bio_integrity_init_user(struct bio *bio, struct bio_vec *bvec,
 	bip->bip_flags |= BIP_INTEGRITY_USER;
 	bip->bip_iter.bi_sector = seed;
 	bip->bip_iter.bi_size = len;
+	bip->bip_vcnt = nr_vecs;
 	return 0;
 }