[PATCH V2] nvme: enable ro namespace for ZNS without append

[PATCH V2] nvme: enable ro namespace for ZNS without append

[Javier González]

Allow ZNS NVMe SSDs to present a read-only namespace when append is not
supported, instead of rejecting the namespace directly.

This allows (i) the namespace to be used in read-only mode, which is not
a problem as the append command only affects the write path, and (ii) to
use standard management tools such as nvme-cli to choose a different
format or firmware slot that is compatible with the Linux zoned block
device.

This patch includes comments from Christoph, Niklas and Keith that
applied to a different approach setting capacity to 0
  https://www.spinics.net/lists/linux-block/msg60747.html

The reminder of the original patch will be submitted separately.

Changes since V1:
  - Change logic to use NVME_NS_ATTR_RO (from Christoph)
  - Set max_zone_append egen in RO. This allows the device to be properly
    revalidated and enables user-space tools such as blkzone to be used when
    interacting with this zoned device.

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 3 ++-
 drivers/nvme/host/nvme.h | 1 +
 drivers/nvme/host/zns.c  | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 40ca71b29bb9..4fcaec0b330b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2058,7 +2058,8 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	nvme_config_discard(disk, ns);
 	nvme_config_write_zeroes(disk, ns);
 
-	if (id->nsattr & NVME_NS_ATTR_RO)
+	if (id->nsattr & NVME_NS_ATTR_RO ||
+			test_bit(NVME_NS_FORCE_RO, &ns->flags))
 		set_disk_ro(disk, true);
 	else
 		set_disk_ro(disk, false);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bc330bf0d3bd..3a985e98ca27 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -448,6 +448,7 @@ struct nvme_ns {
 #define NVME_NS_REMOVING	0
 #define NVME_NS_DEAD     	1
 #define NVME_NS_ANA_PENDING	2
+#define NVME_NS_FORCE_RO	3
 
 	struct nvme_fault_inject fault_inject;
 
diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index 67e87e9f306f..8f3632c4db05 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -57,10 +57,10 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf)
 	/* Driver requires zone append support */
 	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
 			NVME_CMD_EFFECTS_CSUPP)) {
+		set_bit(NVME_NS_FORCE_RO, &ns->flags);
 		dev_warn(ns->ctrl->device,
-			"append not supported for zoned namespace:%d\n",
+			"append not supported for zoned namespace:%d. Forcing to read-only mode\n",
 			ns->head->ns_id);
-		return -EINVAL;
 	}
 
 	/* Lazily query controller append limit for the first zoned namespace */
-- 
2.17.1

[PATCH V2] nvme: enable ro namespace for ZNS without append

[Javier González]

Allow ZNS NVMe SSDs to present a read-only namespace when append is not
supported, instead of rejecting the namespace directly.

This allows (i) the namespace to be used in read-only mode, which is not
a problem as the append command only affects the write path, and (ii) to
use standard management tools such as nvme-cli to choose a different
format or firmware slot that is compatible with the Linux zoned block
device.

This patch includes comments from Christoph, Niklas and Keith that
applied to a different approach setting capacity to 0
  https://www.spinics.net/lists/linux-block/msg60747.html

The reminder of the original patch will be submitted separately.

Changes since V1:
  - Change logic to use NVME_NS_ATTR_RO (from Christoph)
  - Set max_zone_append egen in RO. This allows the device to be properly
    revalidated and enables user-space tools such as blkzone to be used when
    interacting with this zoned device.

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 3 ++-
 drivers/nvme/host/nvme.h | 1 +
 drivers/nvme/host/zns.c  | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 40ca71b29bb9..4fcaec0b330b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2058,7 +2058,8 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	nvme_config_discard(disk, ns);
 	nvme_config_write_zeroes(disk, ns);
 
