[PATCH 0/3 v2] nvme: add rotational support

[PATCH 0/3 v2] nvme: add rotational support

[Matias Bjørling]

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

Enable support for NVMe devices that identifies as rotational.

Changes since v1:

 - Removed random entropy addition.
 - Replaced the nvme rotational patch with the work of Wang Yugui.
 - Included Keith's nvmet test code.

Note: Please expect delay on v3 as I'll be out next week.

Keith Busch (1):
  nvmet: add rotational support

Matias Bjørling (1):
  nvme: make independent ns identify default

Wang Yugui (1):
  nvme: add rotational support

 drivers/nvme/host/core.c        | 13 +++++++++----
 drivers/nvme/host/nvme.h        |  1 +
 drivers/nvme/target/admin-cmd.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/nvme.h            |  1 +
 4 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.46.0

[PATCH 1/3 v2] nvme: make independent ns identify default

[Matias Bjørling]

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>
---
 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 0dc8bcc664f2..9cbef6342c39 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3999,7 +3999,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;
@@ -4015,10 +4015,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_ctrl_limited_cns(ctrl))
 		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.46.0

Re: [PATCH 1/3 v2] nvme: make independent ns identify default

[Christoph Hellwig]

On Thu, Oct 10, 2024 at 02:39:49PM +0200, Matias Bjørling wrote:
> 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.

FYI, I still disagree with this for the same reason as before.
Assuming we're not really going to see hard drivers I'd be fine
with using it by default for 2.0 (or better even 2.1) by default.

Re: [PATCH 1/3 v2] nvme: make independent ns identify default

[Matias Bjørling]

On Fri, Oct 11, 2024, at 11:14, Christoph Hellwig wrote:
> On Thu, Oct 10, 2024 at 02:39:49PM +0200, Matias Bjørling wrote:
>> 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.
>
> FYI, I still disagree with this for the same reason as before.
> Assuming we're not really going to see hard drivers I'd be fine
> with using it by default for 2.0 (or better even 2.1) by default.

Sounds good. When I am back after next week, I'll update the patch set with the version check and add the missing logic for hdd support. 

(Apologies for double e-mail. My phone client formatted the mail in html)

Re: [PATCH 1/3 v2] nvme: make independent ns identify default

[Keith Busch]

On Fri, Oct 11, 2024 at 10:14:52AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 10, 2024 at 02:39:49PM +0200, Matias Bjørling wrote:
> > 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.
> 
> FYI, I still disagree with this for the same reason as before.
> Assuming we're not really going to see hard drivers I'd be fine
> with using it by default for 2.0 (or better even 2.1) by default.

I've got the rest of the required logs and identifications implemented
in nvmet to support this. There's one more issue, though, if we do
restrict the identify to >= 2.0 or 2.1. nvmet reports 1.3, and I suspect
there's a bit more work than just changing the value of NVMET_DEFAULT_VS
in order to comply with claiming that version.

Re: [PATCH 1/3 v2] nvme: make independent ns identify default

[Matias Bjørling]

>> FYI, I still disagree with this for the same reason as before.
>> Assuming we're not really going to see hard drivers I'd be fine
>> with using it by default for 2.0 (or better even 2.1) by default.
> 
> I've got the rest of the required logs and identifications implemented
> in nvmet to support this. There's one more issue, though, if we do
> restrict the identify to >= 2.0 or 2.1. nvmet reports 1.3, and I suspect
> there's a bit more work than just changing the value of NVMET_DEFAULT_VS
> in order to comply with claiming that version.
> 

Awesome. I'll hold off the implementation. Would you like me to take 
your patches for a spin?

Re: [PATCH 1/3 v2] nvme: make independent ns identify default

[Christoph Hellwig]

On Fri, Nov 01, 2024 at 04:45:17PM -0600, Keith Busch wrote:
> I've got the rest of the required logs and identifications implemented
> in nvmet to support this.

Cool!

> There's one more issue, though, if we do
> restrict the identify to >= 2.0 or 2.1. nvmet reports 1.3, and I suspect
> there's a bit more work than just changing the value of NVMET_DEFAULT_VS
> in order to comply with claiming that version.

I don't think there were a lot of new mandatory requirements added that
we didn't already do, or already we already ignored anyway, but it is
worth a double check.

[PATCH 2/3 v2] nvme: add rotational support

[Matias Bjørling]

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: Keith Busch <kbusch@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>

larlar
---
 drivers/nvme/host/core.c | 6 ++++++
 drivers/nvme/host/nvme.h | 1 +
 include/linux/nvme.h     | 1 +
 3 files changed, 8 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9cbef6342c39..a83cf54c5861 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -41,6 +41,7 @@ struct nvme_ns_info {
 	bool is_readonly;
 	bool is_ready;
 	bool is_removed;
+	bool is_rotational;
 };
 
 unsigned int admin_timeout = 60;
@@ -1623,6 +1624,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;
@@ -2170,6 +2172,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
@@ -3619,6 +3624,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 da57947130cc..3a36548d5c05 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -469,6 +469,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;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 7b2ae2e43544..6d0eebb57544 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_ROTATIONAL	= 1 << 4,
 	NVME_LBAF_RP_BEST	= 0,
 	NVME_LBAF_RP_BETTER	= 1,
 	NVME_LBAF_RP_GOOD	= 2,
-- 
2.46.0

Re: [PATCH 2/3 v2] nvme: add rotational support

[Christoph Hellwig]

On Thu, Oct 10, 2024 at 02:39:50PM +0200, Matias Bjørling wrote:
> 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>

If you pass this on, this should be a Signed-off-by.

Otherwise looks good:

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

> larlar

?

[PATCH 3/3 v2] nvmet: add rotational support

[Matias Bjørling]

From: Keith Busch <kbusch@kernel.org>

Rotational block devices can be detected in NVMe through the rotational
attribute in the independent namespace identify data structure.

Extend nvmet with support for the independent namespace identify data
structure and expose the rotational support of the backend device.

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

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f7e1156ac7ec..f08f1226c897 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -675,6 +675,35 @@ 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_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))
@@ -719,6 +748,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
 			break;
 		}
 		break;
+	case NVME_ID_CNS_NS_CS_INDEP:
+		nvmet_execute_id_cs_indep(req);
+		return;
 	}
 
 	pr_debug("unhandled identify cns %d on qid %d\n",
-- 
2.46.0

Re: [PATCH 3/3 v2] nvmet: add rotational support

[Christoph Hellwig]

On Thu, Oct 10, 2024 at 02:39:51PM +0200, Matias Bjørling wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Rotational block devices can be detected in NVMe through the rotational
> attribute in the independent namespace identify data structure.
> 
> Extend nvmet with support for the independent namespace identify data
> structure and expose the rotational support of the backend device.

Most of this patches looks fine, but what it really is, is just an
implementation of the I/O Command Set Independent Identify
Namespace data structure.

NVMe actually requires more for rotational media support (quoting
from section 8.1.23 in the NVMe 2.1 Base Specification):

A controller that supports namespaces that store user data on rotational media
shall:

 a) set the Rotational Media bit to ‘1’ in the NSFEAT field of the I/O
    Command Set Independent Identify Namespace data structure (refer to
    the NVM Command Set Specification) for any namespace that stores data
    on rotational media;
 b) support the Rotational Media Information log page (refer to section
    5.1.12.1.22);
 c) support the Spinup Control feature (refer to section 5.1.25.1.18);
 d) support Endurance Groups (refer to section 3.2.3); and
 e) set the EG Rotational Media bit to ‘1’ in the EGFEAT field in the
    Endurance Group Information log page for each Endurance Group that
    stores data on rotational media.

So we'll need to implement a bit more here.  Most of this should be
pretty trivial stubby code, though.

> Signed-off-by: Keith Busch <kbusch@kernel.org>

This also needs your signoff if you pass it on.

Re: [PATCH 3/3 v2] nvmet: add rotational support

[kernel test robot]

Hi Matias,

kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.12-rc2]
[also build test WARNING on linus/master next-20241011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matias-Bj-rling/nvme-make-independent-ns-identify-default/20241010-204205
base:   v6.12-rc2
patch link:    https://lore.kernel.org/r/20241010123951.1226105-4-m%40bjorling.me
patch subject: [PATCH 3/3 v2] nvmet: add rotational support
config: i386-randconfig-062-20241011 (https://download.01.org/0day-ci/archive/20241012/202410120113.A3HaEkbg-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241012/202410120113.A3HaEkbg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410120113.A3HaEkbg-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/nvme/target/admin-cmd.c:704:22: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] anagrpid @@     got unsigned int [usertype] anagrpid @@
   drivers/nvme/target/admin-cmd.c:704:22: sparse:     expected restricted __le32 [usertype] anagrpid
   drivers/nvme/target/admin-cmd.c:704:22: sparse:     got unsigned int [usertype] anagrpid

vim +704 drivers/nvme/target/admin-cmd.c

   687	
   688	static void nvmet_execute_id_cs_indep(struct nvmet_req *req)
   689	{
   690		struct nvme_id_ns_cs_indep *id;
   691		u16 status;
   692	
   693		status = nvmet_req_find_ns(req);
   694		if (status)
   695			goto out;
   696	
   697		id = kzalloc(sizeof(*id), GFP_KERNEL);
   698		if (!id) {
   699			status = NVME_SC_INTERNAL;
   700			goto out;
   701		}
   702	
   703		id->nstat = NVME_NSTAT_NRDY;
 > 704		id->anagrpid = req->ns->anagrpid;
   705		id->nmic = NVME_NS_NMIC_SHARED;
   706		if (req->ns->readonly)
   707			id->nsattr |= NVME_NS_ATTR_RO;
   708		if (req->ns->bdev && !bdev_nonrot(req->ns->bdev))
   709			id->nsfeat |= NVME_NS_ROTATIONAL;
   710	
   711		status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
   712		kfree(id);
   713	out:
   714		nvmet_req_complete(req, status);
   715	}
   716	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3 v2] nvmet: add rotational support

[Guixin Liu]

在 2024/10/10 20:39, Matias Bjørling 写道:
> From: Keith Busch <kbusch@kernel.org>
>
> Rotational block devices can be detected in NVMe through the rotational
> attribute in the independent namespace identify data structure.
>
> Extend nvmet with support for the independent namespace identify data
> structure and expose the rotational support of the backend device.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/target/admin-cmd.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index f7e1156ac7ec..f08f1226c897 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -675,6 +675,35 @@ 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_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;

Hi Matias,

Here should use "cpu_to_le32(req->ns->anagrpid)",

And I send 3 patches to support ns's respective vwc which depends on 
your patch,

you can search "[PATCH RFC 0/3] set nvme ns's vwc respectively" to find 
my patches,

waiting for your patch applied, and I will work continue.

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))
> @@ -719,6 +748,9 @@ static void nvmet_execute_identify(struct nvmet_req *req)
>   			break;
>   		}
>   		break;
> +	case NVME_ID_CNS_NS_CS_INDEP:
> +		nvmet_execute_id_cs_indep(req);
> +		return;
>   	}
>   
>   	pr_debug("unhandled identify cns %d on qid %d\n",