[PATCH 0/6] nvmet: cleanup and status, error log fix

[PATCH 0/6] nvmet: cleanup and status, error log fix

[Chaitanya Kulkarni]

Hi,

This removes duplicate status value assignments, unnecessary function
parameters and fixes the error log page update.

Since the current nvme-5.12 is broken I've tested this series with the
debug diff which is listed in the test report along with the blktests.

-ck

Chaitanya Kulkarni (6):
  nvmet: remove duplicate status assignment
  nvmet: set status on actual error condition
  nvmet: check and set the right err location
  nvmet: remove unnecessary function parameters
  nvmet: remove unnecessary function parameter
  nvmet: remove unnecessary function parameters

 drivers/nvme/target/admin-cmd.c   |  2 +-
 drivers/nvme/target/core.c        | 58 +++++++++++++++----------------
 drivers/nvme/target/fabrics-cmd.c | 18 +++++-----
 drivers/nvme/target/nvmet.h       | 10 +++---
 4 files changed, 43 insertions(+), 45 deletions(-)
# ./compile_nvme.sh 
+ umount /mnt/nvme0n1
+ clear_dmesg
umount: /mnt/nvme0n1: not mounted
+ modprobe -r nvme-fabrics
+ modprobe -r nvme_loop
+ modprobe -r nvmet
+ modprobe -r nvme
+ sleep 1
+ modprobe -r nvme-core
+ lsmod
+ grep nvme
+ git diff
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d77f3f26d8d3..cedf704adac6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1167,7 +1167,8 @@ static int nvme_submit_user_cmd(struct request_queue *q,
                if (ret)
                        goto out;
                bio = req->bio;
-               bio_set_dev(bio, bdev);
+               if(bdev)
+                       bio_set_dev(bio, bdev);
                if (bdev && meta_buffer && meta_len) {
                        meta = nvme_add_user_metadata(bio, meta_buffer, meta_len,
                                        meta_seed, write);
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index cb6f86572b24..38fffee6b85e 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -688,7 +688,8 @@ static struct nvmf_transport_ops nvme_loop_transport = {
        .name           = "loop",
        .module         = THIS_MODULE,
        .create_ctrl    = nvme_loop_create_ctrl,
-       .allowed_opts   = NVMF_OPT_TRADDR,
+       .allowed_opts   = NVMF_OPT_TRADDR | NVMF_OPT_CTRL_LOSS_TMO,
+
 };
 
 static int __init nvme_loop_init_module(void)
++ nproc
+ make -j 64 M=drivers/nvme/ modules
  CC [M]  drivers/nvme//target/configfs.o
  CC [M]  drivers/nvme//target/core.o
  CC [M]  drivers/nvme//target/admin-cmd.o
  CC [M]  drivers/nvme//target/fabrics-cmd.o
  CC [M]  drivers/nvme//target/discovery.o
  CC [M]  drivers/nvme//target/io-cmd-file.o
  CC [M]  drivers/nvme//target/io-cmd-bdev.o
  CC [M]  drivers/nvme//host/core.o
  CC [M]  drivers/nvme//target/passthru.o
  CC [M]  drivers/nvme//target/trace.o
  CC [M]  drivers/nvme//host/trace.o
  CC [M]  drivers/nvme//host/lightnvm.o
  CC [M]  drivers/nvme//target/rdma.o
  CC [M]  drivers/nvme//target/loop.o
  CC [M]  drivers/nvme//host/zns.o
  CC [M]  drivers/nvme//host/hwmon.o
  CC [M]  drivers/nvme//target/fc.o
  CC [M]  drivers/nvme//target/fcloop.o
  CC [M]  drivers/nvme//host/pci.o
  CC [M]  drivers/nvme//target/tcp.o
  CC [M]  drivers/nvme//host/fabrics.o
  CC [M]  drivers/nvme//host/rdma.o
  CC [M]  drivers/nvme//host/fc.o
  CC [M]  drivers/nvme//host/tcp.o
  LD [M]  drivers/nvme//target/nvme-loop.o
  LD [M]  drivers/nvme//target/nvme-fcloop.o
  LD [M]  drivers/nvme//host/nvme-fabrics.o
  LD [M]  drivers/nvme//target/nvmet-tcp.o
  LD [M]  drivers/nvme//host/nvme.o
  LD [M]  drivers/nvme//target/nvmet-rdma.o
  LD [M]  drivers/nvme//host/nvme-tcp.o
  LD [M]  drivers/nvme//target/nvmet.o
  LD [M]  drivers/nvme//target/nvmet-fc.o
  LD [M]  drivers/nvme//host/nvme-rdma.o
  LD [M]  drivers/nvme//host/nvme-fc.o
  LD [M]  drivers/nvme//host/nvme-core.o
  MODPOST drivers/nvme//Module.symvers
  LD [M]  drivers/nvme//host/nvme-core.ko
  LD [M]  drivers/nvme//host/nvme-fabrics.ko
  LD [M]  drivers/nvme//host/nvme-fc.ko
  LD [M]  drivers/nvme//host/nvme-rdma.ko
  LD [M]  drivers/nvme//host/nvme-tcp.ko
  LD [M]  drivers/nvme//host/nvme.ko
  CC [M]  drivers/nvme//target/nvme-fcloop.mod.o
  CC [M]  drivers/nvme//target/nvme-loop.mod.o
  CC [M]  drivers/nvme//target/nvmet-fc.mod.o
  CC [M]  drivers/nvme//target/nvmet-rdma.mod.o
  CC [M]  drivers/nvme//target/nvmet-tcp.mod.o
  CC [M]  drivers/nvme//target/nvmet.mod.o
  LD [M]  drivers/nvme//target/nvmet.ko
  LD [M]  drivers/nvme//target/nvme-loop.ko
  LD [M]  drivers/nvme//target/nvmet-rdma.ko
  LD [M]  drivers/nvme//target/nvme-fcloop.ko
  LD [M]  drivers/nvme//target/nvmet-tcp.ko
  LD [M]  drivers/nvme//target/nvmet-fc.ko
+ HOST=drivers/nvme/host
+ TARGET=drivers/nvme/target
++ uname -r
+ HOST_DEST=/lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/host/
++ uname -r
+ TARGET_DEST=/lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/target/
+ cp drivers/nvme/host/nvme-core.ko drivers/nvme/host/nvme-fabrics.ko drivers/nvme/host/nvme-fc.ko drivers/nvme/host/nvme.ko drivers/nvme/host/nvme-rdma.ko drivers/nvme/host/nvme-tcp.ko /lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/host//
+ cp drivers/nvme/target/nvme-fcloop.ko drivers/nvme/target/nvme-loop.ko drivers/nvme/target/nvmet-fc.ko drivers/nvme/target/nvmet.ko drivers/nvme/target/nvmet-rdma.ko drivers/nvme/target/nvmet-tcp.ko /lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/target//
+ ls -lrth /lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/host/ /lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/target//
/lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/host/:
total 8.0M
-rw-r--r--. 1 root root 3.0M Feb 16 12:45 nvme-core.ko
-rw-r--r--. 1 root root 643K Feb 16 12:45 nvme-fabrics.ko
-rw-r--r--. 1 root root 1.2M Feb 16 12:45 nvme-fc.ko
-rw-r--r--. 1 root root 1.1M Feb 16 12:45 nvme.ko
-rw-r--r--. 1 root root 1.2M Feb 16 12:45 nvme-rdma.ko
-rw-r--r--. 1 root root 1.1M Feb 16 12:45 nvme-tcp.ko

/lib/modules/5.11.0-rc5nvme+/kernel/drivers/nvme/target//:
total 8.2M
-rw-r--r--. 1 root root 712K Feb 16 12:45 nvme-fcloop.ko
-rw-r--r--. 1 root root 619K Feb 16 12:45 nvme-loop.ko
-rw-r--r--. 1 root root 1.1M Feb 16 12:45 nvmet-fc.ko
-rw-r--r--. 1 root root 4.0M Feb 16 12:45 nvmet.ko
-rw-r--r--. 1 root root 1.1M Feb 16 12:45 nvmet-rdma.ko
-rw-r--r--. 1 root root 852K Feb 16 12:45 nvmet-tcp.ko
+ modprobe nvme
 gitlog -7 
7f0c29a24505 (HEAD -> nvme-5.12) nvmet: remove unnecessary function parameters
e8e1ea8fb333 nvmet: remove unnecessary function parameter
f169254686f6 nvmet: remove unnecessary function parameters
fc38bf1595ce nvmet: check and set the right err location
9b176efea50a nvmet: set status on actual error condition
0384837a4690 nvmet: remove duplicate status assignment
9bb31b0ab2e3 Merge branch 'nvme-5.12' of git://git.infradead.org/nvme into nvme-5.12
# cdblktests 
# ./check nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime  35.423s  ...  36.789s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.159s  ...  10.177s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.808s  ...  1.743s
nvme/005 (reset local loopback target)                       [not run]
    nvme_core module does not have parameter multipath
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.126s  ...  0.141s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.098s  ...  0.096s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.777s  ...  1.756s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.727s  ...  1.709s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  29.228s  ...  19.931s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  199.625s  ...  267.604s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  72.525s  ...  49.320s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime    ...  335.511s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  20.695s  ...  21.308s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  22.596s  ...  21.888s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  16.349s  ...  19.097s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  15.889s  ...  19.393s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.655s  ...  1.718s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.718s  ...  1.747s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.662s  ...  1.700s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.713s  ...  1.702s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  2.100s  ...  2.131s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.711s  ...  1.739s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.663s  ...  1.708s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.666s  ...  1.692s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.652s  ...  1.798s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.650s  ...  1.713s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.648s  ...  1.695s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  2.011s  ...  2.168s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.310s  ...  0.379s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  5.474s  ...  5.774s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.043s  ...  0.051s


-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/6] nvmet: remove duplicate status assignment

[Chaitanya Kulkarni]

In the function nvmet_alloc_ctrl() we assign status value before we
call nvmet_fine_get_subsys() to :-
status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;

After we successfully find the subsystem we again set the status value
to :-
status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;

Remove the duplicate status assignment value.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 67bbf0e3b507..64c08b71be2c 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1314,7 +1314,6 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		goto out;
 	}
 
