[PATCH 0/5] *** bugfixes and code refactoring for nvme target ***

[PATCH 0/5] *** bugfixes and code refactoring for nvme target ***

[Guixin Liu]

This series of patches includes bugfixes and code refactoring.

Guixin Liu (5):
  nvmet: fix wrong error handling approach in nvmet_install_queue
  nvmet: allow identical cntlid_min and cntlid_max settings
  nvmet: remove cntlid_min and cntlid_max check in nvmet_alloc_ctrl
  nvmet: relocate the cntlid_ida to the internal of subsystem
  nvmet: return invalid filed when check fctype fails

 drivers/nvme/target/configfs.c    |  4 ++--
 drivers/nvme/target/core.c        | 11 ++++-------
 drivers/nvme/target/fabrics-cmd.c |  5 ++---
 drivers/nvme/target/nvmet.h       |  2 ++
 4 files changed, 10 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] nvmet: fix wrong error handling approach in nvmet_install_queue

[Guixin Liu]

In the nvmet_install_queue() function, do not set the ctrl pointer
to NULL if the sqsize check fails.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/fabrics-cmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index d8da840..9cffb64 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -147,8 +147,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 		pr_warn("queue size zero!\n");
 		req->error_loc = offsetof(struct nvmf_connect_command, sqsize);
 		req->cqe->result.u32 = IPO_IATTR_CONNECT_SQE(sqsize);
-		ret = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
-		goto err;
+		return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 	}
 
 	if (ctrl->sqs[qid] != NULL) {
-- 
1.8.3.1

Re: [PATCH 1/5] nvmet: fix wrong error handling approach in nvmet_install_queue

[Christoph Hellwig]

On Wed, Dec 13, 2023 at 02:32:48PM +0800, Guixin Liu wrote:
> In the nvmet_install_queue() function, do not set the ctrl pointer
> to NULL if the sqsize check fails.

Can you explain why it is wrong, and why not doing it is desirable?

Re: [PATCH 1/5] nvmet: fix wrong error handling approach in nvmet_install_queue

[Guixin Liu]

在 2023/12/13 16:43, Christoph Hellwig 写道:
> On Wed, Dec 13, 2023 at 02:32:48PM +0800, Guixin Liu wrote:
>> In the nvmet_install_queue() function, do not set the ctrl pointer
>> to NULL if the sqsize check fails.
> Can you explain why it is wrong, and why not doing it is desirable?

If a queue which is already connected, then receive a connect cmd with 
sqsize=0, it will set the

req->sq->ctrl to NULL and break the queue.

Re: [PATCH 1/5] nvmet: fix wrong error handling approach in nvmet_install_queue

[Sagi Grimberg]

>> On Wed, Dec 13, 2023 at 02:32:48PM +0800, Guixin Liu wrote:
>>> In the nvmet_install_queue() function, do not set the ctrl pointer
>>> to NULL if the sqsize check fails.
>> Can you explain why it is wrong, and why not doing it is desirable?
> 
> If a queue which is already connected, then receive a connect cmd with 
> sqsize=0, it will set the
> 
> req->sq->ctrl to NULL and break the queue.

It is not clear what is the issue here. Is there a dereference later on
in sq->ctrl? It belongs in the change log.

[PATCH 2/5] nvmet: allow identical cntlid_min and cntlid_max settings

[Guixin Liu]

When the user wants to restrict to only creating one controller,
they can set cntlid_min and cntlid_max to the same value.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/configfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index d937fe0..2482a0d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1276,7 +1276,7 @@ static ssize_t nvmet_subsys_attr_cntlid_min_store(struct config_item *item,
 		return -EINVAL;
 
 	down_write(&nvmet_config_sem);
-	if (cntlid_min >= to_subsys(item)->cntlid_max)
+	if (cntlid_min > to_subsys(item)->cntlid_max)
 		goto out_unlock;
 	to_subsys(item)->cntlid_min = cntlid_min;
 	up_write(&nvmet_config_sem);
@@ -1306,7 +1306,7 @@ static ssize_t nvmet_subsys_attr_cntlid_max_store(struct config_item *item,
 		return -EINVAL;
 
 	down_write(&nvmet_config_sem);
-	if (cntlid_max <= to_subsys(item)->cntlid_min)
+	if (cntlid_max < to_subsys(item)->cntlid_min)
 		goto out_unlock;
 	to_subsys(item)->cntlid_max = cntlid_max;
 	up_write(&nvmet_config_sem);
-- 
1.8.3.1

Re: [PATCH 2/5] nvmet: allow identical cntlid_min and cntlid_max settings

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 2/5] nvmet: allow identical cntlid_min and cntlid_max settings

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 3/5] nvmet: remove cntlid_min and cntlid_max check in nvmet_alloc_ctrl

[Guixin Liu]

The cntlid_min and cntlid_max are checked in configfs, don't check
again in nvmet_alloc_ctrl().

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 3935165..d26aa30 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1425,9 +1425,6 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	if (!ctrl->sqs)
 		goto out_free_changed_ns_list;
 
-	if (subsys->cntlid_min > subsys->cntlid_max)
-		goto out_free_sqs;
-
 	ret = ida_alloc_range(&cntlid_ida,
 			     subsys->cntlid_min, subsys->cntlid_max,
 			     GFP_KERNEL);
-- 
1.8.3.1

Re: [PATCH 3/5] nvmet: remove cntlid_min and cntlid_max check in nvmet_alloc_ctrl

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 3/5] nvmet: remove cntlid_min and cntlid_max check in nvmet_alloc_ctrl

