[PATCH RFC 0/3] set nvme ns's vwc respectively

[PATCH RFC 0/3] set nvme ns's vwc respectively

[Guixin Liu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 724 bytes --]

Hi,
  These patches are supporting separate vwc attr for each namespace.

  I found that the number 2 and 3 patch are conflict with Matias Bjørling's
patch which implement to supporting rotational device, this is the first
time this happend to me, I dont know how to handle this, so there is a "RFC"
tag in tittle, waiting for the solution.

Guixin Liu (3):
  nvme: check ns's volatile write cache not present
  nvme: query independent identify ns by default
  nvmet: report ns's vwc not present

 drivers/nvme/host/core.c        | 11 +++++-----
 drivers/nvme/target/admin-cmd.c | 36 +++++++++++++++++++++++++++++++++
 include/linux/nvme.h            |  1 +
 3 files changed, 43 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH RFC 1/3] nvme: check ns's volatile write cache not present

[Guixin Liu]

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>
---
 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 b149b638453f..eed41d4d2599 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 no_vwc;
 };
 
 unsigned int admin_timeout = 60;
@@ -1639,6 +1640,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->no_vwc = id->nsfeat & NVME_NS_VWC_NOT_PRESENT;
 	}
 	kfree(id);
 	return ret;
@@ -2185,7 +2187,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 b58d9405d65e..6ce2533cf9b4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -560,6 +560,7 @@ enum {
 	NVME_NS_FLBAS_LBA_SHIFT	= 1,
 	NVME_NS_FLBAS_META_EXT	= 0x10,
 	NVME_NS_NMIC_SHARED	= 1 << 0,
+	NVME_NS_VWC_NOT_PRESENT = 1 << 5,
 	NVME_LBAF_RP_BEST	= 0,
 	NVME_LBAF_RP_BETTER	= 1,
 	NVME_LBAF_RP_GOOD	= 2,
-- 
2.43.0

Re: [PATCH RFC 1/3] nvme: check ns's volatile write cache not present

[Christoph Hellwig]

On Mon, Nov 04, 2024 at 04:55:00PM +0800, Guixin Liu wrote:
> 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.

Looks good:

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

[PATCH RFC 2/3] nvme: query independent identify ns by default

[Guixin Liu]

Query independent identify ns by default, if returning fail,
go back to sending the "identify namespace data structure for the
specified NSID or the namespace capabilities for the NVM Command
Set(cns = 0)".

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index eed41d4d2599..563abc0cb4a1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4018,7 +4018,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;
@@ -4034,10 +4034,9 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	 * data structure to find all the generic information that is needed to
 	 * 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))
+	if (nvme_id_cns_ok(ctrl, NVME_ID_CNS_NS_CS_INDEP))
 		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.0

Re: [PATCH RFC 2/3] nvme: query independent identify ns by default

[Christoph Hellwig]

On Mon, Nov 04, 2024 at 04:55:01PM +0800, Guixin Liu wrote:
> Query independent identify ns by default, if returning fail,
> go back to sending the "identify namespace data structure for the
> specified NSID or the namespace capabilities for the NVM Command
> Set(cns = 0)".

How is this related to the rest of the series?

Re: [PATCH RFC 2/3] nvme: query independent identify ns by default

[Keith Busch]

On Tue, Nov 05, 2024 at 12:11:06PM +0100, Christoph Hellwig wrote:
> On Mon, Nov 04, 2024 at 04:55:01PM +0800, Guixin Liu wrote:
> > Query independent identify ns by default, if returning fail,
> > go back to sending the "identify namespace data structure for the
> > specified NSID or the namespace capabilities for the NVM Command
> > Set(cns = 0)".
> 
> How is this related to the rest of the series?

Patch 3 provides target support for the VWCNP flag, but the host driver
wouldn't check it without this patch. I'm assuming that's why this patch
is included here: so both sides can be tested.

But if you have the time to review a different patchset, I would prefer
we get nvmet up to 2.1 compliance and restrict this identification to
2.0 and above where it's required.

Re: [PATCH RFC 2/3] nvme: query independent identify ns by default

[Guixin Liu]

在 2024/11/6 04:03, Keith Busch 写道:
> On Tue, Nov 05, 2024 at 12:11:06PM +0100, Christoph Hellwig wrote:
>> On Mon, Nov 04, 2024 at 04:55:01PM +0800, Guixin Liu wrote:
>>> Query independent identify ns by default, if returning fail,
>>> go back to sending the "identify namespace data structure for the
>>> specified NSID or the namespace capabilities for the NVM Command
>>> Set(cns = 0)".
>> How is this related to the rest of the series?
> Patch 3 provides target support for the VWCNP flag, but the host driver
> wouldn't check it without this patch. I'm assuming that's why this patch
> is included here: so both sides can be tested.
Yes.
> But if you have the time to review a different patchset, I would prefer
> we get nvmet up to 2.1 compliance and restrict this identification to
> 2.0 and above where it's required.

Yeah sure, I see the Matias's patchset too, you could apply my number 
one patch

first, when the nvmet's version increased to 2.1 and Matia's patchset 
applied,

I will continue to working on nvme target's ns respective vwcnp.

Best Regards,

Guixin Liu

[PATCH RFC 3/3] nvmet: report ns's vwc not present

[Guixin Liu]

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>
---
 drivers/nvme/target/admin-cmd.c | 36 +++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 081f0473cd9e..dffb84f560ad 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -685,6 +685,39 @@ static void nvmet_execute_identify_ctrl_nvm(struct nvmet_req *req)
 		   nvmet_zero_sgl(req, 0, sizeof(struct nvme_id_ctrl_nvm)));
 }
 
+static void nvmet_execute_identify_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;
+	}
+
+	if ((req->ns->file && !req->ns->buffered_io) ||
+	    (req->ns->bdev && !bdev_write_cache(req->ns->bdev)))
+		id->nsfeat |= NVME_NS_VWC_NOT_PRESENT;
+
+	id->nmic |= NVME_NS_NMIC_SHARED;
+	///TODO: report reservation supported when that patch applied.
+	id->anagrpid = cpu_to_le32(req->ns->anagrpid);
+
+	if (req->ns->readonly)
+		id->nsattr |= NVME_NS_ATTR_RO;
+	id->nstat = NVME_NSTAT_NRDY;
+
+	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))
@@ -729,6 +762,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 			break;
 		}
 		break;
+	case NVME_ID_CNS_NS_CS_INDEP:
+		nvmet_execute_identify_cs_indep(req);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
-- 
2.43.0