[PATCHv4 00/13] nvme target 2.1 and independent identify ns

[PATCHv4 00/13] nvme target 2.1 and independent identify ns

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Changes from v3:

  Defined the log structs for logs supported and features supported

  Used defines for those log attribute bits

  Fixed some patch ordering to avoid compiler error hazards part way
  through the last series.

  Removed unnecessary type-casts

  Fixed setting correct request completion status on one of the logs

  Used the correct and reviewed version of the host side rotational
  support

  Added the "no_vwc" patches from Guixin at the end since it is all in
  the same area of the code

  Added more comments explaining the relationship between the namespaces
  and their "endurance groups".

  Added reviews.

Guixin Liu (2):
  nvme: check ns's volatile write cache not present
  nvmet: report ns's vwc not present

Keith Busch (9):
  nvmet: implement id ns for nvm command set
  nvmet: implement active command set ns list
  nvmet: implement supported log pages
  nvmet: implement supported features log
  nvmet: implement crto property
  nvmet: declare 2.1 version compliance
  nvmet: implement endurance groups
  nvmet: implement rotational media information log
  nvmet: support for csi identify ns

Matias Bjørling (1):
  nvme: use command set independent id ns if available

Wang Yugui (1):
  nvme: add rotational support

 drivers/nvme/host/core.c          |  19 ++-
 drivers/nvme/host/nvme.h          |   1 +
 drivers/nvme/target/admin-cmd.c   | 257 +++++++++++++++++++++++++++++-
 drivers/nvme/target/fabrics-cmd.c |   3 +
 drivers/nvme/target/nvmet.h       |   2 +-
 include/linux/nvme.h              |  67 +++++++-
 6 files changed, 337 insertions(+), 12 deletions(-)

-- 
2.43.5

[PATCHv4 01/13] nvmet: implement id ns for nvm command set

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

We don't report anything here, but it's a mandatory identification for
nvme 2.1.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 081f0473cd9ea..8d06dba42bcb3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -685,6 +685,20 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
 		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
 }
 
