[PATCH 2/3] nvmet: per-port discovery subsystem
Sagi Grimberg
sagi at grimberg.me
Mon Apr 11 03:45:19 PDT 2022
> Add a 'disc_subsys' pointer to each port to specify which discovery
> subsystem to use.
> The pointer is set when a discovery subsystem is linked into a port,
> and reset if that link is removed.
Why not make discovery_subsystems/ subdir in a port? We can restrict
it to have only one element if needed.
>
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
> drivers/nvme/target/configfs.c | 10 +++++++++
> drivers/nvme/target/core.c | 15 ++++++++++---
> drivers/nvme/target/discovery.c | 39 ++++++++++++++++++++++-----------
> drivers/nvme/target/nvmet.h | 1 +
> 4 files changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 38b0ab9fb721..44573fe0dfe4 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -821,6 +821,12 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
> link->subsys = subsys;
>
> down_write(&nvmet_config_sem);
> + if (subsys->type == NVME_NQN_CURR &&
> + port->disc_subsys) {
The CURR is really confusing to me...
Can you explain the NQN types mapping here?
> + pr_err("discovery subsystem already present\n");
> + ret = -EAGAIN;
> + goto out_free_link;
> + }
> ret = -EEXIST;
> list_for_each_entry(p, &port->subsystems, entry) {
> if (p->subsys == subsys)
> @@ -835,6 +841,8 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
>
> list_add_tail(&link->entry, &port->subsystems);
> subsys->port_count++;
> + if (subsys->type == NVME_NQN_CURR)
> + port->disc_subsys = subsys;
> nvmet_port_disc_changed(port, subsys);
>
> up_write(&nvmet_config_sem);
> @@ -866,6 +874,8 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
> subsys->port_count--;
> nvmet_port_del_ctrls(port, subsys);
> nvmet_port_disc_changed(port, subsys);
> + if (port->disc_subsys == subsys)
> + port->disc_subsys = NULL;
>
> if (list_empty(&port->subsystems))
> nvmet_disable_port(port);
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 90e75324dae0..afd80999a335 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1495,10 +1495,19 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
> if (!port)
> return NULL;
>
> + /*
> + * As per spec discovery subsystems with a unique NQN
> + * have to respond to both NQNs, the unique NQN and the
> + * standard discovery NQN.
> + */
> if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
> - if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
> - return NULL;
> - return nvmet_disc_subsys;
> + struct nvmet_subsys *disc_subsys;
> +
> + disc_subsys = port->disc_subsys ?
> + port->disc_subsys : nvmet_disc_subsys;
> + if (!kref_get_unless_zero(&disc_subsys->ref))
> + return NULL;
> + return disc_subsys;
> }
Why use the port discovery subsys? why not just use the standard
discovery subsys?
>
> down_read(&nvmet_config_sem);
> diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
> index b5012df790d5..6b8aa6c4e752 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -29,18 +29,22 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
> struct nvmet_subsys *subsys)
> {
> struct nvmet_ctrl *ctrl;
> + struct nvmet_subsys *disc_subsys;
>
> lockdep_assert_held(&nvmet_config_sem);
> nvmet_genctr++;
>
> - mutex_lock(&nvmet_disc_subsys->lock);
> - list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
> + disc_subsys = port->disc_subsys ?
> + port->disc_subsys : nvmet_disc_subsys;
> +
> + mutex_lock(&disc_subsys->lock);
> + list_for_each_entry(ctrl, &disc_subsys->ctrls, subsys_entry) {
> if (subsys && !nvmet_host_allowed(subsys, ctrl->hostnqn))
> continue;
>
> __nvmet_disc_changed(port, ctrl);
> }
> - mutex_unlock(&nvmet_disc_subsys->lock);
> + mutex_unlock(&disc_subsys->lock);
>
> /* If transport can signal change, notify transport */
> if (port->tr_ops && port->tr_ops->discovery_chg)
> @@ -48,19 +52,23 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
> }
>
> static void __nvmet_subsys_disc_changed(struct nvmet_port *port,
> - struct nvmet_subsys *subsys,
> struct nvmet_host *host)
> {
> struct nvmet_ctrl *ctrl;
> + struct nvmet_subsys *disc_subsys;
> +
> + disc_subsys = port->disc_subsys ?
> + port->disc_subsys : nvmet_disc_subsys;
>
> - mutex_lock(&nvmet_disc_subsys->lock);
> - list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
> + mutex_lock(&disc_subsys->lock);
> +
> + list_for_each_entry(ctrl, &disc_subsys->ctrls, subsys_entry) {
> if (host && strcmp(nvmet_host_name(host), ctrl->hostnqn))
> continue;
>
> __nvmet_disc_changed(port, ctrl);
> }
> - mutex_unlock(&nvmet_disc_subsys->lock);
> + mutex_unlock(&disc_subsys->lock);
Why remove the subsys argument from this function?
> }
>
> void nvmet_subsys_disc_changed(struct nvmet_subsys *subsys,
> @@ -76,7 +84,7 @@ void nvmet_subsys_disc_changed(struct nvmet_subsys *subsys,
> list_for_each_entry(s, &port->subsystems, entry) {
> if (s->subsys != subsys)
> continue;
> - __nvmet_subsys_disc_changed(port, subsys, host);
> + __nvmet_subsys_disc_changed(port, host);
> }
> }
>
> @@ -146,7 +154,10 @@ static size_t discovery_log_entries(struct nvmet_req *req)
> struct nvmet_ctrl *ctrl = req->sq->ctrl;
> struct nvmet_subsys_link *p;
> struct nvmet_port *r;
> - size_t entries = 1;
> + size_t entries = 0;
> +
> + if (!req->port->disc_subsys)
> + entries++;
>
> list_for_each_entry(p, &req->port->subsystems, entry) {
> if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
> @@ -208,10 +219,12 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
>
> nvmet_set_disc_traddr(req, req->port, traddr);
>
> - nvmet_format_discovery_entry(hdr, req->port,
> - nvmet_disc_subsys->subsysnqn,
> - traddr, NVME_NQN_CURR, numrec);
> - numrec++;
> + if (!req->port->disc_subsys) {
> + nvmet_format_discovery_entry(hdr, req->port,
> + nvmet_disc_subsys->subsysnqn,
> + traddr, NVME_NQN_CURR, numrec);
> + numrec++;
> + }
For the unique discovery you don't need this?
>
> list_for_each_entry(p, &req->port->subsystems, entry) {
> if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index ecba3890ce65..8df297a190bf 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -144,6 +144,7 @@ struct nvmet_port {
> struct list_head global_entry;
> struct config_group ana_groups_group;
> struct nvmet_ana_group ana_default_group;
> + struct nvmet_subsys *disc_subsys;
> enum nvme_ana_state *ana_state;
> void *priv;
> bool enabled;
More information about the Linux-nvme
mailing list