[Sagi Grimberg]

I think it can be folded into the last patch.
But anyways,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 4/5] nvmet: relocate the cntlid_ida to the internal of subsystem

[Guixin Liu]

As per the NVMe specification, the cntlid must be unique within an NVM
subsystem. Therefore, the cntlid_ida should be moved internally within
the subsystem.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/core.c  | 8 ++++----
 drivers/nvme/target/nvmet.h | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index d26aa30..c4cff43 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -21,7 +21,6 @@
 struct workqueue_struct *buffered_io_wq;
 struct workqueue_struct *zbd_wq;
 static const struct nvmet_fabrics_ops *nvmet_transports[NVMF_TRTYPE_MAX];
-static DEFINE_IDA(cntlid_ida);
 
 struct workqueue_struct *nvmet_wq;
 EXPORT_SYMBOL_GPL(nvmet_wq);
@@ -1425,7 +1424,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	if (!ctrl->sqs)
 		goto out_free_changed_ns_list;
 
-	ret = ida_alloc_range(&cntlid_ida,
+	ret = ida_alloc_range(&subsys->cntlid_ida,
 			     subsys->cntlid_min, subsys->cntlid_max,
 			     GFP_KERNEL);
 	if (ret < 0) {
@@ -1486,7 +1485,7 @@ static void nvmet_ctrl_free(struct kref *ref)
 
 	nvmet_destroy_auth(ctrl);
 
-	ida_free(&cntlid_ida, ctrl->cntlid);
+	ida_free(&subsys->cntlid_ida, ctrl->cntlid);
 
 	nvmet_async_events_free(ctrl);
 	kfree(ctrl->sqs);
@@ -1598,6 +1597,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	xa_init(&subsys->namespaces);
 	INIT_LIST_HEAD(&subsys->ctrls);
 	INIT_LIST_HEAD(&subsys->hosts);
+	ida_init(&subsys->cntlid_ida);
 
 	return subsys;
 
@@ -1619,6 +1619,7 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	xa_destroy(&subsys->namespaces);
 	nvmet_passthru_subsys_free(subsys);
+	ida_destroy(&subsys->cntlid_ida);
 
 	kfree(subsys->subsysnqn);
 	kfree(subsys->model_number);
@@ -1692,7 +1693,6 @@ static void __exit nvmet_exit(void)
 {
 	nvmet_exit_configfs();
 	nvmet_exit_discovery();
-	ida_destroy(&cntlid_ida);
 	destroy_workqueue(nvmet_wq);
 	destroy_workqueue(buffered_io_wq);
 	destroy_workqueue(zbd_wq);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6c8aceb..af27243 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -291,6 +291,8 @@ struct nvmet_subsys {
 #ifdef CONFIG_BLK_DEV_ZONED
 	u8			zasl;
 #endif /* CONFIG_BLK_DEV_ZONED */
+
+	struct ida		cntlid_ida;
 };
 
 static inline struct nvmet_subsys *to_subsys(struct config_item *item)
-- 
1.8.3.1

Re: [PATCH 4/5] nvmet: relocate the cntlid_ida to the internal of subsystem

[Christoph Hellwig]

On Wed, Dec 13, 2023 at 02:32:51PM +0800, Guixin Liu wrote:
> As per the NVMe specification, the cntlid must be unique within an NVM
> subsystem. Therefore, the cntlid_ida should be moved internally within
> the subsystem.

Sagi added this with a commit log of:

"
nvmet: Make cntlid globally unique
    
We usually log the cntlid which is confusing in case
we have multiple subsystems each with it's own cntlid ida.
Instead make cntlid ida globally unique and log the initial
association.
"

which seems reasonable.  If you have a reason for not making it unique,
it needs to be stated here so we can weight it against the original
reason for making the ids globally unique.

Re: [PATCH 4/5] nvmet: relocate the cntlid_ida to the internal of subsystem

[Guixin Liu]

在 2023/12/13 16:46, Christoph Hellwig 写道:
> On Wed, Dec 13, 2023 at 02:32:51PM +0800, Guixin Liu wrote:
>> As per the NVMe specification, the cntlid must be unique within an NVM
>> subsystem. Therefore, the cntlid_ida should be moved internally within
>> the subsystem.
> Sagi added this with a commit log of:
>
> "
> nvmet: Make cntlid globally unique
>      
> We usually log the cntlid which is confusing in case
> we have multiple subsystems each with it's own cntlid ida.
> Instead make cntlid ida globally unique and log the initial
> association.
> "
>
> which seems reasonable.  If you have a reason for not making it unique,
> it needs to be stated here so we can weight it against the original
> reason for making the ids globally unique.

Emm, I just change this per the NVMe spec, Sagi's comment is reasonable.

Re: [PATCH 4/5] nvmet: relocate the cntlid_ida to the internal of subsystem

[Christoph Hellwig]

On Wed, Dec 13, 2023 at 08:59:26PM +0800, Guixin Liu wrote:
>> nvmet: Make cntlid globally unique
>>      We usually log the cntlid which is confusing in case
>> we have multiple subsystems each with it's own cntlid ida.
>> Instead make cntlid ida globally unique and log the initial
>> association.
>> "
>>
>> which seems reasonable.  If you have a reason for not making it unique,
>> it needs to be stated here so we can weight it against the original
>> reason for making the ids globally unique.
>
> Emm, I just change this per the NVMe spec, Sagi's comment is reasonable.

It's probably worth moving the above into a comment to explain what we're
doing here..

[PATCH 5/5] nvmet: return invalid filed when check fctype fails

[Guixin Liu]

Return invalid filed status code when check fctype fails in
nvmet_parse_connect_cmd().

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/fabrics-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 9cffb64..d6e6db2 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -361,7 +361,7 @@ u16 nvmet_parse_connect_cmd(struct nvmet_req *req)
 		pr_debug("invalid capsule type 0x%x on unconnected queue.\n",
 			cmd->fabrics.fctype);
 		req->error_loc = offsetof(struct nvmf_common_command, fctype);
-		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 	}
 
 	if (cmd->connect.qid == 0)
-- 
1.8.3.1

Re: [PATCH 5/5] nvmet: return invalid filed when check fctype fails

[Christoph Hellwig]

On Wed, Dec 13, 2023 at 02:32:52PM +0800, Guixin Liu wrote:
> Return invalid filed status code when check fctype fails in
> nvmet_parse_connect_cmd().

Hmm, the fabrics command type really is part of the opcode.  What is the
reason for you to prefer one error over the other?

Also s/filed/field/g or better 'Invalid Field in Command'

Re: [PATCH 5/5] nvmet: return invalid filed when check fctype fails

[Guixin Liu]

在 2023/12/13 16:48, Christoph Hellwig 写道:
> On Wed, Dec 13, 2023 at 02:32:52PM +0800, Guixin Liu wrote:
>> Return invalid filed status code when check fctype fails in
>> nvmet_parse_connect_cmd().
> Hmm, the fabrics command type really is part of the opcode.  What is the
> reason for you to prefer one error over the other?
>
> Also s/filed/field/g or better 'Invalid Field in Command'
Agree, by the way, should return invalid opcode in 
nvmet_rdma_map_sgl_inline() if nvme_is_write() check fails?

Re: [PATCH 5/5] nvmet: return invalid filed when check fctype fails

[Sagi Grimberg]

>>> Return invalid filed status code when check fctype fails in
>>> nvmet_parse_connect_cmd().
>> Hmm, the fabrics command type really is part of the opcode.  What is the
>> reason for you to prefer one error over the other?
>>
>> Also s/filed/field/g or better 'Invalid Field in Command'
> Agree, by the way, should return invalid opcode in 
> nvmet_rdma_map_sgl_inline() if nvme_is_write() check fails?

I think that in that case the invalid part is the sgl, not the
opcode.

Re: [PATCH 0/5] *** bugfixes and code refactoring for nvme target ***

[Keith Busch]

Reviewed patches 2 and 3 applied to nvme-6.8. Thanks!