Re: [PATCH] btt: fix block integrity

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Fri, 2024-08-30 at 13:42 -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> bip is NULL before bio_integrity_prep().
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  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)