-	status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 	down_read(&nvmet_config_sem);
 	if (!nvmet_host_allowed(subsys, hostnqn)) {
 		pr_info("connect by host %s for subsystem %s not allowed\n",
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/6] nvmet: set status on actual error condition

[Chaitanya Kulkarni]

In the nvmet_alloc_ctrl() set the status variable to its error value
NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR in the actual error
condition just like the rest of the code.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 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 64c08b71be2c..df2d3de0de62 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1305,12 +1305,12 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	int ret;
 	u16 status;
 
-	status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 	subsys = nvmet_find_get_subsys(req->port, subsysnqn);
 	if (!subsys) {
 		pr_warn("connect request for invalid subsystem %s!\n",
 			subsysnqn);
 		req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(subsysnqn);
+		status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 		goto out;
 	}
 
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 3/6] nvmet: check and set the right err location

[Chaitanya Kulkarni]

The function nvmet_execute_admin_connect() doesn't check for the right
error status value that is return from the nvmet_alloc_ctrl().

Check for NVME_SC_CONNECT_INVALID_PARAM & NVME_SC_CONNECT_INVALID_HOST
before we update the error log.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/fabrics-cmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 42bd12b8bf00..cf0baa911db2 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -191,9 +191,10 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
 				  le32_to_cpu(c->kato), &ctrl);
 	if (status) {
-		if (status == (NVME_SC_INVALID_FIELD | NVME_SC_DNR))
+		if (status == (NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR) ||
+		    status == (NVME_SC_CONNECT_INVALID_HOST | NVME_SC_DNR))
 			req->error_loc =
-				offsetof(struct nvme_common_command, opcode);
+				offsetof(struct nvme_common_command, dptr);
 		goto out;
 	}
 
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 4/6] nvmet: remove unnecessary function parameters

