[PATCH 0/5] Fix nvme target identify command handling

[PATCH 0/5] Fix nvme target identify command handling

[Damien Le Moal]

These patches fix compliance of the target identify commands. The last
patch is only a cleanup and does not introduce any functional change.

Damien Le Moal (5):
  nvmet: Fix identify Namespace handling
  nvmet: Fix Identify Controller handling
  nvmet: Fix Identify Active Namespace ID list handling
  nvmet: Fix I/O Command Set specific Identify Controller
  nvmet: Cleanup nvmet_execute_identify()

 drivers/nvme/target/admin-cmd.c | 64 ++++++++++++++++-----------------
 drivers/nvme/target/nvmet.h     |  2 +-
 drivers/nvme/target/zns.c       |  2 +-
 3 files changed, 34 insertions(+), 34 deletions(-)

-- 
2.39.2

[PATCH 1/5] nvmet: Fix identify Namespace handling

[Damien Le Moal]

The identify command with cns set to NVME_ID_CNS_NS does not directly
depend on the command set. The NVMe specifications is rather confusing
here as it appears that this command only applies to the NVM command
set. However, footnote 8 of Figure 273 in the NVMe 2.0 base
specifications clearly state that this command applies to NVM command
sets that support logical blocks, that is, NVM and ZNS. Both the NVM and
ZNS command set specifications also list this identify as mandatory.

The command handling should thus not look at the csi field since it is
defined as unused for this command. Given that we do not support the
KV command set, simply remove the csi switch-case for that command
handling and call directly nvmet_execute_identify_ns() in
nvmet_execute_identify().

Fixes: ab5d0b38c047 ("nvmet: add Command Set Identifier support")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ceffe2989a91..7fa60b1aee58 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -675,13 +675,8 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 
 	switch (req->cmd->identify.cns) {
 	case NVME_ID_CNS_NS:
-		switch (req->cmd->identify.csi) {
-		case NVME_CSI_NVM:
-			return nvmet_execute_identify_ns(req);
-		default:
-			break;
-		}
-		break;
+		nvmet_execute_identify_ns(req);
+		return;
 	case NVME_ID_CNS_CS_NS:
 		if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
 			switch (req->cmd->identify.csi) {
-- 
2.39.2

[PATCH 2/5] nvmet: Fix Identify Controller handling

[Damien Le Moal]

The identify command with cns set to NVME_ID_CNS_CTRL does not depend on
the command set. The execution of this command should thus not look at
the csi specified in the command. Simplify nvmet_execute_identify() to
directly call nvmet_execute_identify_ctrl() without the csi switch-case.

Fixes: ab5d0b38c047 ("nvmet: add Command Set Identifier support")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 7fa60b1aee58..cd1b752c52cb 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -688,11 +688,8 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 		}
 		break;
 	case NVME_ID_CNS_CTRL:
-		switch (req->cmd->identify.csi) {
-		case NVME_CSI_NVM:
-			return nvmet_execute_identify_ctrl(req);
-		}
-		break;
+		nvmet_execute_identify_ctrl(req);
+		return;
 	case NVME_ID_CNS_CS_CTRL:
 		if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
 			switch (req->cmd->identify.csi) {
-- 
2.39.2

[PATCH 3/5] nvmet: Fix Identify Active Namespace ID list handling

[Damien Le Moal]

The identify command with cns set to NVME_ID_CNS_NS_ACTIVE_LIST does
not depend on the command set. The execution of this command should
thus not look at the csi field specified in the command. Simplify
nvmet_execute_identify() to directly call
nvmet_execute_identify_nslist() without the csi switch-case.

Fixes: ab5d0b38c047 ("nvmet: add Command Set Identifier support")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index cd1b752c52cb..ed8faf7a3b2f 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -701,13 +701,8 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 		}
 		break;
 	case NVME_ID_CNS_NS_ACTIVE_LIST:
-		switch (req->cmd->identify.csi) {
-		case NVME_CSI_NVM:
-			return nvmet_execute_identify_nslist(req);
-		default:
-			break;
-		}
-		break;
+		nvmet_execute_identify_nslist(req);
+		return;
 	case NVME_ID_CNS_NS_DESC_LIST:
 		nvmet_execute_identify_desclist(req);
 		return;
-- 
2.39.2

[PATCH 4/5] nvmet: Fix I/O Command Set specific Identify Controller

[Damien Le Moal]

For an identify command with cns set to NVME_ID_CNS_CS_CTRL, the NVMe
2.0 specification states that:

If the I/O Command Set specified by the CSI field does not have an
Identify Controller data structure, then the controller shall return
a zero filled data structure. If the host requests a data structure for
an I/O Command Set that the controller does not support, the controller
shall abort the command with a status code of Invalid Field in Command.

However, the current implementation of this identify command in
nvmet_execute_identify() only handles the ZNS command set, returning an
error for the NVM command set, which is not compliant with the
specifications as we do support this command set.

Fix this by:
1) Renaming nvmet_execute_identify_cns_cs_ctrl() to
   nvmet_execute_identify_cns_cs_ctrl_zns() to continue handling the
   ZNS command set as is.
