All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 00/11] nvmet: fixes and some cleanups
@ 2021-02-09  4:08 Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid Chaitanya Kulkarni
                   ` (10 more replies)
  0 siblings, 11 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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. Patch 9/10 fix
compilation warnings. Last patch removes an unnecessary else at the tail
of the nvmet_parse_io_cmd().

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

Regarding the comment on V1 for bffcd507780e being wrong see [1], we can
continue the discussion.

-ck   

[1] http://lists.infradead.org/pipermail/linux-nvme/2021-February/022637.html

Changes from V1:-

1, In nvmet_execute_identify_ns() zeroout buffer for unallocated nsid.
2. Adjust the patch #2 to reflect above behavior.
3. Merge patch #3 and #4 from V1 to change the nvmet_find_namespace()
   prototype to only accept request.
4. Rename nvmet_find_namespace()->nvmet_req_find_namespace().
5. Remove the backened string print from nvmet_report_invalid_opcode() &
   replace pr_err() with pr_debug().
6. Add a new patch that creates a helper nvmet_req_subsys() to replace
   a chain of the structures all over code to get the subsys from
   nvmet_req.
7. Add a patch to remove the unnecessay else at the tail of the
   nvmet_parse_io_cmd().

Chaitanya Kulkarni (11):
  nvmet: zeroout id-ns buffer for invalid nsid
  nvmet: return uniform error for invalid ns
  nvmet: make nvmet_find_namespace() req based
  nvmet: remove extra variable in id-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
  nvmet: add nvmet_req_subsys() helper
  nvmet: remove else at the end of the function

 drivers/nvme/target/admin-cmd.c   | 69 +++++++++++++++----------------
 drivers/nvme/target/core.c        | 28 +++++++------
 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       | 18 +++++++-
 drivers/nvme/target/passthru.c    |  6 +--
 drivers/nvme/target/trace.h       |  4 +-
 8 files changed, 73 insertions(+), 64 deletions(-)

-- 
2.22.1


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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  7:57   ` Christoph Hellwig
  2021-02-09  4:08 ` [PATCH V2 02/11] nvmet: return uniform error for invalid ns Chaitanya Kulkarni
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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."

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.

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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 613a4d8feac1..fa9a83541d51 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -482,10 +482,16 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		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;
+		/*
+		 * 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 done;
 	}
 
@@ -539,10 +545,11 @@ 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));
 
+done:
 	kfree(id);
 out:
 	nvmet_req_complete(req, status);
-- 
2.22.1


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 02/11] nvmet: return uniform error for invalid ns
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 03/11] nvmet: make nvmet_find_namespace() req based Chaitanya Kulkarni
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

For nvmet_find_namespace() error case we have inconsistent error code
mapping in the function nvmet_get_smart_log_nsid() 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index fa9a83541d51..d6a3ecf66262 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 */
@@ -704,7 +704,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);
-		return status;
+		return status = NVME_SC_INVALID_NS | NVME_SC_DNR;
 	}
 
 	mutex_lock(&subsys->lock);
-- 
2.22.1


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 03/11] nvmet: make nvmet_find_namespace() req based
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 02/11] nvmet: return uniform error for invalid ns Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 04/11] nvmet: remove extra variable in id-ns handler Chaitanya Kulkarni
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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

All callers are nvmet requests based functions, 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.

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->cmd->common.nsid) & remove the extra parameter
from the nvmet_find_namespace().

Lastly now we associate the ns to the req parameter that we pass to the
nvmet_find_namespace(), rename the nvmet_find_namespace() to
nvmet_req_find_ns() i.e. :-

Old:-
struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl,
				      __le32 nsid)
New :-
u16 nvmet_req_find_ns(struct nvmet_req *req);

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index d6a3ecf66262..787e41d48a92 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -75,15 +75,11 @@ 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) {
-		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;
-	}
+	status = nvmet_req_find_ns(req);
+	if (status)
+		return status;
 
 	/* we don't have the right data for file backed ns */
 	if (!req->ns->bdev)
@@ -468,7 +464,7 @@ 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;
+	u16 status;
 
 	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
 		req->error_loc = offsetof(struct nvme_identify, nsid);
@@ -482,8 +478,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 = nvmet_req_find_ns(req);
+	if (status) {
 		status = NVME_SC_INVALID_NS;
 		/*
 		 * According to spec : If the specified namespace is
@@ -611,15 +607,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;
 	off_t off = 0;
+	u16 status;
 
-	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_req_find_ns(req);
+	if (status)
 		goto out;
-	}
 
 	if (memchr_inv(&req->ns->uuid, 0, sizeof(req->ns->uuid))) {
 		status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
@@ -699,13 +692,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);
-		return status = NVME_SC_INVALID_NS | NVME_SC_DNR;
-	}
+	status = nvmet_req_find_ns(req);
+	if (status)
+		return status;
 
 	mutex_lock(&subsys->lock);
 	switch (write_protect) {
@@ -720,6 +711,7 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 		status = 0;
 		break;
 	default:
+		status = NVME_SC_FEATURE_NOT_CHANGEABLE;
 		break;
 	}
 
@@ -806,11 +798,10 @@ static u16 nvmet_get_feat_write_protect(struct nvmet_req *req)
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u32 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;
-	}
+	result = nvmet_req_find_ns(req);
+	if (result)
+		return result;
+
 	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..95b58d4b1af2 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -417,15 +417,18 @@ 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_req_find_ns(struct nvmet_req *req)
 {
-	struct nvmet_ns *ns;
+	u32 nsid = le32_to_cpu(req->cmd->common.nsid);
 
-	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, nsid);
+	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 +865,10 @@ 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_req_find_ns(req);
+	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..954b3d8451f5 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_req_find_ns(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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 04/11] nvmet: remove extra variable in id-ns handler
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (2 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 03/11] nvmet: make nvmet_find_namespace() req based Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 05/11] nvmet: add helper to report invalid opcode Chaitanya Kulkarni
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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 787e41d48a92..d551571b538c 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -462,7 +462,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;
 
@@ -529,7 +528,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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 05/11] nvmet: add helper to report invalid opcode
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (3 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 04/11] nvmet: remove extra variable in id-ns handler Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 06/11] nvmet: use invalid cmd opcode helper Chaitanya Kulkarni
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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       | 9 +++++++++
 2 files changed, 10 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 954b3d8451f5..6b5f1b60cf50 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -613,4 +613,13 @@ 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)
+{
+	pr_debug("unhandled cmd %d on qid %d\n", req->cmd->common.opcode,
+		 req->sq->qid);
+
+	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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 06/11] nvmet: use invalid cmd opcode helper
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (4 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 05/11] nvmet: add helper to report invalid opcode Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 07/11] " Chaitanya Kulkarni
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 07/11] nvmet: use invalid cmd opcode helper
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (5 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 06/11] nvmet: use invalid cmd opcode helper Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 08/11] nvmet: use min of device_path and disk len Chaitanya Kulkarni
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 08/11] nvmet: use min of device_path and disk len
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (6 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 07/11] " Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 09/11] nvme-loop: rename variable to get rid of the warn Chaitanya Kulkarni
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 09/11] nvme-loop: rename variable to get rid of the warn
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (7 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 08/11] nvmet: use min of device_path and disk len Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 10/11] nvmet: add nvmet_req_subsys() helper Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 11/11] nvmet: remove else at the end of the function Chaitanya Kulkarni
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 10/11] nvmet: add nvmet_req_subsys() helper
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (8 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 09/11] nvme-loop: rename variable to get rid of the warn Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  2021-02-09  4:08 ` [PATCH V2 11/11] nvmet: remove else at the end of the function Chaitanya Kulkarni
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

Just like what we have to get the passthru ctrl from the req, add an
helper to get the subsystem associated with the nvmet_req() instead
of open coding the chain of structures.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index d551571b538c..8f8382021c31 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -690,7 +690,7 @@ static u16 nvmet_write_protect_flush_sync(struct nvmet_req *req)
 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;
+	struct nvmet_subsys *subsys = nvmet_req_subsys(req);
 	u16 status;
 
 	status = nvmet_req_find_ns(req);
@@ -750,7 +750,7 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 
 void nvmet_execute_set_features(struct nvmet_req *req)
 {
-	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
+	struct nvmet_subsys *subsys = nvmet_req_subsys(req);
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u32 cdw11 = le32_to_cpu(req->cmd->common.cdw11);
 	u16 status = 0;
@@ -794,7 +794,7 @@ 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;
+	struct nvmet_subsys *subsys = nvmet_req_subsys(req);
 	u32 result;
 
 	result = nvmet_req_find_ns(req);
@@ -824,7 +824,7 @@ void nvmet_get_feat_async_event(struct nvmet_req *req)
 
 void nvmet_execute_get_features(struct nvmet_req *req)
 {
-	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
+	struct nvmet_subsys *subsys = nvmet_req_subsys(req);
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 status = 0;
 
@@ -931,7 +931,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 
 	if (nvme_is_fabrics(cmd))
 		return nvmet_parse_fabrics_cmd(req);
-	if (req->sq->ctrl->subsys->type == NVME_NQN_DISC)
+	if (nvmet_req_subsys(req)->type == NVME_NQN_DISC)
 		return nvmet_parse_discovery_cmd(req);
 
 	ret = nvmet_check_ctrl_status(req, cmd);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 95b58d4b1af2..32221f98f1cc 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -421,7 +421,7 @@ u16 nvmet_req_find_ns(struct nvmet_req *req)
 {
 	u32 nsid = le32_to_cpu(req->cmd->common.nsid);
 
-	req->ns = xa_load(&req->sq->ctrl->subsys->namespaces, nsid);
+	req->ns = xa_load(&nvmet_req_subsys(req)->namespaces, nsid);
 	if (unlikely(!req->ns)) {
 		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		return NVME_SC_INVALID_NS | NVME_SC_DNR;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6b5f1b60cf50..50535563e8c5 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -551,6 +551,11 @@ static inline u32 nvmet_dsm_len(struct nvmet_req *req)
 		sizeof(struct nvme_dsm_range);
 }
 
+static inline struct nvmet_subsys *nvmet_req_subsys(struct nvmet_req *req)
+{
+	return req->sq->ctrl->subsys;
+}
+
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys);
 int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys);
@@ -585,7 +590,7 @@ static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
 static inline struct nvme_ctrl *
 nvmet_req_passthru_ctrl(struct nvmet_req *req)
 {
-	return nvmet_passthru_ctrl(req->sq->ctrl->subsys);
+	return nvmet_passthru_ctrl(nvmet_req_subsys(req));
 }
 
 u16 errno_to_nvme_status(struct nvmet_req *req, int errno);
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 3b22f4a868f4..f50c7b2bf21c 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -239,9 +239,9 @@ static void nvmet_passthru_execute_cmd(struct nvmet_req *req)
 		}
 
 		q = ns->queue;
