[PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Tokunori Ikegami]

The PI is the first bytes or last bytes of the metadata.
So its size is not equal to the metadata size only but below also.
Then fix the function to check PI size if metadata size or below.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
---
Changes since v3 to v4:
- Change the function name nvme_ns_has_pi() to nvme_ns_supports_pract().

Changes since v2 to v3:
- Delete the nvme_submit_io() changes as only change the nvme_ns_has_pi().

Changes since v1:
- Fix the commit message spelling miss hte to the.

 drivers/nvme/host/core.c | 10 +++++-----
 drivers/nvme/host/nvme.h |  4 ++--
 drivers/nvme/host/rdma.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d..37edd0ba423d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -915,7 +915,7 @@ static inline blk_status_t nvme_setup_write_zeroes(struct nvme_ns *ns,
 	    (ns->head->features & NVME_NS_DEAC))
 		cmnd->write_zeroes.control |= cpu_to_le16(NVME_WZ_DEAC);
 
-	if (nvme_ns_has_pi(ns->head)) {
+	if (nvme_ns_supports_pract(ns->head)) {
 		cmnd->write_zeroes.control |= cpu_to_le16(NVME_RW_PRINFO_PRACT);
 
 		switch (ns->head->pi_type) {
@@ -999,7 +999,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 		 * namespace capacity to zero to prevent any I/O.
 		 */
 		if (!blk_integrity_rq(req)) {
-			if (WARN_ON_ONCE(!nvme_ns_has_pi(ns->head)))
+			if (WARN_ON_ONCE(!nvme_ns_supports_pract(ns->head)))
 				return BLK_STS_NOTSUPP;
 			control |= NVME_RW_PRINFO_PRACT;
 		}
@@ -1766,7 +1766,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
 	 */
 	if (!IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) ||
 	    !(head->features & NVME_NS_METADATA_SUPPORTED))
-		return nvme_ns_has_pi(head);
+		return nvme_ns_supports_pract(head);
 
 	switch (head->pi_type) {
 	case NVME_NS_DPS_PI_TYPE3:
@@ -1929,14 +1929,14 @@ static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
 
 		/*
 		 * The current fabrics transport drivers support namespace
-		 * metadata formats only if nvme_ns_has_pi() returns true.
+		 * metadata formats only if nvme_ns_supports_pract() returns true.
 		 * Suppress support for all other formats so the namespace will
 		 * have a 0 capacity and not be usable through the block stack.
 		 *
 		 * Note, this check will need to be modified if any drivers
 		 * gain the ability to use other metadata formats.
 		 */
-		if (ctrl->max_integrity_segments && nvme_ns_has_pi(head))
+		if (ctrl->max_integrity_segments && nvme_ns_supports_pract(head))
 			head->features |= NVME_NS_METADATA_SUPPORTED;
 	} else {
 		/*
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 093cb423f536..881418444a1a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -540,9 +540,9 @@ struct nvme_ns {
 };
 
 /* NVMe ns supports metadata actions by the controller (generate/strip) */
-static inline bool nvme_ns_has_pi(struct nvme_ns_head *head)
+static inline bool nvme_ns_supports_pract(struct nvme_ns_head *head)
 {
-	return head->pi_type && head->ms == head->pi_size;
+	return head->pi_type && head->ms >= head->pi_size;
 }
 
 struct nvme_ctrl_ops {
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 24a2759798d0..e09fee1dd15c 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2037,7 +2037,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	    queue->pi_support &&
 	    (c->common.opcode == nvme_cmd_write ||
 	     c->common.opcode == nvme_cmd_read) &&
-	    nvme_ns_has_pi(ns->head))
+	    nvme_ns_supports_pract(ns->head))
 		req->use_sig_mr = true;
 	else
 		req->use_sig_mr = false;
-- 
2.45.2

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Christoph Hellwig]

On Wed, Oct 23, 2024 at 07:08:36PM +0900, Tokunori Ikegami wrote:
> The PI is the first bytes or last bytes of the metadata.
> So its size is not equal to the metadata size only but below also.
> Then fix the function to check PI size if metadata size or below.

So basically what the patch does is to make PRACT work for PI where
the metadata size is bigger than the PI tuple size, right?  Maybe state
that clearly and put less emphasis on the mechanics of the change.

>  		 * The current fabrics transport drivers support namespace
> -		 * metadata formats only if nvme_ns_has_pi() returns true.
> +		 * metadata formats only if nvme_ns_supports_pract() returns true.

Pleae stick to 80 character lines.  Also a few more offenders below.

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Keith Busch]