2) Introduce nvmet_execute_identify_cns_cs_ctrl() helper to handle the
   NVM command set, returning a zero filled nvme_id_ctrl_nvm data
   structure.
3) Modify nvmet_execute_identify() to call these helpers based on
   the csi specified, returning an error for unsupported command sets.

Fixes: aaf2e048af27 ("nvmet: add ZBD over ZNS backend support")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 22 ++++++++++++++++------
 drivers/nvme/target/nvmet.h     |  2 +-
 drivers/nvme/target/zns.c       |  2 +-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ed8faf7a3b2f..545f98b2cbd9 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -668,6 +668,13 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_identify_cns_cs_ctrl(struct nvmet_req *req)
+{
+	/* Not supported: return zeroes */
+	nvmet_req_complete(req,
+		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
+}
+
 static void nvmet_execute_identify(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -691,13 +698,16 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 		nvmet_execute_identify_ctrl(req);
 		return;
 	case NVME_ID_CNS_CS_CTRL:
-		if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
-			switch (req->cmd->identify.csi) {
-			case NVME_CSI_ZNS:
-				return nvmet_execute_identify_cns_cs_ctrl(req);
-			default:
-				break;
+		switch (req->cmd->identify.csi) {
+		case NVME_CSI_NVM:
+			nvmet_execute_identify_cns_cs_ctrl(req);
+			return;
+		case NVME_CSI_ZNS:
+			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
+				nvmet_execute_identify_cns_cs_ctrl_zns(req);
+				return;
 			}
+			break;
 		}
 		break;
 	case NVME_ID_CNS_NS_ACTIVE_LIST:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 89bedfcd974c..34cced5652cc 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -581,7 +581,7 @@ bool nvmet_ns_revalidate(struct nvmet_ns *ns);
 u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts);
 
 bool nvmet_bdev_zns_enable(struct nvmet_ns *ns);
-void nvmet_execute_identify_cns_cs_ctrl(struct nvmet_req *req);
+void nvmet_execute_identify_cns_cs_ctrl_zns(struct nvmet_req *req);
 void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req);
 void nvmet_bdev_execute_zone_mgmt_recv(struct nvmet_req *req);
 void nvmet_bdev_execute_zone_mgmt_send(struct nvmet_req *req);
diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index 77ef0a86ff61..fb99c3f15356 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -70,7 +70,7 @@ bool nvmet_bdev_zns_enable(struct nvmet_ns *ns)
 	return true;
 }
 
-void nvmet_execute_identify_cns_cs_ctrl(struct nvmet_req *req)
+void nvmet_execute_identify_cns_cs_ctrl_zns(struct nvmet_req *req)
 {
 	u8 zasl = req->sq->ctrl->subsys->zasl;
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
-- 
2.39.2

[PATCH 5/5] nvmet: Cleanup nvmet_execute_identify()

[Damien Le Moal]

Change the order of the cases in nvmet_execute_identify() main
switch-case to match the NVMe 2.0 specification order as defined in
table 273. This is also the increasing order of CNS values.

While at it, for clarity, make it explicit that identify with cns set
to NVME_ID_CNS_CS_NS does not support NVM command set specific data.

No functional changes are introduced by this cleanup.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 545f98b2cbd9..007064aca179 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -684,19 +684,28 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_NS:
 		nvmet_execute_identify_ns(req);
 		return;
+	case NVME_ID_CNS_CTRL:
+		nvmet_execute_identify_ctrl(req);
+		return;
+	case NVME_ID_CNS_NS_ACTIVE_LIST:
+		nvmet_execute_identify_nslist(req);
+		return;
+	case NVME_ID_CNS_NS_DESC_LIST:
+		nvmet_execute_identify_desclist(req);
+		return;
 	case NVME_ID_CNS_CS_NS:
-		if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
-			switch (req->cmd->identify.csi) {
-			case NVME_CSI_ZNS:
-				return nvmet_execute_identify_cns_cs_ns(req);
-			default:
-				break;
+		switch (req->cmd->identify.csi) {
+		case NVME_CSI_NVM:
+			/* Not supported */
+			break;
+		case NVME_CSI_ZNS:
+			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
+				nvmet_execute_identify_cns_cs_ns(req);
+				return;
 			}
+			break;
 		}
 		break;
-	case NVME_ID_CNS_CTRL:
-		nvmet_execute_identify_ctrl(req);
-		return;
 	case NVME_ID_CNS_CS_CTRL:
 		switch (req->cmd->identify.csi) {
 		case NVME_CSI_NVM:
@@ -710,12 +719,6 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 			break;
 		}
 		break;
-	case NVME_ID_CNS_NS_ACTIVE_LIST:
-		nvmet_execute_identify_nslist(req);
-		return;
-	case NVME_ID_CNS_NS_DESC_LIST:
-		nvmet_execute_identify_desclist(req);
-		return;
 	}
 
 	nvmet_req_cns_error_complete(req);