+static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
+{
+	u16 status;
+
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
+				   NVME_IDENTIFY_DATA_SIZE);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -706,8 +720,8 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_CS_NS:
 		switch (req->cmd->identify.csi) {
 		case NVME_CSI_NVM:
-			/* Not supported */
-			break;
+			nvme_execute_identify_ns_nvm(req);
+			return;
 		case NVME_CSI_ZNS:
 			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
 				nvmet_execute_identify_ns_zns(req);
-- 
2.43.5

Re: [PATCHv4 01/13] nvmet: implement id ns for nvm command set

[Matias Bjørling]

On 07-11-2024 20:38, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> We don't report anything here, but it's a mandatory identification for
> nvme 2.1.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 081f0473cd9ea..8d06dba42bcb3 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -685,6 +685,20 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
>   		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
>   }
>   
> +static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
> +{
> +	u16 status;
> +
> +	status = nvmet_req_find_ns(req);
> +	if (status)
> +		goto out;
> +
> +	status = nvmet_copy_to_sgl(req, 0, ZERO_PAGE(0),
> +				   NVME_IDENTIFY_DATA_SIZE);
> +out:
> +	nvmet_req_complete(req, status);
> +}
> +
>   static void nvmet_execute_identify(struct nvmet_req *req)
>   {
>   	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
> @@ -706,8 +720,8 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>   	case NVME_ID_CNS_CS_NS:
>   		switch (req->cmd->identify.csi) {
>   		case NVME_CSI_NVM:
> -			/* Not supported */
> -			break;
> +			nvme_execute_identify_ns_nvm(req);
> +			return;
>   		case NVME_CSI_ZNS:
>   			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
>   				nvmet_execute_identify_ns_zns(req);

Looks good.

Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>

Re: [PATCHv4 01/13] nvmet: implement id ns for nvm command set

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv4 02/13] nvmet: implement active command set ns list

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This is required for nvme 2.1 for targets that support multiple command
sets. We support NVM and ZNS, so are required to support this
identification.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 9 +++++++--
 include/linux/nvme.h            | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 8d06dba42bcb3..a13242e791c0f 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -576,7 +576,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
-static void nvmet_execute_identify_nslist(struct nvmet_req *req)
+static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 {
 	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
@@ -606,6 +606,8 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
 	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
 		if (ns->nsid <= min_nsid)
 			continue;
+		if (match_css && req->ns->csi != req->cmd->identify.csi)
+			continue;
 		list[i++] = cpu_to_le32(ns->nsid);
 		if (i == buf_size / sizeof(__le32))
 			break;
@@ -712,7 +714,7 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 		nvmet_execute_identify_ctrl(req);
 		return;
 	case NVME_ID_CNS_NS_ACTIVE_LIST:
-		nvmet_execute_identify_nslist(req);
+		nvmet_execute_identify_nslist(req, false);
 		return;
 	case NVME_ID_CNS_NS_DESC_LIST:
 		nvmet_execute_identify_desclist(req);
@@ -743,6 +745,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 			break;
 		}
 		break;
+	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
+		nvmet_execute_identify_nslist(req, true);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b58d9405d65e0..0f263c7e63192 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -522,6 +522,7 @@ enum {
 	NVME_ID_CNS_NS_DESC_LIST	= 0x03,
 	NVME_ID_CNS_CS_NS		= 0x05,
 	NVME_ID_CNS_CS_CTRL		= 0x06,
+	NVME_ID_CNS_NS_ACTIVE_LIST_CS	= 0x07,
 	NVME_ID_CNS_NS_CS_INDEP		= 0x08,
 	NVME_ID_CNS_NS_PRESENT_LIST	= 0x10,
 	NVME_ID_CNS_NS_PRESENT		= 0x11,
-- 
2.43.5

Re: [PATCHv4 02/13] nvmet: implement active command set ns list

[Matias Bjørling]

On 07-11-2024 20:38, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This is required for nvme 2.1 for targets that support multiple command
> sets. We support NVM and ZNS, so are required to support this
> identification.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 9 +++++++--
>   include/linux/nvme.h            | 1 +
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 8d06dba42bcb3..a13242e791c0f 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -576,7 +576,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>   	nvmet_req_complete(req, status);
>   }
>   
> -static void nvmet_execute_identify_nslist(struct nvmet_req *req)
> +static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
>   {
>   	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
>   	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> @@ -606,6 +606,8 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
>   	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
>   		if (ns->nsid <= min_nsid)
>   			continue;
> +		if (match_css && req->ns->csi != req->cmd->identify.csi)
> +			continue;
>   		list[i++] = cpu_to_le32(ns->nsid);
>   		if (i == buf_size / sizeof(__le32))
>   			break;
> @@ -712,7 +714,7 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>   		nvmet_execute_identify_ctrl(req);
>   		return;
>   	case NVME_ID_CNS_NS_ACTIVE_LIST:
> -		nvmet_execute_identify_nslist(req);
> +		nvmet_execute_identify_nslist(req, false);
>   		return;
>   	case NVME_ID_CNS_NS_DESC_LIST:
>   		nvmet_execute_identify_desclist(req);
> @@ -743,6 +745,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>   			break;
>   		}
>   		break;
> +	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
> +		nvmet_execute_identify_nslist(req, true);
> +		return;
>   	}
>   
>   	pr_debug("unhandled identify cns %d on qid %d\n",
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index b58d9405d65e0..0f263c7e63192 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -522,6 +522,7 @@ enum {
>   	NVME_ID_CNS_NS_DESC_LIST	= 0x03,
>   	NVME_ID_CNS_CS_NS		= 0x05,
>   	NVME_ID_CNS_CS_CTRL		= 0x06,
> +	NVME_ID_CNS_NS_ACTIVE_LIST_CS	= 0x07,
>   	NVME_ID_CNS_NS_CS_INDEP		= 0x08,
>   	NVME_ID_CNS_NS_PRESENT_LIST	= 0x10,
>   	NVME_ID_CNS_NS_PRESENT		= 0x11,

Looks good.

Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>

[PATCHv4 03/13] nvmet: implement supported log pages

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This log is required for nvme 2.1.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 27 +++++++++++++++++++++++++++
 include/linux/nvme.h            |  9 +++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index a13242e791c0f..712b962fe77ac 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -71,6 +71,31 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
 	nvmet_req_complete(req, 0);
 }
 
+static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
+{
+	struct nvme_supported_log *logs;
+	u16 status;
+
+	logs = kzalloc(sizeof(*logs), GFP_KERNEL);
+	if (!logs) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	logs->lids[NVME_LOG_SUPPORTED] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_ERROR] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_SMART] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_FW_SLOT] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_CHANGED_NS] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_ANA] = cpu_to_le32(NVME_LIDS_LSUPP);
+
+	status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
+	kfree(logs);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
@@ -323,6 +348,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return;
 
 	switch (req->cmd->get_log_page.lid) {
+	case NVME_LOG_SUPPORTED:
+		return nvmet_execute_get_supported_log_pages(req);
 	case NVME_LOG_ERROR:
 		return nvmet_execute_get_log_page_error(req);
 	case NVME_LOG_SMART:
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 0f263c7e63192..692d606edeed3 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1245,6 +1245,7 @@ enum {
 	NVME_FEAT_WRITE_PROTECT	= 0x84,
 	NVME_FEAT_VENDOR_START	= 0xC0,
 	NVME_FEAT_VENDOR_END	= 0xFF,
+	NVME_LOG_SUPPORTED	= 0x00,
 	NVME_LOG_ERROR		= 0x01,
 	NVME_LOG_SMART		= 0x02,
 	NVME_LOG_FW_SLOT	= 0x03,
@@ -1262,6 +1263,14 @@ enum {
 	NVME_FWACT_ACTV		= (2 << 3),
 };
 
+struct nvme_supported_log {
+	__le32	lids[256];
+};
+
+enum {
+	NVME_LIDS_LSUPP	= 1 << 0,
+};
+
 /* NVMe Namespace Write Protect State */
 enum {
 	NVME_NS_NO_WRITE_PROTECT = 0,
-- 
2.43.5

Re: [PATCHv4 03/13] nvmet: implement supported log pages

[Matias Bjørling]

On 07-11-2024 20:38, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 27 +++++++++++++++++++++++++++
>   include/linux/nvme.h            |  9 +++++++++
>   2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index a13242e791c0f..712b962fe77ac 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -71,6 +71,31 @@ static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
>   	nvmet_req_complete(req, 0);
>   }
>   
> +static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
> +{
> +	struct nvme_supported_log *logs;
> +	u16 status;
> +
> +	logs = kzalloc(sizeof(*logs), GFP_KERNEL);
> +	if (!logs) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	logs->lids[NVME_LOG_SUPPORTED] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_ERROR] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_SMART] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_FW_SLOT] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_CHANGED_NS] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_ANA] = cpu_to_le32(NVME_LIDS_LSUPP);
> +
> +	status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
> +	kfree(logs);
> +out:
> +	nvmet_req_complete(req, status);
> +}
> +
>   static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
>   		struct nvme_smart_log *slog)
>   {
> @@ -323,6 +348,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
>   		return;
>   
>   	switch (req->cmd->get_log_page.lid) {
> +	case NVME_LOG_SUPPORTED:
> +		return nvmet_execute_get_supported_log_pages(req);
>   	case NVME_LOG_ERROR:
>   		return nvmet_execute_get_log_page_error(req);
>   	case NVME_LOG_SMART:
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 0f263c7e63192..692d606edeed3 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1245,6 +1245,7 @@ enum {
>   	NVME_FEAT_WRITE_PROTECT	= 0x84,
>   	NVME_FEAT_VENDOR_START	= 0xC0,
>   	NVME_FEAT_VENDOR_END	= 0xFF,
> +	NVME_LOG_SUPPORTED	= 0x00,
>   	NVME_LOG_ERROR		= 0x01,
>   	NVME_LOG_SMART		= 0x02,
>   	NVME_LOG_FW_SLOT	= 0x03,
> @@ -1262,6 +1263,14 @@ enum {
>   	NVME_FWACT_ACTV		= (2 << 3),
>   };
>   
> +struct nvme_supported_log {
> +	__le32	lids[256];
> +};
> +
> +enum {
> +	NVME_LIDS_LSUPP	= 1 << 0,
> +};
> +
>   /* NVMe Namespace Write Protect State */
>   enum {
>   	NVME_NS_NO_WRITE_PROTECT = 0,

Looks good. The patch description could be updated to say it's required 
for NVMe 2.0 and later devices.

Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>

Re: [PATCHv4 03/13] nvmet: implement supported log pages

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv4 03/13] nvmet: implement supported log pages

[Chaitanya Kulkarni]

On 11/7/24 11:38, Keith Busch wrote:
> From: Keith Busch<kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch<kbusch@kernel.org>


Looks good.

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

-ck

[PATCHv4 04/13] nvmet: implement supported features log

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This log is required for nvme 2.1.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 31 +++++++++++++++++++++++++++++++
 include/linux/nvme.h            | 11 +++++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 712b962fe77ac..7a879e2cd8555 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -89,6 +89,7 @@ static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
 	logs->lids[NVME_LOG_CHANGED_NS] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_ANA] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_FEATURES] = cpu_to_le32(NVME_LIDS_LSUPP);
 
 	status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
 	kfree(logs);
@@ -342,6 +343,34 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
+{
+	struct nvme_supported_features_log *features;
+	u16 status;
+
+	features = kzalloc(sizeof(*features), GFP_KERNEL);
+	if (!features) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	features->fis[NVME_FEAT_NUM_QUEUES] = cpu_to_le32(NVME_FIS_FSUPP |
+							  NVME_FIS_CSCPE);
+	features->fis[NVME_FEAT_KATO] = cpu_to_le32(NVME_FIS_FSUPP |
+						    NVME_FIS_CSCPE);
+	features->fis[NVME_FEAT_ASYNC_EVENT] = cpu_to_le32(NVME_FIS_FSUPP |
+							   NVME_FIS_CSCPE);
+	features->fis[NVME_FEAT_HOST_ID] = cpu_to_le32(NVME_FIS_FSUPP |
+						       NVME_FIS_CSCPE);
+	features->fis[NVME_FEAT_WRITE_PROTECT] = cpu_to_le32(NVME_FIS_FSUPP |
+							     NVME_FIS_NSCPE);
+
+	status = nvmet_copy_to_sgl(req, 0, features, sizeof(*features));
+	kfree(features);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, nvmet_get_log_page_len(req->cmd)))
@@ -367,6 +396,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_cmd_effects_ns(req);
 	case NVME_LOG_ANA:
 		return nvmet_execute_get_log_page_ana(req);
