[PATCH] btt: fix block integrity

[PATCH] btt: fix block integrity

[Keith Busch]

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);
 
 	do_acct = blk_queue_io_stat(bio->bi_bdev->bd_disk->queue);
 	if (do_acct)
-- 
2.43.5

Re: [PATCH] btt: fix block integrity

[Dave Jiang]

On 8/30/24 1:42 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> bip is NULL before bio_integrity_prep().
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  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);
>  
>  	do_acct = blk_queue_io_stat(bio->bi_bdev->bd_disk->queue);
>  	if (do_acct)

Re: [PATCH] btt: fix block integrity

[Ira Weiny]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> bip is NULL before bio_integrity_prep().

Did this fail in some way the user might see?  How was this found?

I think the code is correct but should this be backported to stable or
anything?

Ira

> 
> 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);
>  
>  	do_acct = blk_queue_io_stat(bio->bi_bdev->bd_disk->queue);
>  	if (do_acct)
> -- 
> 2.43.5
> 
> 

Re: [PATCH] btt: fix block integrity

[Keith Busch]

On Wed, Nov 13, 2024 at 01:03:58PM -0600, Ira Weiny wrote:
> Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > bip is NULL before bio_integrity_prep().
> 
> Did this fail in some way the user might see?  How was this found?

I think this means no one ever used block integrity with btt. :)

I found this purely from code inspection because I was combing through
bio_integrity for a completely unrelated problem. I was trying to make
sense of how other drivers use it, and this one didn't make any.
 
> I think the code is correct but should this be backported to stable or
> anything?

Up to you! It's not fixing a regression since it appears to have never
worked before. You can also just delete support for it entirely if no
one cares to use this feature.

Re: [PATCH] btt: fix block integrity

[Dan Williams]

Keith Busch wrote:
> On Wed, Nov 13, 2024 at 01:03:58PM -0600, Ira Weiny wrote:
> > Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > bip is NULL before bio_integrity_prep().
> > 
> > Did this fail in some way the user might see?  How was this found?
> 
> I think this means no one ever used block integrity with btt. :)
> 
> I found this purely from code inspection because I was combing through
> bio_integrity for a completely unrelated problem. I was trying to make
> sense of how other drivers use it, and this one didn't make any.
>  
> > I think the code is correct but should this be backported to stable or
> > anything?
> 
> Up to you! It's not fixing a regression since it appears to have never
> worked before. You can also just delete support for it entirely if no
> one cares to use this feature.

I think most people are just hoping that filesystem metadata checksums
are catching torn writes and not using btt. For the few that do not have
a checksumming filesystem and are using btt I only expect they are using
512 or 4K sector sizes and not the sizes with integrity metadata. For
the ones that are using the odd sizes with integrity metadata they
obviously do not care about the integrity data being transferred since
that never apparently worked.

My vote is delete the integrity support, but keep the odd sector size
support for compatibility.

Re: [PATCH] btt: fix block integrity

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> Keith Busch wrote:
>> On Wed, Nov 13, 2024 at 01:03:58PM -0600, Ira Weiny wrote:
>> > Keith Busch wrote:
>> > > From: Keith Busch <kbusch@kernel.org>
>> > > 
>> > > bip is NULL before bio_integrity_prep().
>> > 
>> > Did this fail in some way the user might see?  How was this found?
>> 
>> I think this means no one ever used block integrity with btt. :)
>> 
>> I found this purely from code inspection because I was combing through
>> bio_integrity for a completely unrelated problem. I was trying to make
>> sense of how other drivers use it, and this one didn't make any.
>>  
>> > I think the code is correct but should this be backported to stable or
>> > anything?
>> 
>> Up to you! It's not fixing a regression since it appears to have never
>> worked before. You can also just delete support for it entirely if no
>> one cares to use this feature.
>
> I think most people are just hoping that filesystem metadata checksums
> are catching torn writes and not using btt. For the few that do not have
> a checksumming filesystem and are using btt I only expect they are using
> 512 or 4K sector sizes and not the sizes with integrity metadata. For
> the ones that are using the odd sizes with integrity metadata they
> obviously do not care about the integrity data being transferred since
> that never apparently worked.
>
> My vote is delete the integrity support, but keep the odd sector size
> support for compatibility.

More generally, does anyone use btt?  Should we start the deprecation
process for it?

-Jeff

Re: [PATCH] btt: fix block integrity

[Dan Williams]

Jeff Moyer wrote:
> Dan Williams <dan.j.williams@intel.com> writes:
> 
> > Keith Busch wrote:
> >> On Wed, Nov 13, 2024 at 01:03:58PM -0600, Ira Weiny wrote:
> >> > Keith Busch wrote:
> >> > > From: Keith Busch <kbusch@kernel.org>
> >> > > 
> >> > > bip is NULL before bio_integrity_prep().
> >> > 
> >> > Did this fail in some way the user might see?  How was this found?
> >> 
> >> I think this means no one ever used block integrity with btt. :)
> >> 
> >> I found this purely from code inspection because I was combing through
> >> bio_integrity for a completely unrelated problem. I was trying to make
> >> sense of how other drivers use it, and this one didn't make any.
> >>  
> >> > I think the code is correct but should this be backported to stable or
> >> > anything?
> >> 
> >> Up to you! It's not fixing a regression since it appears to have never
> >> worked before. You can also just delete support for it entirely if no
> >> one cares to use this feature.
> >
> > I think most people are just hoping that filesystem metadata checksums
> > are catching torn writes and not using btt. For the few that do not have
> > a checksumming filesystem and are using btt I only expect they are using
> > 512 or 4K sector sizes and not the sizes with integrity metadata. For
> > the ones that are using the odd sizes with integrity metadata they
> > obviously do not care about the integrity data being transferred since
> > that never apparently worked.
> >
> > My vote is delete the integrity support, but keep the odd sector size
> > support for compatibility.
> 
> More generally, does anyone use btt?  Should we start the deprecation
> process for it?

True, I think it is worth starting the deprecation process and see who
screams. It is easy enough to revive if there are still users out there
that have not since moved back to NVME for storage.

Re: [PATCH] btt: fix block integrity

[Christoph Hellwig]

On Wed, Nov 13, 2024 at 02:06:02PM -0800, Dan Williams wrote:
> True, I think it is worth starting the deprecation process and see who
> screams. It is easy enough to revive if there are still users out there
> that have not since moved back to NVME for storage.

Were there every any users?  I've seen bug reports for all kinds of
weird drivers, but never ever btt..

Re: [PATCH] btt: fix block integrity

[Verma, Vishal L]

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)

Re: [PATCH] btt: fix block integrity

[Keith Busch]

On Thu, Nov 14, 2024 at 09:33:05PM +0000, Verma, Vishal L wrote:
> 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?

One of us might be missing something. :)

The only upper layers I find that pass an integrity payload to the lower
level driver are the scsi and nvme targets. It may make sense to use btt
as the backend for those, but is that the only use case this was trying
to enable? Or is there some other path I didn't find?

Re: [PATCH] btt: fix block integrity

[Verma, Vishal L]

On Fri, 2024-11-15 at 08:19 -0700, Keith Busch wrote:
> On Thu, Nov 14, 2024 at 09:33:05PM +0000, Verma, Vishal L wrote:
> > 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?
> 
> One of us might be missing something. :)
> 
> The only upper layers I find that pass an integrity payload to the lower
> level driver are the scsi and nvme targets. It may make sense to use btt
> as the backend for those, but is that the only use case this was trying
> to enable? Or is there some other path I didn't find?

Right, that was the only use case this was trying to enable. I'm not
sure btt can even be a backend to scsi or nvme, I think one possibilyty
could've been something like md, or something more custom. The goal
with btt was to pass through any integrity payloads if it encounters
them, but otherwise not generate one by itself.