-- 
2.39.2

Re: [PATCH 1/5] nvmet: Fix identify Namespace handling

[Christoph Hellwig]

On Wed, Mar 15, 2023 at 07:59:35PM +0900, Damien Le Moal wrote:
> The identify command with cns set to NVME_ID_CNS_NS does not directly
> depend on the command set. The NVMe specifications is rather confusing
> here as it appears that this command only applies to the NVM command
> set. However, footnote 8 of Figure 273 in the NVMe 2.0 base
> specifications clearly state that this command applies to NVM command
> sets that support logical blocks, that is, NVM and ZNS. Both the NVM and
> ZNS command set specifications also list this identify as mandatory.
> 
> The command handling should thus not look at the csi field since it is
> defined as unused for this command. Given that we do not support the
> KV command set, simply remove the csi switch-case for that command
> handling and call directly nvmet_execute_identify_ns() in
> nvmet_execute_identify().

Yes, this CNS value pre-dates the addition of the CSI value,
and thus CSI should be ignored.

Re: [PATCH 2/5] nvmet: Fix Identify Controller handling

[Christoph Hellwig]

On Wed, Mar 15, 2023 at 07:59:36PM +0900, Damien Le Moal wrote:
> The identify command with cns set to NVME_ID_CNS_CTRL does not depend on
> the command set. The execution of this command should thus not look at
> the csi specified in the command. Simplify nvmet_execute_identify() to
> directly call nvmet_execute_identify_ctrl() without the csi switch-case.

Indeed - same as the last one.

Re: [PATCH 4/5] nvmet: Fix I/O Command Set specific Identify Controller

[Christoph Hellwig]

> 1) Renaming nvmet_execute_identify_cns_cs_ctrl() to
>    nvmet_execute_identify_cns_cs_ctrl_zns() to continue handling the
>    ZNS command set as is.

I think nvmet_execute_identify_ctrl_cs_zns might be a better name.

> 2) Introduce nvmet_execute_identify_cns_cs_ctrl() helper to handle the
>    NVM command set, returning a zero filled nvme_id_ctrl_nvm data
>    structure.

and this might better be named nvmet_execute_identify_ctrl_cs.

As the _cns in all these and the Identify Namespace variants is
rather redundant.

Otherwise this looks good to me (and I can fix up the naming when
applying the series)

Re: [PATCH 5/5] nvmet: Cleanup nvmet_execute_identify()

[Christoph Hellwig]

On Wed, Mar 15, 2023 at 07:59:39PM +0900, Damien Le Moal wrote:
> Change the order of the cases in nvmet_execute_identify() main
> switch-case to match the NVMe 2.0 specification order as defined in
> table 273. This is also the increasing order of CNS values.
> 
> While at it, for clarity, make it explicit that identify with cns set
> to NVME_ID_CNS_CS_NS does not support NVM command set specific data.

Looks sensible to me, thanks.

Re: [PATCH 0/5] Fix nvme target identify command handling

[Christoph Hellwig]

Thanks,

applied to nvme-6.3.

Re: [PATCH 0/5] Fix nvme target identify command handling

[Chaitanya Kulkarni]