-		timeout = req->sq->ctrl->subsys->io_timeout;
+		timeout = nvmet_req_subsys(req)->io_timeout;
 	} else {
-		timeout = req->sq->ctrl->subsys->admin_timeout;
+		timeout = nvmet_req_subsys(req)->admin_timeout;
 	}
 
 	rq = nvme_alloc_request(q, req->cmd, 0);
-- 
2.22.1


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH V2 11/11] nvmet: remove else at the end of the function
  2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
                   ` (9 preceding siblings ...)
  2021-02-09  4:08 ` [PATCH V2 10/11] nvmet: add nvmet_req_subsys() helper Chaitanya Kulkarni
@ 2021-02-09  4:08 ` Chaitanya Kulkarni
  10 siblings, 0 replies; 13+ messages in thread
From: Chaitanya Kulkarni @ 2021-02-09  4:08 UTC (permalink / raw)
  To: linux-nvme; +Cc: hch, Chaitanya Kulkarni, sagi

The function nvmet_parse_io_cmd() returns value from
nvmet_file_parse_io_cmd() or nvmet_bdev_parse_io_cmd() based on which
backend is set for the request. Remove the else and just return the
value from nvmet_bdev_parse_io_cmd().

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

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 32221f98f1cc..1d5516272530 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -882,8 +882,8 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 
 	if (req->ns->file)
 		return nvmet_file_parse_io_cmd(req);
-	else
-		return nvmet_bdev_parse_io_cmd(req);
+
+	return nvmet_bdev_parse_io_cmd(req);
 }
 
 bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
-- 
2.22.1


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid
  2021-02-09  4:08 ` [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid Chaitanya Kulkarni
@ 2021-02-09  7:57   ` Christoph Hellwig
  0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-02-09  7:57 UTC (permalink / raw)
  To: Chaitanya Kulkarni; +Cc: hch, linux-nvme, sagi

NAK.  Please don't add dead code for a slow path, and please fix the
actual bug in that we need to return 0 here and not an error.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-09  7:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09  4:08 [PATCH V2 00/11] nvmet: fixes and some cleanups Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 01/11] nvmet: zeroout id-ns buffer for invalid nsid Chaitanya Kulkarni
2021-02-09  7:57   ` Christoph Hellwig
2021-02-09  4:08 ` [PATCH V2 02/11] nvmet: return uniform error for invalid ns Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 03/11] nvmet: make nvmet_find_namespace() req based Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 04/11] nvmet: remove extra variable in id-ns handler Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 05/11] nvmet: add helper to report invalid opcode Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 06/11] nvmet: use invalid cmd opcode helper Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 07/11] " Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 08/11] nvmet: use min of device_path and disk len Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 09/11] nvme-loop: rename variable to get rid of the warn Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 10/11] nvmet: add nvmet_req_subsys() helper Chaitanya Kulkarni
2021-02-09  4:08 ` [PATCH V2 11/11] nvmet: remove else at the end of the function Chaitanya Kulkarni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.