[PATCH v3 2/2] nvme: enable generic interface (/dev/ngXnY) for unknown command sets
Sagi Grimberg
sagi at grimberg.me
Sun Jun 26 06:53:32 PDT 2022
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 ?
> +
> + /* 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?
> /*
> * 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,
More information about the Linux-nvme
mailing list