Re: [PATCHv5 0/4] block integrity: directly map user space addresses

[Keith Busch <kbusch@kernel.org>]

On Fri, Dec 01, 2023 at 11:42:53AM -0700, Keith Busch wrote:
> On Fri, Dec 01, 2023 at 04:13:45PM +0530, Kanchan Joshi wrote:
> > On 12/1/2023 3:23 AM, Keith Busch wrote:
> > > From: Keith Busch<kbusch@kernel.org>
> > 
> > This causes a regression (existed in previous version too).
> > System freeze on issuing single read/write io that used to work fine 
> > earlier:
> > fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -cmd_type=nvme 
> > -bs=4096 -numjobs=1 -size=4096 -filename=/dev/ng0n1 -md_per_io_size=8 
> > -name=pt
> > 
> > This is because we pin one bvec during submission, but unpin 4 on 
> > completion. bio_integrity_unpin_bvec() uses bip->bip_max_vcnt, which is 
> > set to 4 (equal to BIO_INLINE_VECS) in this case.
> > 
> > To use bip_max_vcnt the way this series uses, we need below patch/fix:
> 
> Thanks for the catch! Earlier versions of this series was capped by the
> byte count rather than the max_vcnt value, so the inline condition
> didn't matter before. I think your update looks good. I'll double check
> what's going on with my custom tests to see why it didn't see this
> problem.

Got it: I was using ioctl instead of iouring. ioctl doesn't set
REQ_ALLOC_CACHE, so we don't get a bio_set in bio_integrity_alloc(), and
that makes inline_vecs set similiar to what your diff does.

Jens already applied the latest series for the next merge. We can append
this or fold atop, or back it out and we can rework it for another
version. No rush; for your patch:

Reviewed-by: Keith Busch <kbusch@kernel.org>

Thanks again!

> > diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> > index 674a2c80454b..feef615e2c9c 100644
> > --- a/block/bio-integrity.c
> > +++ b/block/bio-integrity.c
> > @@ -69,15 +69,15 @@ struct bio_integrity_payload 
> > *bio_integrity_alloc(struct bio *bio,
> > 
> >          memset(bip, 0, sizeof(*bip));
> > 
> > +       /* always report as many vecs as asked explicitly, not inline 
> > vecs */
> > +       bip->bip_max_vcnt = nr_vecs;
> >          if (nr_vecs > inline_vecs) {
> > -               bip->bip_max_vcnt = nr_vecs;
> >                  bip->bip_vec = bvec_alloc(&bs->bvec_integrity_pool,
> >                                            &bip->bip_max_vcnt, gfp_mask);
> >                  if (!bip->bip_vec)
> >                          goto err;
> >          } else {
> >                  bip->bip_vec = bip->bip_inline_vecs;
> > -               bip->bip_max_vcnt = inline_vecs;
> >          }
> > 
> >          bip->bip_bio = bio;
>