[PATCH 2/3] nvmet: per-port discovery subsystem
Hannes Reinecke
hare at suse.de
Tue Apr 12 04:51:11 PDT 2022
On 4/12/22 12:40, Sagi Grimberg wrote:
>
>
> On 4/11/22 15:07, Hannes Reinecke wrote:
>> On 4/11/22 12:45, Sagi Grimberg wrote:
>>>
>>>> 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.
>>
>>>> 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.
> If so, that behavior was not clear in the cover-letter/change-log at
> all.
>
Okay, I'll change it.
Unless you object to this approach in general; then it hardly makes
sense to continue.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare at suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
More information about the Linux-nvme
mailing list