[Chaitanya Kulkarni]

The function nvmet_ctrl_find_get() accepts subsysnqn, hostnqn, cntlid,
nvmet_req, and out pointer to nvmet_ctrl structure. The parameters
subsysnqn, hostnqn and cntlid can be derived from the caller's
struct nvmf_connect_data.

Replace these parameters with structure pointer nvmf_connect_data *d.

Also, this function returns the same error value from two places. First
on failure to find subsystem from connect data and failure to find ctrl
from cntlid connect data:- NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR.

Move this to the caller to change the return type so we can return ctrl.

Now that we can change the return type instead of taking the pointer to
pointer to the nvmet_ctrl structure remove that function parameter and
return the valid nvmet_ctrl pointer on success and NULL on failure.

Also, add and rename the goto labels for more readability with comment.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/core.c        | 30 +++++++++++++++---------------
 drivers/nvme/target/fabrics-cmd.c | 10 +++++-----
 drivers/nvme/target/nvmet.h       |  4 ++--
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index df2d3de0de62..518dadc395b3 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1179,45 +1179,45 @@ static void nvmet_init_cap(struct nvmet_ctrl *ctrl)
 	ctrl->cap |= NVMET_QUEUE_SIZE - 1;
 }
 
-u16 nvmet_ctrl_find_get(const char *subsysnqn, const char *hostnqn, u16 cntlid,
-		struct nvmet_req *req, struct nvmet_ctrl **ret)
+struct nvmet_ctrl *nvmet_ctrl_find_get(struct nvmf_connect_data *d,
+				       struct nvmet_req *req)
 {
+	struct nvmet_ctrl *ctrl = NULL;
 	struct nvmet_subsys *subsys;
-	struct nvmet_ctrl *ctrl;
-	u16 status = 0;
 
-	subsys = nvmet_find_get_subsys(req->port, subsysnqn);
+	subsys = nvmet_find_get_subsys(req->port, d->subsysnqn);
 	if (!subsys) {
 		pr_warn("connect request for invalid subsystem %s!\n",
-			subsysnqn);
+			d->subsysnqn);
 		req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(subsysnqn);
-		return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
+		goto out;
 	}
 
 	mutex_lock(&subsys->lock);
 	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
-		if (ctrl->cntlid == cntlid) {
-			if (strncmp(hostnqn, ctrl->hostnqn, NVMF_NQN_SIZE)) {
+		if (ctrl->cntlid == le16_to_cpu(d->cntlid)) {
+			if (strncmp(d->hostnqn, ctrl->hostnqn, NVMF_NQN_SIZE)) {
 				pr_warn("hostnqn mismatch.\n");
 				continue;
 			}
 			if (!kref_get_unless_zero(&ctrl->ref))
 				continue;
 
-			*ret = ctrl;
-			goto out;
+			/* ctrl found */
+			goto found;
 		}
 	}
 
+	ctrl = NULL; /* ctrl not found */
 	pr_warn("could not find controller %d for subsys %s / host %s\n",
-		cntlid, subsysnqn, hostnqn);
+		le16_to_cpu(d->cntlid), d->subsysnqn, d->hostnqn);
 	req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(cntlid);
-	status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 
-out:
+found:
 	mutex_unlock(&subsys->lock);
 	nvmet_subsys_put(subsys);
-	return status;
+out:
+	return ctrl;
 }
 
 u16 nvmet_check_ctrl_status(struct nvmet_req *req, struct nvme_command *cmd)
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index cf0baa911db2..46578f98abcc 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -223,7 +223,7 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 {
 	struct nvmf_connect_command *c = &req->cmd->connect;
 	struct nvmf_connect_data *d;
-	struct nvmet_ctrl *ctrl = NULL;
+	struct nvmet_ctrl *ctrl;
 	u16 qid = le16_to_cpu(c->qid);
 	u16 status = 0;
 
@@ -250,11 +250,11 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 		goto out;
 	}
 
-	status = nvmet_ctrl_find_get(d->subsysnqn, d->hostnqn,
-				     le16_to_cpu(d->cntlid),
-				     req, &ctrl);
-	if (status)
+	ctrl = nvmet_ctrl_find_get(d, req);
+	if (!ctrl) {
+		status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 		goto out;
+	}
 
 	if (unlikely(qid > ctrl->subsys->max_qid)) {
 		pr_warn("invalid queue id (%d)\n", qid);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index cdfa537b1c0a..e8211a63eac3 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -433,8 +433,8 @@ void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl);
 void nvmet_update_cc(struct nvmet_ctrl *ctrl, u32 new);
 u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 		struct nvmet_req *req, u32 kato, struct nvmet_ctrl **ctrlp);
-u16 nvmet_ctrl_find_get(const char *subsysnqn, const char *hostnqn, u16 cntlid,
-		struct nvmet_req *req, struct nvmet_ctrl **ret);
+struct nvmet_ctrl *nvmet_ctrl_find_get(struct nvmf_connect_data *d,
+				       struct nvmet_req *req);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
 u16 nvmet_check_ctrl_status(struct nvmet_req *req, struct nvme_command *cmd);
 
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 5/6] nvmet: remove unnecessary function parameter

[Chaitanya Kulkarni]

In nvmet_check_ctrl_status() cmd can be derived from nvmet_req. Remove
the local variable cmd in the nvmet_check_ctrl_status() and function
parameter cmd for nvmet_check_ctrl_status(). Derive the cmd value from
req parameter in the nvmet_check_ctrl_status().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 drivers/nvme/target/core.c      | 9 ++++-----
 drivers/nvme/target/nvmet.h     | 2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index bc6a774f2124..e15a2191500a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -926,7 +926,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 	if (nvmet_req_subsys(req)->type == NVME_NQN_DISC)
 		return nvmet_parse_discovery_cmd(req);
 
-	ret = nvmet_check_ctrl_status(req, cmd);
+	ret = nvmet_check_ctrl_status(req);
 	if (unlikely(ret))
 		return ret;
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 518dadc395b3..5915b7cc65a6 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -864,10 +864,9 @@ static inline u16 nvmet_io_cmd_check_access(struct nvmet_req *req)
 
 static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 {
-	struct nvme_command *cmd = req->cmd;
 	u16 ret;
 
-	ret = nvmet_check_ctrl_status(req, cmd);
+	ret = nvmet_check_ctrl_status(req);
 	if (unlikely(ret))
 		return ret;
 
@@ -1220,17 +1219,17 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(struct nvmf_connect_data *d,
 	return ctrl;
 }
 
-u16 nvmet_check_ctrl_status(struct nvmet_req *req, struct nvme_command *cmd)
+u16 nvmet_check_ctrl_status(struct nvmet_req *req)
 {
 	if (unlikely(!(req->sq->ctrl->cc & NVME_CC_ENABLE))) {
 		pr_err("got cmd %d while CC.EN == 0 on qid = %d\n",
-		       cmd->common.opcode, req->sq->qid);
+		       req->cmd->common.opcode, req->sq->qid);
 		return NVME_SC_CMD_SEQ_ERROR | NVME_SC_DNR;
 	}
 
 	if (unlikely(!(req->sq->ctrl->csts & NVME_CSTS_RDY))) {
 		pr_err("got cmd %d while CSTS.RDY == 0 on qid = %d\n",
-		       cmd->common.opcode, req->sq->qid);
+		       req->cmd->common.opcode, req->sq->qid);
 		return NVME_SC_CMD_SEQ_ERROR | NVME_SC_DNR;
 	}
 	return 0;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index e8211a63eac3..2dd4a91e5e0e 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -436,7 +436,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 struct nvmet_ctrl *nvmet_ctrl_find_get(struct nvmf_connect_data *d,
 				       struct nvmet_req *req);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
-u16 nvmet_check_ctrl_status(struct nvmet_req *req, struct nvme_command *cmd);
+u16 nvmet_check_ctrl_status(struct nvmet_req *req);
 
 struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		enum nvme_subsys_type type);
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 6/6] nvmet: remove unnecessary function parameters

[Chaitanya Kulkarni]

The function nvmet_alloc_ctrl() accepts subsysnqn, hostnqn, cntlid,
nvmet_req, and out pointer to nvmet_ctrl structure. The parameters
subsysnqn, hostnqn and cntlid can be derived from the caller's
struct nvmf_connect_data.

Replace these parameters with structure pointer nvmf_connect_data *d.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/core.c        | 16 ++++++++--------
 drivers/nvme/target/fabrics-cmd.c |  3 +--
 drivers/nvme/target/nvmet.h       |  4 ++--
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 5915b7cc65a6..9ad001fc07f9 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1296,27 +1296,27 @@ static void nvmet_fatal_error_handler(struct work_struct *work)
 	ctrl->ops->delete_ctrl(ctrl);
 }
 
-u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
-		struct nvmet_req *req, u32 kato, struct nvmet_ctrl **ctrlp)
+u16 nvmet_alloc_ctrl(struct nvmf_connect_data *d, struct nvmet_req *req,
+		     u32 kato, struct nvmet_ctrl **ctrlp)
 {
 	struct nvmet_subsys *subsys;
 	struct nvmet_ctrl *ctrl;
 	int ret;
 	u16 status;
 
-	subsys = nvmet_find_get_subsys(req->port, subsysnqn);
+	subsys = nvmet_find_get_subsys(req->port, d->subsysnqn);
 	if (!subsys) {
 		pr_warn("connect request for invalid subsystem %s!\n",
-			subsysnqn);
+			d->subsysnqn);
 		req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(subsysnqn);
 		status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 		goto out;
 	}
 
 	down_read(&nvmet_config_sem);
-	if (!nvmet_host_allowed(subsys, hostnqn)) {
+	if (!nvmet_host_allowed(subsys, d->hostnqn)) {
 		pr_info("connect by host %s for subsystem %s not allowed\n",
-			hostnqn, subsysnqn);
+			d->hostnqn, d->subsysnqn);
 		req->cqe->result.u32 = IPO_IATTR_CONNECT_DATA(hostnqn);
 		up_read(&nvmet_config_sem);
 		status = NVME_SC_CONNECT_INVALID_HOST | NVME_SC_DNR;
@@ -1339,8 +1339,8 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	INIT_RADIX_TREE(&ctrl->p2p_ns_map, GFP_KERNEL);
 	INIT_WORK(&ctrl->fatal_err_work, nvmet_fatal_error_handler);
 
-	memcpy(ctrl->subsysnqn, subsysnqn, NVMF_NQN_SIZE);
-	memcpy(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE);
+	memcpy(ctrl->subsysnqn, d->subsysnqn, NVMF_NQN_SIZE);
+	memcpy(ctrl->hostnqn, d->hostnqn, NVMF_NQN_SIZE);
 
 	kref_init(&ctrl->ref);
 	ctrl->subsys = subsys;
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 46578f98abcc..aa530e0e112d 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -188,8 +188,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 		goto out;
 	}
 
-	status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
-				  le32_to_cpu(c->kato), &ctrl);
+	status = nvmet_alloc_ctrl(d, req, le32_to_cpu(c->kato), &ctrl);
 	if (status) {
 		if (status == (NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR) ||
 		    status == (NVME_SC_CONNECT_INVALID_HOST | NVME_SC_DNR))
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 2dd4a91e5e0e..eaa5b7208a36 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -431,8 +431,8 @@ int nvmet_sq_init(struct nvmet_sq *sq);
 void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl);
 
 void nvmet_update_cc(struct nvmet_ctrl *ctrl, u32 new);
-u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
-		struct nvmet_req *req, u32 kato, struct nvmet_ctrl **ctrlp);
+u16 nvmet_alloc_ctrl(struct nvmf_connect_data *d, struct nvmet_req *req,
+		     u32 kato, struct nvmet_ctrl **ctrlp);
 struct nvmet_ctrl *nvmet_ctrl_find_get(struct nvmf_connect_data *d,
 				       struct nvmet_req *req);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
-- 
2.22.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 2/6] nvmet: set status on actual error condition

[Christoph Hellwig]

On Tue, Feb 16, 2021 at 01:31:08PM -0800, Chaitanya Kulkarni wrote:
> In the nvmet_alloc_ctrl() set the status variable to its error value
> NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR in the actual error
> condition just like the rest of the code.

This is a pretty normal pattern in the kernel, so I don't see much
of a point to change it.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/6] nvmet: check and set the right err location

[Christoph Hellwig]

On Tue, Feb 16, 2021 at 01:31:09PM -0800, Chaitanya Kulkarni wrote:
> The function nvmet_execute_admin_connect() doesn't check for the right
> error status value that is return from the nvmet_alloc_ctrl().
> 
> Check for NVME_SC_CONNECT_INVALID_PARAM & NVME_SC_CONNECT_INVALID_HOST
> before we update the error log.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/target/fabrics-cmd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 42bd12b8bf00..cf0baa911db2 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -191,9 +191,10 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
>  	status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
>  				  le32_to_cpu(c->kato), &ctrl);
>  	if (status) {
> -		if (status == (NVME_SC_INVALID_FIELD | NVME_SC_DNR))
> +		if (status == (NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR) ||
> +		    status == (NVME_SC_CONNECT_INVALID_HOST | NVME_SC_DNR))
>  			req->error_loc =
> -				offsetof(struct nvme_common_command, opcode);
> +				offsetof(struct nvme_common_command, dptr);

Can we just set req->error_loc inside of nvmet_alloc_ctrl?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/6] nvmet: cleanup and status, error log fix

[Christoph Hellwig]

Except for the two comments this looks fine to me.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme