[PATCH] nvmet: return 'namespace not ready' for disabled namespaces

[PATCH] nvmet: return 'namespace not ready' for disabled namespaces

[Hannes Reinecke]

When a namespace is disabled from configfs returning 'path error'
is quite an odd choice; the actual description for that error is:
'The command was not completed as a result of a controller internal
error that is specific to the controller processing the command.'
Which is not the case here; once disabled the namespace is disabled
for the entire subsystem, not just the controller.
So return 'namespace not ready' here to clarify the problem.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index ed2424f8a396..d9ce1326f4ab 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -444,7 +444,7 @@ u16 nvmet_req_find_ns(struct nvmet_req *req)
 	if (unlikely(!req->ns)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		if (nvmet_subsys_nsid_exists(subsys, nsid))
-			return NVME_SC_INTERNAL_PATH_ERROR;
+			return NVME_SC_NS_NOT_READY;
 		return NVME_SC_INVALID_NS | NVME_STATUS_DNR;
 	}
 
-- 
2.35.3

Re: [PATCH] nvmet: return 'namespace not ready' for disabled namespaces

[Sagi Grimberg]

On 19/08/2024 15:29, Hannes Reinecke wrote:
> When a namespace is disabled from configfs returning 'path error'
> is quite an odd choice; the actual description for that error is:
> 'The command was not completed as a result of a controller internal
> error that is specific to the controller processing the command.'
> Which is not the case here; once disabled the namespace is disabled
> for the entire subsystem, not just the controller.
> So return 'namespace not ready' here to clarify the problem.

Hannes, can you explain what this patch is solving?

The reason why this error was selected is because this is a path
error, which will trigger a host IO failover.

Re: [PATCH] nvmet: return 'namespace not ready' for disabled namespaces

[Hannes Reinecke]

On 8/19/24 15:17, Sagi Grimberg wrote:
> 
> 
> 
> On 19/08/2024 15:29, Hannes Reinecke wrote:
>> When a namespace is disabled from configfs returning 'path error'
>> is quite an odd choice; the actual description for that error is:
>> 'The command was not completed as a result of a controller internal
>> error that is specific to the controller processing the command.'
>> Which is not the case here; once disabled the namespace is disabled
>> for the entire subsystem, not just the controller.
>> So return 'namespace not ready' here to clarify the problem.
> 
> Hannes, can you explain what this patch is solving?
> 
> The reason why this error was selected is because this is a path
> error, which will trigger a host IO failover.

???

This is not a path error.

# echo 0 > 
/sys/kernel/config/nvmet/subsystems/<NQN>/namespaces/<NSID>/enable

will disable the namespace for all controllers. Not just the individual
path (as there is not individual path). Failover won't change anything.

Hence this change.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] nvmet: return 'namespace not ready' for disabled namespaces

[Sagi Grimberg]

On 19/08/2024 16:54, Hannes Reinecke wrote:
> On 8/19/24 15:17, Sagi Grimberg wrote:
>>
>>
>>
>> On 19/08/2024 15:29, Hannes Reinecke wrote:
>>> When a namespace is disabled from configfs returning 'path error'
>>> is quite an odd choice; the actual description for that error is:
>>> 'The command was not completed as a result of a controller internal
>>> error that is specific to the controller processing the command.'
>>> Which is not the case here; once disabled the namespace is disabled
>>> for the entire subsystem, not just the controller.
>>> So return 'namespace not ready' here to clarify the problem.
>>
>> Hannes, can you explain what this patch is solving?
>>
>> The reason why this error was selected is because this is a path
>> error, which will trigger a host IO failover.
>
> ???
>
> This is not a path error.

Depends on how you interpret  it.

>
> # echo 0 > 
> /sys/kernel/config/nvmet/subsystems/<NQN>/namespaces/<NSID>/enable
>
> will disable the namespace for all controllers. Not just the individual
> path (as there is not individual path). Failover won't change anything.

subsystems in nvmet based implementations are not limited to a single 
server.
The originated reporter ran a subsystem over multiple hosts with drbd as the
backend.

In this case, disabled NS does need to signal a path error, and hence 
why a path
error was chosen.

If you want to change that, please make sure that io failover works as 
expected
in this case as well.

Re: [PATCH] nvmet: return 'namespace not ready' for disabled namespaces

[Christoph Hellwig]

On Mon, Aug 19, 2024 at 05:22:18PM +0300, Sagi Grimberg wrote:
> subsystems in nvmet based implementations are not limited to a single 
> server.  The originated reporter ran a subsystem over multiple hosts
> with drbd as the backend.
>
> In this case, disabled NS does need to signal a path error, and hence why
> a path error was chosen.
>
> If you want to change that, please make sure that io failover works as 
> expected in this case as well.

Exactly.  nvmet_subsys_nsid_exists indicates the nsid is configured
in the subsystem, so NVME_SC_INTERNAL_PATH_ERROR is the right error
to return here.  For the !nvmet_subsys_nsid_exists case we already
return NVME_SC_INVALID_NS, which is the correct error if it does not
exist.  NVME_SC_NS_NOT_READY is always wrong.