[PATCH] nvme: check PI size if metadata size or below

[PATCH] nvme: check PI size if metadata size or below

[Tokunori Ikegami]

The PI is the first bytes or last bytes of the metadata.
So its size is not equal to hte metadata size only but below also.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
---
 drivers/nvme/host/ioctl.c | 4 ++--
 drivers/nvme/host/nvme.h  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index b9b79ccfabf8..9fddaaddbbf1 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -232,8 +232,8 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 
 	length = (io.nblocks + 1) << ns->head->lba_shift;
 
-	if ((io.control & NVME_RW_PRINFO_PRACT) &&
-	    (ns->head->ms == ns->head->pi_size)) {
+	if (io.control & NVME_RW_PRINFO_PRACT &&
+	    ns->head->ms >= ns->head->pi_size) {
 		/*
 		 * Protection information is stripped/inserted by the
 		 * controller.
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 093cb423f536..fbcb2243ba84 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -542,7 +542,7 @@ 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)
 {
-	return head->pi_type && head->ms == head->pi_size;
+	return head->pi_type && head->ms >= head->pi_size;
 }
 
 struct nvme_ctrl_ops {
-- 
2.45.2

Re: [PATCH] nvme: check PI size if metadata size or below

[Keith Busch]

On Wed, Oct 23, 2024 at 01:34:27AM +0900, Tokunori Ikegami wrote:
> -	if ((io.control & NVME_RW_PRINFO_PRACT) &&
> -	    (ns->head->ms == ns->head->pi_size)) {
> +	if (io.control & NVME_RW_PRINFO_PRACT &&
> +	    ns->head->ms >= ns->head->pi_size) {
>  		/*
>  		 * Protection information is stripped/inserted by the
>  		 * controller.
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 093cb423f536..fbcb2243ba84 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -542,7 +542,7 @@ 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)
>  {
> -	return head->pi_type && head->ms == head->pi_size;
> +	return head->pi_type && head->ms >= head->pi_size;
>  }

It would have been nice if PRACT worked this way, but spec says it
doesn't. The control bit is a no-op if Metadata Size does not equal the
PI size, which is what these checks are looking for, so we can't take
this.

Re: [PATCH] nvme: check PI size if metadata size or below

[Tokunori Ikegami]

The existing function nvme_configure_metadata() code checks as same as 
below but is this checking case only okay?

static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
         struct nvme_ns_head *head, struct nvme_id_ns *id,
         struct nvme_id_ns_nvm *nvm, struct nvme_ns_info *info)
{

...

     if (head->pi_size && head->ms >= head->pi_size)
         head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;

On 2024/10/23 1:54, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 01:34:27AM +0900, Tokunori Ikegami wrote:
>> -	if ((io.control & NVME_RW_PRINFO_PRACT) &&
>> -	    (ns->head->ms == ns->head->pi_size)) {
>> +	if (io.control & NVME_RW_PRINFO_PRACT &&
>> +	    ns->head->ms >= ns->head->pi_size) {
>>   		/*
>>   		 * Protection information is stripped/inserted by the
>>   		 * controller.
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index 093cb423f536..fbcb2243ba84 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -542,7 +542,7 @@ 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)
>>   {
>> -	return head->pi_type && head->ms == head->pi_size;
>> +	return head->pi_type && head->ms >= head->pi_size;
>>   }
> It would have been nice if PRACT worked this way, but spec says it
> doesn't. The control bit is a no-op if Metadata Size does not equal the
> PI size, which is what these checks are looking for, so we can't take
> this.

Re: [PATCH] nvme: check PI size if metadata size or below

[Keith Busch]

On Wed, Oct 23, 2024 at 02:08:52AM +0900, Tokunori Ikegami wrote:
> The existing function nvme_configure_metadata() code checks as same as below
> but is this checking case only okay?
> 
> static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
>         struct nvme_ns_head *head, struct nvme_id_ns *id,
>         struct nvme_id_ns_nvm *nvm, struct nvme_ns_info *info)
> {
> 
> ...
> 
>     if (head->pi_size && head->ms >= head->pi_size)
>         head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;

This is merely checking if the metadata is sufficient to do PI. Nothing
to do with usage of PRACT, which is what you're changing.

Re: [PATCH] nvme: check PI size if metadata size or below

[Tokunori Ikegami]

I see. Thanks for your explanation. To make sure let me confirm again below.
The nvme_ns_has_pi changed does not use PRACT as mentioned but still is 
it not okay to change the function only also? (I could understand as 
nvme_submit_io() uses PRACT then it does not work correctly as mentioned.)

On 2024/10/23 2:33, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 02:08:52AM +0900, Tokunori Ikegami wrote:
>> The existing function nvme_configure_metadata() code checks as same as below
>> but is this checking case only okay?
>>
>> static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
>>          struct nvme_ns_head *head, struct nvme_id_ns *id,
>>          struct nvme_id_ns_nvm *nvm, struct nvme_ns_info *info)
>> {
>>
>> ...
>>
>>      if (head->pi_size && head->ms >= head->pi_size)
>>          head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
> This is merely checking if the metadata is sufficient to do PI. Nothing
> to do with usage of PRACT, which is what you're changing.

Re: [PATCH] nvme: check PI size if metadata size or below

[Keith Busch]

On Wed, Oct 23, 2024 at 02:51:16AM +0900, Tokunori Ikegami wrote:
> I see. Thanks for your explanation. To make sure let me confirm again below.
> The nvme_ns_has_pi changed does not use PRACT as mentioned but still is it
> not okay to change the function only also? (I could understand as
> nvme_submit_io() uses PRACT then it does not work correctly as mentioned.)

The function name could be better. It's just reporting if the namespace
format supports inserting/stripping metadata, such that the host doesn't
need to provide it.

Re: [PATCH] nvme: check PI size if metadata size or below

[Tokunori Ikegami]

Thanks for your confirmation. Just sent the v3 patch to change only 
nvme_ns_has_pi(). Thank you.

On 2024/10/23 2:57, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 02:51:16AM +0900, Tokunori Ikegami wrote:
>> I see. Thanks for your explanation. To make sure let me confirm again below.
>> The nvme_ns_has_pi changed does not use PRACT as mentioned but still is it
>> not okay to change the function only also? (I could understand as
>> nvme_submit_io() uses PRACT then it does not work correctly as mentioned.)
> The function name could be better. It's just reporting if the namespace
> format supports inserting/stripping metadata, such that the host doesn't
> need to provide it.