[PATCH v3 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
Joel Granados
j.granados at samsung.com
Tue Jun 28 02:47:24 PDT 2022
On Sun, Jun 26, 2022 at 04:53:32PM +0300, Sagi Grimberg wrote:
>
>
> On 6/22/22 16:55, Joel Granados wrote:
> > Extend nvme_alloc_ns() and nvme_validate_ns() for unknown command-set as
> > well. Both are made to use a new helper (nvme_update_ns_info_cs_indep)
> > which is similar to nvme_update_ns_info but performs fewer operations
> > to get the generic interface up.
> >
> > Suggested-by: Christoph Hellwig <hch at lst.de>
> > Signed-off-by: Joel Granados <j.granados at samsung.com>
> > Signed-off-by: Kanchan Joshi <joshi.k at samsung.com>
> > ---
> > drivers/nvme/host/core.c | 132 +++++++++++++++++++++++++++------------
> > 1 file changed, 93 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 0fd6127da8d1..03ba3d0f4d3a 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1908,6 +1908,34 @@ static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
> > blk_queue_chunk_sectors(ns->queue, iob);
> > }
> > +static int nvme_update_ns_info_cs_indep(struct nvme_ns *ns,
> > + struct nvme_id_ns_cs_indep *id)
> > +{
> > + blk_mq_freeze_queue(ns->disk->queue);
> > + nvme_set_queue_limits(ns->ctrl, ns->queue);
> > + set_disk_ro(ns->disk, (id->nsattr & NVME_NS_ATTR_RO) ||
> > + test_bit(NVME_NS_FORCE_RO, &ns->flags));
> > + blk_mq_unfreeze_queue(ns->disk->queue);
> > +
> > + if (nvme_ns_head_multipath(ns->head)) {
> > + blk_mq_freeze_queue(ns->head->disk->queue);
> > + set_disk_ro(ns->head->disk,
> > + (id->nsattr & NVME_NS_ATTR_RO) ||
> > + test_bit(NVME_NS_FORCE_RO, &ns->flags));
> > + nvme_mpath_revalidate_paths(ns);
> > + blk_stack_limits(&ns->head->disk->queue->limits,
> > + &ns->queue->limits, 0);
> > + ns->head->disk->flags |= GENHD_FL_HIDDEN;
> > + blk_mq_unfreeze_queue(ns->head->disk->queue);
> > + }
>
> We can probably share this code with the existing update_ns_info ?
There are three differences in the multipath section of these two functions:
1. The command-set-indep does not call nvme_update_disk_info
2. The command-set-indep does not call disk_update_readahead
3. The command-set-indep hides the device "GENHD_FL_HIDDEN" at the end
I'll add a function that gathers the three common lines.
Something like this:
static inline void nvme_update_ns_mpath_info_general(struct nvme_ns *ns, __u8 nsattr)
{
set_disk_ro(....)
nvme_mpath_revalidate_paths(...)
blk_stack_limits(...)
}
>
> > +
> > + /* Hide the block-interface for these devices */
> > + ns->disk->flags |= GENHD_FL_HIDDEN;
> > + set_bit(NVME_NS_READY, &ns->flags);
> > +
> > + return 0;
> > +}
> > +
> > static int nvme_update_ns_info(struct nvme_ns *ns, struct nvme_id_ns *id)
> > {
> > unsigned lbaf = nvme_lbaf_index(id->flbas);
> > @@ -3951,15 +3979,29 @@ static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
> > }
> > static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> > - struct nvme_ns_ids *ids)
> > + struct nvme_ns_ids *ids, struct nvme_id_ns_cs_indep *indep_id)
> > {
> > + int ret = 0;
> > struct nvme_ns *ns;
> > struct gendisk *disk;
> > - struct nvme_id_ns *id;
> > + struct nvme_id_ns *id = NULL;
> > int node = ctrl->numa_node;
> > + __u8 *nmic;
> > + __le32 *anagrpid;
> > - if (nvme_identify_ns(ctrl, nsid, ids, &id))
> > - return;
> > + switch (ids->csi) {
> > + case NVME_CSI_NVM:
> > + case NVME_CSI_ZNS:
> > + if (nvme_identify_ns(ctrl, nsid, ids, &id))
> > + return;
> > + nmic = &id->nmic;
> > + anagrpid = &id->anagrpid;
> > + break;
> > + default:
> > + nmic = &indep_id->nmic;
> > + anagrpid = &indep_id->anagrpid;
> > + break;
> > + }
> > ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
> > if (!ns)
> > @@ -3984,7 +4026,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> > ns->ctrl = ctrl;
> > kref_init(&ns->kref);
> > - if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
> > + if (nvme_init_ns_head(ns, nsid, ids, *nmic & NVME_NS_NMIC_SHARED))
> > goto out_cleanup_disk;
> > /*
> > @@ -4010,7 +4052,16 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> > ns->head->instance);
> > }
> > - if (nvme_update_ns_info(ns, id))
> > + switch (ids->csi) {
> > + case NVME_CSI_NVM:
> > + case NVME_CSI_ZNS:
> > + ret = nvme_update_ns_info(ns, id);
> > + break;
> > + default:
> > + ret = nvme_update_ns_info_cs_indep(ns, indep_id);
> > + break;
> > + }
> > + if (ret)
> > goto out_unlink_ns;
> > down_write(&ctrl->namespaces_rwsem);
> > @@ -4024,11 +4075,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> > if (!nvme_ns_head_multipath(ns->head))
> > nvme_add_ns_cdev(ns);
> > - nvme_mpath_add_disk(ns, id->anagrpid);
> > + nvme_mpath_add_disk(ns, *anagrpid);
> > nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> > - kfree(id);
> > - return;
> > + goto out_free_id;
> > out_cleanup_ns_from_list:
> > nvme_put_ctrl(ctrl);
> > @@ -4106,29 +4156,32 @@ static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid)
> > }
> > }
> > -static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
> > +static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids,
> > + struct nvme_id_ns_cs_indep *indep_id)
> > {
> > - struct nvme_id_ns *id;
> > + struct nvme_id_ns *id = NULL;
> > int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
> > if (test_bit(NVME_NS_DEAD, &ns->flags))
> > goto out;
> > - ret = nvme_identify_ns(ns->ctrl, ns->head->ns_id, ids, &id);
> > - if (ret)
> > - goto out;
> > -
> > - ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
> > - if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
> > - dev_err(ns->ctrl->device,
> > - "identifiers changed for nsid %d\n", ns->head->ns_id);
> > - goto out_free_id;
> > + switch (ids->csi) {
> > + case NVME_CSI_NVM:
> > + case NVME_CSI_ZNS:
> > + if (nvme_identify_ns(ns->ctrl, ns->head->ns_id, ids, &id))
> > + goto out;
> > + if (!nvme_ns_ids_equal(&ns->head->ids, ids))
> > + dev_err(ns->ctrl->device,
> > + "identifiers changed for nsid %d\n", ns->head->ns_id);
> > + else
> > + ret = nvme_update_ns_info(ns, id);
> > + kfree(id);
> > + break;
> > + default:
> > + ret = nvme_update_ns_info_cs_indep(ns, indep_id);
> > + break;
> > }
> > - ret = nvme_update_ns_info(ns, id);
> > -
> > -out_free_id:
> > - kfree(id);
> > out:
> > /*
> > * Only remove the namespace if we got a fatal error back from the
> > @@ -4143,36 +4196,34 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
> > static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> > {
> > struct nvme_ns_ids ids = { };
> > - struct nvme_id_ns_cs_indep *id;
> > + struct nvme_id_ns_cs_indep *id_indep;
> > struct nvme_ns *ns;
> > - bool ready = true;
> > if (nvme_identify_ns_descs(ctrl, nsid, &ids))
> > return;
> > + if (nvme_identify_ns_cs_indep(ctrl, nsid, &id_indep))
> > + return;
> > +
>
> Is it ok to issue this without checking that the namespace is ready?
This is a great question. Gave TP4084 a read and this is what I
understand (please correct me if I'm wrong):
* For the first controller ready mode (Controller ready with media), once
the controller is ready the namespaces are required to process admin
commands. Which basically means commands are not allowed to be aborted
with "namespace not ready" status code. In this case it is OK to issue
without checking NS ready.
* For the second controller ready mode (Controller ready independent of
media), the identify command is not allowed to abort with
"namespace not ready" status code In this case it is OK to issue
without checking NS ready.
* In the case that the command set indep identify NS is NOT supported we
ignore the returned error and continue with validation in the
expectation that the controller just forgot to implement the indep
identify (08h) but actually supports the normal identify namespace
command (00h).
All this is contained in my V4 which I'll post as soon as I have tested
Thx for the review
>
> > /*
> > * Check if the namespace is ready. If not ignore it, we will get an
> > * AEN once it becomes ready and restart the scan.
> > */
> > - if ((ctrl->cap & NVME_CAP_CRMS_CRIMS) &&
> > - !nvme_identify_ns_cs_indep(ctrl, nsid, &id)) {
> > - ready = id->nstat & NVME_NSTAT_NRDY;
> > - kfree(id);
> > + if (ctrl->cap & NVME_CAP_CRMS_CRIMS) {
> > + if (!(id_indep->nstat & NVME_NSTAT_NRDY))
> > + goto free_id_indep;
> > }
> > - if (!ready)
> > - return;
> > -
> > ns = nvme_find_get_ns(ctrl, nsid);
> > if (ns) {
> > - nvme_validate_ns(ns, &ids);
> > + nvme_validate_ns(ns, &ids, id_indep);
> > nvme_put_ns(ns);
> > - return;
> > + goto free_id_indep;
> > }
> > switch (ids.csi) {
> > case NVME_CSI_NVM:
> > - nvme_alloc_ns(ctrl, nsid, &ids);
> > + nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
> > break;
> > case NVME_CSI_ZNS:
> > if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED)) {
> > @@ -4187,13 +4238,16 @@ static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
> > nsid);
> > break;
> > }
> > - nvme_alloc_ns(ctrl, nsid, &ids);
> > + nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
> > break;
> > default:
> > - dev_warn(ctrl->device, "unknown csi %u for nsid %u\n",
> > - ids.csi, nsid);
> > + /* required to enable char-interface for unknown command sets*/
> > + nvme_alloc_ns(ctrl, nsid, &ids, id_indep);
> > break;
> > }
> > +
> > +free_id_indep:
> > + kfree(id_indep);
> > }
> > static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20220628/122749ab/attachment.sig>
More information about the Linux-nvme
mailing list