[V3 0/4] nvmet: admin-cmd related cleanups and a fix

[V3 0/4] nvmet: admin-cmd related cleanups and a fix

[Chaitanya Kulkarni]

Hi,

There are three functions where local variable ns is used instead of
using req->ns which leads to duplicate nvmet_put_namespace() calls.
We can just use req->ns and not the local variable to avoid duplicate
calls. First three patches fix that.

The first patch sets the right error status code when namespace is not
found in nvmet_execute_identify_ns().

I've tested this series with nvme blktests, all the testcases are
passing.

-ck

Changes from V2:-

1. Rebase and retest on the latest nvme-5.11.
2. Add ns status error patch at the front of the series.

Changes from V1:-

1. Rebase and retest on the latest nvme-5.11.

Chaitanya Kulkarni (4):
  nvmet: set right status on error in id-ns handler
  nvmet: remove extra variable in smart log nsid
  nvmet: remove extra variable in id-desclist
  nvmet: remove extra variable in identify ns

 drivers/nvme/target/admin-cmd.c | 74 ++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 38 deletions(-)

# !./check
./check tests/nvme
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime    ...  27.806s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.143s  ...  10.136s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.697s  ...  1.730s
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.115s  ...  0.113s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.072s  ...  0.074s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.740s  ...  1.720s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.671s  ...  1.683s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  19.842s  ...  25.213s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  232.526s  ...  257.846s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  46.703s  ...  47.453s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  284.149s  ...  249.408s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  21.989s  ...  22.446s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  19.330s  ...  18.614s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  16.634s  ...  16.945s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  16.120s  ...  17.179s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.665s  ...  1.676s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.692s  ...  1.701s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.655s  ...  1.662s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.648s  ...  1.666s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  2.086s  ...  2.099s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.683s  ...  1.699s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.653s  ...  1.659s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.653s  ...  1.649s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.663s  ...  1.658s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.661s  ...  1.665s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.659s  ...  1.663s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  2.009s  ...  2.025s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.316s  ...  0.317s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  5.608s  ...  5.559s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.045s  ...  0.046s

-- 
2.22.1


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

[V3 1/4] nvmet: set right status on error in id-ns handler

[Chaitanya Kulkarni]

The function nvmet_execute_identify_ns() does't set the status
if call to nvmet_find_namespace() fails. In that case we set the
status of the request to the value return by the nvmet_copy_sgl().

Set the status to NVME_SC_INVALID_NS and adjust the code such that
request will have the right status on nvmet_find_namespace() failure.

Without this patch :-
NVME Identify Namespace 3:
nsze    : 0
ncap    : 0
nuse    : 0
nsfeat  : 0
nlbaf   : 0
flbas   : 0
mc      : 0
dpc     : 0
dps     : 0
nmic    : 0
rescap  : 0
fpi     : 0
dlfeat  : 0
nawun   : 0
nawupf  : 0
nacwu   : 0
nabsn   : 0
nabo    : 0
nabspf  : 0
noiob   : 0
nvmcap  : 0
mssrl   : 0
mcl     : 0
msrc    : 0
nsattr	: 0
nvmsetid: 0
anagrpid: 0
endgid  : 0
nguid   : 00000000000000000000000000000000
eui64   : 0000000000000000
lbaf  0 : ms:0   lbads:0  rp:0 (in use)

With this patch-series :-
# gitlog -4
feb3b88b501e (HEAD -> nvme-5.11) nvmet: remove extra variable in identify ns
6302aa67210a nvmet: remove extra variable in id-desclist
ed57951da453 nvmet: remove extra variable in smart log nsid
be384b8c24dc nvmet: set right status on error in id-ns handler

NVMe status: INVALID_NS: The namespace or the format of that namespace is invalid(0xb)

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 8d90235e4fcc..9bf1fd6933cc 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -487,8 +487,10 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 
 	/* return an all zeroed buffer if we can't find an active namespace */
 	ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
-	if (!ns)
+	if (!ns) {
+		status = NVME_SC_INVALID_NS;
 		goto done;
+	}
 
 	nvmet_ns_revalidate(ns);
 
@@ -541,7 +543,9 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		id->nsattr |= (1 << 0);
 	nvmet_put_namespace(ns);
 done:
-	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
+	if (nvmet_copy_to_sgl(req, 0, id, sizeof(*id)) && !status)
+		status = NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;
+
 	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

[V3 2/4] nvmet: remove extra variable in smart log nsid

[Chaitanya Kulkarni]

