[PATCH 2/3] nvmet: per-port discovery subsystem
Sagi Grimberg
sagi at grimberg.me
Tue Apr 12 05:21:05 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.
>>>>
>>>
>>> Because that would make the discovery subsystems 'special'.
>>> My idea was to treat discovery subsystems just like 'normal' subsystems,
>>> ie being part of the subsystem list per port, and able to be created
>>> like other subsystems.
>>
>> They are obviously special.
>>
> Yes and no. Both are 'subsystems'. And I wanted to avoid having to
> special-case things (like we do for referrals), as this would require
> changes to nvmetcli.
> So aim was to make the necessary user interface changes as small as
> possible.
>
>>> And using a 'discovery_subsys' subdir does raise issues with handling
>>> the default discovery subsystem.
>>> If it's not listed one would assume that this port doesn't have a
>>> discovery subsystem.
>>> If it's listed we cannot make it a default entry, as then we can't
>>> remove it.
>>
>> I think that the current interface is confusing as well.
>>
> Yes. And that's why I wanted to have the discovery subsystem showing up
> in configfs.
>
>>> And I've yet to find a way how to create dynamic entries from kernel
>>> space. If you have a way, please tell.
>>>
>>>>>
>>>>> 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?
>>>>
>>>
>>> TPAR 8014 changed the meaning of the original 'discovery subsystem'
>>> type to 'discovery subsystem referral' (as this was the actual
>>> meaning of the original spec), and added a new entry 'current
>>> discovery subsystem' to return information about the current
>>> subsystem (ie that one where the controller is attached to).
>>
>> Eh, at least the usage of CURR in the code is weird. I understand what
>> it means in the log page, but the usage here is strange. I suggest to
>> use a different enumeration naming in the code and leave CURR to the
>> log page, where it makes sense.
>>
> Okay, will be doing so.
>
>>>
>>>>> + 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?
>>>>
>>>
>>> Because (due to TPAR8014) we have to return information about the
>>> current discovery subsystem, including all port details.
>>
>> So you got a discovery log page request on the default discovery
>> subsys and you are filling the log page with content of the unique
>> discovery subsys? The current discovery subsystem is the default
>> discovery subsystem in this case, no?
>>
>> Maybe I'm missing something...
>>
> That's what TP8013 is all about.
> The unique discovery subsystem has to react to both subsystem NQNs, the
> unique NQN _and_ the default discovery subsystem NQN.
> But there will be only _one_ discovery subsystem per port.
Not sure why the spec mandates that, but OK.
In essence the unique discovery subsystem "takes over" the port
and fills up whatever it has, regardless of the disc-subsys NQN
the host is accessing nor accessed in the past...
>>>>> 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?
>>>>
>>>
>>> Because it's not needed?
>>> But I can make it a separate patch if you prefer.
>>
>> But different hosts can be connected to different discovery subsystems.
>>
>
> Sure. But the 'subsys' argument is unused even in the current code.
>
>> Is the implementation intent is that you always use the unique
>> discovery subsystem also when hosts refer to the default discovery
>> subsystem?
>>
> Yes. That is what TPAR8013 mandates.
I wasn't aware of that. This makes a stronger case to why the unique
disc subsys should behave similar to the standard one.
More information about the Linux-nvme
mailing list