On Fri, 2024-08-30 at 13:42 -0700, Keith Busch wrote: > From: Keith Busch <[email protected]> > > bip is NULL before bio_integrity_prep(). > > Signed-off-by: Keith Busch <[email protected]> > --- > drivers/nvdimm/btt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c > index 423dcd1909061..13594fb712186 100644 > --- a/drivers/nvdimm/btt.c > +++ b/drivers/nvdimm/btt.c > @@ -1435,8 +1435,8 @@ static int btt_do_bvec(struct btt *btt, struct > bio_integrity_payload *bip, > > static void btt_submit_bio(struct bio *bio) > { > - struct bio_integrity_payload *bip = bio_integrity(bio); > struct btt *btt = bio->bi_bdev->bd_disk->private_data; > + struct bio_integrity_payload *bip; > struct bvec_iter iter; > unsigned long start; > struct bio_vec bvec; > @@ -1445,6 +1445,7 @@ static void btt_submit_bio(struct bio *bio) > > if (!bio_integrity_prep(bio)) > return; > + bip = bio_integrity(bio);
Hi Keith, I suspect this isn't actually needed, since the btt never generated its own protection payload. See the /* Already protected? */ case in bio_integrity_prep() - I think that's the only case we were trying to account for - i.e. 'some other layer' set the integrity payload, and we're just passing it on to it's right spot in pmem, and reading it back. The btt itself doesn't ever try to generate and set a protection payload of its own. If you look at the original flow in 41cd8b70c37ace40077c8d6ec0b74b983178c192, btt never actually wants to call bio_integrity_prep and allocate the bip - if it has to do that, that's treated as an error. Since some of the reworks then to eliminate bio_integrity_enabled, and other block level changes, this has changed to actually allocating bip and continuing instead of erroring, but coincidentially since we assign bip before the allocation (i.e. NULL as you point out), any future steps nicely ignore it, but if it was set by another subsystem, things should still 'work' - as in bio_integrity_prep() would return true, and bip would be non-NULL, and would get written/read as needed, and this is the happy path. Does this makes sense or am I missing something? It's probably irrelevant if we deprecate all of this soon anyway :) > > do_acct = blk_queue_io_stat(bio->bi_bdev->bd_disk->queue); > if (do_acct)
