diff mbox series

[v11,07/10] block: introduce BIP_CHECK_GUARD/REFTAG/APPTAG bip_flags

Message ID 20241128112240.8867-8-anuj20.g@samsung.com
State New
Headers show
Series Read/Write with meta/integrity | expand

Commit Message

Anuj Gupta Nov. 28, 2024, 11:22 a.m. UTC 
This patch introduces BIP_CHECK_GUARD/REFTAG/APPTAG bip_flags which
indicate how the hardware should check the integrity payload.
BIP_CHECK_GUARD/REFTAG are conversion of existing semantics, while
BIP_CHECK_APPTAG is a new flag. The driver can now just rely on block
layer flags, and doesn't need to know the integrity source. Submitter
of PI decides which tags to check. This would also give us a unified
interface for user and kernel generated integrity.

Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Keith Busch <kbusch@kernel.org>
---
 block/bio-integrity.c         |  5 +++++
 drivers/nvme/host/core.c      | 11 +++--------
 include/linux/bio-integrity.h |  6 +++++-
 3 files changed, 13 insertions(+), 9 deletions(-)

Comments

Christoph Hellwig Feb. 3, 2025, 6:53 a.m. UTC | #1 
Hi Anuj,

I just stumbled over this patch when forward porting my XFS PI support
code over the weekend, which failed badly because it didn't set the
new BIP_CHECK_GUARD and BIP_CHECK_REFTAG flags.  Now for the XFS side
that was just me being to lazy to forward port, but when I started
looking over bio_integrity_add_page users as part of doing this I think
I found a regression caused by this patch.

The scsi and nvme targets never sets these new flags when passing on PI,
so that will probably stop working.  So we'll need to set them and for
nvmet we could also improve the code to actually pass through the
individual flags.  Note that this is just by observation, I didn't find
time to actually set up the SCSI and NVMe target code with PI support.
Maybe we also need blktests test cases to exercise the code and avoid
regressions in the future?
Anuj Gupta Feb. 3, 2025, 2:39 p.m. UTC | #2 
On Mon, Feb 03, 2025 at 07:53:31AM +0100, Christoph Hellwig wrote:
> Hi Anuj,
> 
> I just stumbled over this patch when forward porting my XFS PI support
> code over the weekend, which failed badly because it didn't set the
> new BIP_CHECK_GUARD and BIP_CHECK_REFTAG flags.  Now for the XFS side
> that was just me being to lazy to forward port, but when I started
> looking over bio_integrity_add_page users as part of doing this I think
> I found a regression caused by this patch.
> 
> The scsi and nvme targets never sets these new flags when passing on PI,
> so that will probably stop working.  So we'll need to set them and for
> nvmet we could also improve the code to actually pass through the
> individual flags.  Note that this is just by observation, I didn't find
> time to actually set up the SCSI and NVMe target code with PI support.

Hi Christoph,

