[PATCH v2 0/2] nvmet: fix FID 82h status codes for invalid NSIDs

[PATCH v2 0/2] nvmet: fix FID 82h status codes for invalid NSIDs

[Guixin Liu]

The Reservation Notification Mask feature (FID 82h) defines its own
status code for NSIDs that do not name a namespace supporting
reservations:

  "A Get Features command that uses a namespace ID of FFFFFFFFh
  shall be aborted with status code of Invalid Field in Command. [...]
  If a Set Features command or a Get Features command attempts to
  access the Reservation Notification Mask on a namespace that does
  not support reservations or is invalid, then that command is
  aborted with status code of Invalid Field in Command."

  -- NVMe Base Specification, section 5.2.26.1.33

nvmet routes the namespace lookup of both handlers through
nvmet_req_find_ns(), which returns Invalid Namespace or Format for an
unknown NSID.  That is the correct status for the generic
namespace-scoped admin commands, but not for this feature.  A Set
Features 82h naming a nonexistent NSID, a Get Features 82h naming a
nonexistent NSID, and a Get Features 82h using the broadcast NSID are
all aborted with Invalid Namespace or Format instead of Invalid Field
in Command.

Each patch fixes one handler and translates the lookup failure to
Invalid Field in Command.  The Set Features broadcast, the
namespace-without-reservations case, the disabled namespace path
(Internal Path Error, commit 505363957fad ("nvmet: fix nvme status
code when namespace is disabled")) and everything else keep their
current behavior.

Verified with two hosts (distinct host identifiers) connected over
NVMe/TCP to an nvmet subsystem exporting two reservation-enabled
null_blk namespaces: all three rejected cases now return 0x6002,
while Set/Get Features on an active namespace, the Set Features
broadcast and the reservation commands behave as before, with no
splat in dmesg.

Changes since v1:

 - drop the comments above the status translation.(Christoph Hellwig)
 - return Invalid Field in Command directly instead of assigning it
   to status first, which also removes the braces and the overly long
   (Christoph Hellwig)

Guixin Liu (2):
  nvmet: return Invalid Field for invalid NSIDs in Set Features 82h
  nvmet: return Invalid Field for invalid NSIDs in Get Features 82h

 drivers/nvme/target/pr.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.43.7

[PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Guixin Liu]

The Reservation Notification Mask feature (FID 82h) defines its own
status code for namespaces that do not support reservations:

  "If a Set Features command or a Get Features command attempts to
  access the Reservation Notification Mask on a namespace that does
  not support reservations or is invalid, then that command is
  aborted with status code of Invalid Field in Command."

  -- NVMe Base Specification, section 5.2.26.1.33

The Set Features handler routes the namespace lookup through
nvmet_req_find_ns(), which returns Invalid Namespace or Format for an
unknown NSID.  That is the correct status for the generic
namespace-scoped admin commands, but not for this feature: a Set
Features 82h naming a nonexistent NSID is aborted with Invalid
Namespace or Format instead of Invalid Field in Command.

Translate the lookup failure to Invalid Field in Command.  The
broadcast NSID, the namespace-without-reservations case and everything
else keep their current behavior.

Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/pr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index 0948a690a1c0..a36c732883d4 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -51,6 +51,8 @@ u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask)
 
 	if (nsid != U32_MAX) {
 		status = nvmet_req_find_ns(req);
+		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
+			return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 		if (status)
 			return status;
 		if (!req->ns->pr.enable)
-- 
2.43.7

[PATCH v2 2/2] nvmet: return Invalid Field for invalid NSIDs in Get Features 82h

[Guixin Liu]

The Reservation Notification Mask feature (FID 82h) rejects the same
NSIDs for Get Features as for Set Features, plus the broadcast NSID:

  "A Get Features command that uses a namespace ID of FFFFFFFFh
  shall be aborted with status code of Invalid Field in Command.
  If a Set Features command or a Get Features command attempts to
  access the Reservation Notification Mask on a namespace that does
  not support reservations or is invalid, then that command is
  aborted with status code of Invalid Field in Command."

  -- NVMe Base Specification, section 5.2.26.1.33

The Get Features handler routes the namespace lookup through
nvmet_req_find_ns(), which returns Invalid Namespace or Format for an
unknown NSID.  Both rejected cases land there: a nonexistent NSID
fails the lookup, and so does the broadcast NSID, which never names a
namespace.  Both are aborted with Invalid Namespace or Format instead
of Invalid Field in Command.

Translate the lookup failure to Invalid Field in Command, as was done
for Set Features.

Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/pr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index a36c732883d4..62ef8e85f7e9 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -77,6 +77,8 @@ u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req)
 	u16 status;
 
 	status = nvmet_req_find_ns(req);
+	if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
+		return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 	if (status)
 		return status;
 
-- 
2.43.7

Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Nilay Shroff]