On Wed, Oct 23, 2024 at 05:45:54AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 23, 2024 at 07:08:36PM +0900, Tokunori Ikegami wrote:
> > The PI is the first bytes or last bytes of the metadata.
> > So its size is not equal to the metadata size only but below also.
> > Then fix the function to check PI size if metadata size or below.
> 
> So basically what the patch does is to make PRACT work for PI where
> the metadata size is bigger than the PI tuple size, right?  Maybe state
> that clearly and put less emphasis on the mechanics of the change.

You can't just have a patch declare that PRACT works this way. The spec
says it only works if ms == PI tuple size, otherwise it doesn't do
anything.

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Christoph Hellwig]

On Wed, Oct 23, 2024 at 07:14:20AM -0600, Keith Busch wrote:
> You can't just have a patch declare that PRACT works this way. The spec
> says it only works if ms == PI tuple size, otherwise it doesn't do
> anything.

Well, that means the patch as-is is broken, isn't it?

Which leaves me even more confused about what is is trying to do.

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Keith Busch]

On Wed, Oct 23, 2024 at 06:19:03AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 23, 2024 at 07:14:20AM -0600, Keith Busch wrote:
> > You can't just have a patch declare that PRACT works this way. The spec
> > says it only works if ms == PI tuple size, otherwise it doesn't do
> > anything.
> 
> Well, that means the patch as-is is broken, isn't it?

That's what I've been saying since v1. For some reason that feedback
isn't getting through...

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Tokunori Ikegami]

On 2024/10/23 22:21, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 06:19:03AM -0700, Christoph Hellwig wrote:
>> On Wed, Oct 23, 2024 at 07:14:20AM -0600, Keith Busch wrote:
>>> You can't just have a patch declare that PRACT works this way. The spec
>>> says it only works if ms == PI tuple size, otherwise it doesn't do
>>> anything.
>> Well, that means the patch as-is is broken, isn't it?
> That's what I've been saying since v1. For some reason that feedback
> isn't getting through...
Sorry for wasting your time and confusing but if you have a time please 
let me confirm again below. (It is okay when you have a time case.)
About the mention "The spec says it only works if ms == PI tuple size, 
otherwise it doesn't do anything." do you mention for example the 
specification description below?
--------------------------------------------------------------------------------
(NVM-Express-NVM-Command-Set-Specification-Revision-1.1-2024.08.05-Ratified)
5.3.2.1 Protection Information and Write Commands
Figure 166 provides some examples of the protection information 
processing that may occur as a side effect
of a Write command.
...
If the namespace is formatted with protection information and the PRACT 
bit is set to ‘1’, then:
...
2. If the namespace is formatted with Metadata Size greater than 
protection information size, then the
logical block data and the metadata are transferred from the host buffer 
to the controller. As the
metadata passes through the controller, the controller overwrites the 
protection information portion
of the metadata without checking the protection information portion 
regardless of PRCHK settings.
The logical block data and metadata are written to the NVM (i.e., the 
metadata field remains the
same size in the NVM and the host buffer). The location of the 
protection information within the
metadata is configured when the namespace is formatted (refer to the DPS 
field in Figure 114).
Figure 166: Write Command 16b Guard Protection Information Processing
...
[figure b]
b) MD>8 (e.g., 16), PI, PRACT=0: Metadata remains same size in NVM and 
host buffer
...
[figure d]
d) MD>8 (e.g., 16), PI, PRACT=1: Metadata remains same size in NVM and 
host buffer
--------------------------------------------------------------------------------
In the example case b) and d) the 16B MD data should be sent by the host 
application but the driver does not set or handle any 8MD and 8PI. Is 
this understanding correct?
By the way actually does the kernel nvme driver support the ms >= PI 
LBAF case as correct by the current implementation? (Seems only 
supported for the ms == PI LBAF case only.)
   Note: Only the function nvme_configure_metadata() checks the ms >= PI 
case to set PI type.

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Keith Busch]

