| Message ID | 20240607055912.3586772-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/11] dm-integrity: use the nop integrity profile | expand |
On 6/7/24 07:58, Christoph Hellwig wrote: > Both flags are only checked, but never set. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Bart Van Assche <bvanassche@acm.org> > Reviewed-by: Kanchan Joshi <joshi.k@samsung.com> > Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/scsi/sd.c | 14 +++----------- > include/linux/bio.h | 2 -- > 2 files changed, 3 insertions(+), 13 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
Christoph,
> Both flags are only checked, but never set.
Sad to see all the DIX1.1 enablement removed when we're this close to
finally having a userland interface plumbed through. I've been working
on porting our test tooling to work on top of Kanchan's and Anuj's
series. The intent is to add the tests to blktests and fio.
Fundamentally, the biggest problem we had with the original
implementation was that the "integrity profile" was static on a per
controller+device basis. The purpose of 1.1 was to make sure that how to
handle integrity metadata was a per-I/O decision with what to check and
how to do it driven by whichever entity attached the PI. As opposed to
being inferred by controllers and targets (through INQUIRY snooping,
etc.).
We can add the flags back as part of the io_uring series but it does
seem like unnecessary churn to remove things in one release only to add
them back in the next (I'm assuming passthrough will be in 6.12).
On Mon, Jun 10, 2024 at 07:48:52AM -0400, Martin K. Petersen wrote: > Fundamentally, the biggest problem we had with the original > implementation was that the "integrity profile" was static on a per > controller+device basis. The purpose of 1.1 was to make sure that how to > handle integrity metadata was a per-I/O decision with what to check and > how to do it driven by whichever entity attached the PI. As opposed to > being inferred by controllers and targets (through INQUIRY snooping, > etc.). > > We can add the flags back as part of the io_uring series but it does > seem like unnecessary churn to remove things in one release only to add > them back in the next (I'm assuming passthrough will be in 6.12). I can just keep the flags in, they aren't really in the way of anything else here. That being said, if you want opt-in aren't they the wrong polarity anyway?
Christoph, > I can just keep the flags in, they aren't really in the way of anything > else here. That being said, if you want opt-in aren't they the wrong > polarity anyway? I don't particularly like the polarity. It is an artifact of the fact that unless otherwise noted, checking will be enabled both at HBA and storage device. So if we reverse the polarity, it would mean that sd.c, somewhat counter-intuitively, would enable checking on a bio that has no bip attached. Since checking is enabled by default, regardless of whether a bip is provided, it seemed more appropriate to opt in to disabling the checks. I believe one of my review comments wrt. to the io_uring passthrough series was that I'd prefer to see the userland flag have the right polarity, though. Because at that level, explicitly enabling checking makes more sense. I don't really mind reversing the BIP flag polarity either. It's mostly a historical artifact since non-DIX HBAs would snoop INQUIRY and READ CAPACITY and transparently enable T10 PI on the wire. DIX moved that decision to sd.c instead of being done by HBA firmware. But we'd still want checking to be enabled by default even if no integrity was passed down from the HBA.
I can just leave them in for now. Or I can remove them and we add them back with the right polarity and support in nvme when we add an actual user. Either way is pretty simple unlike the weird ip checksum thing.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index d957e29b17a98a..b477383ccc3b2a 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -806,25 +806,17 @@ static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd, if (dix) { /* DIX Type 0, 1, 2, 3 */ if (bio_integrity_flagged(bio, BIP_IP_CHECKSUM)) scmd->prot_flags |= SCSI_PROT_IP_CHECKSUM; - - if (bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false) - scmd->prot_flags |= SCSI_PROT_GUARD_CHECK; + scmd->prot_flags |= SCSI_PROT_GUARD_CHECK; } if (dif != T10_PI_TYPE3_PROTECTION) { /* DIX/DIF Type 0, 1, 2 */ scmd->prot_flags |= SCSI_PROT_REF_INCREMENT; - - if (bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false) - scmd->prot_flags |= SCSI_PROT_REF_CHECK; + scmd->prot_flags |= SCSI_PROT_REF_CHECK; } if (dif) { /* DIX/DIF Type 1, 2, 3 */ scmd->prot_flags |= SCSI_PROT_TRANSFER_PI; - - if (bio_integrity_flagged(bio, BIP_DISK_NOCHECK)) - protect = 3 << 5; /* Disable target PI checking */ - else - protect = 1 << 5; /* Enable target PI checking */ + protect = 1 << 5; /* Enable target PI checking */ } scsi_set_prot_op(scmd, prot_op); diff --git a/include/linux/bio.h b/include/linux/bio.h index d5379548d684e1..ec5dcf8635ac66 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -324,8 +324,6 @@ static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio) enum bip_flags { BIP_BLOCK_INTEGRITY = 1 << 0, /* block layer owns integrity data */ BIP_MAPPED_INTEGRITY = 1 << 1, /* ref tag has been remapped */ - BIP_CTRL_NOCHECK = 1 << 2, /* disable HBA integrity checking */ - BIP_DISK_NOCHECK = 1 << 3, /* disable disk integrity checking */ BIP_IP_CHECKSUM = 1 << 4, /* IP checksum */ BIP_INTEGRITY_USER = 1 << 5, /* Integrity payload is user address */ BIP_COPY_USER = 1 << 6, /* Kernel bounce buffer in use */