On 9/20/26 9:04 AM, Guixin Liu wrote:
> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
> index 0948a690a1c0..a36c732883d4 100644
> --- a/drivers/nvme/target/pr.c
> +++ b/drivers/nvme/target/pr.c
> @@ -51,6 +51,8 @@ u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask)
>   
>   	if (nsid != U32_MAX) {
>   		status = nvmet_req_find_ns(req);
> +		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
> +			return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>   		if (status)
>   			return status;
>   		if (!req->ns->pr.enable)
> -- 

This looks good to me. However, to avoid hand-coding the same error
return code multiple times in the function and to make it a bit more
readable, how about modifying it as follows?

u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req)
{
	u16 status;
	u16 err = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;

	status = nvmet_req_find_ns(req);
	if (status) {
		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
			return err;
		return status;
	}

	if (!req->ns->pr.enable)
		return err;

	nvmet_set_result(req, READ_ONCE(req->ns->pr.notify_mask));
	return NVME_SC_SUCCESS;
}

Thanks,
--Nilay

Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Christoph Hellwig]

On Sun, Sep 20, 2026 at 04:37:08PM +0530, Nilay Shroff wrote:
> On 9/20/26 9:04 AM, Guixin Liu wrote:
>> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
>> index 0948a690a1c0..a36c732883d4 100644
>> --- a/drivers/nvme/target/pr.c
>> +++ b/drivers/nvme/target/pr.c
>> @@ -51,6 +51,8 @@ u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask)
>>     	if (nsid != U32_MAX) {
>>   		status = nvmet_req_find_ns(req);
>> +		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
>> +			return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>>   		if (status)
>>   			return status;
>>   		if (!req->ns->pr.enable)
>> -- 
>
> This looks good to me. However, to avoid hand-coding the same error
> return code multiple times in the function and to make it a bit more
> readable, how about modifying it as follows?

I don't understand how that helps.  If we really want to refactor,
we could add a version of nvmet_req_find_ns that does it, but
that feels like a bit of overkill.

> 		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))

Still no need for the inner braces here in all versions.

Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Guixin Liu]

在 2026/9/21 14:53, Christoph Hellwig 写道:
> On Sun, Sep 20, 2026 at 04:37:08PM +0530, Nilay Shroff wrote:
>> On 9/20/26 9:04 AM, Guixin Liu wrote:
>>> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
>>> index 0948a690a1c0..a36c732883d4 100644
>>> --- a/drivers/nvme/target/pr.c
>>> +++ b/drivers/nvme/target/pr.c
>>> @@ -51,6 +51,8 @@ u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask)
>>>     	if (nsid != U32_MAX) {
>>>   		status = nvmet_req_find_ns(req);
>>> +		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
>>> +			return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>>>   		if (status)
>>>   			return status;
>>>   		if (!req->ns->pr.enable)
>>> -- 
>> This looks good to me. However, to avoid hand-coding the same error
>> return code multiple times in the function and to make it a bit more
>> readable, how about modifying it as follows?
> I don't understand how that helps.  If we really want to refactor,
> we could add a version of nvmet_req_find_ns that does it, but
> that feels like a bit of overkill.
Agree.
>
>> 		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
> Still no need for the inner braces here in all versions.
Well, in C, the "==" operator has higher precedence than "|",
so the inner braces are required here.

Best Regards,
Guixin Liu

Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Nilay Shroff]

On 9/21/26 12:23 PM, Christoph Hellwig wrote:
> On Sun, Sep 20, 2026 at 04:37:08PM +0530, Nilay Shroff wrote:
>> On 9/20/26 9:04 AM, Guixin Liu wrote:
>>> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
>>> index 0948a690a1c0..a36c732883d4 100644
>>> --- a/drivers/nvme/target/pr.c
>>> +++ b/drivers/nvme/target/pr.c
>>> @@ -51,6 +51,8 @@ u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask)
>>>      	if (nsid != U32_MAX) {
>>>    		status = nvmet_req_find_ns(req);
>>> +		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
>>> +			return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>>>    		if (status)
>>>    			return status;
>>>    		if (!req->ns->pr.enable)
>>> -- 
>>
>> This looks good to me. However, to avoid hand-coding the same error
>> return code multiple times in the function and to make it a bit more
>> readable, how about modifying it as follows?
> 
> I don't understand how that helps.  If we really want to refactor,
> we could add a version of nvmet_req_find_ns that does it, but
> that feels like a bit of overkill.
> 
I agree that adding a separate helper just for this would be unnecessary.

My suggestion was mainly for readability and to avoid repeating the same
NVME_SC_INVALID_FIELD | NVME_STATUS_DNR error code and keep the error
handling in one place. I also found the two status checks a little
less straightforward to read. When applying the proposed patch, the
resulting code looks like below:

u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req)
{
	u16 status;

	status = nvmet_req_find_ns(req);
	if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
		return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
	if (status)
		return status;

	if (!req->ns->pr.enable)
		return NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;

	nvmet_set_result(req, READ_ONCE(req->ns->pr.notify_mask));
	return status;
}