-	if (id->nsattr & NVME_NS_ATTR_RO)
+	if (id->nsattr & NVME_NS_ATTR_RO ||
+			test_bit(NVME_NS_FORCE_RO, &ns->flags))
 		set_disk_ro(disk, true);
 	else
 		set_disk_ro(disk, false);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bc330bf0d3bd..3a985e98ca27 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -448,6 +448,7 @@ struct nvme_ns {
 #define NVME_NS_REMOVING	0
 #define NVME_NS_DEAD     	1
 #define NVME_NS_ANA_PENDING	2
+#define NVME_NS_FORCE_RO	3
 
 	struct nvme_fault_inject fault_inject;
 
diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index 67e87e9f306f..8f3632c4db05 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -57,10 +57,10 @@ int nvme_update_zone_info(struct nvme_ns *ns, unsigned lbaf)
 	/* Driver requires zone append support */
 	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
 			NVME_CMD_EFFECTS_CSUPP)) {
+		set_bit(NVME_NS_FORCE_RO, &ns->flags);
 		dev_warn(ns->ctrl->device,
-			"append not supported for zoned namespace:%d\n",
+			"append not supported for zoned namespace:%d. Forcing to read-only mode\n",
 			ns->head->ns_id);
-		return -EINVAL;
 	}
 
 	/* Lazily query controller append limit for the first zoned namespace */
-- 
2.17.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: enable ro namespace for ZNS without append

[Christoph Hellwig]

> -	if (id->nsattr & NVME_NS_ATTR_RO)
> +	if (id->nsattr & NVME_NS_ATTR_RO ||
> +			test_bit(NVME_NS_FORCE_RO, &ns->flags))
>  		set_disk_ro(disk, true);
>  	else
>  		set_disk_ro(disk, false);

This has a very minor conflict with the patch from Sagi to remove the
else side of the clause.  I'll merge your patch and will fix unless
someone else objects to the approach.

Note that as discussed we really need a hard read-only settings
instead of set_disk_ro for both NVME_NS_ATTR_RO and the zns with missing
features case, but I'll look into that once I've got a few other things
off my plate.

Re: [PATCH V2] nvme: enable ro namespace for ZNS without append

[Christoph Hellwig]

> -	if (id->nsattr & NVME_NS_ATTR_RO)
> +	if (id->nsattr & NVME_NS_ATTR_RO ||
> +			test_bit(NVME_NS_FORCE_RO, &ns->flags))
>  		set_disk_ro(disk, true);
>  	else
>  		set_disk_ro(disk, false);

This has a very minor conflict with the patch from Sagi to remove the
else side of the clause.  I'll merge your patch and will fix unless
someone else objects to the approach.

Note that as discussed we really need a hard read-only settings
instead of set_disk_ro for both NVME_NS_ATTR_RO and the zns with missing
features case, but I'll look into that once I've got a few other things
off my plate.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: enable ro namespace for ZNS without append

[Javier González]

On 10.11.2020 10:43, Christoph Hellwig wrote:
>> -	if (id->nsattr & NVME_NS_ATTR_RO)
>> +	if (id->nsattr & NVME_NS_ATTR_RO ||
>> +			test_bit(NVME_NS_FORCE_RO, &ns->flags))
>>  		set_disk_ro(disk, true);
>>  	else
>>  		set_disk_ro(disk, false);
>
>This has a very minor conflict with the patch from Sagi to remove the
>else side of the clause.  I'll merge your patch and will fix unless
>someone else objects to the approach.

Sounds good.
>
>Note that as discussed we really need a hard read-only settings
>instead of set_disk_ro for both NVME_NS_ATTR_RO and the zns with missing
>features case, but I'll look into that once I've got a few other things
>off my plate.

Thanks! Please, let me know if there is anything we can help with. I'm
looking into the char device now.

Re: nvme: enable ro namespace for ZNS without append

[Javier González]

On 10.11.2020 10:43, Christoph Hellwig wrote:
>> -	if (id->nsattr & NVME_NS_ATTR_RO)
>> +	if (id->nsattr & NVME_NS_ATTR_RO ||
>> +			test_bit(NVME_NS_FORCE_RO, &ns->flags))
>>  		set_disk_ro(disk, true);
>>  	else
>>  		set_disk_ro(disk, false);
>
>This has a very minor conflict with the patch from Sagi to remove the
>else side of the clause.  I'll merge your patch and will fix unless
>someone else objects to the approach.