On Wed, Oct 23, 2024 at 11:44:13PM +0900, Tokunori Ikegami wrote:
> About the mention "The spec says it only works if ms == PI tuple size,
> otherwise it doesn't do anything." do you mention for example the
> specification description below?
> --------------------------------------------------------------------------------
> (NVM-Express-NVM-Command-Set-Specification-Revision-1.1-2024.08.05-Ratified)
> 5.3.2.1 Protection Information and Write Commands
> Figure 166 provides some examples of the protection information processing
> that may occur as a side effect
> of a Write command.
> ...
> If the namespace is formatted with protection information and the PRACT bit
> is set to `1´, then:
> ...
> 2. If the namespace is formatted with Metadata Size greater than
> protection information size, then the logical block data and the
> metadata are transferred from the host buffer to the controller. As
> the metadata passes through the controller, the controller overwrites
> the protection information portion of the metadata without checking
> the protection information portion regardless of PRCHK settings.  The
> logical block data and metadata are written to the NVM (i.e., the
> metadata field remains the same size in the NVM and the host buffer).
> The location of the protection information within the metadata is
> configured when the namespace is formatted (refer to the DPS field in
> Figure 114).
> Figure 166: Write Command 16b Guard Protection Information Processing
> ...
> [figure b]
> b) MD>8 (e.g., 16), PI, PRACT=0: Metadata remains same size in NVM and host
> buffer
> ...
> [figure d]
> d) MD>8 (e.g., 16), PI, PRACT=1: Metadata remains same size in NVM and host
> buffer
> --------------------------------------------------------------------------------
> In the example case b) and d) the 16B MD data should be sent by the host
> application but the driver does not set or handle any 8MD and 8PI. Is this
> understanding correct?

The important point for those MS>8 formats is the host *must* provide a
buffer for the transfer, otherwise you'll get corruption. This means
those formats are not usable if your kernel didn't set
CONFIG_BLK_DEV_INTEGRTY, or you have a fabrics transport that doesn't
support these.

The format where MS == PI size can still work without
CONFIG_BLK_DEV_INTEGRTY or fabrics support because the the pr-action
disables the host<->device transfers.

The function, nvme_ns_has_pi(), is specifically looking for the formats
that don't require the host to provide a metadata buffer. The comment
above the function says exactly this:

  "NVMe ns supports metadata actions by the controller (generate/strip)"

The only time that happens with PRACT is if metadata size equals the PI
size. Section 2.1.5, Figure 9 says this more succiently.

> By the way actually does the kernel nvme driver support the ms >= PI LBAF
> case as correct by the current implementation? (Seems only supported for the
> ms == PI LBAF case only.)
>
>   Note: Only the function nvme_configure_metadata() checks the ms >= PI case
> to set PI type.

It should work fine with any MS>PI size as long as the disk can
successfully register with blk_integrity, which again, depends on your
kernel config and transport type. That block layer can handle protection
information with extra metadata in the same buffer.

Re: [PATCH v4] nvme: change nvme_ns_has_pi() to nvme_ns_supports_pract()

[Tokunori Ikegami]

On 2024/10/24 0:11, Keith Busch wrote:
> The important point for those MS>8 formats is the host *must* provide a
> buffer for the transfer, otherwise you'll get corruption. This means
> those formats are not usable if your kernel didn't set
> CONFIG_BLK_DEV_INTEGRTY, or you have a fabrics transport that doesn't
> support these.
>
> The format where MS == PI size can still work without
> CONFIG_BLK_DEV_INTEGRTY or fabrics support because the the pr-action
> disables the host<->device transfers.
Okay I will do study more later about the block device integrity feature.
> The function, nvme_ns_has_pi(), is specifically looking for the formats
> that don't require the host to provide a metadata buffer. The comment
> above the function says exactly this:
>
>    "NVMe ns supports metadata actions by the controller (generate/strip)"
>
> The only time that happens with PRACT is if metadata size equals the PI
> size. Section 2.1.5, Figure 9 says this more succiently.
Now I could understand fully with your advice including the driver 
implementation and the section 5.3.2 also checked again.
> It should work fine with any MS>PI size as long as the disk can
> successfully register with blk_integrity, which again, depends on your
> kernel config and transport type. That block layer can handle protection
> information with extra metadata in the same buffer.
Noted this also. Thank you so much for your explanation.