That said, this is mostly a matter of personal preference rather than
a functional concern, so I'm fine with keeping the current version if
everyone agrees.

>> 		if (status == (NVME_SC_INVALID_NS | NVME_STATUS_DNR))
> 
> Still no need for the inner braces here in all versions.
> 
I think Guixin has already replied to this comment in another
email. The inner braces are required here otherwise compiler
would complain:

drivers/nvme/target/pr.c:80:35: error: | has lower precedence than ==; == will be evaluated first [-Werror,-Wparentheses]
    80 |         if (status == NVME_SC_INVALID_NS | NVME_STATUS_DNR)
       |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^


Thanks,
--Nilay

Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] nvmet: return Invalid Field for invalid NSIDs in Get Features 82h

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/2] nvmet: return Invalid Field for invalid NSIDs in Set Features 82h

[Nilay Shroff]

On 9/20/26 9:04 AM, Guixin Liu wrote:
> The Reservation Notification Mask feature (FID 82h) defines its own
> status code for namespaces that do not support reservations:
> 
>    "If a Set Features command or a Get Features command attempts to
>    access the Reservation Notification Mask on a namespace that does
>    not support reservations or is invalid, then that command is
>    aborted with status code of Invalid Field in Command."
> 
>    -- NVMe Base Specification, section 5.2.26.1.33
> 
> The Set Features handler routes the namespace lookup through
> nvmet_req_find_ns(), which returns Invalid Namespace or Format for an
> unknown NSID.  That is the correct status for the generic
> namespace-scoped admin commands, but not for this feature: a Set
> Features 82h naming a nonexistent NSID is aborted with Invalid
> Namespace or Format instead of Invalid Field in Command.
> 
> Translate the lookup failure to Invalid Field in Command.  The
> broadcast NSID, the namespace-without-reservations case and everything
> else keep their current behavior.
> 
> Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH v2 2/2] nvmet: return Invalid Field for invalid NSIDs in Get Features 82h

[Nilay Shroff]

On 9/20/26 9:05 AM, Guixin Liu wrote:
> The Reservation Notification Mask feature (FID 82h) rejects the same
> NSIDs for Get Features as for Set Features, plus the broadcast NSID:
> 
>    "A Get Features command that uses a namespace ID of FFFFFFFFh
>    shall be aborted with status code of Invalid Field in Command.
>    If a Set Features command or a Get Features command attempts to
>    access the Reservation Notification Mask on a namespace that does
>    not support reservations or is invalid, then that command is
>    aborted with status code of Invalid Field in Command."
> 
>    -- NVMe Base Specification, section 5.2.26.1.33
> 
> The Get Features handler routes the namespace lookup through
> nvmet_req_find_ns(), which returns Invalid Namespace or Format for an
> unknown NSID.  Both rejected cases land there: a nonexistent NSID
> fails the lookup, and so does the broadcast NSID, which never names a
> namespace.  Both are aborted with Invalid Namespace or Format instead
> of Invalid Field in Command.
> 
> Translate the lookup failure to Invalid Field in Command, as was done
> for Set Features.
> 
> Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>


Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>

Re: [PATCH v2 0/2] nvmet: fix FID 82h status codes for invalid NSIDs

[Keith Busch]

On Sun, Sep 20, 2026 at 11:34:58AM +0800, Guixin Liu wrote:
> The Reservation Notification Mask feature (FID 82h) defines its own
> status code for NSIDs that do not name a namespace supporting
> reservations:
> 
>   "A Get Features command that uses a namespace ID of FFFFFFFFh
>   shall be aborted with status code of Invalid Field in Command. [...]
>   If a Set Features command or a Get Features command attempts to
>   access the Reservation Notification Mask on a namespace that does
>   not support reservations or is invalid, then that command is
>   aborted with status code of Invalid Field in Command."
> 
>   -- NVMe Base Specification, section 5.2.26.1.33
> 
> nvmet routes the namespace lookup of both handlers through
> nvmet_req_find_ns(), which returns Invalid Namespace or Format for an
> unknown NSID.  That is the correct status for the generic
> namespace-scoped admin commands, but not for this feature.  A Set
> Features 82h naming a nonexistent NSID, a Get Features 82h naming a
> nonexistent NSID, and a Get Features 82h using the broadcast NSID are
> all aborted with Invalid Namespace or Format instead of Invalid Field
> in Command.
> 
> Each patch fixes one handler and translates the lookup failure to
> Invalid Field in Command.  The Set Features broadcast, the
> namespace-without-reservations case, the disabled namespace path
> (Internal Path Error, commit 505363957fad ("nvmet: fix nvme status
> code when namespace is disabled")) and everything else keep their
> current behavior.

Thanks, applied to nvme-7.3.