| Message ID | 20240830204255.4130362-1-kbusch@meta.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btt: fix block integrity | expand |
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)
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 > >
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.
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.
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
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.
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..
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)
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?
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.
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)