| Message ID | 20201110093938.25386-1-javier.gonz@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] nvme: enable ro namespace for ZNS without append | expand |
> - 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.
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.
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
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.
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?
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.
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 */
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(-)