Thanks for sharing. Right, the target code is not setting these flags.
I tried to reproduce it by creating a target setup. nvme-tcp doesn't
support T-10 PI (it doesn't set the NVMF_METADATA_SUPPORTED flag).
nvme-rdma supports T-10 PI, trying to reproduce it there.

Something like this (compile-tested only) [1] could work for
nvme-fabrics. Will investigate more and test.

[1]

diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index c1f574fe3280..a3152699b7de 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -210,6 +210,10 @@ static int nvmet_bdev_alloc_bip(struct nvmet_req *req, struct bio *bio,
 		return PTR_ERR(bip);
 	}
 
+	if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
+		bip->bip_flags |= BIP_IP_CHECKSUM;
+	if (bi->flags & BLK_INTEGRITY_REF_TAG)
+		bip->bip_flags |= BIP_CHECK_REFTAG;
 	/* virtual start sector must be in integrity interval units */
 	bip_set_seed(bip, bio->bi_iter.bi_sector >>
 		     (bi->interval_exp - SECTOR_SHIFT));


Thanks,
Anuj Gupta
Christoph Hellwig Feb. 5, 2025, 3:42 p.m. UTC | #3 
On Wed, Feb 05, 2025 at 05:21:34PM +0530, Anuj Gupta wrote:
> On Tue, Feb 04, 2025 at 06:39:14AM +0100, Christoph Hellwig wrote:
> > On Mon, Feb 03, 2025 at 08:09:48PM +0530, Anuj Gupta wrote:
> > > +	if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
> > > +		bip->bip_flags |= BIP_IP_CHECKSUM;
> > 
> > We'll also need to set the BIP_CHECK_GUARD flag here I think.
> 
> Right, I think this patch should address the problem [*]
> I couldn't test this patch, as nvme-tcp doesn't support T10-PI and so
> does rdma_rxe. I don't have rdma h/w to test this.
> It would be great if someone can give this a run.

I'll add support to nvmet-loop, which should make testing either.

Note that the SCSI target code has the same issues and might be a
little easier to test.
diff mbox series

Patch

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index f56d01cec689..3bee43b87001 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -434,6 +434,11 @@  bool bio_integrity_prep(struct bio *bio)
 	if (bi->csum_type == BLK_INTEGRITY_CSUM_IP)
 		bip->bip_flags |= BIP_IP_CHECKSUM;
 
+	/* describe what tags to check in payload */
+	if (bi->csum_type)
+		bip->bip_flags |= BIP_CHECK_GUARD;
+	if (bi->flags & BLK_INTEGRITY_REF_TAG)
+		bip->bip_flags |= BIP_CHECK_REFTAG;
 	if (bio_integrity_add_page(bio, virt_to_page(buf), len,
 			offset_in_page(buf)) < len) {
 		printk(KERN_ERR "could not attach integrity payload\n");
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 40e7be3b0339..e4e3653c27fb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1017,18 +1017,13 @@  static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 			control |= NVME_RW_PRINFO_PRACT;
 		}
 
-		switch (ns->head->pi_type) {
-		case NVME_NS_DPS_PI_TYPE3:
+		if (bio_integrity_flagged(req->bio, BIP_CHECK_GUARD))
 			control |= NVME_RW_PRINFO_PRCHK_GUARD;
-			break;
-		case NVME_NS_DPS_PI_TYPE1:
-		case NVME_NS_DPS_PI_TYPE2:
-			control |= NVME_RW_PRINFO_PRCHK_GUARD |
-					NVME_RW_PRINFO_PRCHK_REF;
+		if (bio_integrity_flagged(req->bio, BIP_CHECK_REFTAG)) {
+			control |= NVME_RW_PRINFO_PRCHK_REF;
 			if (op == nvme_cmd_zone_append)
 				control |= NVME_RW_APPEND_PIREMAP;
 			nvme_set_ref_tag(ns, cmnd, req);
-			break;
 		}
 	}
 
diff --git a/include/linux/bio-integrity.h b/include/linux/bio-integrity.h
index 58ff9988433a..fe2bfe122db2 100644
--- a/include/linux/bio-integrity.h
+++ b/include/linux/bio-integrity.h
@@ -11,6 +11,9 @@  enum bip_flags {
 	BIP_DISK_NOCHECK	= 1 << 3, /* disable disk integrity checking */
 	BIP_IP_CHECKSUM		= 1 << 4, /* IP checksum */
 	BIP_COPY_USER		= 1 << 5, /* Kernel bounce buffer in use */
+	BIP_CHECK_GUARD		= 1 << 6, /* guard check */
+	BIP_CHECK_REFTAG	= 1 << 7, /* reftag check */
+	BIP_CHECK_APPTAG	= 1 << 8, /* apptag check */
 };
 
 struct bio_integrity_payload {
@@ -31,7 +34,8 @@  struct bio_integrity_payload {
 };
 
 #define BIP_CLONE_FLAGS (BIP_MAPPED_INTEGRITY | BIP_CTRL_NOCHECK | \
-			 BIP_DISK_NOCHECK | BIP_IP_CHECKSUM)
+			 BIP_DISK_NOCHECK | BIP_IP_CHECKSUM | \
+			 BIP_CHECK_GUARD | BIP_CHECK_REFTAG | BIP_CHECK_APPTAG)
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY