[PATCH 00/10] nvmet: fixes and some cleanups

[PATCH 00/10] nvmet: fixes and some cleanups

[Chaitanya Kulkarni]

Hi,

This patch-series fixes behavior for id-ns that is against the spec and
does cleanup for nvmet_find_namespace() to get rid of the duplicate code
and remove the inconsistent error behavior of the host, Also this adds
an helper for unhandled commands to have uniform error message
reporting for the bdev, file and passthru backend. Last two patches
fix compilation warnings.

This is based on nvme-5.12. All the blktests are seem to pass on this
series.                

-ck

Chaitanya Kulkarni (10):
  nvmet: zeroout id-ns buffer for invalid nsid
  nvmet: return uniform error for invalid ns
  nvmet: set error in nvmet_find_namespace()
  nvmet: remove nsid param from nvmet_find_namespace()
  nvmet: remove extra variable in is-ns handler
  nvmet: add helper to report invalid opcode
  nvmet: use invalid cmd opcode helper
  nvmet: use invalid cmd opcode helper
  nvmet: use min of device_path and disk len
  nvme-loop: rename variable to get rid of the warn

 drivers/nvme/target/admin-cmd.c   | 63 +++++++++++++++----------------
 drivers/nvme/target/core.c        | 23 +++++------
 drivers/nvme/target/io-cmd-bdev.c |  5 +--
 drivers/nvme/target/io-cmd-file.c |  5 +--
 drivers/nvme/target/loop.c        |  2 +-
 drivers/nvme/target/nvmet.h       | 22 ++++++++++-
 drivers/nvme/target/passthru.c    |  2 +-
 drivers/nvme/target/trace.h       |  4 +-
 8 files changed, 70 insertions(+), 56 deletions(-)

# gitlog -11
6eb10edb698e (HEAD -> nvme-5.12) nvme-loop: rename variable to get rid of the warn
d94df0995f71 nvmet: use min of device_path and disk len
a98b4f9e46e0 nvmet: use invalid cmd opcode helper
a3ca782d82e5 nvmet: use invalid cmd opcode helper
102c6698b403 nvmet: add helper to report invalid opcode
eed0b3baae83 nvmet: remove extra variable in is-ns handler
4b47817fab94 nvmet: remove nsid param from nvmet_find_namespace()
68a97104491a nvmet: set error in nvmet_find_namespace()
e4079ffeb06e nvmet: return uniform error for invalid ns
5102bfe49832 nvmet: zeroout id-ns buffer for invalid nsid
ab9dbdb20926 (origin/nvme-5.12) nvme-tcp: use cancel tagset helper for tear down

blktests (master) # ./check tests/nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime  26.465s  ...  25.746s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.135s  ...  10.134s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.677s  ...  1.708s
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.120s  ...  0.115s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.071s  ...  0.069s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.691s  ...  1.693s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.667s  ...  1.648s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  26.581s  ...  32.500s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  274.955s  ...  256.070s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  45.972s  ...  39.458s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  274.262s  ...  285.493s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  21.649s  ...  20.740s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  19.500s  ...  18.742s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  14.570s  ...  14.262s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  14.831s  ...  14.774s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.646s  ...  1.654s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.689s  ...  1.708s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.638s  ...  1.651s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.659s  ...  1.685s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  2.089s  ...  2.078s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.670s  ...  1.680s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.640s  ...  1.644s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.646s  ...  1.655s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.654s  ...  1.637s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.722s  ...  1.655s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.641s  ...  1.679s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.999s  ...  2.016s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.305s  ...  0.290s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  5.488s  ...  5.596s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.043s  ...  0.043s

Tracing test :-

nvmet_req_init: nvmet1: qid=0, cmdid=2, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_admin_identify, cns=3, ctrlid=0)
nvmet_req_init: nvmet1: qid=0, cmdid=3, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_admin_identify, cns=0, ctrlid=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=10, cmdid=81, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=0, len=0, ctrl=0x0, dsmgmt=0, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=10, cmdid=82, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=1, len=0, ctrl=0x0, dsmgmt=0, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=113, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709104, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=114, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709118, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=115, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=0, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=116, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=1, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=117, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709119, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=118, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709087, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=119, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709112, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=120, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709088, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=121, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709070, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=122, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709046, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=123, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709035, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=124, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709028, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=125, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709006, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=126, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368708998, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=127, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368708996, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=113, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709001, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=114, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368708734, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=115, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=256, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=116, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=3, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=117, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=7, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=118, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=15, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=119, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=2, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=120, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=4, len=2, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=121, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=8, len=6, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=122, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=16, len=47, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=123, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=64, len=63, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=124, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368708992, len=3, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=125, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368708997, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=126, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368708999, len=1, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=127, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709002, len=3, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=113, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709007, len=20, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=114, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709029, len=5, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=115, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709036, len=9, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=116, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709047, len=8, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=117, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709056, len=13, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=118, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709071, len=15, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=119, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709089, len=14, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=120, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709105, len=6, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=121, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=5368709113, len=4, ctrl=0x8000, dsmgmt=7, reftag=0)
nvmet_req_init: nvmet1: disk=/dev/nullb0, qid=23, cmdid=122, nsid=1, flags=0x40, meta=0x0, cmd=(nvme_cmd_read, slba=512, len=0, ctrl=0x8000, dsmgmt=7, reftag=0)
kkjkj
-- 
2.22.1


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

[PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid

[Chaitanya Kulkarni]

According to spec :-
"5.15.2.7 Identify Namespace data structure for an Allocated Namespace
ID (CNS 11h) The Identify Namespace data structure (refer to Figure 245)
is returned to the host for the namespace specified in the Namespace
Identifier (NSID) field if it is an allocated NSID. If the specified
namespace is an unallocated NSID then the controller returns a zero
filled data structure."

Move call to nvmet_find_namespace() in nvmet_execute_identify_ns()
above the id-ns buffer allocation since there is no point in allocating
a buffer if namespace doesn't exist. Call nvmet_zero_sgl() when call to
nvmet_find_namespace() fails and add an explicit comment to specify the
reason for zeroing the buffer which is not common for the NVMe commands.
Remove the done label and we can directly jump to out label from
nvmet_find_namespace() error case.

Fixes: bffcd507780e ("nvmet: set right status on error in id-ns handler")
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 613a4d8feac1..8cc7bb25d10d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -476,19 +476,25 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		goto out;
 	}
 
+	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
+	if (!req->ns) {
+		status = NVME_SC_INVALID_NS;
+		/*
+		 * According to spec : If the specified namespace is
+		 * an unallocated NSID then the controller returns a zero filled
+		 * data structure. Also don't override the error status as invalid
+		 * namespace takes priority over the failed zeroout buffer case.
+		 */
+		nvmet_zero_sgl(req, 0, sizeof(*id));
+		goto out;
+	}
+
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
 		status = NVME_SC_INTERNAL;
 		goto out;
 	}
 
-	/* return an all zeroed buffer if we can't find an active namespace */
-	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
-	if (!req->ns) {
-		status = NVME_SC_INVALID_NS;
-		goto done;
-	}
-
 	nvmet_ns_revalidate(req->ns);
 
 	/*
@@ -539,7 +545,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 
 	if (req->ns->readonly)
 		id->nsattr |= (1 << 0);
-done:
+
 	if (!status)
 		status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
-- 
2.22.1


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

[PATCH 02/10] nvmet: return uniform error for invalid ns

[Chaitanya Kulkarni]

For nvmet_find_namespace() error case we have inconsistent error code
mapping in the function nvmet_get_smart_log_nsid(),
nvmet_execute_identify_ns() and nvmet_set_feat_write_protect().

There is no point in retrying for the invalid namesapce from the host
side. Set the error code to the NVME_SC_INVALID_NS | NVME_SC_DNR which
matches what we have in nvmet_execute_identify_desclist().

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 8cc7bb25d10d..ee3121b447fe 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -82,7 +82,7 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		pr_err("Could not find namespace id : %d\n",
 				le32_to_cpu(req->cmd->get_log_page.nsid));
 		req->error_loc = offsetof(struct nvme_rw_command, nsid);
-		return NVME_SC_INVALID_NS;
+		return NVME_SC_INVALID_NS | NVME_SC_DNR;
 	}
 
 	/* we don't have the right data for file backed ns */
@@ -478,7 +478,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 
 	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
 	if (!req->ns) {
-		status = NVME_SC_INVALID_NS;
+		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
 		/*
 		 * According to spec : If the specified namespace is
 		 * an unallocated NSID then the controller returns a zero filled
@@ -703,6 +703,7 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 	req->ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->rw.nsid);
 	if (unlikely(!req->ns)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
+		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
 		return status;
 	}
 
-- 
2.22.1


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

[PATCH 03/10] nvmet: set error in nvmet_find_namespace()

[Chaitanya Kulkarni]

The six callers of nvmet_find_namespace() duplicate the error log page
update and status setting code for each call.

All callers are nvmet requests based functions and so we can pass req
to the nvmet_find_namesapce() & derive ctrl from req, that'll allow us
to update the error log page in nvmet_find_namespace(). Now that we
pass the request we can also get rid of the local variable in
nvmet_find_namespace() and use the req->ns and return the error code.

Replace the ctrl parameter with nvmet_req for nvmet_find_namespace(),
centralize the error log page update for non allocated namesapces, and
return uniform error for non-allocated namespace.

With this change now we have ten less lines and most importantly we
get rid of the inconsistent behaviour for different commands when ns is
not allocated.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ee3121b447fe..fbb5fe18f2d4 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -75,14 +75,13 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
 	u64 host_reads, host_writes, data_units_read, data_units_written;
+	u16 status;
 
-	req->ns = nvmet_find_namespace(req->sq->ctrl,
-				       req->cmd->get_log_page.nsid);
-	if (!req->ns) {
+	status = nvmet_find_namespace(req, req->cmd->get_log_page.nsid);
+	if (status) {
 		pr_err("Could not find namespace id : %d\n",
 				le32_to_cpu(req->cmd->get_log_page.nsid));
-		req->error_loc = offsetof(struct nvme_rw_command, nsid);
-		return NVME_SC_INVALID_NS | NVME_SC_DNR;
+		return status;
 	}
 
 	/* we don't have the right data for file backed ns */
@@ -476,9 +475,8 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		goto out;
 	}
 
-	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
-	if (!req->ns) {
-		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+	status = nvmet_find_namespace(req, req->cmd->identify.nsid);
+	if (status) {
 		/*
 		 * According to spec : If the specified namespace is
 		 * an unallocated NSID then the controller returns a zero filled
@@ -610,15 +608,12 @@ static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
 
 static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 {
-	u16 status = 0;
+	u16 status;
 	off_t off = 0;
 
-	req->ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
-	if (!req->ns) {
-		req->error_loc = offsetof(struct nvme_identify, nsid);
-		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+	status = nvmet_find_namespace(req, req->cmd->identify.nsid);
+	if (status)
 		goto out;
-	}
 
 	if (memchr_inv(&req->ns->uuid, 0, sizeof(req->ns->uuid))) {
 		status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
@@ -698,14 +693,11 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 {
 	u32 write_protect = le32_to_cpu(req->cmd->common.cdw11);
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
-	u16 status = NVME_SC_FEATURE_NOT_CHANGEABLE;
+	u16 status;
 
-	req->ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->rw.nsid);
-	if (unlikely(!req->ns)) {
-		req->error_loc = offsetof(struct nvme_common_command, nsid);
-		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+	status = nvmet_find_namespace(req, req->cmd->rw.nsid);
+	if (unlikely(status))
 		return status;
-	}
 
 	mutex_lock(&subsys->lock);
 	switch (write_protect) {
@@ -717,9 +709,9 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 		break;
 	case NVME_NS_NO_WRITE_PROTECT:
 		req->ns->readonly = false;
-		status = 0;
 		break;
 	default:
+		status = NVME_SC_FEATURE_NOT_CHANGEABLE;
 		break;
 	}
 
@@ -804,13 +796,12 @@ void nvmet_execute_set_features(struct nvmet_req *req)
 static u16 nvmet_get_feat_write_protect(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
-	u32 result;
+	u16 result;
+
+	result = nvmet_find_namespace(req, req->cmd->common.nsid);
+	if (result)
+		return result;
 
-	req->ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->common.nsid);
-	if (!req->ns)  {
-		req->error_loc = offsetof(struct nvme_common_command, nsid);
-		return NVME_SC_INVALID_NS | NVME_SC_DNR;
-	}
 	mutex_lock(&subsys->lock);
 	if (req->ns->readonly == true)
 		result = NVME_NS_WRITE_PROTECT;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 8ce4d59cc9e7..6ebcbc637265 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -417,15 +417,16 @@ void nvmet_stop_keep_alive_timer(struct nvmet_ctrl *ctrl)
 	cancel_delayed_work_sync(&ctrl->ka_work);
 }
 
-struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl, __le32 nsid)
+u16 nvmet_find_namespace(struct nvmet_req *req, __le32 id)
 {
-	struct nvmet_ns *ns;
-
-	ns = xa_load(&ctrl->subsys->namespaces, le32_to_cpu(nsid));
-	if (ns)
-		percpu_ref_get(&ns->ref);
+	req->ns = xa_load(&req->sq->ctrl->subsys->namespaces, le32_to_cpu(id));
+	if (unlikely(!req->ns)) {
+		req->error_loc = offsetof(struct nvme_common_command, nsid);
+		return NVME_SC_INVALID_NS | NVME_SC_DNR;
+	}
 
-	return ns;
+	percpu_ref_get(&req->ns->ref);
+	return NVME_SC_SUCCESS;
 }
 
 static void nvmet_destroy_namespace(struct percpu_ref *ref)
@@ -862,11 +863,9 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 	if (nvmet_req_passthru_ctrl(req))
 		return nvmet_parse_passthru_io_cmd(req);
 
-	req->ns = nvmet_find_namespace(req->sq->ctrl, cmd->rw.nsid);
-	if (unlikely(!req->ns)) {
-		req->error_loc = offsetof(struct nvme_common_command, nsid);
-		return NVME_SC_INVALID_NS | NVME_SC_DNR;
-	}
+	ret = nvmet_find_namespace(req, cmd->rw.nsid);
+	if (unlikely(ret))
+		return ret;
 	ret = nvmet_check_ana_state(req->port, req->ns);
 	if (unlikely(ret)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 8776dd1a0490..80811fccb431 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -443,7 +443,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 void nvmet_subsys_put(struct nvmet_subsys *subsys);
 void nvmet_subsys_del_ctrls(struct nvmet_subsys *subsys);
 
-struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl, __le32 nsid);
+u16 nvmet_find_namespace(struct nvmet_req *req, __le32 id);
 void nvmet_put_namespace(struct nvmet_ns *ns);
 int nvmet_ns_enable(struct nvmet_ns *ns);
 void nvmet_ns_disable(struct nvmet_ns *ns);
-- 
2.22.1


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

[PATCH 04/10] nvmet: remove nsid param from nvmet_find_namespace()

[Chaitanya Kulkarni]

The nvmet_find_namespace() takes nsid parameter which is from NVMe
commands structures such as get_log_page, identify, rw and common. All
these commands have same offset for the nsid field.

Derive nsid from req (req->cmd->common.nsid) and remove the extra
parameter from the nvmet_find_namespace().

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index fbb5fe18f2d4..ec64218db03c 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -77,7 +77,7 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 	u64 host_reads, host_writes, data_units_read, data_units_written;
 	u16 status;
 
-	status = nvmet_find_namespace(req, req->cmd->get_log_page.nsid);
+	status = nvmet_find_namespace(req);
 	if (status) {
 		pr_err("Could not find namespace id : %d\n",
 				le32_to_cpu(req->cmd->get_log_page.nsid));
@@ -475,7 +475,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		goto out;
 	}
 
-	status = nvmet_find_namespace(req, req->cmd->identify.nsid);
+	status = nvmet_find_namespace(req);
 	if (status) {
 		/*
 		 * According to spec : If the specified namespace is
@@ -611,7 +611,7 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 	u16 status;
 	off_t off = 0;
 
-	status = nvmet_find_namespace(req, req->cmd->identify.nsid);
+	status = nvmet_find_namespace(req);
 	if (status)
 		goto out;
 
@@ -695,7 +695,7 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u16 status;
 
-	status = nvmet_find_namespace(req, req->cmd->rw.nsid);
+	status = nvmet_find_namespace(req);
 	if (unlikely(status))
 		return status;
 
@@ -798,7 +798,7 @@ static u16 nvmet_get_feat_write_protect(struct nvmet_req *req)
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u16 result;
 
-	result = nvmet_find_namespace(req, req->cmd->common.nsid);
+	result = nvmet_find_namespace(req);
 	if (result)
 		return result;
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 6ebcbc637265..9c6683f8e790 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -417,8 +417,10 @@ void nvmet_stop_keep_alive_timer(struct nvmet_ctrl *ctrl)
 	cancel_delayed_work_sync(&ctrl->ka_work);
 }
 
-u16 nvmet_find_namespace(struct nvmet_req *req, __le32 id)
+u16 nvmet_find_namespace(struct nvmet_req *req)
 {
+	__le32 id = le32_to_cpu(req->cmd->common.nsid);
+
 	req->ns = xa_load(&req->sq->ctrl->subsys->namespaces, le32_to_cpu(id));
 	if (unlikely(!req->ns)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
@@ -863,7 +865,7 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 	if (nvmet_req_passthru_ctrl(req))
 		return nvmet_parse_passthru_io_cmd(req);
 
-	ret = nvmet_find_namespace(req, cmd->rw.nsid);
+	ret = nvmet_find_namespace(req);
 	if (unlikely(ret))
 		return ret;
 	ret = nvmet_check_ana_state(req->port, req->ns);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 80811fccb431..6864fd916bb5 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -443,7 +443,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 void nvmet_subsys_put(struct nvmet_subsys *subsys);
 void nvmet_subsys_del_ctrls(struct nvmet_subsys *subsys);
 
-u16 nvmet_find_namespace(struct nvmet_req *req, __le32 id);
+u16 nvmet_find_namespace(struct nvmet_req *req);
 void nvmet_put_namespace(struct nvmet_ns *ns);
 int nvmet_ns_enable(struct nvmet_ns *ns);
 void nvmet_ns_disable(struct nvmet_ns *ns);
-- 
2.22.1


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

[PATCH 05/10] nvmet: remove extra variable in is-ns handler

[Chaitanya Kulkarni]

In nvmet_execute_identify_ns() local variable ctrl is accessed only in
one place, remove that and directly use it from nvmet_req->sq->ctrl.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ec64218db03c..d4e8f76ae59a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -465,7 +465,6 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 static void nvmet_execute_identify_ns(struct nvmet_req *req)
 {
-	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvme_id_ns *id;
 	u16 status = 0;
 
@@ -531,7 +530,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 
 	id->lbaf[0].ds = req->ns->blksize_shift;
 
-	if (ctrl->pi_support && nvmet_ns_has_pi(req->ns)) {
+	if (req->sq->ctrl->pi_support && nvmet_ns_has_pi(req->ns)) {
 		id->dpc = NVME_NS_DPC_PI_FIRST | NVME_NS_DPC_PI_LAST |
 			  NVME_NS_DPC_PI_TYPE1 | NVME_NS_DPC_PI_TYPE2 |
 			  NVME_NS_DPC_PI_TYPE3;
-- 
2.22.1


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

[PATCH 06/10] nvmet: add helper to report invalid opcode

[Chaitanya Kulkarni]

In the NVMeOF block device backend, file backend, and passthru backend
we reject and report the commands if opcode is not handled.

Add an helper and use it in block device backend to keep the code
and error message uniform.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/io-cmd-bdev.c |  5 +----
 drivers/nvme/target/nvmet.h       | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 23095bdfce06..105ef2b125cf 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -449,9 +449,6 @@ u16 nvmet_bdev_parse_io_cmd(struct nvmet_req *req)
 		req->execute = nvmet_bdev_execute_write_zeroes;
 		return 0;
 	default:
-		pr_err("unhandled cmd %d on qid %d\n", cmd->common.opcode,
-		       req->sq->qid);
-		req->error_loc = offsetof(struct nvme_common_command, opcode);
-		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		return nvmet_report_invalid_opcode(req);
 	}
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6864fd916bb5..2572e386e2b3 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -613,4 +613,24 @@ static inline sector_t nvmet_lba_to_sect(struct nvmet_ns *ns, __le64 lba)
 	return le64_to_cpu(lba) << (ns->blksize_shift - SECTOR_SHIFT);
 }
 
+static inline u16 nvmet_report_invalid_opcode(struct nvmet_req *req)
+{
+	char *backend;
+
+	if (req->ns->bdev)
+		backend = "bdev";
+	else if (req->ns->file)
+		backend = "file";
+	else if (nvmet_req_passthru_ctrl(req))
+		backend = "passthru";
+	else
+		backend = "unknown";
+
+	pr_err("unhandled cmd %d on qid %d for %s backend\n",
+		req->cmd->common.opcode, req->sq->qid, backend);
+
+	req->error_loc = offsetof(struct nvme_common_command, opcode);
+	return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+}
+
 #endif /* _NVMET_H */
-- 
2.22.1


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

[PATCH 07/10] nvmet: use invalid cmd opcode helper

[Chaitanya Kulkarni]

In the NVMeOF block device backend, file backend, and passthru backend
we reject and report the commands if opcode is not handled.

Use the previously introduced helper in file backend to reduce the
duplicate code and make the error message uniform.

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

diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 0abbefd9925e..715d4376c997 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -400,9 +400,6 @@ u16 nvmet_file_parse_io_cmd(struct nvmet_req *req)
 		req->execute = nvmet_file_execute_write_zeroes;
 		return 0;
 	default:
-		pr_err("unhandled cmd for file ns %d on qid %d\n",
-				cmd->common.opcode, req->sq->qid);
-		req->error_loc = offsetof(struct nvme_common_command, opcode);
-		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		return nvmet_report_invalid_opcode(req);
 	}
 }
-- 
2.22.1


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

[PATCH 08/10] nvmet: use invalid cmd opcode helper

[Chaitanya Kulkarni]

In the NVMeOF block device backend, file backend, and passthru backend
we reject and report the commands if opcode is not handled.

Use the previously introduced helper in the passthru backend to make the
error message uniform.

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

diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index cbc88acdd233..3b22f4a868f4 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -494,7 +494,7 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
 		return nvmet_setup_passthru_command(req);
 	default:
 		/* Reject commands not in the allowlist above */
-		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		return nvmet_report_invalid_opcode(req);
 	}
 }
 
-- 
2.22.1


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

[PATCH 09/10] nvmet: use min of device_path and disk len

[Chaitanya Kulkarni]

In function __assign_req_name() instead of using the DEVICE_NAME_LEN in
strncpy() use min of DISK_NAME_LEN and strlen(req->ns->device_path).

This is needed to turn off the following warnings:-

In file included from drivers/nvme/target/core.c:14:
In function ‘__assign_req_name’,
    inlined from ‘trace_event_raw_event_nvmet_req_init’ at drivers/nvme/target/./trace.h:58:1:
drivers/nvme/target/trace.h:52:3: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
   strncpy(name, req->ns->device_path, DISK_NAME_LEN);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘__assign_req_name’,
    inlined from ‘perf_trace_nvmet_req_complete’ at drivers/nvme/target/./trace.h:100:1:
drivers/nvme/target/trace.h:52:3: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
   strncpy(name, req->ns->device_path, DISK_NAME_LEN);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘__assign_req_name’,
    inlined from ‘perf_trace_nvmet_req_init’ at drivers/nvme/target/./trace.h:58:1:
drivers/nvme/target/trace.h:52:3: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
   strncpy(name, req->ns->device_path, DISK_NAME_LEN);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘__assign_req_name’,
    inlined from ‘trace_event_raw_event_nvmet_req_complete’ at drivers/nvme/target/./trace.h:100:1:
drivers/nvme/target/trace.h:52:3: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
   strncpy(name, req->ns->device_path, DISK_NAME_LEN);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/trace.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/trace.h b/drivers/nvme/target/trace.h
index c14e3249a14d..958dca7bcfc7 100644
--- a/drivers/nvme/target/trace.h
+++ b/drivers/nvme/target/trace.h
@@ -48,8 +48,10 @@ static inline struct nvmet_ctrl *nvmet_req_to_ctrl(struct nvmet_req *req)
 
 static inline void __assign_req_name(char *name, struct nvmet_req *req)
 {
+	size_t len = min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path));
+
 	if (req->ns)
-		strncpy(name, req->ns->device_path, DISK_NAME_LEN);
+		strncpy(name, req->ns->device_path, len);
 	else
 		memset(name, 0, DISK_NAME_LEN);
 }
-- 
2.22.1


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

[PATCH 10/10] nvme-loop: rename variable to get rid of the warn

[Chaitanya Kulkarni]

Rename the nvme_loop_init_request() numa_node parameter to
hw_queue_numa_node so that we can get rid of the following warning :-

drivers/nvme/target/loop.c: In function ‘nvme_loop_init_request’:
drivers/nvme/target/loop.c:205:16: warning: declaration of ‘numa_node’ shadows a global declaration [-Wshadow]
   unsigned int numa_node)

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

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index cb6f86572b24..88d0656ad4a5 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -202,7 +202,7 @@ static int nvme_loop_init_iod(struct nvme_loop_ctrl *ctrl,
 
 static int nvme_loop_init_request(struct blk_mq_tag_set *set,
 		struct request *req, unsigned int hctx_idx,
-		unsigned int numa_node)
+		unsigned int hw_queue_numa_node)
 {
 	struct nvme_loop_ctrl *ctrl = set->driver_data;
 
-- 
2.22.1


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

Re: [PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid

[Christoph Hellwig]

On Sun, Jan 31, 2021 at 09:41:29PM -0800, Chaitanya Kulkarni wrote:
> According to spec :-
> "5.15.2.7 Identify Namespace data structure for an Allocated Namespace
> ID (CNS 11h) The Identify Namespace data structure (refer to Figure 245)
> is returned to the host for the namespace specified in the Namespace
> Identifier (NSID) field if it is an allocated NSID. If the specified
> namespace is an unallocated NSID then the controller returns a zero
> filled data structure."
> 
> Move call to nvmet_find_namespace() in nvmet_execute_identify_ns()
> above the id-ns buffer allocation since there is no point in allocating
> a buffer if namespace doesn't exist. Call nvmet_zero_sgl() when call to
> nvmet_find_namespace() fails and add an explicit comment to specify the
> reason for zeroing the buffer which is not common for the NVMe commands.
> Remove the done label and we can directly jump to out label from
> nvmet_find_namespace() error case.
> 
> Fixes: bffcd507780e ("nvmet: set right status on error in id-ns handler")
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/target/admin-cmd.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 613a4d8feac1..8cc7bb25d10d 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -476,19 +476,25 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>  		goto out;
>  	}
>  
> +	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
> +	if (!req->ns) {
> +		status = NVME_SC_INVALID_NS;
> +		/*
> +		 * According to spec : If the specified namespace is
> +		 * an unallocated NSID then the controller returns a zero filled
> +		 * data structure. Also don't override the error status as invalid
> +		 * namespace takes priority over the failed zeroout buffer case.
> +		 */
> +		nvmet_zero_sgl(req, 0, sizeof(*id));
> +		goto out;
> +	}
> +
>  	id = kzalloc(sizeof(*id), GFP_KERNEL);
>  	if (!id) {
>  		status = NVME_SC_INTERNAL;
>  		goto out;
>  	}
>  
> -	/* return an all zeroed buffer if we can't find an active namespace */
> -	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
> -	if (!req->ns) {
> -		status = NVME_SC_INVALID_NS;
> -		goto done;

I think all we need to do is to remove the status assignment here.
While you're patch avoids the memory allocation for this case, it isn't
really the fast path so I'd rather avoid the extra code.

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

Re: [PATCH 03/10] nvmet: set error in nvmet_find_namespace()

[Christoph Hellwig]

This does look like a nice cleanup, but I think we also need to
rename the function now that it does not return the nvmet_ns
but only sets it in the nvmet_req structure.

What about nvmet_req_find_namespace?

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

Re: [PATCH 04/10] nvmet: remove nsid param from nvmet_find_namespace()

[Christoph Hellwig]

On Sun, Jan 31, 2021 at 09:41:32PM -0800, Chaitanya Kulkarni wrote:
> The nvmet_find_namespace() takes nsid parameter which is from NVMe
> commands structures such as get_log_page, identify, rw and common. All
> these commands have same offset for the nsid field.
> 
> Derive nsid from req (req->cmd->common.nsid) and remove the extra
> parameter from the nvmet_find_namespace().

I'd merge this into the previous one as that totally changes the
calling conventions anyway.  Otherwise this looks good to me.

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

Re: [PATCH 06/10] nvmet: add helper to report invalid opcode

[Christoph Hellwig]

On Sun, Jan 31, 2021 at 09:41:34PM -0800, Chaitanya Kulkarni wrote:
> +static inline u16 nvmet_report_invalid_opcode(struct nvmet_req *req)
> +{
> +	char *backend;
> +
> +	if (req->ns->bdev)
> +		backend = "bdev";
> +	else if (req->ns->file)
> +		backend = "file";
> +	else if (nvmet_req_passthru_ctrl(req))
> +		backend = "passthru";
> +	else
> +		backend = "unknown";
> +
> +	pr_err("unhandled cmd %d on qid %d for %s backend\n",
> +		req->cmd->common.opcode, req->sq->qid, backend);

I think we can just skip reporting the backend, as this doesn't
really add much value.  In fact given that this can be triggered
remotely it should probably be downgraded to a pr_debug.

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

Re: [PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid

[Chaitanya Kulkarni]

On 2/1/21 09:24, Christoph Hellwig wrote:
>>  drivers/nvme/target/admin-cmd.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>> index 613a4d8feac1..8cc7bb25d10d 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -476,19 +476,25 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>>  		goto out;
>>  	}
>>  
>> +	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
>> +	if (!req->ns) {
>> +		status = NVME_SC_INVALID_NS;
>> +		/*
>> +		 * According to spec : If the specified namespace is
>> +		 * an unallocated NSID then the controller returns a zero filled
>> +		 * data structure. Also don't override the error status as invalid
>> +		 * namespace takes priority over the failed zeroout buffer case.
>> +		 */
>> +		nvmet_zero_sgl(req, 0, sizeof(*id));
>> +		goto out;
>> +	}
>> +
>>  	id = kzalloc(sizeof(*id), GFP_KERNEL);
>>  	if (!id) {
>>  		status = NVME_SC_INTERNAL;
>>  		goto out;
>>  	}
>>  
>> -	/* return an all zeroed buffer if we can't find an active namespace */
>> -	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
>> -	if (!req->ns) {
>> -		status = NVME_SC_INVALID_NS;
>> -		goto done;
> I think all we need to do is to remove the status assignment here.
> While you're patch avoids the memory allocation for this case, it isn't
> really the fast path so I'd rather avoid the extra code.
>
Sorry, I didn't understand this comment. If we remove the status assignment
then host will not get the right error. That is the bug we fixed it
initially
with bffcd507780e.

Regarding fast path, if system is under pressure even single memory
allocation
can be costly, especially when host tries to do read id-ns, is there any
reason why we should not consider this scenario ?

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

Re: [PATCH 03/10] nvmet: set error in nvmet_find_namespace()

[Chaitanya Kulkarni]

On 2/1/21 09:27, Christoph Hellwig wrote:
> This does look like a nice cleanup, but I think we also need to
> rename the function now that it does not return the nvmet_ns
> but only sets it in the nvmet_req structure.
>
> What about nvmet_req_find_namespace?
Perfect.

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

Re: [PATCH 04/10] nvmet: remove nsid param from nvmet_find_namespace()

[Chaitanya Kulkarni]

On 2/1/21 09:28, Christoph Hellwig wrote:
>> The nvmet_find_namespace() takes nsid parameter which is from NVMe
>> commands structures such as get_log_page, identify, rw and common. All
>> these commands have same offset for the nsid field.
>>
>> Derive nsid from req (req->cmd->common.nsid) and remove the extra
>> parameter from the nvmet_find_namespace().
> I'd merge this into the previous one as that totally changes the
> calling conventions anyway.  Otherwise this looks good to me.
>
Okay.

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

Re: [PATCH 06/10] nvmet: add helper to report invalid opcode

[Chaitanya Kulkarni]

On 2/1/21 09:29, Christoph Hellwig wrote:
> I think we can just skip reporting the backend, as this doesn't
> really add much value.  In fact given that this can be triggered
> remotely it should probably be downgraded to a pr_debug.
It is easy to grep the backend given that we have 3 and ZBD is
being worked on. I'll remove it for now with pr_debug.

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

Re: [PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid

[Christoph Hellwig]

On Tue, Feb 02, 2021 at 12:12:33AM +0000, Chaitanya Kulkarni wrote:
> >> +	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
> >> +	if (!req->ns) {
> >> +		status = NVME_SC_INVALID_NS;
> >> +		/*
> >> +		 * According to spec : If the specified namespace is
> >> +		 * an unallocated NSID then the controller returns a zero filled
> >> +		 * data structure. Also don't override the error status as invalid
> >> +		 * namespace takes priority over the failed zeroout buffer case.
> >> +		 */
> >> +		nvmet_zero_sgl(req, 0, sizeof(*id));
> >> +		goto out;
> >> +	}
> >> +
> >>  	id = kzalloc(sizeof(*id), GFP_KERNEL);
> >>  	if (!id) {
> >>  		status = NVME_SC_INTERNAL;
> >>  		goto out;
> >>  	}
> >>  
> >> -	/* return an all zeroed buffer if we can't find an active namespace */
> >> -	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
> >> -	if (!req->ns) {
> >> -		status = NVME_SC_INVALID_NS;
> >> -		goto done;
> > I think all we need to do is to remove the status assignment here.
> > While you're patch avoids the memory allocation for this case, it isn't
> > really the fast path so I'd rather avoid the extra code.
> >
> Sorry, I didn't understand this comment. If we remove the status assignment
> then host will not get the right error. That is the bug we fixed it
> initially
> with bffcd507780e.

Actually, looking at it, bffcd507780e is wrong.  Identify Namespace
to a namespace that is not active should return a status of 0 and
a zeroed structure.

> Regarding fast path, if system is under pressure even single memory
> allocation
> can be costly, especially when host tries to do read id-ns, is there any
> reason why we should not consider this scenario ?

No sensible host gets there.  I'd rather keep the code simple.

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

Re: [PATCH 06/10] nvmet: add helper to report invalid opcode

[Christoph Hellwig]

On Tue, Feb 02, 2021 at 12:14:47AM +0000, Chaitanya Kulkarni wrote:
> On 2/1/21 09:29, Christoph Hellwig wrote:
> > I think we can just skip reporting the backend, as this doesn't
> > really add much value.  In fact given that this can be triggered
> > remotely it should probably be downgraded to a pr_debug.
> It is easy to grep the backend given that we have 3 and ZBD is
> being worked on. I'll remove it for now with pr_debug.

Well, you'd better know what backend your namespaces uses when looking
at the logs.

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

Re: [PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid

[Chaitanya Kulkarni]

On 2/2/21 01:38, Christoph Hellwig wrote:
> On Tue, Feb 02, 2021 at 12:12:33AM +0000, Chaitanya Kulkarni wrote:
>>>> +	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
>>>> +	if (!req->ns) {
>>>> +		status = NVME_SC_INVALID_NS;
>>>> +		/*
>>>> +		 * According to spec : If the specified namespace is
>>>> +		 * an unallocated NSID then the controller returns a zero filled
>>>> +		 * data structure. Also don't override the error status as invalid
>>>> +		 * namespace takes priority over the failed zeroout buffer case.
>>>> +		 */
>>>> +		nvmet_zero_sgl(req, 0, sizeof(*id));
>>>> +		goto out;
>>>> +	}
>>>> +
>>>>  	id = kzalloc(sizeof(*id), GFP_KERNEL);
>>>>  	if (!id) {
>>>>  		status = NVME_SC_INTERNAL;
>>>>  		goto out;
>>>>  	}
>>>>  
>>>> -	/* return an all zeroed buffer if we can't find an active namespace */
>>>> -	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
>>>> -	if (!req->ns) {
>>>> -		status = NVME_SC_INVALID_NS;
>>>> -		goto done;
>>> I think all we need to do is to remove the status assignment here.
>>> While you're patch avoids the memory allocation for this case, it isn't
>>> really the fast path so I'd rather avoid the extra code.
>>>
>> Sorry, I didn't understand this comment. If we remove the status assignment
>> then host will not get the right error. That is the bug we fixed it
>> initially
>> with bffcd507780e.
> Actually, looking at it, bffcd507780e is wrong.  Identify Namespace
> to a namespace that is not active should return a status of 0 and
> a zeroed structure.

I didn't find anything in the spec that says we need to return the
the status of 0 for unallocated [1] NSID.

I've checked with two different PCIe controllers for id-ns with invalid
nsid they both returned NVME_SC_INVALID_NS and so does QEMU :-

PCIe:-

# nvme list | tr -s ' ' ' '
Node SN Model Namespace Usage Format FW Rev
/dev/nvme0n1 191515805865 XXXXXXXXXXX-00SJG0 1 250.06 GB / 250.06 GB 512
B + 0 B 102000XX
/dev/nvme1n1 191912800089 XXXXXXXXXXX-00SJG0 1 2.00 TB / 2.00 TB 512 B +
0 B 102430XX

# nvme id-ns /dev/nvme0n1 -n 100
NVMe Status:INVALID_NS(400b) NSID:100

# nvme id-ns /dev/nvme1n1 -n 100
NVMe Status:INVALID_NS(400b) NSID:100

QEMU:-

# nvme list | tr -s ' ' ' '
Node SN Model Namespace Usage Format FW Rev
/dev/nvme0n1 foo QEMU NVMe Ctrl 1 1.07 GB / 1.07 GB 512 B + 0 B 1.0
# nvme id-ns /dev/nvme0 -n 100
NVMe status: INVALID_NS: The namespace or the format of that namespace
is invalid(0x400b)

Without bffcd507780e targetwas returning success and zeroed buffer [2].
Now with bffcd507780etarget is returning NVME_SC_INVALID but the buffer
is not
zeroed (which was there in original patch [3] then we changed to not copy
buffer [4]). Above call to nvmet_zero_sgl() in this patch fixes that.

Please correct me if I missed something.
>> Regarding fast path, if system is under pressure even single memory
>> allocation
>> can be costly, especially when host tries to do read id-ns, is there any
>> reason why we should not consider this scenario ?
> No sensible host gets there.  I'd rather keep the code simple.
>

[1] Unallocated NSIDs do not refer to any namespaces that exist in the
NVM subsystem.
[2] http://lists.infradead.org/pipermail/linux-nvme/2021-January/021954.html
[3] http://lists.infradead.org/pipermail/linux-nvme/2021-January/021954.html
[4] http://lists.infradead.org/pipermail/linux-nvme/2021-January/021979.html

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