We remove the extra local variable struct nvmet_ns in
nvmet_get_smart_log_nsid() since req already has ns member that can be
reused, this also eliminates the explicit call to nvmet_put_namespace()
which is already present in the request completion path.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 9bf1fd6933cc..d6713139708a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -74,11 +74,11 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
 static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
-	struct nvmet_ns *ns;
 	u64 host_reads, host_writes, data_units_read, data_units_written;
 
-	ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->get_log_page.nsid);
-	if (!ns) {
+	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);
@@ -86,23 +86,21 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 	}
 
 	/* we don't have the right data for file backed ns */
-	if (!ns->bdev)
+	if (!req->ns->bdev)
 		goto out;
 
-	host_reads = part_stat_read(ns->bdev, ios[READ]);
+	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
 	data_units_read =
-		DIV_ROUND_UP(part_stat_read(ns->bdev, sectors[READ]), 1000);
-	host_writes = part_stat_read(ns->bdev, ios[WRITE]);
+		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
+	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
 	data_units_written =
-		DIV_ROUND_UP(part_stat_read(ns->bdev, sectors[WRITE]), 1000);
+		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);
 
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
 	put_unaligned_le64(host_writes, &slog->host_writes[0]);
 	put_unaligned_le64(data_units_written, &slog->data_units_written[0]);
 out:
-	nvmet_put_namespace(ns);
-
 	return NVME_SC_SUCCESS;
 }
 
-- 
2.22.1


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

[V3 3/4] nvmet: remove extra variable in id-desclist

[Chaitanya Kulkarni]

We remove the extra local variable struct nvmet_ns in
nvmet_execute_identify_desclist() since req already has ns member that
can be reused, this also eliminates the explicit call to
nvmet_put_namespace() which is already present in the request
completion path.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index d6713139708a..5a75676da259 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -605,37 +605,35 @@ static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
 
 static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 {
-	struct nvmet_ns *ns;
 	u16 status = 0;
 	off_t off = 0;
 
-	ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
-	if (!ns) {
+	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;
 		goto out;
 	}
 
-	if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+	if (memchr_inv(&req->ns->uuid, 0, sizeof(req->ns->uuid))) {
 		status = nvmet_copy_ns_identifier(req, NVME_NIDT_UUID,
 						  NVME_NIDT_UUID_LEN,
-						  &ns->uuid, &off);
+						  &req->ns->uuid, &off);
 		if (status)
-			goto out_put_ns;
+			goto out;
 	}
-	if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+	if (memchr_inv(req->ns->nguid, 0, sizeof(req->ns->nguid))) {
 		status = nvmet_copy_ns_identifier(req, NVME_NIDT_NGUID,
 						  NVME_NIDT_NGUID_LEN,
-						  &ns->nguid, &off);
+						  &req->ns->nguid, &off);
 		if (status)
-			goto out_put_ns;
+			goto out;
 	}
 
 	if (sg_zero_buffer(req->sg, req->sg_cnt, NVME_IDENTIFY_DATA_SIZE - off,
 			off) != NVME_IDENTIFY_DATA_SIZE - off)
 		status = NVME_SC_INTERNAL | NVME_SC_DNR;
-out_put_ns:
-	nvmet_put_namespace(ns);
+
 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

[V3 4/4] nvmet: remove extra variable in identify ns

[Chaitanya Kulkarni]

We remove the extra local variable struct nvmet_ns in
nvmet_execute_identify_ns() since req already has ns member that can be
reused, this also eliminates the explicit call to nvmet_put_namespace()
which is already present in the request completion path.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 5a75676da259..1952ff28f95c 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -467,7 +467,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 nvmet_ns *ns;
 	struct nvme_id_ns *id;
 	u16 status = 0;
 
@@ -484,20 +483,20 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	}
 
 	/* return an all zeroed buffer if we can't find an active namespace */