+	case NVME_LOG_FEATURES:
+		return nvmet_execute_get_log_page_features(req);
 	}
 	pr_debug("unhandled lid %d on qid %d\n",
 	       req->cmd->get_log_page.lid, req->sq->qid);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 692d606edeed3..4b896fa651e02 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1256,6 +1256,7 @@ enum {
 	NVME_LOG_TELEMETRY_CTRL = 0x08,
 	NVME_LOG_ENDURANCE_GROUP = 0x09,
 	NVME_LOG_ANA		= 0x0c,
+	NVME_LOG_FEATURES	= 0x12,
 	NVME_LOG_DISC		= 0x70,
 	NVME_LOG_RESERVATION	= 0x80,
 	NVME_FWACT_REPL		= (0 << 3),
@@ -1271,6 +1272,16 @@ enum {
 	NVME_LIDS_LSUPP	= 1 << 0,
 };
 
+struct nvme_supported_features_log {
+	__le32	fis[256];
+};
+
+enum {
+	NVME_FIS_FSUPP	= 1 << 0,
+	NVME_FIS_NSCPE	= 1 << 20,
+	NVME_FIS_CSCPE	= 1 << 21,
+};
+
 /* NVMe Namespace Write Protect State */
 enum {
 	NVME_NS_NO_WRITE_PROTECT = 0,
-- 
2.43.5

Re: [PATCHv4 04/13] nvmet: implement supported features log

[Matias Bjørling]

On 07-11-2024 20:38, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 31 +++++++++++++++++++++++++++++++
>   include/linux/nvme.h            | 11 +++++++++++
>   2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 712b962fe77ac..7a879e2cd8555 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -89,6 +89,7 @@ static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
>   	logs->lids[NVME_LOG_CHANGED_NS] = cpu_to_le32(NVME_LIDS_LSUPP);
>   	logs->lids[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(NVME_LIDS_LSUPP);
>   	logs->lids[NVME_LOG_ANA] = cpu_to_le32(NVME_LIDS_LSUPP);
> +	logs->lids[NVME_LOG_FEATURES] = cpu_to_le32(NVME_LIDS_LSUPP);
>   
>   	status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
>   	kfree(logs);
> @@ -342,6 +343,34 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>   	nvmet_req_complete(req, status);
>   }
>   
> +static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
> +{
> +	struct nvme_supported_features_log *features;
> +	u16 status;
> +
> +	features = kzalloc(sizeof(*features), GFP_KERNEL);
> +	if (!features) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	features->fis[NVME_FEAT_NUM_QUEUES] = cpu_to_le32(NVME_FIS_FSUPP |
> +							  NVME_FIS_CSCPE);
> +	features->fis[NVME_FEAT_KATO] = cpu_to_le32(NVME_FIS_FSUPP |
> +						    NVME_FIS_CSCPE);
> +	features->fis[NVME_FEAT_ASYNC_EVENT] = cpu_to_le32(NVME_FIS_FSUPP |
> +							   NVME_FIS_CSCPE);
> +	features->fis[NVME_FEAT_HOST_ID] = cpu_to_le32(NVME_FIS_FSUPP |
> +						       NVME_FIS_CSCPE);
> +	features->fis[NVME_FEAT_WRITE_PROTECT] = cpu_to_le32(NVME_FIS_FSUPP |
> +							     NVME_FIS_NSCPE);
> +
> +	status = nvmet_copy_to_sgl(req, 0, features, sizeof(*features));
> +	kfree(features);
> +out:
> +	nvmet_req_complete(req, status);
> +}
> +
>   static void nvmet_execute_get_log_page(struct nvmet_req *req)
>   {
>   	if (!nvmet_check_transfer_len(req, nvmet_get_log_page_len(req->cmd)))
> @@ -367,6 +396,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
>   		return nvmet_execute_get_log_cmd_effects_ns(req);
>   	case NVME_LOG_ANA:
>   		return nvmet_execute_get_log_page_ana(req);
> +	case NVME_LOG_FEATURES:
> +		return nvmet_execute_get_log_page_features(req);
>   	}
>   	pr_debug("unhandled lid %d on qid %d\n",
>   	       req->cmd->get_log_page.lid, req->sq->qid);
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 692d606edeed3..4b896fa651e02 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1256,6 +1256,7 @@ enum {
>   	NVME_LOG_TELEMETRY_CTRL = 0x08,
>   	NVME_LOG_ENDURANCE_GROUP = 0x09,
>   	NVME_LOG_ANA		= 0x0c,
> +	NVME_LOG_FEATURES	= 0x12,
>   	NVME_LOG_DISC		= 0x70,
>   	NVME_LOG_RESERVATION	= 0x80,
>   	NVME_FWACT_REPL		= (0 << 3),
> @@ -1271,6 +1272,16 @@ enum {
>   	NVME_LIDS_LSUPP	= 1 << 0,
>   };
>   
> +struct nvme_supported_features_log {
> +	__le32	fis[256];
> +};
> +
> +enum {
> +	NVME_FIS_FSUPP	= 1 << 0,
> +	NVME_FIS_NSCPE	= 1 << 20,
> +	NVME_FIS_CSCPE	= 1 << 21,
> +};
> +
>   /* NVMe Namespace Write Protect State */
>   enum {
>   	NVME_NS_NO_WRITE_PROTECT = 0,

Looks good. Same comment wrt to 2.1 vs 2.0 as the previous patch.

Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>

Re: [PATCHv4 04/13] nvmet: implement supported features log

[Christoph Hellwig]

On Thu, Nov 07, 2024 at 11:38:39AM -0800, Keith Busch wrote:
> +	features->fis[NVME_FEAT_NUM_QUEUES] = cpu_to_le32(NVME_FIS_FSUPP |
> +							  NVME_FIS_CSCPE);
> +	features->fis[NVME_FEAT_KATO] = cpu_to_le32(NVME_FIS_FSUPP |
> +						    NVME_FIS_CSCPE);

Totally nitpicky, but I find this version much easier to read:

	features->fis[NVME_FEAT_NUM_QUEUES] =
		cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_CSCPE);
	features->fis[NVME_FEAT_KATO] =
		cpu_to_le32(NVME_FIS_FSUPP | NVME_FIS_CSCPE);

especially if the number of flags keeps growing.

Either way this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv4 05/13] nvmet: implement crto property

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This property is required for nvme 2.1. The target only supports ready
with media, so this is just the same value as CAP.TO.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/fabrics-cmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index c4b2eddd5666a..bb80db6a7950b 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -64,6 +64,9 @@ static void nvmet_execute_prop_get(struct nvmet_req *req)
 		case NVME_REG_CSTS:
 			val = ctrl->csts;
 			break;
+		case NVME_REG_CRTO:
+			val = NVME_CAP_TIMEOUT(ctrl->csts);
+			break;
 		default:
 			status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
 			break;
-- 
2.43.5

Re: [PATCHv4 05/13] nvmet: implement crto property

[Matias Bjørling]

On 07-11-2024 20:38, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This property is required for nvme 2.1. The target only supports ready
> with media, so this is just the same value as CAP.TO.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/fabrics-cmd.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index c4b2eddd5666a..bb80db6a7950b 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -64,6 +64,9 @@ static void nvmet_execute_prop_get(struct nvmet_req *req)
>   		case NVME_REG_CSTS:
>   			val = ctrl->csts;
>   			break;
> +		case NVME_REG_CRTO:
> +			val = NVME_CAP_TIMEOUT(ctrl->csts);
> +			break;
>   		default:
>   			status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
>   			break;

Looks good.

Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>

[PATCHv4 06/13] nvmet: declare 2.1 version compliance

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The target driver implements all the mandatory logs, identifications,
features, and properties up to nvme sepcification 2.1.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/nvmet.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 190f55e6d7532..f902a630a5cf4 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -21,7 +21,7 @@
 #include <linux/radix-tree.h>
 #include <linux/t10-pi.h>
 
-#define NVMET_DEFAULT_VS		NVME_VS(1, 3, 0)
+#define NVMET_DEFAULT_VS		NVME_VS(2, 1, 0)
 
 #define NVMET_ASYNC_EVENTS		4
 #define NVMET_ERROR_LOG_SLOTS		128
-- 
2.43.5

Re: [PATCHv4 06/13] nvmet: declare 2.1 version compliance

[Matias Bjørling]

On 07-11-2024 20:38, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The target driver implements all the mandatory logs, identifications,
> features, and properties up to nvme sepcification 2.1.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/nvmet.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 190f55e6d7532..f902a630a5cf4 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -21,7 +21,7 @@
>   #include <linux/radix-tree.h>
>   #include <linux/t10-pi.h>
>   
> -#define NVMET_DEFAULT_VS		NVME_VS(1, 3, 0)
> +#define NVMET_DEFAULT_VS		NVME_VS(2, 1, 0)
>   
>   #define NVMET_ASYNC_EVENTS		4
>   #define NVMET_ERROR_LOG_SLOTS		128

Looks good.

Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>

[PATCHv4 07/13] nvmet: implement endurance groups

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Most of the returned information is just stubbed data. The target must
support these in order to report rotational media. Since this driver
doesn't know any better, each namespace is its own endurance group with
the engid value matching the nsid.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c        |  1 +
 drivers/nvme/target/admin-cmd.c | 95 +++++++++++++++++++++++++++++++++
 include/linux/nvme.h            | 29 +++++++++-
 3 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e9aac07f4c26d..426d4b90ecd7e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5001,6 +5001,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_id_ctrl_nvm) != NVME_IDENTIFY_DATA_SIZE);
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
+	BUILD_BUG_ON(sizeof(struct nvme_endurance_group_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_feat_host_behavior) != 512);
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 7a879e2cd8555..db26cd50be909 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -88,6 +88,7 @@ static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
 	logs->lids[NVME_LOG_FW_SLOT] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_CHANGED_NS] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_CMD_EFFECTS] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_ENDURANCE_GROUP] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_ANA] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_FEATURES] = cpu_to_le32(NVME_LIDS_LSUPP);
 
