On 1/11/21 23:48, Christoph Hellwig wrote:

quoted

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index a50b7bcac67a..bdf09d8faa48 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c

@@ -191,6 +191,15 @@ static void nvmet_execute_get_log_cmd_effects_ns(struct nvmet_req *req)
 		log->iocs[nvme_cmd_dsm]			= cpu_to_le32(1 << 0);
 		log->iocs[nvme_cmd_write_zeroes]	= cpu_to_le32(1 << 0);
 		break;
+	case NVME_CSI_ZNS:
+		if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
+			u32 *iocs = log->iocs;
+
+			iocs[nvme_cmd_zone_append]	= cpu_to_le32(1 << 0);
+			iocs[nvme_cmd_zone_mgmt_send]	= cpu_to_le32(1 << 0);
+			iocs[nvme_cmd_zone_mgmt_recv]	= cpu_to_le32(1 << 0);
+		}
+		break;

We need to return errors if the command set is not actually supported.
I also think splitting this into one helper per command set would
be nice.

Okay.

quoted

@@ -644,6 +653,17 @@ static void nvmet_execuIt should be te_identify_desclist(struct nvmet_req *req)
 	if (status)
 		goto out;
 
+	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
+		u16 nvme_cis_zns = NVME_CSI_ZNS;
+
+		if (req->ns->csi == NVME_CSI_ZNS)
+			status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI,
+							  NVME_NIDT_CSI_LEN,
+							  &nvme_cis_zns, &off);
+		if (status)
+			goto out;
+	}

We need to add the CSI for every namespace, i.e. something like:

	status = nvmet_copy_ns_identifier(req, NVME_NIDT_CSI, NVME_NIDT_CSI_LEN,
					  &req->ns->csi);		
	if (status)
		goto out;

and this hunk needs to go into the CSI patch.

even better, we can get rid of the local variables...

quoted

 	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;

@@ -660,8 +680,16 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	switch (req->cmd->identify.cns) {
 	case NVME_ID_CNS_NS:
 		return nvmet_execute_identify_ns(req);
+	case NVME_ID_CNS_CS_NS:
+		if (req->cmd->identify.csi == NVME_CSI_ZNS)
+			return nvmet_execute_identify_cns_cs_ns(req);
+		break;
 	case NVME_ID_CNS_CTRL:
 		return nvmet_execute_identify_ctrl(req);
+	case NVME_ID_CNS_CS_CTRL:
+		if (req->cmd->identify.csi == NVME_CSI_ZNS)
+			return nvmet_execute_identify_cns_cs_ctrl(req);
+		break;

How does the CSI get mirrored into the cns field?

There is only one cns and one csi value we set from host/zns.c
This is just to reject req if we receive anything else or there is
any change on the host we fail.

quoted

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 672e4009f8d6..17d5da062a5a 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c

@@ -1107,6 +1107,7 @@ static inline u8 nvmet_cc_iocqes(u32 cc)
 static inline bool nvmet_cc_css_check(u8 cc_css)
 {
 	switch (cc_css <<= NVME_CC_CSS_SHIFT) {
+	case NVME_CC_CSS_CSI:
 	case NVME_CC_CSS_NVM:
 		return true;
 	default:

@@ -1173,6 +1174,8 @@ static void nvmet_init_cap(struct nvmet_ctrl *ctrl)
 {
 	/* command sets supported: NVMe command set: */
 	ctrl->cap = (1ULL << 37);
+	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
+		ctrl->cap |= (1ULL << 43);
 	/* CC.EN timeout in 500msec units: */
 	ctrl->cap |= (15ULL << 24);
 	/* maximum queue entries supported: */

This needs to go into a separate patch for multiple command set support.
We can probably merge the CAP and CC bits with the CSI support, though.

Do you mean previous patch ? but we don't add handlers non-default I/O
command set until this patch..

quoted

+	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) && bdev_is_zoned(ns->bdev)) {

bdev_is_zoned should be probably stubbed out for !CONFIG_BLK_DEV_ZONED
these days.

Are you saying something like following in the prep patch ?or should
just remove
theIS_ENABLED(CONFIG_BLK_DEV_ZONED)part in above if?

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 028ccc9bdf8d..124086c1a0ba 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h

@@ -1570,6 +1570,9 @@ static inline bool bdev_is_zoned(struct

block_device *bdev)
 {
        struct request_queue *q = bdev_get_queue(bdev);
 
+       if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED))
+               return false;
+
        if (q)
                return blk_queue_is_zoned(q);

quoted

+/*
+ *  ZNS related command implementation and helpers.
+ */

Well, that is the description of the whole file, isn't it?  I don't think
this comment adds much value.

Stupid comment, will remove it.

quoted

+	/*
+	 * For ZBC and ZAC devices, writes into sequential zones must be aligned
+	 * to the device physical block size. So use this value as the logical
+	 * block size to avoid errors.
+	 */

I do not understand the logic here, given that NVMe does not have
conventional zones.

It should be :-

	/*
	 * For ZBC and ZAC devices, writes into sequential zones must be aligned
	 * to the device physical block size. So use this value as the *physical*
	 * block size to avoid errors.
	 */