-	ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
-	if (!ns) {
+	req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
+	if (!req->ns) {
 		status = NVME_SC_INVALID_NS;
 		goto done;
 	}
 
-	nvmet_ns_revalidate(ns);
+	nvmet_ns_revalidate(req->ns);
 
 	/*
 	 * nuse = ncap = nsze isn't always true, but we have no way to find
 	 * that out from the underlying device.
 	 */
-	id->ncap = id->nsze = cpu_to_le64(ns->size >> ns->blksize_shift);
-	switch (req->port->ana_state[ns->anagrpid]) {
+	id->ncap = id->nsze = cpu_to_le64(req->ns->size >> req->ns->blksize_shift);
+	switch (req->port->ana_state[req->ns->anagrpid]) {
 	case NVME_ANA_INACCESSIBLE:
 	case NVME_ANA_PERSISTENT_LOSS:
 		break;
@@ -506,8 +505,8 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 		break;
         }
 
-	if (ns->bdev)
-		nvmet_bdev_set_limits(ns->bdev, id);
+	if (req->ns->bdev)
+		nvmet_bdev_set_limits(req->ns->bdev, id);
 
 	/*
 	 * We just provide a single LBA format that matches what the
@@ -521,25 +520,24 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	 * controllers, but also with any other user of the block device.
 	 */
 	id->nmic = (1 << 0);
-	id->anagrpid = cpu_to_le32(ns->anagrpid);
+	id->anagrpid = cpu_to_le32(req->ns->anagrpid);
 
-	memcpy(&id->nguid, &ns->nguid, sizeof(id->nguid));
+	memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
 
-	id->lbaf[0].ds = ns->blksize_shift;
+	id->lbaf[0].ds = req->ns->blksize_shift;
 
-	if (ctrl->pi_support && nvmet_ns_has_pi(ns)) {
+	if (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;
 		id->mc = NVME_MC_EXTENDED_LBA;
-		id->dps = ns->pi_type;
+		id->dps = req->ns->pi_type;
 		id->flbas = NVME_NS_FLBAS_META_EXT;
-		id->lbaf[0].ms = cpu_to_le16(ns->metadata_size);
+		id->lbaf[0].ms = cpu_to_le16(req->ns->metadata_size);
 	}
 
-	if (ns->readonly)
+	if (req->ns->readonly)
 		id->nsattr |= (1 << 0);
-	nvmet_put_namespace(ns);
 done:
 	if (nvmet_copy_to_sgl(req, 0, id, sizeof(*id)) && !status)
 		status = NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;
-- 
2.22.1


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

Re: [V3 1/4] nvmet: set right status on error in id-ns handler

[Keith Busch]

On Tue, Jan 12, 2021 at 01:14:07PM -0800, Chaitanya Kulkarni wrote:
> @@ -541,7 +543,9 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>  		id->nsattr |= (1 << 0);
>  	nvmet_put_namespace(ns);
>  done:
> -	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
> +	if (nvmet_copy_to_sgl(req, 0, id, sizeof(*id)) && !status)
> +		status = NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;

If the status is already in error, you don't need to copy out the
blank struct, right? Shouldn't this be like:

	if (!status)
		status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));

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

Re: [V3 1/4] nvmet: set right status on error in id-ns handler

[Christoph Hellwig]

On Tue, Jan 12, 2021 at 01:48:01PM -0800, Keith Busch wrote:
> On Tue, Jan 12, 2021 at 01:14:07PM -0800, Chaitanya Kulkarni wrote:
> > @@ -541,7 +543,9 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
> >  		id->nsattr |= (1 << 0);
> >  	nvmet_put_namespace(ns);
> >  done:
> > -	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
> > +	if (nvmet_copy_to_sgl(req, 0, id, sizeof(*id)) && !status)
> > +		status = NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;
> 
> If the status is already in error, you don't need to copy out the
> blank struct, right? Shouldn't this be like:
> 
> 	if (!status)
> 		status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));

Yes.

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

Re: [V3 2/4] nvmet: remove extra variable in smart log nsid

[Sagi Grimberg]

> @@ -86,23 +86,21 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
>   	}
>   
>   	/* we don't have the right data for file backed ns */
> -	if (!ns->bdev)
> +	if (!req->ns->bdev)
>   		goto out;
>   
> -	host_reads = part_stat_read(ns->bdev, ios[READ]);
> +	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
>   	data_units_read =
> -		DIV_ROUND_UP(part_stat_read(ns->bdev, sectors[READ]), 1000);
> -	host_writes = part_stat_read(ns->bdev, ios[WRITE]);
> +		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[READ]), 1000);
> +	host_writes = part_stat_read(req->ns->bdev, ios[WRITE]);
>   	data_units_written =
> -		DIV_ROUND_UP(part_stat_read(ns->bdev, sectors[WRITE]), 1000);
> +		DIV_ROUND_UP(part_stat_read(req->ns->bdev, sectors[WRITE]), 1000);
>   
>   	put_unaligned_le64(host_reads, &slog->host_reads[0]);
>   	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
>   	put_unaligned_le64(host_writes, &slog->host_writes[0]);
>   	put_unaligned_le64(data_units_written, &slog->data_units_written[0]);
>   out:
> -	nvmet_put_namespace(ns);
> -

I think the out label can also be eliminated now.

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

Re: [V3 3/4] nvmet: remove extra variable in id-desclist

[Sagi Grimberg]

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

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

Re: [V3 4/4] nvmet: remove extra variable in identify ns

[Sagi Grimberg]

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

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