Sounds good.
>
>Note that as discussed we really need a hard read-only settings
>instead of set_disk_ro for both NVME_NS_ATTR_RO and the zns with missing
>features case, but I'll look into that once I've got a few other things
>off my plate.

Thanks! Please, let me know if there is anything we can help with. I'm
looking into the char device now.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: enable ro namespace for ZNS without append

[Niklas Cassel]

On Tue, Nov 10, 2020 at 10:43:54AM +0100, Christoph Hellwig wrote:
> > -	if (id->nsattr & NVME_NS_ATTR_RO)
> > +	if (id->nsattr & NVME_NS_ATTR_RO ||
> > +			test_bit(NVME_NS_FORCE_RO, &ns->flags))

Indentation for the test_bit() looks off.
I assume that Christoph can fixup that when applying.

$ ./scripts/checkpatch.pl --strict ~/javier.patch
CHECK: Alignment should match open parenthesis
#280: FILE: drivers/nvme/host/core.c:2062:
+       if (id->nsattr & NVME_NS_ATTR_RO ||
+                       test_bit(NVME_NS_FORCE_RO, &ns->flags))


For the record:

WARNING: From:/Signed-off-by: email address mismatch: 'From: "Javier González" <javier@javigon.com>' != 'Signed-off-by: Javier González <javier.gonz@samsung.com>'

If you want to use a SoB that is different from the email address which
you are sending from, then according to the The canonical patch format:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

"""
The from line must be the very first line in the message body, and has the form:

    From: Patch Author <author@example.com>

The from line specifies who will be credited as the author of the patch in the permanent changelog.
If the from line is missing, then the From: line from the email header will be used to determine
the patch author in the changelog.

Note, the From: tag is optional when the From: author is also the person (and email) listed in the
From: line of the email header.
"""


That way, when the maintainers use git am, it will pick the author
from the "From:" in the message body, rather than from the email header.

Otherwise the Author: field in the git log will be different from your SoB.

There are several ways you can fix this, either by using the correct email
when you do the commit in the first place, then git format-patch will add
From: automatically, or by using the git config sendemail.from, or --from
option to git-send-email.


Kind regards,
Niklas

Re: [PATCH V2] nvme: enable ro namespace for ZNS without append

[Niklas Cassel]

On Tue, Nov 10, 2020 at 10:43:54AM +0100, Christoph Hellwig wrote:
> > -	if (id->nsattr & NVME_NS_ATTR_RO)
> > +	if (id->nsattr & NVME_NS_ATTR_RO ||
> > +			test_bit(NVME_NS_FORCE_RO, &ns->flags))

Indentation for the test_bit() looks off.
I assume that Christoph can fixup that when applying.

$ ./scripts/checkpatch.pl --strict ~/javier.patch
CHECK: Alignment should match open parenthesis
#280: FILE: drivers/nvme/host/core.c:2062:
+       if (id->nsattr & NVME_NS_ATTR_RO ||
+                       test_bit(NVME_NS_FORCE_RO, &ns->flags))


For the record:

WARNING: From:/Signed-off-by: email address mismatch: 'From: "Javier González" <javier@javigon.com>' != 'Signed-off-by: Javier González <javier.gonz@samsung.com>'

If you want to use a SoB that is different from the email address which
you are sending from, then according to the The canonical patch format:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

"""
The from line must be the very first line in the message body, and has the form:

    From: Patch Author <author@example.com>

The from line specifies who will be credited as the author of the patch in the permanent changelog.
If the from line is missing, then the From: line from the email header will be used to determine
the patch author in the changelog.

Note, the From: tag is optional when the From: author is also the person (and email) listed in the
From: line of the email header.
"""


That way, when the maintainers use git am, it will pick the author
from the "From:" in the message body, rather than from the email header.

Otherwise the Author: field in the git log will be different from your SoB.

There are several ways you can fix this, either by using the correct email
when you do the commit in the first place, then git format-patch will add
From: automatically, or by using the git config sendemail.from, or --from
option to git-send-email.


Kind regards,
Niklas
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: enable ro namespace for ZNS without append

[Javier González]