On 3/15/23 03:59, Damien Le Moal wrote:
> These patches fix compliance of the target identify commands. The last
> patch is only a cleanup and does not introduce any functional change.
>
>

Looks good, I've tested with blktests nvme-loop
see [1].

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Tested-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[1]
blktests (master) # ./check nvme
nvme/002 (create many subsystems and test discovery) [passed]
     runtime  19.514s  ...  19.093s
nvme/003 (test if we're sending keep-alives to a discovery controller) 
[passed]
     runtime  10.098s  ...  10.087s
nvme/004 (test nvme and nvmet UUID NS descriptors) [passed]
     runtime  1.439s  ...  1.457s
nvme/005 (reset local loopback target) [passed]
     runtime  1.804s  ...  1.812s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
     runtime  0.061s  ...  0.059s
nvme/007 (create an NVMeOF target with a file-backed ns) [passed]
     runtime  0.031s  ...  0.034s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
     runtime  1.487s  ...  1.485s
nvme/009 (create an NVMeOF host with a file-backed ns) [passed]
     runtime  1.433s  ...  1.421s
nvme/010 (run data verification fio job on NVMeOF block device-backed 
ns) [passed]
     runtime  82.421s  ...  87.332s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
     runtime  81.191s  ...  80.243s
nvme/012 (run mkfs and data verification fio job on NVMeOF block 
device-backed ns) [passed]
     runtime  79.530s  ...  75.895s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed 
ns) [passed]
     runtime  77.714s  ...  68.238s
nvme/014 (flush a NVMeOF block device-backed ns) [passed]
     runtime  4.476s  ...  4.376s
nvme/015 (unit test for NVMe flush for file backed ns) [passed]
     runtime  3.735s  ...  3.807s
nvme/016 (create/delete many NVMeOF block device-backed ns and test 
discovery) [passed]
     runtime  12.765s  ...  13.068s
nvme/017 (create/delete many file-ns and test discovery) [passed]
     runtime  12.961s  ...  13.194s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
     runtime  1.448s  ...  1.431s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
     runtime  1.472s  ...  1.460s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
     runtime  1.422s  ...  1.410s
nvme/021 (test NVMe list command on NVMeOF file-backed ns) [passed]
     runtime  1.430s  ...  1.419s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns) [passed]
     runtime  1.764s  ...  1.746s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
     runtime  1.449s  ...  1.457s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
     runtime  1.422s  ...  1.420s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
     runtime  1.431s  ...  1.414s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
     runtime  1.416s  ...  1.417s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
     runtime  1.426s  ...  1.456s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
     runtime  1.438s  ...  1.424s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
     runtime  1.564s  ...  1.558s
nvme/030 (ensure the discovery generation counter is updated 
appropriately) [passed]
     runtime  0.221s  ...  0.203s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) 
[passed]
     runtime  3.939s  ...  3.890s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
     runtime  0.013s  ...  0.013s
nvme/040 (test nvme fabrics controller reset/disconnect operation during 
I/O) [passed]
     runtime  7.989s  ...  7.961s
nvme/041 (Create authenticated connections) [passed]
     runtime  0.734s  ...  0.730s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
     runtime  4.723s  ...  4.694s
nvme/043 (Test hash and DH group variations for authenticated 
connections) [passed]
     runtime  6.921s  ...  6.937s
nvme/044 (Test bi-directional authentication) [passed]
     runtime  1.710s  ...  1.707s
nvme/045 (Test re-authentication) [passed]
     runtime  3.963s  ...  3.997s

Re: [PATCH 4/5] nvmet: Fix I/O Command Set specific Identify Controller

[Damien Le Moal]

On 2023/03/15 22:38, Christoph Hellwig wrote:
>> 1) Renaming nvmet_execute_identify_cns_cs_ctrl() to
>>    nvmet_execute_identify_cns_cs_ctrl_zns() to continue handling the
>>    ZNS command set as is.
> 
> I think nvmet_execute_identify_ctrl_cs_zns might be a better name.
> 
>> 2) Introduce nvmet_execute_identify_cns_cs_ctrl() helper to handle the
>>    NVM command set, returning a zero filled nvme_id_ctrl_nvm data
>>    structure.
> 
> and this might better be named nvmet_execute_identify_ctrl_cs.
> 
> As the _cns in all these and the Identify Namespace variants is
> rather redundant.
> 
> Otherwise this looks good to me (and I can fix up the naming when
> applying the series)

Yes, please rename !

-- 
Damien Le Moal
Western Digital Research