[PATCH] nvme: module parameter to disable pi checks

[PATCH] nvme: module parameter to disable pi checks

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

A recent commit enables integrity checks for formats the previous kernel
versions registered with the "nop" integrity profile. This means
namespaces using that format become unreadable when upgrading the kernel
past that commit.

Introduce a module parameter to restore the "nop" integrity profile so
that storage can be readable once again. This could be a boot device, so
the setting needs to happen at module load time.

Fixes: 921e81db524d17 ("nvme: allow integrity when PI is not in first bytes")
Reported-by: David Wei <dw@davidwei.uk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ba6508455e185..67f8125df96a9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -91,6 +91,10 @@ module_param(apst_secondary_latency_tol_us, ulong, 0644);
 MODULE_PARM_DESC(apst_secondary_latency_tol_us,
 	"secondary APST latency tolerance in us");
 
+static bool disable_pi = false;
+module_param(disable_pi, bool, 0444);
+MODULE_PARM_DESC(disable_pi, "disable protection-information if used");
+
 /*
  * nvme_wq - hosts nvme related works that are not reset or delete
  * nvme_reset_wq - hosts nvme reset works
@@ -1763,6 +1767,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
 		struct queue_limits *lim, struct nvme_ns_info *info)
 {
 	struct blk_integrity *bi = &lim->integrity;
+	int pi_type = head->pi_type;
 
 	memset(bi, 0, sizeof(*bi));
 
@@ -1777,7 +1782,10 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
 	    !(head->features & NVME_NS_METADATA_SUPPORTED))
 		return nvme_ns_has_pi(head);
 
-	switch (head->pi_type) {
+	if (disable_pi)
+		pi_type = 0;
+
+	switch (pi_type) {
 	case NVME_NS_DPS_PI_TYPE3:
 		switch (head->guard_type) {
 		case NVME_NVM_NS_16B_GUARD:
-- 
2.43.5

Re: [PATCH] nvme: module parameter to disable pi checks

[Chaitanya Kulkarni]

On 10/23/24 08:50, Keith Busch wrote:
>   
> +static bool disable_pi = false;
> +module_param(disable_pi, bool, 0444);
> +MODULE_PARM_DESC(disable_pi, "disable protection-information if used");
> +
>   /*
>    * nvme_wq - hosts nvme related works that are not reset or delete
>    * nvme_reset_wq - hosts nvme reset works
> @@ -1763,6 +1767,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
>   		struct queue_limits *lim, struct nvme_ns_info *info)
>   {
>   	struct blk_integrity *bi = &lim->integrity;
> +	int pi_type = head->pi_type;
>   
>   	memset(bi, 0, sizeof(*bi));
>   
> @@ -1777,7 +1782,10 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
>   	    !(head->features & NVME_NS_METADATA_SUPPORTED))
>   		return nvme_ns_has_pi(head);
>   
> -	switch (head->pi_type) {
> +	if (disable_pi)
> +		pi_type = 0;
> +
> +	switch (pi_type) {
>   	case NVME_NS_DPS_PI_TYPE3:
>   		switch (head->guard_type) {
>   		case NVME_NVM_NS_16B_GUARD:

may be we can get rid of the local var pi_type ? totally untested [1].

Irrespective of that :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1]

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2f3b3911bce4..d587f541937e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1758,7 +1758,6 @@ static bool nvme_init_integrity(struct 
nvme_ns_head *head,
                 struct queue_limits *lim, struct nvme_ns_info *info)
  {
         struct blk_integrity *bi = &lim->integrity;
-       int pi_type = head->pi_type;

         memset(bi, 0, sizeof(*bi));

@@ -1774,9 +1773,9 @@ static bool nvme_init_integrity(struct 
nvme_ns_head *head,
                 return nvme_ns_has_pi(head);

         if (disable_pi)
-               pi_type = 0;
+               goto out;

-       switch (pi_type) {
+       switch (head->pi_type) {
         case NVME_NS_DPS_PI_TYPE3:
                 switch (head->guard_type) {
                 case NVME_NVM_NS_16B_GUARD:
@@ -1816,6 +1815,7 @@ static bool nvme_init_integrity(struct 
nvme_ns_head *head,
                 break;
         }

+out:
         bi->tuple_size = head->ms;
         bi->pi_offset = info->pi_offset;
         return true;

Re: [PATCH] nvme: module parameter to disable pi checks

[Keith Busch]

On Wed, Oct 23, 2024 at 08:01:43PM +0000, Chaitanya Kulkarni wrote:
> @@ -1774,9 +1773,9 @@ static bool nvme_init_integrity(struct 
> nvme_ns_head *head,
>                  return nvme_ns_has_pi(head);
> 
>          if (disable_pi)
> -               pi_type = 0;
> +               goto out;

I'm not against 'goto' in principle, but I try to restrict it to
unwinding an error, and this isn't an error. I know, personal preference
and all...

Re: [PATCH] nvme: module parameter to disable pi checks

[David Wei]

On 2024-10-23 13:21, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 08:01:43PM +0000, Chaitanya Kulkarni wrote:
>> @@ -1774,9 +1773,9 @@ static bool nvme_init_integrity(struct 
>> nvme_ns_head *head,
>>                  return nvme_ns_has_pi(head);
>>
>>          if (disable_pi)
>> -               pi_type = 0;
>> +               goto out;
> 
> I'm not against 'goto' in principle, but I try to restrict it to
> unwinding an error, and this isn't an error. I know, personal preference
> and all...

Agreed. This is more unreadable and a modern compiler will most likely
do the right thing anyway.

Re: [PATCH] nvme: module parameter to disable pi checks

[Tokunori Ikegami]

Seems can be changed as below also.

tokunori@tokunori-desktop:~/linux$ git diff
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 5a7f7ce2bc49..59bb656a1d5e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1768,6 +1768,12 @@ static bool nvme_init_integrity(struct 
nvme_ns_head *head,
             !(head->features & NVME_NS_METADATA_SUPPORTED))
                 return nvme_ns_supports_pract(head);

+       bi->tuple_size = head->ms;
+       bi->pi_offset = info->pi_offset;
+
+       if (disable_pi)
+               return true;
+
         switch (head->pi_type) {
         case NVME_NS_DPS_PI_TYPE3:
                 switch (head->guard_type) {
@@ -1808,8 +1814,6 @@ static bool nvme_init_integrity(struct 
nvme_ns_head *head,
                 break;
         }

-       bi->tuple_size = head->ms;
-       bi->pi_offset = info->pi_offset;
         return true;
  }

On 2024/10/24 6:21, David Wei wrote:
> On 2024-10-23 13:21, Keith Busch wrote:
>> On Wed, Oct 23, 2024 at 08:01:43PM +0000, Chaitanya Kulkarni wrote:
>>> @@ -1774,9 +1773,9 @@ static bool nvme_init_integrity(struct
>>> nvme_ns_head *head,
>>>                   return nvme_ns_has_pi(head);
>>>
>>>           if (disable_pi)
>>> -               pi_type = 0;
>>> +               goto out;
>> I'm not against 'goto' in principle, but I try to restrict it to
>> unwinding an error, and this isn't an error. I know, personal preference
>> and all...
> Agreed. This is more unreadable and a modern compiler will most likely
> do the right thing anyway.
>
>

Re: [PATCH] nvme: module parameter to disable pi checks

[Christoph Hellwig]

On Wed, Oct 23, 2024 at 08:50:48AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> A recent commit enables integrity checks for formats the previous kernel
> versions registered with the "nop" integrity profile. This means
> namespaces using that format become unreadable when upgrading the kernel
> past that commit.
> 
> Introduce a module parameter to restore the "nop" integrity profile so
> that storage can be readable once again. This could be a boot device, so
> the setting needs to happen at module load time.

Not exactly a fan of module option, especially with such broad scope.
This will actually disable PI entirely as far as I can tell and not just
for the PI at the end of metadata formats.

Can we at least restrict it to those formats?  And maybe add a few
comments in the code why we have all this.

Re: [PATCH] nvme: module parameter to disable pi checks

[Kanchan Joshi]

On 10/23/2024 9:20 PM, Keith Busch wrote:
> @@ -1763,6 +1767,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
>   		struct queue_limits *lim, struct nvme_ns_info *info)
>   {
>   	struct blk_integrity *bi = &lim->integrity;
> +	int pi_type = head->pi_type;
>   
>   	memset(bi, 0, sizeof(*bi));
>   
> @@ -1777,7 +1782,10 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
>   	    !(head->features & NVME_NS_METADATA_SUPPORTED))
>   		return nvme_ns_has_pi(head);
>   
> -	switch (head->pi_type) {
> +	if (disable_pi)
> +		pi_type = 0;

I think this will not give the desired outcome. Since, regardless of the 
nop profile, driver continues to send PRCHK_GUARD/REF to the device 
during nvme_setup_rw. And you will continue to see read-failures.

Instead, you probably want this patch [1]?

Also in current boolean form, "disable_pi" will do its thing on all 
connected nvme devices. Should we make this fine granular. Like this 
patch [2]?


[1]
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d..c93557621bcb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -91,6 +91,10 @@ module_param(apst_secondary_latency_tol_us, ulong, 0644);
  MODULE_PARM_DESC(apst_secondary_latency_tol_us,
         "secondary APST latency tolerance in us");

+static bool disable_pi = false;
+module_param(disable_pi, bool, 0444);
+MODULE_PARM_DESC(disable_pi, "disable protection-information if used");
+
  /*
   * nvme_wq - hosts nvme related works that are not reset or delete
   * nvme_reset_wq - hosts nvme reset works
@@ -1911,7 +1915,7 @@ static void nvme_configure_metadata(struct 
nvme_ctrl *ctrl,
                 head->guard_type = NVME_NVM_NS_16B_GUARD;
         }

-       if (head->pi_size && head->ms >= head->pi_size)
+       if (!disable_pi && head->pi_size && head->ms >= head->pi_size)
                 head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
         if (!(id->dps & NVME_NS_DPS_PI_FIRST))
                 info->pi_offset = head->ms - head->pi_size;

[2]
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d..0876acadb81d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -91,6 +91,20 @@ module_param(apst_secondary_latency_tol_us, ulong, 0644);
  MODULE_PARM_DESC(apst_secondary_latency_tol_us,
         "secondary APST latency tolerance in us");

+static u8 disable_pi;
+module_param(disable_pi, byte, 0444);
+MODULE_PARM_DESC(disable_pi, "disable protection-information if used");
+
+/*
+ * disable_pi = 0, do nothing.
+ * disable_pi = 1, disables pi only if it is in the last bytes.
+ * disable_pi > 1, always disables pi.
+ */
+enum {
+       NVME_DISABLE_PI_NONE,
+       NVME_DISABLE_PI_LAST_BYTE,
+};
+
  /*
   * nvme_wq - hosts nvme related works that are not reset or delete
   * nvme_reset_wq - hosts nvme reset works
@@ -1897,6 +1911,8 @@ 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)
  {
+       bool last = !(id->dps & NVME_NS_DPS_PI_FIRST);
+
         head->features &= ~(NVME_NS_METADATA_SUPPORTED | NVME_NS_EXT_LBAS);
         head->pi_type = 0;
         head->pi_size = 0;
@@ -1913,9 +1929,20 @@ static void nvme_configure_metadata(struct 
nvme_ctrl *ctrl,

         if (head->pi_size && head->ms >= head->pi_size)
                 head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
-       if (!(id->dps & NVME_NS_DPS_PI_FIRST))
+       if (last)
                 info->pi_offset = head->ms - head->pi_size;

+       switch (disable_pi) {
+       case NVME_DISABLE_PI_NONE:
+               break;
+       case NVME_DISABLE_PI_LAST_BYTE:
+               if (last)
+                       head->pi_type = 0;
+               break;
+       default:
+               head->pi_type = 0;
+       }
+
         if (ctrl->ops->flags & NVME_F_FABRICS) {
                 /*
                  * The NVMe over Fabrics specification only supports 
metadata as

Re: [PATCH] nvme: module parameter to disable pi checks

[Keith Busch]

On Thu, Oct 24, 2024 at 11:40:11AM +0530, Kanchan Joshi wrote:
> On 10/23/2024 9:20 PM, Keith Busch wrote:
> > @@ -1763,6 +1767,7 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
> >   		struct queue_limits *lim, struct nvme_ns_info *info)
> >   {
> >   	struct blk_integrity *bi = &lim->integrity;
> > +	int pi_type = head->pi_type;
> >   
> >   	memset(bi, 0, sizeof(*bi));
> >   
> > @@ -1777,7 +1782,10 @@ static bool nvme_init_integrity(struct nvme_ns_head *head,
> >   	    !(head->features & NVME_NS_METADATA_SUPPORTED))
> >   		return nvme_ns_has_pi(head);
> >   
> > -	switch (head->pi_type) {
> > +	if (disable_pi)
> > +		pi_type = 0;
> 
> I think this will not give the desired outcome. Since, regardless of the 
> nop profile, driver continues to send PRCHK_GUARD/REF to the device 
> during nvme_setup_rw. And you will continue to see read-failures.

Oh, I tested this and I thought it was working, but that was only
because it was a freshly formated drive. Yeah, I see what you're saying.
I'll fix it up.

> Also in current boolean form, "disable_pi" will do its thing on all 
> connected nvme devices. Should we make this fine granular. Like this 
> patch [2]?

Let's just make it specific to pi offsets. That's the only format that
I'm hearing problems with, so no need to make this a solution to a
problem no one has.

Re: [PATCH] nvme: module parameter to disable pi checks

[Keith Busch]

On Thu, Oct 24, 2024 at 07:02:39AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 23, 2024 at 08:50:48AM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > A recent commit enables integrity checks for formats the previous kernel
> > versions registered with the "nop" integrity profile. This means
> > namespaces using that format become unreadable when upgrading the kernel
> > past that commit.
> > 
> > Introduce a module parameter to restore the "nop" integrity profile so
> > that storage can be readable once again. This could be a boot device, so
> > the setting needs to happen at module load time.
> 
> Not exactly a fan of module option, especially with such broad scope.
> This will actually disable PI entirely as far as I can tell and not just
> for the PI at the end of metadata formats.

I don't like using module parameters for either, but I think I'm stuck
with no better option here.
 
> Can we at least restrict it to those formats?  And maybe add a few
> comments in the code why we have all this.

Yes, that's a good idea. Come to think of it, if you had a mix of
formats, some with pi at the beginning others at the end, the
"beginning" formats that were previously working would get incorrect PI
on subsequent writes.