[PATCH 0/2] nvme: add rotational support

[PATCH 0/2] nvme: add rotational support

[Matias Bjørling]

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

Enable support for NVMe devices that identifies as rotational.

Thanks to Keith, Damien, and Niklas for their feedback on the patchset.

Matias Bjørling (2):
  nvme: make independent ns identify default
  nvme: add rotational support

 drivers/nvme/host/core.c | 12 ++++++++----
 include/linux/nvme.h     |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.46.0

[PATCH 1/2] 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>
---
 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

[PATCH 2/2] nvme: add rotational support

[Matias Bjørling]

From: Matias Bjørling <matias.bjorling@wdc.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.

Note that rotational devices typically can be used to generate random
entropy, the device is therefore also added as a block device that adds
entropy.

Signed-off-by: Matias Bjørling <matias.bjorling@wdc.com>
---
 drivers/nvme/host/core.c | 5 +++++
 include/linux/nvme.h     | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9cbef6342c39..a445f13f5a28 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 | BLK_FEAT_ADD_RANDOM;
+
 	/*
 	 * Register a metadata profile for PI, or the plain non-integrity NVMe
 	 * metadata masquerading as Type 0 if supported, otherwise reject block
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 0/2] nvme: add rotational support

[Keith Busch]

On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
> From: Matias Bjørling <matias.bjorling@wdc.com>
> 
> Enable support for NVMe devices that identifies as rotational.

Thanks, this looks good to me.

I still hope to see nvmet report this. It would be great to test this
with HDD backed nvme-loop target.

Re: [PATCH 0/2] nvme: add rotational support

[Martin K. Petersen]

Matias,

> Enable support for NVMe devices that identifies as rotational.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] nvme: add rotational support

[Keith Busch]

On Tue, Oct 08, 2024 at 09:13:48AM -0600, Keith Busch wrote:
> I still hope to see nvmet report this. It would be great to test this
> with HDD backed nvme-loop target.

I took the liberty to write one up. Looks like everything is reporting
as expected.

---
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 954d4c0747704..e167c9a2ff995 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -685,6 +685,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))
@@ -729,6 +758,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",
--

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

[Hannes Reinecke]

On 10/8/24 16:55, 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.
> 
> Signed-off-by: Matias Bjørling <matias.bjorling@wdc.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)

That is a very odd coding. 'info' will only be filled out for a non-zero
return value of nvme_ns_info_from_cs_indep().
So why not check for that?
But if we get an NVME status back there is a fair chance that something 
else than 'invalid field' (or whatever indicated that the command is not 
supported). That then would cause the device to be misdetected without 
the admin knowning.
Shouldn't we add a message if we fall back to nvme_ns_info_from_identify()?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Hannes Reinecke]

On 10/8/24 16:55, Matias Bjørling wrote:
> From: Matias Bjørling <matias.bjorling@wdc.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.
> 
> Note that rotational devices typically can be used to generate random
> entropy, the device is therefore also added as a block device that adds
> entropy.
> 
> Signed-off-by: Matias Bjørling <matias.bjorling@wdc.com>
> ---
>   drivers/nvme/host/core.c | 5 +++++
>   include/linux/nvme.h     | 1 +
>   2 files changed, 6 insertions(+)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 0/2] nvme: add rotational support

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
> From: Matias Bjørling <matias.bjorling@wdc.com>
> 
> Enable support for NVMe devices that identifies as rotational.
> 
> Thanks to Keith, Damien, and Niklas for their feedback on the patchset.

Hmm, the only previous version I've seen was the the RFCs from
Wang Yugui, last seen in August.

What the improvement over that version?  Note that it also came
with basic nvmet support which is kinda nice.

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

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 04:55:02PM +0200, Matias Bjørling wrote:
> 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.

I'm not a huge fan of this.  For pre-2.0 controllers this means
we'll now send a command that will fail most of them time.  And for
all the cheap low-end consumer device I'm actually worried that they'll
get it wrong and break something.

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

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 04:55:03PM +0200, Matias Bjørling wrote:
> +	if (info->is_rotational)
> +		lim.features |= BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM;

Entropy from block devices is pretty useless.  The only reason we still
keep it for SCSI is because of retro-computing platforms without a proper
platform hardware RNG.  NVMe HDDs reall should not show up in those kinds
of environments.  Also without a add_disk_randomness in the nvme I/O
completion handler this won't actually do anything.

Re: [PATCH 0/2] nvme: add rotational support

[Matias Bjørling]

On 09-10-2024 00:04, Keith Busch wrote:
> On Tue, Oct 08, 2024 at 09:13:48AM -0600, Keith Busch wrote:
>> I still hope to see nvmet report this. It would be great to test this
>> with HDD backed nvme-loop target.
> 
> I took the liberty to write one up. Looks like everything is reporting
> as expected.
> 
> ---
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 954d4c0747704..e167c9a2ff995 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -685,6 +685,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))
> @@ -729,6 +758,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",
> --

That was quick! Nice. Would you like me to pack it up in the serie and 
resend?

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

[Matias Bjørling]

On 09-10-2024 09:48, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:03PM +0200, Matias Bjørling wrote:
>> +	if (info->is_rotational)
>> +		lim.features |= BLK_FEAT_ROTATIONAL | BLK_FEAT_ADD_RANDOM;
> 
> Entropy from block devices is pretty useless.  The only reason we still
> keep it for SCSI is because of retro-computing platforms without a proper
> platform hardware RNG.  NVMe HDDs reall should not show up in those kinds
> of environments.  Also without a add_disk_randomness in the nvme I/O
> completion handler this won't actually do anything.
> 

Thanks for the details. I'll remove it in the next revision.

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

[Matias Bjørling]

On 09-10-2024 09:46, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:02PM +0200, Matias Bjørling wrote:
>> 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.
> 
> I'm not a huge fan of this.  For pre-2.0 controllers this means
> we'll now send a command that will fail most of them time.  And for
> all the cheap low-end consumer device I'm actually worried that they'll
> get it wrong and break something.
> 

It's a good point. Damien, Keith, and I were debating it during ALPSS. 
They preferred the "send command and see if it fails" approach over 
writing specific conditions where it would apply. Note that Keith did 
suggest to avoid the command on 1.0 and 1.1 devices, and they were known 
to fail with unsupported CNS ids.

If making the check conditional, I think checking if the device follows 
2.0 specification isn't sufficient, as some devices may implement a 
subset of the 2.0 features (for example the independent ns data struct), 
while reporting as a 1.4 device.

Is there maybe better way, that isn't dependent on some feature being 
implemented (such as CRIMS capability)?

Re: [PATCH 0/2] nvme: add rotational support

[Matias Bjørling]

On 09-10-2024 09:43, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
>> From: Matias Bjørling <matias.bjorling@wdc.com>
>>
>> Enable support for NVMe devices that identifies as rotational.
>>
>> Thanks to Keith, Damien, and Niklas for their feedback on the patchset.
> 
> Hmm, the only previous version I've seen was the the RFCs from
> Wang Yugui, last seen in August.
> 
> What the improvement over that version?  Note that it also came
> with basic nvmet support which is kinda nice.

Ah, I made the patches without awareness of the earlier efforts.

This version, together with Keith's nvmet patch, does not rely on 
setting/checking the CRIMS flag.

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

[Matias Bjørling]

On 09-10-2024 08:16, Hannes Reinecke wrote:
> On 10/8/24 16:55, 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.
>>
>> Signed-off-by: Matias Bjørling <matias.bjorling@wdc.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)
> 
> That is a very odd coding. 'info' will only be filled out for a non-zero
> return value of nvme_ns_info_from_cs_indep().

I may have misunderstood. Only if nvme_ns_info_from_cs_indep() return 0 
will the information be filled. Otherwise, if it is an NVMe error, 
nvme_ns_info_from_identify() is tried, otherwise it's a hard error, and 
it errors out completely.

> So why not check for that?
> But if we get an NVME status back there is a fair chance that something 
> else than 'invalid field' (or whatever indicated that the command is not 
> supported). That then would cause the device to be misdetected without 
> the admin knowning.
> Shouldn't we add a message if we fall back to nvme_ns_info_from_identify()?

Hmm, we could. Buuuut, at this point, there's more devices falling back 
to nvme_ns_info_from_identify(), than devices that implements the 
independent ns identify data structure. So I wouldn't mind it being 
silent. If we want to debug a potential misdetection, tracing could be 
enabled to track down what's happening.

> 
> Cheers,
> 
> Hannes

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

[Keith Busch]

On Wed, Oct 09, 2024 at 09:46:11AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:02PM +0200, Matias Bjørling wrote:
> > 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.
> 
> I'm not a huge fan of this.  For pre-2.0 controllers this means
> we'll now send a command that will fail most of them time.  And for
> all the cheap low-end consumer device I'm actually worried that they'll
> get it wrong and break something.

We already send identify commands that we expect may break on pre-2.0
controllers: the Identify NS Descriptor List.

We have other quirks for disabling specific identifications (ex:
nvme_ctrl_limited_cns, NVME_QUIRK_NO_NS_DESC_LIST) in case something
really break certain identifies. But I think anything >= 1.3 should be
fine: the CNS handling is well defined from that point onward, so we
shouldn't make anything harder than necessary from assuming someone got
identication this wrong.

The only pain I can think of is that some controllers increment their
error log count, and SMART tools creates unnecessary alerts for that.

Re: [PATCH 0/2] nvme: add rotational support

[Keith Busch]

On Wed, Oct 09, 2024 at 09:43:55AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
> > From: Matias Bjørling <matias.bjorling@wdc.com>
> > 
> > Enable support for NVMe devices that identifies as rotational.
> > 
> > Thanks to Keith, Damien, and Niklas for their feedback on the patchset.
> 
> Hmm, the only previous version I've seen was the the RFCs from
> Wang Yugui, last seen in August.

Oops, that slipped by me as well. I think the right thing to do is bring
that one forward and retain the original credit. I agree with Matias
that we ought to be able to query the independent identification without
relying on CRWMS, though.

Re: [PATCH 0/2] nvme: add rotational support

[Matias Bjørling]

On 09-10-2024 17:39, Keith Busch wrote:
> On Wed, Oct 09, 2024 at 09:43:55AM +0200, Christoph Hellwig wrote:
>> On Tue, Oct 08, 2024 at 04:55:01PM +0200, Matias Bjørling wrote:
>>> From: Matias Bjørling <matias.bjorling@wdc.com>
>>>
>>> Enable support for NVMe devices that identifies as rotational.
>>>
>>> Thanks to Keith, Damien, and Niklas for their feedback on the patchset.
>>
>> Hmm, the only previous version I've seen was the the RFCs from
>> Wang Yugui, last seen in August.
> 
> Oops, that slipped by me as well. I think the right thing to do is bring
> that one forward and retain the original credit. I agree with Matias
> that we ought to be able to query the independent identification without
> relying on CRWMS, though.

Works for me. Yugui, are you okay with me posting the updated patch 
serie with your patch?

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

[Christoph Hellwig]

On Wed, Oct 09, 2024 at 08:56:32AM -0600, Keith Busch wrote:
> On Wed, Oct 09, 2024 at 09:46:11AM +0200, Christoph Hellwig wrote:
> > On Tue, Oct 08, 2024 at 04:55:02PM +0200, Matias Bjørling wrote:
> > > 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.
> > 
> > I'm not a huge fan of this.  For pre-2.0 controllers this means
> > we'll now send a command that will fail most of them time.  And for
> > all the cheap low-end consumer device I'm actually worried that they'll
> > get it wrong and break something.
> 
> We already send identify commands that we expect may break on pre-2.0
> controllers: the Identify NS Descriptor List.

Identify NS Descriptor List is mandatory starting with NVMe 1.3.  We only
issue it for 1.3 or if the controller advertises supporting multiple
command sets.

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

[Daniel Wagner]

On Wed, Oct 09, 2024 at 03:19:59PM GMT, Matias Bjørling wrote:
> On 09-10-2024 09:46, Christoph Hellwig wrote:
> > On Tue, Oct 08, 2024 at 04:55:02PM +0200, Matias Bjørling wrote:
> > > 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.
> > 
> > I'm not a huge fan of this.  For pre-2.0 controllers this means
> > we'll now send a command that will fail most of them time.  And for
> > all the cheap low-end consumer device I'm actually worried that they'll
> > get it wrong and break something.
> > 
> 
> It's a good point. Damien, Keith, and I were debating it during ALPSS. They
> preferred the "send command and see if it fails" approach over writing
> specific conditions where it would apply. Note that Keith did suggest to
> avoid the command on 1.0 and 1.1 devices, and they were known to fail with
> unsupported CNS ids.

FWIW, there are some devices out there which will log these attempts and
spam their error logs. There were plenty of reports against nvme-cli
when nvme-cli issued a command which could fail.

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

[Matias Bjørling]

On 10-10-2024 16:47, Daniel Wagner wrote:
> On Wed, Oct 09, 2024 at 03:19:59PM GMT, Matias Bjørling wrote:
>> On 09-10-2024 09:46, Christoph Hellwig wrote:
>>> On Tue, Oct 08, 2024 at 04:55:02PM +0200, Matias Bjørling wrote:
>>>> 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.
>>>
>>> I'm not a huge fan of this.  For pre-2.0 controllers this means
>>> we'll now send a command that will fail most of them time.  And for
>>> all the cheap low-end consumer device I'm actually worried that they'll
>>> get it wrong and break something.
>>>
>>
>> It's a good point. Damien, Keith, and I were debating it during ALPSS. They
>> preferred the "send command and see if it fails" approach over writing
>> specific conditions where it would apply. Note that Keith did suggest to
>> avoid the command on 1.0 and 1.1 devices, and they were known to fail with
>> unsupported CNS ids.
> 
> FWIW, there are some devices out there which will log these attempts and
> spam their error logs. There were plenty of reports against nvme-cli
> when nvme-cli issued a command which could fail.

Got it. So, given the feedback from you, Keith, and Christoph. It's safe 
to say it needs to be a conditional check.

Would anyone object if the

  if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) ||
      (info.ids.csi != NVME_CSI_NVM && info.ids.csi != NVME_CSI_ZNS))

statement would include a check for endurance group support?

The idea being that it's mandatory for a device to implement an 
endurance group in case it exposes the rotational flag. That check would 
limit the failed command check to relatively new devices.