@@ -298,6 +299,49 @@ static u32 nvmet_format_ana_group(struct nvmet_req *req, u32 grpid,
 	return struct_size(desc, nsids, count);
 }
 
+static void nvmet_execute_get_log_page_endgrp(struct nvmet_req *req)
+{
+	u64 host_reads, host_writes, data_units_read, data_units_written;
+	struct nvme_endurance_group_log *log;
+	u16 status;
+
+	/*
+	 * The target driver emulates each endurance group as its own
+	 * namespace, reusing the nsid as the endurance group identifier.
+	 */
+	req->cmd->common.nsid = cpu_to_le32(le16_to_cpu(
+					    req->cmd->get_log_page.lsi));
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL);
+	if (!log) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	if (!req->ns->bdev)
+		goto copy;
+
+	host_reads = part_stat_read(req->ns->bdev, ios[READ]);
+	data_units_read =
+		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(req->ns->bdev, sectors[WRITE]), 1000);
+
+	put_unaligned_le64(host_reads, &log->hrc[0]);
+	put_unaligned_le64(data_units_read, &log->dur[0]);
+	put_unaligned_le64(host_writes, &log->hwc[0]);
+	put_unaligned_le64(data_units_written, &log->duw[0]);
+copy:
+	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
+	kfree(log);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 {
 	struct nvme_ana_rsp_hdr hdr = { 0, };
@@ -394,6 +438,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_changed_ns(req);
 	case NVME_LOG_CMD_EFFECTS:
 		return nvmet_execute_get_log_cmd_effects_ns(req);
+	case NVME_LOG_ENDURANCE_GROUP:
+		return nvmet_execute_get_log_page_endgrp(req);
 	case NVME_LOG_ANA:
 		return nvmet_execute_get_log_page_ana(req);
 	case NVME_LOG_FEATURES:
@@ -525,6 +571,13 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	id->msdbd = ctrl->ops->msdbd;
 
+	/*
+	 * Endurance group identifier is 16 bits, so we can't let namespaces
+	 * overflow that since we reuse the nsid
+	 */
+	BUILD_BUG_ON(NVMET_MAX_NAMESPACES > USHRT_MAX);
+	id->endgidmax = cpu_to_le16(NVMET_MAX_NAMESPACES);
+
 	id->anacap = (1 << 0) | (1 << 1) | (1 << 2) | (1 << 3) | (1 << 4);
 	id->anatt = 10; /* random value */
 	id->anagrpmax = cpu_to_le32(NVMET_MAX_ANAGRPS);
@@ -609,6 +662,12 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	id->nmic = NVME_NS_NMIC_SHARED;
 	id->anagrpid = cpu_to_le32(req->ns->anagrpid);
 
+	/*
+	 * Since we don't know any better, every namespace is its own endurance
+	 * group.
+	 */
+	id->endgid = cpu_to_le16(req->ns->nsid);
+
 	memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
 
 	id->lbaf[0].ds = req->ns->blksize_shift;
@@ -634,6 +693,39 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_identify_endgrp_list(struct nvmet_req *req)
+{
+	u16 min_endgid = le16_to_cpu(req->cmd->identify.cnssid);
+	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvmet_ns *ns;
+	unsigned long idx;
+	__le16 *list;
+	u16 status;
+	int i = 1;
+
+	list = kzalloc(buf_size, GFP_KERNEL);
+	if (!list) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
+		if (ns->nsid <= min_endgid)
+			continue;
+
+		list[i++] = cpu_to_le16(ns->nsid);
+		if (i == buf_size / sizeof(__le16))
+			break;
+	}
+
+	list[0] = cpu_to_le16(i - 1);
+	status = nvmet_copy_to_sgl(req, 0, list, buf_size);
+	kfree(list);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 {
 	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
@@ -806,6 +898,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
 		nvmet_execute_identify_nslist(req, true);
 		return;
+	case NVME_ID_CNS_ENDGRP_LIST:
+		nvmet_execute_identify_endgrp_list(req);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 4b896fa651e02..9b5867f1aa591 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -327,7 +327,8 @@ struct nvme_id_ctrl {
 	__le32			sanicap;
 	__le32			hmminds;
 	__le16			hmmaxd;
-	__u8			rsvd338[4];
+	__le16			nvmsetidmax;
+	__le16			endgidmax;
 	__u8			anatt;
 	__u8			anacap;
 	__le32			anagrpmax;
@@ -531,6 +532,7 @@ enum {
 	NVME_ID_CNS_SCNDRY_CTRL_LIST	= 0x15,
 	NVME_ID_CNS_NS_GRANULARITY	= 0x16,
 	NVME_ID_CNS_UUID_LIST		= 0x17,
+	NVME_ID_CNS_ENDGRP_LIST		= 0x19,
 };
 
 enum {
@@ -618,6 +620,28 @@ enum {
 	NVME_NIDT_CSI		= 0x04,
 };
 
+struct nvme_endurance_group_log {
+	__u8	egcw;
+	__u8	egfeat;
+	__u8	rsvd2;
+	__u8	avsp;
+	__u8	avspt;
+	__u8	pused;
+	__le16	did;
+	__u8	rsvd8[24];
+	__u8	ee[16];
+	__u8	dur[16];
+	__u8	duw[16];
+	__u8	muw[16];
+	__u8	hrc[16];
+	__u8	hwc[16];
+	__u8	mdie[16];
+	__u8	neile[16];
+	__u8	tegcap[16];
+	__u8	uegcap[16];
+	__u8	rsvd192[320];
+};
+
 struct nvme_smart_log {
 	__u8			critical_warning;
 	__u8			temperature[2];
@@ -1302,7 +1326,8 @@ struct nvme_identify {
 	__u8			cns;
 	__u8			rsvd3;
 	__le16			ctrlid;
-	__u8			rsvd11[3];
+	__le16			cnssid;
+	__u8			rsvd11;
 	__u8			csi;
 	__u32			rsvd12[4];
 };
-- 
2.43.5

Re: [PATCHv4 07/13] nvmet: implement endurance groups

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv4 08/13] nvmet: implement rotational media information log

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Most of the information is stubbed. Supporting this log is a requirement
for supporting rotational media.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c        |  1 +
 drivers/nvme/target/admin-cmd.c | 42 +++++++++++++++++++++++++++++++++
 include/linux/nvme.h            | 15 +++++++++++-
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 426d4b90ecd7e..279b0f445904d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5002,6 +5002,7 @@ static inline void _nvme_check_size(void)
 	BUILD_BUG_ON(sizeof(struct nvme_lba_range_type) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_smart_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_endurance_group_log) != 512);
+	BUILD_BUG_ON(sizeof(struct nvme_rotational_media_log) != 512);
 	BUILD_BUG_ON(sizeof(struct nvme_dbbuf) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_directive_cmd) != 64);
 	BUILD_BUG_ON(sizeof(struct nvme_feat_host_behavior) != 512);
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index db26cd50be909..6d3a1d2103bf3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -91,6 +91,7 @@ static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
 	logs->lids[NVME_LOG_ENDURANCE_GROUP] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_ANA] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_FEATURES] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_RMI] = cpu_to_le32(NVME_LIDS_LSUPP);
 
 	status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
 	kfree(logs);
@@ -157,6 +158,45 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	return NVME_SC_SUCCESS;
 }
 
+static void nvmet_execute_get_log_page_rmi(struct nvmet_req *req)
+{
+	struct nvme_rotational_media_log *log;
+	struct gendisk *disk;
+	u16 status;
+
+	req->cmd->common.nsid = cpu_to_le32(le16_to_cpu(
+					    req->cmd->get_log_page.lsi));
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	if (!req->ns->bdev || bdev_nonrot(req->ns->bdev)) {
+		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+		goto out;
+	}
+
+	if (req->transfer_len != sizeof(*log)) {
+		status = NVME_SC_SGL_INVALID_DATA | NVME_STATUS_DNR;
+		goto out;
+	}
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL);
+	if (!log)
+		goto out;
+
+	log->endgid = req->cmd->get_log_page.lsi;
+	disk = req->ns->bdev->bd_disk;
+	if (disk && disk->ia_ranges)
+		log->numa = cpu_to_le16(disk->ia_ranges->nr_ia_ranges);
+	else
+		log->numa = cpu_to_le16(1);
+
+	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
+	kfree(log);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 {
 	struct nvme_smart_log *log;
@@ -444,6 +484,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_page_ana(req);
 	case NVME_LOG_FEATURES:
 		return nvmet_execute_get_log_page_features(req);
+	case NVME_LOG_RMI:
+		return nvmet_execute_get_log_page_rmi(req);
 	}
 	pr_debug("unhandled lid %d on qid %d\n",
 	       req->cmd->get_log_page.lid, req->sq->qid);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 9b5867f1aa591..93a0abfab5b0e 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -642,6 +642,18 @@ struct nvme_endurance_group_log {
 	__u8	rsvd192[320];
 };
 
+struct nvme_rotational_media_log {
+	__le16	endgid;
+	__le16	numa;
+	__le16	nrs;
+	__u8	rsvd6[2];
+	__le32	spinc;
+	__le32	fspinc;
+	__le32	ldc;
+	__le32	fldc;
+	__u8	rsvd24[488];
+};
+
 struct nvme_smart_log {
 	__u8			critical_warning;
 	__u8			temperature[2];
@@ -1281,6 +1293,7 @@ enum {
 	NVME_LOG_ENDURANCE_GROUP = 0x09,
 	NVME_LOG_ANA		= 0x0c,
 	NVME_LOG_FEATURES	= 0x12,
+	NVME_LOG_RMI		= 0x16,
 	NVME_LOG_DISC		= 0x70,
 	NVME_LOG_RESERVATION	= 0x80,
 	NVME_FWACT_REPL		= (0 << 3),
@@ -1435,7 +1448,7 @@ struct nvme_get_log_page_command {
 	__u8			lsp; /* upper 4 bits reserved */
 	__le16			numdl;
 	__le16			numdu;
-	__u16			rsvd11;
+	__le16			lsi;
 	union {
 		struct {
 			__le32 lpol;
-- 
2.43.5

[PATCHv4 09/13] nvmet: implement csi identify ns

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Implements reporting the I/O Command Set Independent Identify Namespace
command.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/nvme.h            |  1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 6d3a1d2103bf3..516af7b419189 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -893,6 +893,35 @@ static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
+{
+	struct nvme_id_ns_cs_indep *id;
+	u16 status;
+
+	status = nvmet_req_find_ns(req);
+	if (status)
+		goto out;
+
+	id = kzalloc(sizeof(*id), GFP_KERNEL);
+	if (!id) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	id->nstat = NVME_NSTAT_NRDY;
+	id->anagrpid = req->ns->anagrpid;
+	id->nmic = NVME_NS_NMIC_SHARED;
+	if (req->ns->readonly)
+		id->nsattr |= NVME_NS_ATTR_RO;
+	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
+		id->nsfeat |= NVME_NS_ROTATIONAL;
+
+	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
+	kfree(id);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_identify(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -940,6 +969,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
 		nvmet_execute_identify_nslist(req, true);
 		return;
+	case NVME_ID_CNS_NS_CS_INDEP:
+		nvmet_execute_id_cs_indep(req);
+		return;
 	case NVME_ID_CNS_ENDGRP_LIST:
 		nvmet_execute_identify_endgrp_list(req);
 		return;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 93a0abfab5b0e..22375c87591a2 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -563,6 +563,7 @@ enum {
 	NVME_NS_FLBAS_LBA_SHIFT	= 1,
 	NVME_NS_FLBAS_META_EXT	= 0x10,
 	NVME_NS_NMIC_SHARED	= 1 << 0,
+	NVME_NS_ROTATIONAL	= 1 << 4,
 	NVME_LBAF_RP_BEST	= 0,
 	NVME_LBAF_RP_BETTER	= 1,
 	NVME_LBAF_RP_GOOD	= 2,
-- 
2.43.5

Re: [PATCHv4 09/13] nvmet: implement csi identify ns

[Guixin Liu]

在 2024/11/8 03:38, Keith Busch 写道:
> From: Keith Busch <kbusch@kernel.org>
>
> Implements reporting the I/O Command Set Independent Identify Namespace
> command.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 32 ++++++++++++++++++++++++++++++++
>   include/linux/nvme.h            |  1 +
>   2 files changed, 33 insertions(+)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 6d3a1d2103bf3..516af7b419189 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -893,6 +893,35 @@ static void nvme_execute_identify_ns_nvm(struct nvmet_req *req)
>   	nvmet_req_complete(req, status);
>   }
>   
> +static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
> +{
> +	struct nvme_id_ns_cs_indep *id;
> +	u16 status;
> +
> +	status = nvmet_req_find_ns(req);
> +	if (status)
> +		goto out;
> +
> +	id = kzalloc(sizeof(*id), GFP_KERNEL);
> +	if (!id) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	id->nstat = NVME_NSTAT_NRDY;
> +	id->anagrpid = req->ns->anagrpid;

Well, I think you overlooked my comments for v3,

should use cpu_to_le32(req->ns->anagrpid) here.

Best Regards,

Guixin Liu

> +	id->nmic = NVME_NS_NMIC_SHARED;
> +	if (req->ns->readonly)
> +		id->nsattr |= NVME_NS_ATTR_RO;
> +	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
> +		id->nsfeat |= NVME_NS_ROTATIONAL;
> +
> +	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
> +	kfree(id);
> +out:
> +	nvmet_req_complete(req, status);
> +}
> +
>   static void nvmet_execute_identify(struct nvmet_req *req)
>   {
>   	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
> @@ -940,6 +969,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>   	case NVME_ID_CNS_NS_ACTIVE_LIST_CS:
>   		nvmet_execute_identify_nslist(req, true);
>   		return;
> +	case NVME_ID_CNS_NS_CS_INDEP:
> +		nvmet_execute_id_cs_indep(req);
> +		return;
>   	case NVME_ID_CNS_ENDGRP_LIST:
>   		nvmet_execute_identify_endgrp_list(req);
>   		return;
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 93a0abfab5b0e..22375c87591a2 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -563,6 +563,7 @@ enum {
>   	NVME_NS_FLBAS_LBA_SHIFT	= 1,
>   	NVME_NS_FLBAS_META_EXT	= 0x10,
>   	NVME_NS_NMIC_SHARED	= 1 << 0,
> +	NVME_NS_ROTATIONAL	= 1 << 4,
>   	NVME_LBAF_RP_BEST	= 0,
>   	NVME_LBAF_RP_BETTER	= 1,
>   	NVME_LBAF_RP_GOOD	= 2,

Re: [PATCHv4 09/13] nvmet: implement csi identify ns

[Christoph Hellwig]

Looks good with the missing anagripd endianess conversion added:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv4 10/13] nvme: use command set independent id ns if available

[Keith Busch]

From: Matias Bjørling <matias.bjorling@wdc.com>

The NVMe 2.0 specification adds an independent identify namespace
data structure that contains generic attributes that apply to all
namespace types. Some attributes carry over from the NVM command set
identify namespace data structure, and others are new.

Currently, the data structure only considered when CRIMS is enabled or
when the namespace type is key-value.

However, the independent namespace data structure is mandatory for
devices that implement features from the 2.0+ specification. Therefore,
we can check this data structure first. If unavailable, retrieve the
generic attributes from the NVM command set identify namespace data
structure.

Signed-off-by: Matias Bjørling <matias.bjorling@wdc.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 279b0f445904d..162889b6382aa 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3988,7 +3988,7 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns_info info = { .nsid = nsid };
 	struct nvme_ns *ns;
-	int ret;
+	int ret = 1;
 
 	if (nvme_identify_ns_descs(ctrl, &info))
 		return;
@@ -4005,9 +4005,10 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	 * set up a namespace.  If not fall back to the legacy version.
 	 */
 	if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
-	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS))
+	    (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS) ||
+	    ctrl->vs >= NVME_VS(2, 0, 0))
 		ret = nvme_ns_info_from_id_cs_indep(ctrl, &info);
-	else
+	if (ret > 0)
 		ret = nvme_ns_info_from_identify(ctrl, &info);
 
 	if (info.is_removed)
-- 
2.43.5

Re: [PATCHv4 10/13] nvme: use command set independent id ns if available

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv4 11/13] nvme: add rotational support

[Keith Busch]

From: Wang Yugui <wangyugui@e16-tech.com>

Rotational devices, such as hard-drives, can be detected using
the rotational bit in the namespace independent identify namespace
data structure. Make the bit visible to the block layer through the
rotational queue setting.

Signed-off-by: Wang Yugui <wangyugui@e16-tech.com>
Reviewed-by: Matias Bjørling <matias.bjorling@wdc.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 6 ++++++
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 162889b6382aa..6f51dde7de6ce 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -42,6 +42,7 @@ struct nvme_ns_info {
 	bool is_readonly;
 	bool is_ready;
 	bool is_removed;
+	bool is_rotational;
 };
 
 unsigned int admin_timeout = 60;
@@ -1615,6 +1616,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
 		info->is_shared = id->nmic & NVME_NS_NMIC_SHARED;
 		info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
 		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
+		info->is_rotational = id->nsfeat & NVME_NS_ROTATIONAL;
 	}
 	kfree(id);
 	return ret;
@@ -2162,6 +2164,9 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	else
 		lim.features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA);
 
+	if (info->is_rotational)
+		lim.features |= BLK_FEAT_ROTATIONAL;
+
 	/*
 	 * Register a metadata profile for PI, or the plain non-integrity NVMe
 	 * metadata masquerading as Type 0 if supported, otherwise reject block
@@ -3608,6 +3613,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	head->ns_id = info->nsid;
 	head->ids = info->ids;
 	head->shared = info->is_shared;
+	head->rotational = info->is_rotational;
 	ratelimit_state_init(&head->rs_nuse, 5 * HZ, 1);
 	ratelimit_set_flags(&head->rs_nuse, RATELIMIT_MSG_ON_RELEASE);
 	kref_init(&head->ref);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 093cb423f536b..900719c4c70c1 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -474,6 +474,7 @@ struct nvme_ns_head {
 	struct list_head	entry;
 	struct kref		ref;
 	bool			shared;
+	bool			rotational;
 	bool			passthru_err_log_enabled;
 	struct nvme_effects_log *effects;
 	u64			nuse;
-- 
2.43.5

Re: [PATCHv4 11/13] nvme: add rotational support

[Guixin Liu]

Reviewed-by: Guixin Liu <kanie@linux.alibaba.com>

[PATCHv4 12/13] nvme: check ns's volatile write cache not present

[Keith Busch]

From: Guixin Liu <kanie@linux.alibaba.com>

When the VWC of a namespace does not exist, the BLK_FEAT_WRITE_CACHE
flag should not be set when registering the block device, regardless
of whether the controller supports VWC.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 4 +++-
 include/linux/nvme.h     | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6f51dde7de6ce..e119ba0f8ab8b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -43,6 +43,7 @@ struct nvme_ns_info {
 	bool is_ready;
 	bool is_removed;
 	bool is_rotational;
+	bool no_vwc;
 };
 
 unsigned int admin_timeout = 60;
@@ -1617,6 +1618,7 @@ static int nvme_ns_info_from_id_cs_indep(struct nvme_ctrl *ctrl,
 		info->is_readonly = id->nsattr & NVME_NS_ATTR_RO;
 		info->is_ready = id->nstat & NVME_NSTAT_NRDY;
 		info->is_rotational = id->nsfeat & NVME_NS_ROTATIONAL;
+		info->no_vwc = id->nsfeat & NVME_NS_VWC_NOT_PRESENT;
 	}
 	kfree(id);
 	return ret;
@@ -2159,7 +2161,7 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
 	    ns->head->ids.csi == NVME_CSI_ZNS)
 		nvme_update_zone_info(ns, &lim, &zi);
 
-	if (ns->ctrl->vwc & NVME_CTRL_VWC_PRESENT)
+	if ((ns->ctrl->vwc & NVME_CTRL_VWC_PRESENT) && !info->no_vwc)
 		lim.features |= BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA;
 	else
 		lim.features &= ~(BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 22375c87591a2..5c6d065c2f178 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -564,6 +564,7 @@ enum {
 	NVME_NS_FLBAS_META_EXT	= 0x10,
 	NVME_NS_NMIC_SHARED	= 1 << 0,
 	NVME_NS_ROTATIONAL	= 1 << 4,
+	NVME_NS_VWC_NOT_PRESENT = 1 << 5,
 	NVME_LBAF_RP_BEST	= 0,
 	NVME_LBAF_RP_BETTER	= 1,
 	NVME_LBAF_RP_GOOD	= 2,
-- 
2.43.5

[PATCHv4 13/13] nvmet: report ns's vwc not present

[Keith Busch]

From: Guixin Liu <kanie@linux.alibaba.com>

Currently, we report that controller has vwc even though the ns may
not have vwc. Report ns's vwc not present when not buffered_io or
backdev doesn't have vwc.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/target/admin-cmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 516af7b419189..f688d7fdf7d67 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -915,6 +915,9 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
 		id->nsattr |= NVME_NS_ATTR_RO;
 	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
 		id->nsfeat |= NVME_NS_ROTATIONAL;
+	if ((req->ns->file && !req->ns->buffered_io) ||
+	    (req->ns->bdev && !bdev_write_cache(req->ns->bdev)))
+		id->nsfeat |= NVME_NS_VWC_NOT_PRESENT;
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 	kfree(id);
-- 
2.43.5

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Christoph Hellwig]

On Thu, Nov 07, 2024 at 11:38:48AM -0800, Keith Busch wrote:
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 516af7b419189..f688d7fdf7d67 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -915,6 +915,9 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
>  		id->nsattr |= NVME_NS_ATTR_RO;
>  	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
>  		id->nsfeat |= NVME_NS_ROTATIONAL;
> +	if ((req->ns->file && !req->ns->buffered_io) ||

I don't think this check is correct.  Direct I/O still requires cache
flushes for durability.

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Keith Busch]

On Fri, Nov 08, 2024 at 03:27:43PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 07, 2024 at 11:38:48AM -0800, Keith Busch wrote:
> > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> > index 516af7b419189..f688d7fdf7d67 100644
> > --- a/drivers/nvme/target/admin-cmd.c
> > +++ b/drivers/nvme/target/admin-cmd.c
> > @@ -915,6 +915,9 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
> >  		id->nsattr |= NVME_NS_ATTR_RO;
> >  	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
> >  		id->nsfeat |= NVME_NS_ROTATIONAL;
> > +	if ((req->ns->file && !req->ns->buffered_io) ||
> 
> I don't think this check is correct.  Direct I/O still requires cache
> flushes for durability.

Oh, right. Would it be sufficient to check the file's block device for
write caching?

	if ((req->ns->file && !req->ns->buffered_io &&
		bdev_write_cache(file_inode(req->ns->file)->i_sb->s_bdev))

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Guixin Liu]

在 2024/11/9 00:15, Keith Busch 写道:
> On Fri, Nov 08, 2024 at 03:27:43PM +0100, Christoph Hellwig wrote:
>> On Thu, Nov 07, 2024 at 11:38:48AM -0800, Keith Busch wrote:
>>> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
>>> index 516af7b419189..f688d7fdf7d67 100644
>>> --- a/drivers/nvme/target/admin-cmd.c
>>> +++ b/drivers/nvme/target/admin-cmd.c
>>> @@ -915,6 +915,9 @@ static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
>>>   		id->nsattr |= NVME_NS_ATTR_RO;
>>>   	if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
>>>   		id->nsfeat |= NVME_NS_ROTATIONAL;
>>> +	if ((req->ns->file && !req->ns->buffered_io) ||
>> I don't think this check is correct.  Direct I/O still requires cache
>> flushes for durability.
> Oh, right. Would it be sufficient to check the file's block device for
> write caching?
>
> 	if ((req->ns->file && !req->ns->buffered_io &&
> 		bdev_write_cache(file_inode(req->ns->file)->i_sb->s_bdev))

Or just remove !req->ns->buffered_io check? All file backend return

support vwc.

Best Regards,

Guixin Liu

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Guixin Liu]

在 2024/11/9 22:59, Guixin Liu 写道:
>
> 在 2024/11/9 00:15, Keith Busch 写道:
>> On Fri, Nov 08, 2024 at 03:27:43PM +0100, Christoph Hellwig wrote:
>>> On Thu, Nov 07, 2024 at 11:38:48AM -0800, Keith Busch wrote:
>>>> diff --git a/drivers/nvme/target/admin-cmd.c 
>>>> b/drivers/nvme/target/admin-cmd.c
>>>> index 516af7b419189..f688d7fdf7d67 100644
>>>> --- a/drivers/nvme/target/admin-cmd.c
>>>> +++ b/drivers/nvme/target/admin-cmd.c
>>>> @@ -915,6 +915,9 @@ static void nvmet_execute_id_cs_indep(struct 
>>>> nvmet_req *req)
>>>>           id->nsattr |= NVME_NS_ATTR_RO;
>>>>       if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
>>>>           id->nsfeat |= NVME_NS_ROTATIONAL;
>>>> +    if ((req->ns->file && !req->ns->buffered_io) ||
>>> I don't think this check is correct.  Direct I/O still requires cache
>>> flushes for durability.
>> Oh, right. Would it be sufficient to check the file's block device for
>> write caching?
>>
>>     if ((req->ns->file && !req->ns->buffered_io &&
>> bdev_write_cache(file_inode(req->ns->file)->i_sb->s_bdev))
>
> Or just remove !req->ns->buffered_io check? All file backend return
>
> support vwc.
Sorry, remove "req->ns->file && !req->ns->buffered_io".
>
> Best Regards,
>
> Guixin Liu
>

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Keith Busch]

On Sat, Nov 09, 2024 at 11:00:32PM +0800, Guixin Liu wrote:
> 在 2024/11/9 22:59, Guixin Liu 写道:
> > 
> > Or just remove !req->ns->buffered_io check? All file backend return
> > 
> > support vwc.
> Sorry, remove "req->ns->file && !req->ns->buffered_io".

Do you want to send an update the with the updated check?

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Guixin Liu]

在 2024/11/13 06:49, Keith Busch 写道:
> On Sat, Nov 09, 2024 at 11:00:32PM +0800, Guixin Liu wrote:
>> 在 2024/11/9 22:59, Guixin Liu 写道:
>>> Or just remove !req->ns->buffered_io check? All file backend return
>>>
>>> support vwc.
>> Sorry, remove "req->ns->file && !req->ns->buffered_io".
> Do you want to send an update the with the updated check?

Sure, I will send an update one.

Best Regards,

Guixin Liu

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Christoph Hellwig]

On Sat, Nov 09, 2024 at 10:59:19PM +0800, Guixin Liu wrote:
>> 	if ((req->ns->file && !req->ns->buffered_io &&
>> 		bdev_write_cache(file_inode(req->ns->file)->i_sb->s_bdev))
>
> Or just remove !req->ns->buffered_io check? All file backend return
>
> support vwc.

Yes, that is the right thing to do.

Re: [PATCHv4 13/13] nvmet: report ns's vwc not present

[Christoph Hellwig]

On Fri, Nov 08, 2024 at 09:15:47AM -0700, Keith Busch wrote:
> > > +	if ((req->ns->file && !req->ns->buffered_io) ||
> > 
> > I don't think this check is correct.  Direct I/O still requires cache
> > flushes for durability.
> 
> Oh, right. Would it be sufficient to check the file's block device for
> write caching?
> 
> 	if ((req->ns->file && !req->ns->buffered_io &&
> 		bdev_write_cache(file_inode(req->ns->file)->i_sb->s_bdev))

No, that's wrong for two reasons:

 1) sb->s_bdev is only really relevant for mounting and st_dev reported
    in stat.  It might not relate to where the file data is placed at
    all
 2) the presence of cache in the underlying block device only has a
    minimal relation to the need to fdatasync a file you wrote to.
    Except for the corner case of overwritting a fully allocated (and
    not just preallocated) extent on an in-place write file system there
    always is metadata for a write that needs to be synchronized.
    You could work around that with an O_DYNC write that does that
    sync per I/O, but performance would be awful (and nvmet donsn't
    support that anyway)

Re: [PATCHv4 00/13] nvme target 2.1 and independent identify ns

[Keith Busch]

Patches 1-12 applied after merging to nvme-6.13, folding in the
reservation logs and features. Also included feedback from patch 4 (code
style alignment suggestion) and patch 9 (cpu_to_le32(anagrpid)).

Patch 13 is on hold.