On 10.11.2020 11:25, Niklas Cassel wrote:
>On Tue, Nov 10, 2020 at 10:43:54AM +0100, Christoph Hellwig wrote:
>> > -	if (id->nsattr & NVME_NS_ATTR_RO)
>> > +	if (id->nsattr & NVME_NS_ATTR_RO ||
>> > +			test_bit(NVME_NS_FORCE_RO, &ns->flags))
>
>Indentation for the test_bit() looks off.
>I assume that Christoph can fixup that when applying.
>
>$ ./scripts/checkpatch.pl --strict ~/javier.patch
>CHECK: Alignment should match open parenthesis
>#280: FILE: drivers/nvme/host/core.c:2062:
>+       if (id->nsattr & NVME_NS_ATTR_RO ||
>+                       test_bit(NVME_NS_FORCE_RO, &ns->flags))
>
>
>For the record:
>
>WARNING: From:/Signed-off-by: email address mismatch: 'From: "Javier González" <javier@javigon.com>' != 'Signed-off-by: Javier González <javier.gonz@samsung.com>'
>
>If you want to use a SoB that is different from the email address which
>you are sending from, then according to the The canonical patch format:
>
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
>
>"""
>The from line must be the very first line in the message body, and has the form:
>
>    From: Patch Author <author@example.com>
>
>The from line specifies who will be credited as the author of the patch in the permanent changelog.
>If the from line is missing, then the From: line from the email header will be used to determine
>the patch author in the changelog.
>
>Note, the From: tag is optional when the From: author is also the person (and email) listed in the
>From: line of the email header.
>"""
>
>
>That way, when the maintainers use git am, it will pick the author
>from the "From:" in the message body, rather than from the email header.
>
>Otherwise the Author: field in the git log will be different from your SoB.
>
>There are several ways you can fix this, either by using the correct email
>when you do the commit in the first place, then git format-patch will add
>From: automatically, or by using the git config sendemail.from, or --from
>option to git-send-email.
>

Thanks for taking the time Niklas. I have done this intentionally for
quite some time - I use javier@javigon.com to submit and commit and then
sign-off with my current employer.

If this presents an inconvenience, I don't mind changing the committer
to my current Samsung email.

Re: nvme: enable ro namespace for ZNS without append

[Javier González]

On 10.11.2020 11:25, Niklas Cassel wrote:
>On Tue, Nov 10, 2020 at 10:43:54AM +0100, Christoph Hellwig wrote:
>> > -	if (id->nsattr & NVME_NS_ATTR_RO)
>> > +	if (id->nsattr & NVME_NS_ATTR_RO ||
>> > +			test_bit(NVME_NS_FORCE_RO, &ns->flags))
>
>Indentation for the test_bit() looks off.
>I assume that Christoph can fixup that when applying.
>
>$ ./scripts/checkpatch.pl --strict ~/javier.patch
>CHECK: Alignment should match open parenthesis
>#280: FILE: drivers/nvme/host/core.c:2062:
>+       if (id->nsattr & NVME_NS_ATTR_RO ||
>+                       test_bit(NVME_NS_FORCE_RO, &ns->flags))
>
>
>For the record:
>
>WARNING: From:/Signed-off-by: email address mismatch: 'From: "Javier González" <javier@javigon.com>' != 'Signed-off-by: Javier González <javier.gonz@samsung.com>'
>
>If you want to use a SoB that is different from the email address which
>you are sending from, then according to the The canonical patch format:
>
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
>
>"""
>The from line must be the very first line in the message body, and has the form:
>
>    From: Patch Author <author@example.com>
>
>The from line specifies who will be credited as the author of the patch in the permanent changelog.
>If the from line is missing, then the From: line from the email header will be used to determine
>the patch author in the changelog.
>
>Note, the From: tag is optional when the From: author is also the person (and email) listed in the
>From: line of the email header.
>"""
>
>
>That way, when the maintainers use git am, it will pick the author
>from the "From:" in the message body, rather than from the email header.
>
>Otherwise the Author: field in the git log will be different from your SoB.
>
>There are several ways you can fix this, either by using the correct email
>when you do the commit in the first place, then git format-patch will add
>From: automatically, or by using the git config sendemail.from, or --from
>option to git-send-email.
>

