diff mbox series

[02/11] block: remove the unused BIP_{CTRL,DISK}_NOCHECK flags

Message ID 20240607055912.3586772-3-hch@lst.de (mailing list archive)
State Not Applicable
Headers show
Series [01/11] dm-integrity: use the nop integrity profile | expand

Commit Message

Christoph Hellwig June 7, 2024, 5:58 a.m. UTC
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(-)

Comments

Hannes Reinecke June 7, 2024, 6:10 a.m. UTC | #1
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
Martin K. Petersen June 10, 2024, 11:48 a.m. UTC | #2
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).
Christoph Hellwig June 10, 2024, 11:51 a.m. UTC | #3
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?
Martin K. Petersen June 11, 2024, 8:02 p.m. UTC | #4
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.
Christoph Hellwig June 12, 2024, 3:57 a.m. UTC | #5
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 mbox series

Patch

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 */