track subsystem relationships and shared namespaces V2
Subsystems (3)
Christoph Hellwig
maintainerHi all,
this set adds the first bits of required infrastructure for proper multipath support in the NVMe driver. It tracks if multiple controllers belong to the same subsystem, and if they do which namespaces on the controller refer to the same data. As part of that it validates a lot of the restrictions in the NVMe spec related to these facts.
Btw, qemu is a perfect way to create controllers with the same NQN that fail these checks, as it allows you to specify the same serial number for multiple controllers, and never sets the required CMIC bit for multi-controller subsystems.
Changes since V1:
- don't hardcode offsets in nvme_init_subnqn
- rename __nvme_find_subsystem to __nvme_find_get_subsystem
- rename nvme_ns_link_siblings and move the list_add out of it
Christoph Hellwig
maintainerPATCH 1/6 nvme: remove a misleading comment on strut nvme_ns
While a NVMe Namespace is somewhat similar to a SCSI Logical Unit (and not a Logical Unit Number anyway) there are subtile differences. Remove the misleading comment.
--- drivers/nvme/host/nvme.h | 3 --- 1 file changed, 3 deletions(-)
drivers/nvme/host/nvme.h +0 -3
@@ -179,9 +179,6 @@ struct nvme_ctrl {struct nvmf_ctrl_options *opts; }; -/* -* An NVM Express namespace is equivalent to a SCSI LUN -*/ struct nvme_ns {struct list_head list;
Signature · 1 line
2.11.0
PATCH 1/6 nvme: remove a misleading comment on strut nvme_ns
Quoted 3 lines · Christoph Hellwig
While a NVMe Namespace is somewhat similar to a SCSI Logical Unit (and not a Logical Unit Number anyway) there are subtile differences. Remove the misleading comment.
Looks good.
Christoph Hellwig
maintainerPATCH 2/6 nvme: read the subsystem NQN from Identify Controller
In reply to Christoph HellwigNVMe 1.2.1 or later requires controllers to provide a subsystem NQN in the Identify controller data structures. Use this NQN for the subsysnqn sysfs attribute by storing it in the nvme_ctrl structure after verifying it. For older controllers we generate a "fake" NQN per non-normative text in the NVMe 1.3 spec.
--- drivers/nvme/host/core.c | 31 ++++++++++++++++++++++++++++--- drivers/nvme/host/fabrics.c | 10 ---------- drivers/nvme/host/fabrics.h | 1 - drivers/nvme/host/fc.c | 1 - drivers/nvme/host/nvme.h | 2 +- drivers/nvme/host/rdma.c | 1 - drivers/nvme/target/loop.c | 1 - 7 files changed, 29 insertions(+), 18 deletions(-)
drivers/nvme/host/core.c +28 -3
@@ -1642,6 +1642,31 @@ static bool quirk_matches(const struct nvme_id_ctrl *id,string_matches (id->fr, q->fr,sizeof (id->fr)); } +static void nvme_init_subnqn (struct nvme_ctrl *ctrl,struct nvme_id_ctrl *id) +{ +size_t nqnlen; +int off; + + nqnlen =strnlen (id->subnqn,NVMF_NQN_SIZE ); +if (nqnlen >0 && nqnlen <NVMF_NQN_SIZE ) { +strcpy (ctrl->subnqn, id->subnqn); +return ; + } + +if (ctrl->vs >=NVME_VS (1 ,2 ,1 )) +dev_warn (ctrl->device,"missing or invalid SUBNQN field.\n" ); + +/* Generate a "fake" NQN per Figure 254 in NVMe 1.3 + ECN 001 */ + off =snprintf (ctrl->subnqn,NVMF_NQN_SIZE , +"nqn.2014.08.org.nvmexpress:%4x%4x" , +le16_to_cpu (id->vid),le16_to_cpu (id->ssvid)); +memcpy (ctrl->subnqn + off, id->sn,sizeof (id->sn)); + off +=sizeof (id->sn); +memcpy (ctrl->subnqn + off, id->mn,sizeof (id->mn)); + off +=sizeof (id->mn); +memset (ctrl->subnqn + off,0 ,sizeof (ctrl->subnqn) - off); +} +/* * Initialize the cached copies of the Identify data and various controller * register in our nvme_ctrl structure. This should be called as soon as
@@ -1677,6 +1702,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)return -EIO ; } +nvme_init_subnqn (ctrl, id); +if (!ctrl->identified) {/* * Check for quirks. Quirk can depend on firmware version,
@@ -2070,8 +2097,7 @@ static ssize_t nvme_sysfs_show_subsysnqn(struct device *dev, {struct nvme_ctrl *ctrl =dev_get_drvdata (dev); -return snprintf (buf,PAGE_SIZE ,"%s\n" , - ctrl->ops->get_subsysnqn (ctrl)); +return snprintf (buf,PAGE_SIZE ,"%s\n" , ctrl->subnqn); } static DEVICE_ATTR(subsysnqn, S_IRUGO, nvme_sysfs_show_subsysnqn, NULL);
@@ -2116,7 +2142,6 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,return 0 ; } -CHECK_ATTR (ctrl, a, subsysnqn);CHECK_ATTR (ctrl, a, address);return a->mode;
drivers/nvme/host/fabrics.c +0 -10
@@ -128,16 +128,6 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size)EXPORT_SYMBOL_GPL (nvmf_get_address);/** -* nvmf_get_subsysnqn() - Get subsystem NQN -* @ctrl: Host NVMe controller instance which we got the NQN -*/ -const char *nvmf_get_subsysnqn (struct nvme_ctrl *ctrl) -{ -return ctrl->opts->subsysnqn; -} -EXPORT_SYMBOL_GPL (nvmf_get_subsysnqn); - -/** * nvmf_reg_read32() - NVMe Fabrics "Property Get" API function. * @ctrl: Host NVMe controller instance maintaining the admin * queue used to submit the property read command to
drivers/nvme/host/fabrics.h +0 -1
@@ -138,7 +138,6 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid);int nvmf_register_transport (struct nvmf_transport_ops *ops);void nvmf_unregister_transport (struct nvmf_transport_ops *ops);void nvmf_free_options (struct nvmf_ctrl_options *opts); -const char *nvmf_get_subsysnqn (struct nvme_ctrl *ctrl);int nvmf_get_address (struct nvme_ctrl *ctrl,char *buf,int size);bool nvmf_should_reconnect (struct nvme_ctrl *ctrl);
drivers/nvme/host/fc.c +0 -1
@@ -2631,7 +2631,6 @@ static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = { .free_ctrl = nvme_fc_nvme_ctrl_freed, .submit_async_event = nvme_fc_submit_async_event, .delete_ctrl = nvme_fc_del_nvme_ctrl, - .get_subsysnqn = nvmf_get_subsysnqn, .get_address = nvmf_get_address, };
drivers/nvme/host/nvme.h +1 -1
@@ -138,6 +138,7 @@ struct nvme_ctrl {char serial[20 ];char model[40 ];char firmware_rev[8 ]; +char subnqn[NVMF_NQN_SIZE ];u16 cntlid;u32 ctrl_config;
@@ -220,7 +221,6 @@ struct nvme_ctrl_ops {void (*free_ctrl)(struct nvme_ctrl *ctrl);void (*submit_async_event)(struct nvme_ctrl *ctrl,int aer_idx);int (*delete_ctrl)(struct nvme_ctrl *ctrl); -const char *(*get_subsysnqn)(struct nvme_ctrl *ctrl);int (*get_address)(struct nvme_ctrl *ctrl,char *buf,int size); };
drivers/nvme/host/rdma.c +0 -1
@@ -1757,7 +1757,6 @@ static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = { .free_ctrl = nvme_rdma_free_ctrl, .submit_async_event = nvme_rdma_submit_async_event, .delete_ctrl = nvme_rdma_del_ctrl, - .get_subsysnqn = nvmf_get_subsysnqn, .get_address = nvmf_get_address, };
drivers/nvme/target/loop.c +0 -1
@@ -542,7 +542,6 @@ static const struct nvme_ctrl_ops nvme_loop_ctrl_ops = { .free_ctrl = nvme_loop_free_ctrl, .submit_async_event = nvme_loop_submit_async_event, .delete_ctrl = nvme_loop_del_ctrl, - .get_subsysnqn = nvmf_get_subsysnqn, };static int nvme_loop_create_io_queues (struct nvme_loop_ctrl *ctrl)
Signature · 1 line
2.11.0
PATCH 2/6 nvme: read the subsystem NQN from Identify Controller
Quoted 8 lines · Christoph Hellwig
NVMe 1.2.1 or later requires controllers to provide a subsystem NQN in the Identify controller data structures. Use this NQN for the subsysnqn sysfs attribute by storing it in the
nvme_ctrlstructure after verifying it. For older controllers we generate a "fake" NQN per non-normative text in the NVMe 1.3 spec.Signed-off-by Christoph Hellwig <hch at lst.de>---
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 2/6 nvme: read the subsystem NQN from Identify Controller
In reply to Christoph HellwigQuoted 5 lines · Christoph Hellwig
NVMe 1.2.1 or later requires controllers to provide a subsystem NQN in the Identify controller data structures. Use this NQN for the subsysnqn sysfs attribute by storing it in the
nvme_ctrlstructure after verifying it. For older controllers we generate a "fake" NQN per non-normative text in the NVMe 1.3 spec.
Looks good.
Christoph Hellwig
maintainerPATCH 3/6 nvme: simplify nvme_dev_attrs_are_visible
In reply to Christoph Hellwig--- drivers/nvme/host/core.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
drivers/nvme/host/core.c +4 -11
@@ -2126,23 +2126,16 @@ static struct attribute *nvme_dev_attrs[] = {NULL }; -#define CHECK_ATTR (ctrl, a, name) \ - if ((a) == &dev_attr_##name.attr && \ - !(ctrl)->ops->get_##name) \ - return 0 -static umode_t nvme_dev_attrs_are_visible (struct kobject *kobj,struct attribute *a,int n) { struct device *dev = container_of(kobj, struct device, kobj);struct nvme_ctrl *ctrl =dev_get_drvdata (dev); -if (a == &dev_attr_delete_controller.attr) { -if (!ctrl->ops->delete_ctrl) -return 0 ; - } - -CHECK_ATTR (ctrl, a, address); +if (a == &dev_attr_delete_controller.attr && !ctrl->ops->delete_ctrl) +return 0 ; +if (a == &dev_attr_address.attr && !ctrl->ops->get_address) +return 0 ;return a->mode; }
Signature · 1 line
2.11.0
PATCH 3/6 nvme: simplify nvme_dev_attrs_are_visible
Looks fine.
Christoph Hellwig
maintainerPATCH 4/6 nvme-fabrics: verify that a controller returns the correct NQN
In reply to Christoph Hellwig--- drivers/nvme/host/fabrics.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/nvme/host/fabrics.c +9 -0
@@ -862,6 +862,15 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count) goto out_unlock; } +if (strcmp (ctrl->subnqn, opts->subsysnqn)) { +dev_warn (ctrl->device, +"controller returned incorrect NQN: \"%s\".\n" , + ctrl->subnqn); +mutex_unlock (&nvmf_transports_mutex); + ctrl->ops->delete_ctrl (ctrl); +return ERR_PTR (-EINVAL ); + } +mutex_unlock (&nvmf_transports_mutex);return ctrl;
Signature · 1 line
2.11.0
Christoph Hellwig
maintainerPATCH 5/6 nvme: track subsystems
In reply to Christoph HellwigThis adds a new nvme_subsystem structure so that we can track multiple controllers that belong to a single subsystem. For now we only use it to store the NQN, and to check that we don't have duplicate NQNs unless the involved subsystems support multiple controllers.
--- drivers/nvme/host/core.c | 111 ++++++++++++++++++++++++++++++++++++++++---- drivers/nvme/host/fabrics.c | 4 +- drivers/nvme/host/nvme.h | 12 ++++- 3 files changed, 116 insertions(+), 11 deletions(-)
drivers/nvme/host/core.c +103 -8
@@ -68,6 +68,9 @@ MODULE_PARM_DESC(force_apst, "allow APST for newly enumerated devices even if qustruct workqueue_struct *nvme_wq;EXPORT_SYMBOL_GPL (nvme_wq); +static LIST_HEAD (nvme_subsystems); +static DEFINE_MUTEX (nvme_subsystems_lock); +static LIST_HEAD (nvme_ctrl_list);static DEFINE_SPINLOCK (dev_list_lock);
@@ -1642,14 +1645,15 @@ static bool quirk_matches(const struct nvme_id_ctrl *id,string_matches (id->fr, q->fr,sizeof (id->fr)); } -static void nvme_init_subnqn (struct nvme_ctrl *ctrl,struct nvme_id_ctrl *id) +static void nvme_init_subnqn (struct nvme_subsystem *subsys,struct nvme_ctrl *ctrl, +struct nvme_id_ctrl *id) {size_t nqnlen;int off; nqnlen =strnlen (id->subnqn,NVMF_NQN_SIZE );if (nqnlen >0 && nqnlen <NVMF_NQN_SIZE ) { -strcpy (ctrl->subnqn, id->subnqn); +strcpy (subsys->subnqn, id->subnqn);return ; }
@@ -1657,14 +1661,91 @@ static void nvme_init_subnqn(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)dev_warn (ctrl->device,"missing or invalid SUBNQN field.\n" );/* Generate a "fake" NQN per Figure 254 in NVMe 1.3 + ECN 001 */ - off =snprintf (ctrl->subnqn,NVMF_NQN_SIZE , + off =snprintf (subsys->subnqn,NVMF_NQN_SIZE ,"nqn.2014.08.org.nvmexpress:%4x%4x" ,le16_to_cpu (id->vid),le16_to_cpu (id->ssvid)); -memcpy (ctrl->subnqn + off, id->sn,sizeof (id->sn)); +memcpy (subsys->subnqn + off, id->sn,sizeof (id->sn)); off +=sizeof (id->sn); -memcpy (ctrl->subnqn + off, id->mn,sizeof (id->mn)); +memcpy (subsys->subnqn + off, id->mn,sizeof (id->mn)); off +=sizeof (id->mn); -memset (ctrl->subnqn + off,0 ,sizeof (ctrl->subnqn) - off); +memset (subsys->subnqn + off,0 ,sizeof (subsys->subnqn) - off); +} + +static void nvme_destroy_subsystem (struct kref *ref) +{ +struct nvme_subsystem *subsys = + container_of(ref, struct nvme_subsystem, ref); + +mutex_lock (&nvme_subsystems_lock); +list_del (&subsys->entry); +mutex_unlock (&nvme_subsystems_lock); + +kfree (ref); +} + +static void nvme_put_subsystem (struct nvme_subsystem *subsys) +{ +kref_put (&subsys->ref, nvme_destroy_subsystem); +} + +static struct nvme_subsystem *__nvme_find_get_subsystem (const char *subsysnqn) +{ +struct nvme_subsystem *subsys; + +lockdep_assert_held (&nvme_subsystems_lock); + +list_for_each_entry (subsys, &nvme_subsystems, entry) { +if (strcmp (subsys->subnqn, subsysnqn)) +continue ; +if (!kref_get_unless_zero (&subsys->ref)) +continue ; +return subsys; + } + +return NULL ; +} + +static int nvme_init_subsystem (struct nvme_ctrl *ctrl,struct nvme_id_ctrl *id) +{ +struct nvme_subsystem *subsys, *found; + + subsys =kzalloc (sizeof (*subsys),GFP_KERNEL ); +if (!subsys) +return -ENOMEM ; +INIT_LIST_HEAD (&subsys->ctrls); +kref_init (&subsys->ref); +nvme_init_subnqn (subsys, ctrl, id); +mutex_init (&subsys->lock); + +mutex_lock (&nvme_subsystems_lock); + found =__nvme_find_get_subsystem (subsys->subnqn); +if (found) { +/* +* Verify that the subsystem actually supports multiple +* controllers, else bail out. +*/ +kfree (subsys); +if (!(id->cmic & (1 <<1 ))) { +dev_err (ctrl->device, +"ignoring ctrl due to duplicate subnqn (%s).\n" , + found->subnqn); +mutex_unlock (&nvme_subsystems_lock); +return -EINVAL ; + } + + subsys = found; + }else { +list_add_tail (&subsys->entry, &nvme_subsystems); + } + + ctrl->subsys = subsys; +mutex_unlock (&nvme_subsystems_lock); + +mutex_lock (&subsys->lock); +list_add_tail (&ctrl->subsys_entry, &subsys->ctrls); +mutex_unlock (&subsys->lock); + +return 0 ; }/*
@@ -1702,7 +1783,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)return -EIO ; } -nvme_init_subnqn (ctrl, id); + ret =nvme_init_subsystem (ctrl, id); +if (ret) { +kfree (id); +return ret; + }if (!ctrl->identified) {/*
@@ -2097,7 +2182,7 @@ static ssize_t nvme_sysfs_show_subsysnqn(struct device *dev, {struct nvme_ctrl *ctrl =dev_get_drvdata (dev); -return snprintf (buf,PAGE_SIZE ,"%s\n" , ctrl->subnqn); +return snprintf (buf,PAGE_SIZE ,"%s\n" , ctrl->subsys->subnqn); } static DEVICE_ATTR(subsysnqn, S_IRUGO, nvme_sysfs_show_subsysnqn, NULL);
@@ -2516,12 +2601,22 @@ EXPORT_SYMBOL_GPL(nvme_uninit_ctrl);static void nvme_free_ctrl (struct kref *kref) { struct nvme_ctrl *ctrl = container_of(kref, struct nvme_ctrl, kref); +struct nvme_subsystem *subsys = ctrl->subsys;put_device (ctrl->device);nvme_release_instance (ctrl);ida_destroy (&ctrl->ns_ida); +if (subsys) { +mutex_lock (&subsys->lock); +list_del (&ctrl->subsys_entry); +mutex_unlock (&subsys->lock); + } + ctrl->ops->free_ctrl (ctrl); + +if (subsys) +nvme_put_subsystem (subsys); }void nvme_put_ctrl (struct nvme_ctrl *ctrl)
drivers/nvme/host/fabrics.c +2 -2
@@ -862,10 +862,10 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count) goto out_unlock; } -if (strcmp (ctrl->subnqn, opts->subsysnqn)) { +if (strcmp (ctrl->subsys->subnqn, opts->subsysnqn)) {dev_warn (ctrl->device,"controller returned incorrect NQN: \"%s\".\n" , - ctrl->subnqn); + ctrl->subsys->subnqn);mutex_unlock (&nvmf_transports_mutex); ctrl->ops->delete_ctrl (ctrl);return ERR_PTR (-EINVAL );
drivers/nvme/host/nvme.h +11 -1
@@ -132,13 +132,15 @@ struct nvme_ctrl {struct ida ns_ida;struct work_struct reset_work; +struct nvme_subsystem *subsys; +struct list_head subsys_entry; +struct opal_dev *opal_dev;char name[12 ];char serial[20 ];char model[40 ];char firmware_rev[8 ]; -char subnqn[NVMF_NQN_SIZE ];u16 cntlid;u32 ctrl_config;
@@ -180,6 +182,14 @@ struct nvme_ctrl {struct nvmf_ctrl_options *opts; }; +struct nvme_subsystem { +struct list_head entry; +struct mutex lock; +struct list_head ctrls; +struct kref ref; +char subnqn[NVMF_NQN_SIZE ]; +}; +struct nvme_ns {struct list_head list;
Signature · 1 line
2.11.0
PATCH 5/6 nvme: track subsystems
Quoted 7 lines · Christoph Hellwig
This adds a new
nvme_subsystemstructure so that we can track multiple controllers that belong to a single subsystem. For now we only use it to store the NQN, and to check that we don't have duplicate NQNs unless the involved subsystems support multiple controllers.Signed-off-by Christoph Hellwig <hch at lst.de>---
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
PATCH 5/6 nvme: track subsystems
In reply to Christoph HellwigQuoted 4 lines · Christoph Hellwig
This adds a new
nvme_subsystemstructure so that we can track multiple controllers that belong to a single subsystem. For now we only use it to store the NQN, and to check that we don't have duplicate NQNs unless the involved subsystems support multiple controllers.
Looks good.
Christoph Hellwig
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
In reply to Christoph HellwigVerify that our IDs match for shared namespaces in a subystem, and link them up in a headless queue so that we can find siblings easily.
--- drivers/nvme/host/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 1 + 2 files changed, 100 insertions(+)
drivers/nvme/host/core.c +99 -0
@@ -2261,6 +2261,100 @@ static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)return ret; } +/* +* Check if the two nvme_ns structures refer to the same namespace. +* +* Returns 1 if they refer to the same namespace, 0 if not, or a negative +* errno value on error. +*/ +static int nvme_ns_compare_ids (struct nvme_ns *ns,struct nvme_id_ns *id, +struct nvme_ns *cur) +{ +bool is_shared = id->nmic & (1 <<0 ); +bool have_id = false; + +if (!uuid_is_null (&ns->uuid)) { + have_id = true; +if (!uuid_equal (&ns->uuid, &cur->uuid)) +return 0 ; + } +if (memchr_inv (ns->nguid,0 ,sizeof (ns->nguid))) { + have_id = true; +if (memcmp (&ns->nguid, &cur->nguid,sizeof (ns->nguid))) +return 0 ; + } +if (memchr_inv (ns->eui,0 ,sizeof (ns->eui))) { + have_id = true; +if (memcmp (&ns->eui, &cur->eui,sizeof (ns->eui))) +return 0 ; + } + +/* +* Note that the unique identifiers are only a "should" condition in +* the specification. But we require them for multipath operations +* due to safety concerns. +*/ +if (!have_id && is_shared) { +dev_err (ns->ctrl->device, +"shared namespace %u without unique ID.\n" , + ns->ns_id); +return -EINVAL ; + } + +if (have_id && !is_shared) { +dev_err (ns->ctrl->device, +"private namespace %u with duplicate ID.\n" , + ns->ns_id); +return -EINVAL ; + } + +/* +* As of NVMe 1.3 (Section 6.1) this is only strictly required if namespace +* management is supported, but sort of implied for shared namespaces +* by omitting the defintion. We'll need an ECN for this language +* eventually. +*/ +if (is_shared && ns->ns_id != cur->ns_id) { +dev_err (ns->ctrl->device, +"non-matching nsids (%u vs %u) for shared namespaces\n" , + ns->ns_id, cur->ns_id); +return -EINVAL ; + } + +return 1 ; +} + +static int nvme_ns_find_siblings (struct nvme_ns *ns,struct nvme_id_ns *id) +{ +struct nvme_subsystem *subsys = ns->ctrl->subsys; +struct nvme_ctrl *ctrl; +struct nvme_ns *cur; +int err =0 , ret; + +mutex_lock (&subsys->lock); +list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) { +if (ctrl == ns->ctrl) +continue ; + +mutex_lock (&ctrl->namespaces_mutex); +list_for_each_entry (cur, &ctrl->namespaces, list) { + ret =nvme_ns_compare_ids (ns, id, cur); +if (ret <0 ) { + err = ret; +break ; + } +if (ret >0 ) +list_add (&ns->siblings, &cur->siblings); + } +mutex_unlock (&ctrl->namespaces_mutex); + +if (err) +break ; + } +mutex_unlock (&subsys->lock); +return err; +} +static void nvme_alloc_ns (struct nvme_ctrl *ctrl,unsigned nsid) {struct nvme_ns *ns;
@@ -2283,6 +2377,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)queue_flag_set_unlocked (QUEUE_FLAG_NONROT , ns->queue); ns->queue->queuedata = ns; ns->ctrl = ctrl; +INIT_LIST_HEAD (&ns->siblings);kref_init (&ns->kref); ns->ns_id = nsid;
@@ -2296,6 +2391,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)if (nvme_revalidate_ns (ns, &id)) goto out_free_queue; +if (nvme_ns_find_siblings (ns, id)) + goto out_free_id; +if (nvme_nvm_ns_supported (ns, id) &&nvme_nvm_register (ns, disk_name, node)) {dev_warn (ctrl->device,"%s: LightNVM init failure\n" , __func__);
@@ -2360,6 +2458,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)mutex_lock (&ns->ctrl->namespaces_mutex);list_del_init (&ns->list); +list_del_init (&ns->siblings);mutex_unlock (&ns->ctrl->namespaces_mutex);nvme_put_ns (ns);
drivers/nvme/host/nvme.h +1 -0
@@ -196,6 +196,7 @@ struct nvme_ns {struct nvme_ctrl *ctrl;struct request_queue *queue;struct gendisk *disk; +struct list_head siblings;struct nvm_dev *ndev;struct kref kref;int instance;
Signature · 1 line
2.11.0
Sagi Grimberg
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 90 lines · Christoph Hellwig
Verify that our IDs match for shared namespaces in a subystem, and link them up in a headless queue so that we can find siblings easily.
Signed-off-by Christoph Hellwig <hch at lst.de>--- drivers/nvme/host/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 1 + 2 files changed, 100 insertions(+)drivers/nvme/host/core.c +73
-0@@ -2261,6 +2261,100 @@ static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)return ret; } +/* +* Check if the two nvme_ns structures refer to the same namespace. +* +* Returns 1 if they refer to the same namespace, 0 if not, or a negative +* errno value on error. +*/ +static int nvme_ns_compare_ids (struct nvme_ns *ns,struct nvme_id_ns *id, +struct nvme_ns *cur) +{ +bool is_shared = id->nmic & (1 <<0 ); +bool have_id = false; + +if (!uuid_is_null (&ns->uuid)) { + have_id = true; +if (!uuid_equal (&ns->uuid, &cur->uuid)) +return 0 ; + } +if (memchr_inv (ns->nguid,0 ,sizeof (ns->nguid))) { + have_id = true; +if (memcmp (&ns->nguid, &cur->nguid,sizeof (ns->nguid))) +return 0 ; + } +if (memchr_inv (ns->eui,0 ,sizeof (ns->eui))) { + have_id = true; +if (memcmp (&ns->eui, &cur->eui,sizeof (ns->eui))) +return 0 ; + } + +/* +* Note that the unique identifiers are only a "should" condition in +* the specification. But we require them for multipath operations +* due to safety concerns. +*/ +if (!have_id && is_shared) { +dev_err (ns->ctrl->device, +"shared namespace %u without unique ID.\n" , + ns->ns_id); +return -EINVAL ; + } + +if (have_id && !is_shared) { +dev_err (ns->ctrl->device, +"private namespace %u with duplicate ID.\n" , + ns->ns_id); +return -EINVAL ; + } + +/* +* As of NVMe 1.3 (Section 6.1) this is only strictly required if namespace +* management is supported, but sort of implied for shared namespaces +* by omitting the defintion. We'll need an ECN for this language +* eventually. +*/ +if (is_shared && ns->ns_id != cur->ns_id) { +dev_err (ns->ctrl->device, +"non-matching nsids (%u vs %u) for shared namespaces\n" , + ns->ns_id, cur->ns_id); +return -EINVAL ; + } + +return 1 ; +} + +static int nvme_ns_find_siblings (struct nvme_ns *ns,struct nvme_id_ns *id) +{ +struct nvme_subsystem *subsys = ns->ctrl->subsys; +struct nvme_ctrl *ctrl; +struct nvme_ns *cur; +int err =0 , ret; + +mutex_lock (&subsys->lock); +list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) { +if (ctrl == ns->ctrl)
drivers/nvme/host/core.c static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid) · line 2346
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+continue ; + +mutex_lock (&ctrl->namespaces_mutex); +list_for_each_entry (cur, &ctrl->namespaces, list) { + ret =nvme_ns_compare_ids (ns, id, cur); +if (ret <0 ) { + err = ret; +break ; + } +if (ret >0 )
} else if (ret > 0) {
list_add(&ns->siblings, &cur->siblings);
}drivers/nvme/host/core.c static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid) · line 2357
Quoted 11 lines
drivers/nvme/host/core.c +11
-0+list_add (&ns->siblings, &cur->siblings); + } +mutex_unlock (&ctrl->namespaces_mutex); + +if (err) +break ; + } +mutex_unlock (&subsys->lock); +return err; +} +
General question, Would it make sense to invoke this search from the block layer as part of adding a new disk (a new block_device operation)? And should the sibling association maybe move up to struct block_device? (we could cache nvme_id_ns in nvme_ns for that to happen)?
Christoph Hellwig
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 4 lines · Sagi Grimberg
General question, Would it make sense to invoke this search from the block layer as part of adding a new disk (a new
block_deviceoperation)? And should the sibling association maybe move up to structblock_device? (we could cachenvme_id_nsinnvme_nsfor that to happen)?
For the block/nvme multipath code I need to expose the relationship to the block layer. But the locking is already complicated enough as is, so I want to keep the NVMe list as well with that.
Sagi Grimberg
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 9 lines
Quoted 5 lines
General question, Would it make sense to invoke this search from the block layer as part of adding a new disk (a new
block_deviceoperation)? And should the sibling association maybe move up to structblock_device? (we could cachenvme_id_nsinnvme_nsfor that to happen)?For the block/nvme multipath code I need to expose the relationship to the block layer. But the locking is already complicated enough as is, so I want to keep the NVMe list as well with that.
Not sure what you mean, but I didn't mean to change any locking scheme, just have the sibling list exist in struct block_device and call nvme_ns_find_siblings as a fops callout when the bdev is created, driver stays responsible for locking.
Christoph Hellwig
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 4 lines · Sagi Grimberg
Not sure what you mean, but I didn't mean to change any locking scheme, just have the sibling list exist in struct
block_deviceand callnvme_ns_find_siblingsas a fops callout when the bdev is created, driver stays responsible for locking.
A common list with driver specific locking isn't going to work, we'll need block locking for using it for multipath decisions.
Sagi Grimberg
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 8 lines
Quoted 5 lines
Not sure what you mean, but I didn't mean to change any locking scheme, just have the sibling list exist in struct
block_deviceand callnvme_ns_find_siblingsas a fops callout when the bdev is created, driver stays responsible for locking.A common list with driver specific locking isn't going to work, we'll need block locking for using it for multipath decisions.
Didn't mean driver specific locking on the sibling list itself, just on the sibling search. The sibling list should obviously have its own locking. You have yet to reveal how the block layer should handle the siblings, but I imagine you have some family representation, which will have its own locking scheme to manage siblings/paths.
Christoph Hellwig
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 5 lines · Sagi Grimberg
Didn't mean driver specific locking on the sibling list itself, just on the sibling search. The sibling list should obviously have its own locking. You have yet to reveal how the block layer should handle the siblings, but I imagine you have some family representation, which will have its own locking scheme to manage siblings/paths.
Oh. No, I don't want to do the search in the block layer at all.
The are two (or more) request_queues siblings thing makes total sense for the block layer. But how we decide that fact is totally driver specif?c. E.g. for SCSI the search would look completely different as we don't have the equivalent of the controllers per subsystem list.
Sagi Grimberg
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 7 lines
Quoted 6 lines
Didn't mean driver specific locking on the sibling list itself, just on the sibling search. The sibling list should obviously have its own locking. You have yet to reveal how the block layer should handle the siblings, but I imagine you have some family representation, which will have its own locking scheme to manage siblings/paths.
Quoted 7 lines
Oh. No, I don't want to do the search in the block layer at all.
The are two (or more)
request_queuessiblings thing makes total sense for the block layer. But how we decide that fact is totally driver specif?c. E.g. for SCSI the search would look completely different as we don't have the equivalent of the controllers per subsystem list.
OK, I'm totally not getting my point across obviously...
I completely agree that the multipath map (sibling list) is driver specific, I'm just arguing that the search itself can be invoked from the block layer through a block_device_operation when the bdev is created (in there, the driver sibling search has its own driver specific locking).
When a match is found, driver calls something like blk_link_sibling(a, b) which grows a sibling relationship map, this call has block layer locking protection.
[somehow I still have a feeling this won't get across either...]
btw, I didn't see handling of the case where a sibling match found where the sibling is already linked (a sibling too).
say we have namespaces a, b and c, where b,c are siblings of a (all with the same nsid=3).
If I read the code correctly, c will link to both a and b wouldn't it?
Do we need to check: list_empty(&cur->siblings)?
Or am I not understanding the data structure?
Christoph Hellwig
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 5 lines · Sagi Grimberg
I completely agree that the multipath map (sibling list) is driver specific, I'm just arguing that the search itself can be invoked from the block layer through a
block_device_operationwhen the bdev is created (in there, the driver sibling search has its own driver specific locking).
Let's wait for that until we have some block layer code posted. I'll see if I can make this work, but I suspect driving the search from the driver is going to be a lot easier.
Quoted 11 lines
btw, I didn't see handling of the case where a sibling match found where the sibling is already linked (a sibling too).
say we have namespaces a, b and c, where b,c are siblings of a (all with the same nsid=3).
If I read the code correctly, c will link to both a and b wouldn't it?
Do we need to check:
list_empty(&cur->siblings)?Or am I not understanding the data structure?
Yeah, we should break after finding the first sibling. Let me create a test setup with three controllers with the same namespaces..
Sagi Grimberg
maintainerPATCH 6/6 nvme: track shared namespaces in a siblings list
Quoted 8 lines
Quoted 6 lines
I completely agree that the multipath map (sibling list) is driver specific, I'm just arguing that the search itself can be invoked from the block layer through a
block_device_operationwhen the bdev is created (in there, the driver sibling search has its own driver specific locking).Let's wait for that until we have some block layer code posted.
I was actually going to ask you to share the block code :) It will clear a lot for me...