Thanks for taking the time Niklas. I have done this intentionally for
quite some time - I use javier@javigon.com to submit and commit and then
sign-off with my current employer.

If this presents an inconvenience, I don't mind changing the committer
to my current Samsung email.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V2] nvme: enable ro namespace for ZNS without append

[Keith Busch]

On Tue, Nov 10, 2020 at 10:39:38AM +0100, Javier González wrote:
>  	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
>  			NVME_CMD_EFFECTS_CSUPP)) {
> +		set_bit(NVME_NS_FORCE_RO, &ns->flags);
>  		dev_warn(ns->ctrl->device,
> -			"append not supported for zoned namespace:%d\n",
> +			"append not supported for zoned namespace:%d. Forcing to read-only mode\n",
>  			ns->head->ns_id);
> -		return -EINVAL;
>  	}

In the unlikely event that a f/w upgrade adds append support, do we want
to bother clearing this flag? If so, we would need to refresh the
command effects log page.

If not, you'd have to rebind the driver to make it writable. I don't see
that as being a big deal, so I think the patch is probably fine as-is.

Re: [PATCH V2] nvme: enable ro namespace for ZNS without append

[Keith Busch]

On Tue, Nov 10, 2020 at 10:39:38AM +0100, Javier González wrote:
>  	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
>  			NVME_CMD_EFFECTS_CSUPP)) {
> +		set_bit(NVME_NS_FORCE_RO, &ns->flags);
>  		dev_warn(ns->ctrl->device,
> -			"append not supported for zoned namespace:%d\n",
> +			"append not supported for zoned namespace:%d. Forcing to read-only mode\n",
>  			ns->head->ns_id);
> -		return -EINVAL;
>  	}

In the unlikely event that a f/w upgrade adds append support, do we want
to bother clearing this flag? If so, we would need to refresh the
command effects log page.

If not, you'd have to rebind the driver to make it writable. I don't see
that as being a big deal, so I think the patch is probably fine as-is.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: nvme: enable ro namespace for ZNS without append

[Javier González]

On 10.11.2020 06:16, Keith Busch wrote:
>On Tue, Nov 10, 2020 at 10:39:38AM +0100, Javier González wrote:
>>  	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
>>  			NVME_CMD_EFFECTS_CSUPP)) {
>> +		set_bit(NVME_NS_FORCE_RO, &ns->flags);
>>  		dev_warn(ns->ctrl->device,
>> -			"append not supported for zoned namespace:%d\n",
>> +			"append not supported for zoned namespace:%d. Forcing to read-only mode\n",
>>  			ns->head->ns_id);
>> -		return -EINVAL;
>>  	}
>
>In the unlikely event that a f/w upgrade adds append support, do we want
>to bother clearing this flag? If so, we would need to refresh the
>command effects log page.

Good point. Also useful when selecting a new FW slot. Would it make
sense to move the check to the revalidate path and eventually clear the
flag?

Re: nvme: enable ro namespace for ZNS without append

[Javier González]

On 10.11.2020 06:16, Keith Busch wrote:
>On Tue, Nov 10, 2020 at 10:39:38AM +0100, Javier González wrote:
>>  	if (!(le32_to_cpu(log->iocs[nvme_cmd_zone_append]) &
>>  			NVME_CMD_EFFECTS_CSUPP)) {
>> +		set_bit(NVME_NS_FORCE_RO, &ns->flags);
>>  		dev_warn(ns->ctrl->device,
>> -			"append not supported for zoned namespace:%d\n",
>> +			"append not supported for zoned namespace:%d. Forcing to read-only mode\n",
>>  			ns->head->ns_id);
>> -		return -EINVAL;
>>  	}
>
>In the unlikely event that a f/w upgrade adds append support, do we want
>to bother clearing this flag? If so, we would need to refresh the
>command effects log page.

Good point. Also useful when selecting a new FW slot. Would it make
sense to move the check to the revalidate path and eventually clear the
flag?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme