[RFC PATCH 0/3] nvmet: export discovery subsystem

[RFC PATCH 0/3] nvmet: export discovery subsystem

[Hannes Reinecke]

Hi all,

here's a patchset to expose the discovery subsystem in configfs.
The current implementation is has a rather 'odd' discovery subsystem
implementation; there is one discovery subsystem structure, but the
details of this subsystem will be modified depending from which port
(and, consequently, port->subsystem link) is has been called.
This makes is really awkward to assign unique properties to it
(like the unique discovery NQN).

With this patchset the discovery subsystem is elevated to a 'real'
subsystem, with a normal subsystem entry in sysfs and with activation
of it by linking this subsystem to ports.
This gives full control to the user which subsystem should be visible
on this discovery controller, and also allows to have a discovery
controller with no I/O subsystems at all.

Drawback is that it's a change in operation, as the user _has_ to
link the subsystem to individual ports.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  nvmet: expose discovery subsystem in sysfs
  nvmet: restrict setting of discovery_nqn to discovery subsystem
  nvmet: do not allow to create a subsystem with the discovery NQN

 drivers/nvme/target/configfs.c  | 46 +++++++++++++++++++++++++++++----
 drivers/nvme/target/core.c      |  1 +
 drivers/nvme/target/discovery.c |  7 +----
 3 files changed, 43 insertions(+), 11 deletions(-)

-- 
2.29.2

[PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Hannes Reinecke]

Expose the discovery subsystem in sysfs instead of having it as
an internal structure. This requires the admin to manually
enable the discovery subsystem by linking it to a port, but
this can be seen as a feature as it will allows us to expose
subsystems without discovery subsystems and discovery subsystems
with no I/O subsystems.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/configfs.c  | 11 +++++++++++
 drivers/nvme/target/core.c      |  1 +
 drivers/nvme/target/discovery.c |  7 +------
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index a54cbddd764e..9b94af2444b2 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1041,6 +1041,11 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,
 	if (strtobool(page, &allow_any_host))
 		return -EINVAL;
 
+	if (subsys == nvmet_disc_subsys) {
+		pr_err("Can't change allow_any_host on discovery subsystem!\n");
+		return -EPERM;
+	}
+
 	down_write(&nvmet_config_sem);
 	if (allow_any_host && !list_empty(&subsys->hosts)) {
 		pr_err("Can't set allow_any_host when explicit hosts are set!\n");
@@ -1965,6 +1970,12 @@ int __init nvmet_init_configfs(void)
 	configfs_add_default_group(&nvmet_subsystems_group,
 			&nvmet_configfs_subsystem.su_group);
 
+	config_group_init_type_name(&nvmet_disc_subsys->group,
+				    NVME_DISC_SUBSYS_NAME,
+				    &nvmet_subsys_type);
+	configfs_add_default_group(&nvmet_disc_subsys->group,
+				   &nvmet_subsystems_group);
+
 	config_group_init_type_name(&nvmet_ports_group,
 			"ports", &nvmet_ports_type);
 	configfs_add_default_group(&nvmet_ports_group,
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 44f544356081..fab92c008c53 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1557,6 +1557,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		break;
 	case NVME_NQN_DISC:
 	case NVME_NQN_CURR:
+		subsys->allow_any_host = 1;
 		subsys->max_qid = 0;
 		break;
 	default:
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index c2162eef8ce1..90bcc829c12a 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -208,18 +208,13 @@ 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++;
-
 	list_for_each_entry(p, &req->port->subsystems, entry) {
 		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
 			continue;
 
 		nvmet_format_discovery_entry(hdr, req->port,
 				p->subsys->subsysnqn, traddr,
-				NVME_NQN_NVME, numrec);
+				p->subsys->type, numrec);
 		numrec++;
 	}
 
-- 
2.29.2

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Christoph Hellwig]

On Mon, Mar 14, 2022 at 11:53:31AM +0100, Hannes Reinecke wrote:
> Expose the discovery subsystem in sysfs instead of having it as
> an internal structure. This requires the admin to manually
> enable the discovery subsystem by linking it to a port, but
> this can be seen as a feature as it will allows us to expose
> subsystems without discovery subsystems and discovery subsystems
> with no I/O subsystems.

... and completely breaks any existing setup.

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Sagi Grimberg]

>> Expose the discovery subsystem in sysfs instead of having it as
>> an internal structure. This requires the admin to manually
>> enable the discovery subsystem by linking it to a port, but
>> this can be seen as a feature as it will allows us to expose
>> subsystems without discovery subsystems and discovery subsystems
>> with no I/O subsystems.
> 
> ... and completely breaks any existing setup.

Yes, this needs to be backward compatible...

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Hannes Reinecke]

On 3/15/22 09:40, Sagi Grimberg wrote:
> 
>>> Expose the discovery subsystem in sysfs instead of having it as
>>> an internal structure. This requires the admin to manually
>>> enable the discovery subsystem by linking it to a port, but
>>> this can be seen as a feature as it will allows us to expose
>>> subsystems without discovery subsystems and discovery subsystems
>>> with no I/O subsystems.
>>
>> ... and completely breaks any existing setup.
> 
> Yes, this needs to be backward compatible...

The core question really is: do we _want_ to expose the discovery 
subsystem in configfs?
(And this was actually why I prefixed this series with 'RFC').

Unfortunately, exposing the discovery subsystem and trying to configure 
it with configfs does _not_ match with the way discovery is implemented 
today. While we currently only have a single discovery subsystem, it 
will only ever return the subsystems visible from this particular port.
(So really, it's more like a discovery subsystem per port.)
When using configfs we can link ports to the subsystem, but seeing that 
we'll only ever have _one_ discovery subsystem we cannot restrict the 
discovery log page contents to that specific port.

Hence this rather simple approach, having the 'normal' discovery 
subsystem exposed, and let the admin configure it accordingly.

But yes, this approach is not backward compatible, or, rather, backwards 
compatible only after it has been configured accordingly.

I can look at keeping the internal implementation, and only expose 
unique discovery controller (ie those with a unique subsystem NQN).
That would remove the need to having the 'discovery_nqn' attribute, and 
address Christophs concerns.

Hmm.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Christoph Hellwig]

On Tue, Mar 15, 2022 at 10:06:26AM +0100, Hannes Reinecke wrote:
> The core question really is: do we _want_ to expose the discovery subsystem 
> in configfs?

Well, if you want a freely configurable one we kinda have to, right?

> Unfortunately, exposing the discovery subsystem and trying to configure it 
> with configfs does _not_ match with the way discovery is implemented today. 
> While we currently only have a single discovery subsystem, it will only 
> ever return the subsystems visible from this particular port.

Well.  The original Fabrics spec had this concept of that magic discovery
NQN, which implies that there is one subsystem (or many pretending to be
one).  And that is what the implementation followed.  The varipus 80?? TPs
then made a complete mess off that.

> Hence this rather simple approach, having the 'normal' discovery subsystem 
> exposed, and let the admin configure it accordingly.
>
> I can look at keeping the internal implementation, and only expose unique 
> discovery controller (ie those with a unique subsystem NQN).
> That would remove the need to having the 'discovery_nqn' attribute, and 
> address Christophs concerns.

I suspect if we want to support all the new mess from the FMDS group
(and maybe we need to question the why a little more), then we should
so something like:

 (1) keep the existing global NQN-based discovery as-is.
 (2) maybe add a per-port known to allow disabling it if people really care
 (3) allow creating additional discovery subsystems with non-default
     NQNs that do not automatically get anything added to them and will
     just be configured as needed through configfs

But maybe first we should take a step back and figure out what supporting
TPAR8013 even buys us?

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Sagi Grimberg]

>> The core question really is: do we _want_ to expose the discovery subsystem
>> in configfs?
> 
> Well, if you want a freely configurable one we kinda have to, right?
> 
>> Unfortunately, exposing the discovery subsystem and trying to configure it
>> with configfs does _not_ match with the way discovery is implemented today.
>> While we currently only have a single discovery subsystem, it will only
>> ever return the subsystems visible from this particular port.
> 
> Well.  The original Fabrics spec had this concept of that magic discovery
> NQN, which implies that there is one subsystem (or many pretending to be
> one).  And that is what the implementation followed.  The varipus 80?? TPs
> then made a complete mess off that.
> 
>> Hence this rather simple approach, having the 'normal' discovery subsystem
>> exposed, and let the admin configure it accordingly.
>>
>> I can look at keeping the internal implementation, and only expose unique
>> discovery controller (ie those with a unique subsystem NQN).
>> That would remove the need to having the 'discovery_nqn' attribute, and
>> address Christophs concerns.
> 
> I suspect if we want to support all the new mess from the FMDS group
> (and maybe we need to question the why a little more)

IIRC there were different reasons that we probably shouldn't care about
that were related to the centralize discovery thing, but a side issue
was a problem with authenticating against a discovery subsystem that
have a universal name (not exactly sure about the details, because we do
also have the hostnqn in the psk identity). However assuming this is a
real issue, then we probably want to support it.

>, then we should so something like:
> 
>   (1) keep the existing global NQN-based discovery as-is.
>   (2) maybe add a per-port known to allow disabling it if people really care
>   (3) allow creating additional discovery subsystems with non-default
>       NQNs that do not automatically get anything added to them and will
>       just be configured as needed through configfs

I don't think we should do a discovery subsystem that tells you about
subsystems in other ports, that is most likely asking for connectivity
issues. Unless the log page is constructed based on explicit
configuration.

> But maybe first we should take a step back and figure out what supporting
> TPAR8013 even buys us?

Hannes may know better on the authentication aspect...

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Hannes Reinecke]

On 3/15/22 10:49, Christoph Hellwig wrote:
> On Tue, Mar 15, 2022 at 10:06:26AM +0100, Hannes Reinecke wrote:
>> The core question really is: do we _want_ to expose the discovery subsystem
>> in configfs?
> 
> Well, if you want a freely configurable one we kinda have to, right?
> 
>> Unfortunately, exposing the discovery subsystem and trying to configure it
>> with configfs does _not_ match with the way discovery is implemented today.
>> While we currently only have a single discovery subsystem, it will only
>> ever return the subsystems visible from this particular port.
> 
> Well.  The original Fabrics spec had this concept of that magic discovery
> NQN, which implies that there is one subsystem (or many pretending to be
> one).  And that is what the implementation followed.  The varipus 80?? TPs
> then made a complete mess off that.
> 
>> Hence this rather simple approach, having the 'normal' discovery subsystem
>> exposed, and let the admin configure it accordingly.
>>
>> I can look at keeping the internal implementation, and only expose unique
>> discovery controller (ie those with a unique subsystem NQN).
>> That would remove the need to having the 'discovery_nqn' attribute, and
>> address Christophs concerns.
> 
> I suspect if we want to support all the new mess from the FMDS group
> (and maybe we need to question the why a little more), then we should
> so something like:
> 
>   (1) keep the existing global NQN-based discovery as-is.
>   (2) maybe add a per-port known to allow disabling it if people really care
>   (3) allow creating additional discovery subsystems with non-default
>       NQNs that do not automatically get anything added to them and will
>       just be configured as needed through configfs
> 
Yeah, that's the line I'm following.

> But maybe first we should take a step back and figure out what supporting
> TPAR8013 even buys us?

Properly supporting persistent discovery controllers.

The current problem we're having (on linux) is that we cannot identify 
discovery controllers. Each discovery controller has the same subsystem 
NQN, and hence that's not sufficient to identify the subsystem
Which resulted in linux to always create a new 'nvme_subsystem' instance 
when creating a new nvme discover controller.

Problem now starts when you try to use persistent discovery controllers; 
it's quite easy to _create_ a persistent discovery controller, but less 
easy to _use_ an existing one; how would you know which one to pick?

More importantly, as linux will always create a new nvme_subsystem for 
each discovery controller you'll end up with several 'nvme_subsystem' 
instances which in fact all refer to the very same subsystem.

nvme-cli tries to work around that problem by requiring the user to 
specify the device name, but that requires quite some knowledge from the 
admin side, and making that work in a scripted fashion is a pain.

So with TPAR8013 we now _have_ distinct discovery subsystems, can create 
unique subsystem, won't have duplicate subsystems, and can automate 
nvme-cli to pick the correct existing persistent discovery controller 
even if the user does _not_ specify the device name.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Sagi Grimberg]

>>> The core question really is: do we _want_ to expose the discovery 
>>> subsystem
>>> in configfs?
>>
>> Well, if you want a freely configurable one we kinda have to, right?
>>
>>> Unfortunately, exposing the discovery subsystem and trying to 
>>> configure it
>>> with configfs does _not_ match with the way discovery is implemented 
>>> today.
>>> While we currently only have a single discovery subsystem, it will only
>>> ever return the subsystems visible from this particular port.
>>
>> Well.  The original Fabrics spec had this concept of that magic discovery
>> NQN, which implies that there is one subsystem (or many pretending to be
>> one).  And that is what the implementation followed.  The varipus 80?? 
>> TPs
>> then made a complete mess off that.
>>
>>> Hence this rather simple approach, having the 'normal' discovery 
>>> subsystem
>>> exposed, and let the admin configure it accordingly.
>>>
>>> I can look at keeping the internal implementation, and only expose 
>>> unique
>>> discovery controller (ie those with a unique subsystem NQN).
>>> That would remove the need to having the 'discovery_nqn' attribute, and
>>> address Christophs concerns.
>>
>> I suspect if we want to support all the new mess from the FMDS group
>> (and maybe we need to question the why a little more), then we should
>> so something like:
>>
>>   (1) keep the existing global NQN-based discovery as-is.
>>   (2) maybe add a per-port known to allow disabling it if people 
>> really care
>>   (3) allow creating additional discovery subsystems with non-default
>>       NQNs that do not automatically get anything added to them and will
>>       just be configured as needed through configfs
>>
> Yeah, that's the line I'm following.
> 
>> But maybe first we should take a step back and figure out what supporting
>> TPAR8013 even buys us?
> 
> Properly supporting persistent discovery controllers.
> 
> The current problem we're having (on linux) is that we cannot identify 
> discovery controllers. Each discovery controller has the same subsystem 
> NQN, and hence that's not sufficient to identify the subsystem
> Which resulted in linux to always create a new 'nvme_subsystem' instance 
> when creating a new nvme discover controller.

That is the case regardless of naming... there is no multipathing with
discovery subsystems so there will be a subsystem for every discovery
controller...

> Problem now starts when you try to use persistent discovery controllers; 
> it's quite easy to _create_ a persistent discovery controller, but less 
> easy to _use_ an existing one; how would you know which one to pick?

You are not supposed to "use" an existing one, the whole point of the
persistent discovery controllers is that you don't need to worry about
it. We already perfectly handle that with udev using the kernel of
the uevent.

> More importantly, as linux will always create a new nvme_subsystem for 
> each discovery controller you'll end up with several 'nvme_subsystem' 
> instances which in fact all refer to the very same subsystem.

OK, still don't understand why that is a problem.

> nvme-cli tries to work around that problem by requiring the user to 
> specify the device name, but that requires quite some knowledge from the 
> admin side, and making that work in a scripted fashion is a pain.

I am not aware at all on what scripting are you referring to. But udev
scripts are written already. Can you explain?

> So with TPAR8013 we now _have_ distinct discovery subsystems, can create 
> unique subsystem, won't have duplicate subsystems, and can automate 
> nvme-cli to pick the correct existing persistent discovery controller 
> even if the user does _not_ specify the device name.

And instead specify the subsysnqn? I am not aware of this reasoning you
are referring to at all, but I was probably absent from these
discussions... I thought that this TP was solely related to 8010 stuff
and also solves an issue that came up with the discovery subsystem and
the authentication TPs.

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Hannes Reinecke]

On 3/15/22 11:49, Sagi Grimberg wrote:
> 
>>>> The core question really is: do we _want_ to expose the discovery 
>>>> subsystem
>>>> in configfs?
>>>
>>> Well, if you want a freely configurable one we kinda have to, right?
>>>
>>>> Unfortunately, exposing the discovery subsystem and trying to 
>>>> configure it
>>>> with configfs does _not_ match with the way discovery is implemented 
>>>> today.
>>>> While we currently only have a single discovery subsystem, it will only
>>>> ever return the subsystems visible from this particular port.
>>>
>>> Well.  The original Fabrics spec had this concept of that magic 
>>> discovery
>>> NQN, which implies that there is one subsystem (or many pretending to be
>>> one).  And that is what the implementation followed.  The varipus 
>>> 80?? TPs
>>> then made a complete mess off that.
>>>
>>>> Hence this rather simple approach, having the 'normal' discovery 
>>>> subsystem
>>>> exposed, and let the admin configure it accordingly.
>>>>
>>>> I can look at keeping the internal implementation, and only expose 
>>>> unique
>>>> discovery controller (ie those with a unique subsystem NQN).
>>>> That would remove the need to having the 'discovery_nqn' attribute, and
>>>> address Christophs concerns.
>>>
>>> I suspect if we want to support all the new mess from the FMDS group
>>> (and maybe we need to question the why a little more), then we should
>>> so something like:
>>>
>>>   (1) keep the existing global NQN-based discovery as-is.
>>>   (2) maybe add a per-port known to allow disabling it if people 
>>> really care
>>>   (3) allow creating additional discovery subsystems with non-default
>>>       NQNs that do not automatically get anything added to them and will
>>>       just be configured as needed through configfs
>>>
>> Yeah, that's the line I'm following.
>>
>>> But maybe first we should take a step back and figure out what 
>>> supporting
>>> TPAR8013 even buys us?
>>
>> Properly supporting persistent discovery controllers.
>>
>> The current problem we're having (on linux) is that we cannot identify 
>> discovery controllers. Each discovery controller has the same 
>> subsystem NQN, and hence that's not sufficient to identify the subsystem
>> Which resulted in linux to always create a new 'nvme_subsystem' 
>> instance when creating a new nvme discover controller.
> 
> That is the case regardless of naming... there is no multipathing with
> discovery subsystems so there will be a subsystem for every discovery
> controller...
>
Well, technically, no, but in practice there is, as one can easily 
specify more than one discovery port (to the same discovery subsystem).
And quite some target implementations do.
Discussion is probably moot anyway as multipathing would only affect 
I/O, which won't be done anyway.

>> Problem now starts when you try to use persistent discovery 
>> controllers; it's quite easy to _create_ a persistent discovery 
>> controller, but less easy to _use_ an existing one; how would you know 
>> which one to pick?
> 
> You are not supposed to "use" an existing one, the whole point of the
> persistent discovery controllers is that you don't need to worry about
> it. We already perfectly handle that with udev using the kernel of
> the uevent.
> 

And my point is that you _do_. Yes, udev does work, but only in so far 
AENs are involved. When you want to retrieve the discovery log manually 
_and_ have existing persistent discovery controllers you'll need to know 
which device to pick.

Which is getting especially interesting if you have several different 
subsystems to contend with.

>> More importantly, as linux will always create a new nvme_subsystem for 
>> each discovery controller you'll end up with several 'nvme_subsystem' 
>> instances which in fact all refer to the very same subsystem.
> 
> OK, still don't understand why that is a problem.
> 
>> nvme-cli tries to work around that problem by requiring the user to 
>> specify the device name, but that requires quite some knowledge from 
>> the admin side, and making that work in a scripted fashion is a pain.
> 
> I am not aware at all on what scripting are you referring to. But udev
> scripts are written already. Can you explain?
> 
>> So with TPAR8013 we now _have_ distinct discovery subsystems, can 
>> create unique subsystem, won't have duplicate subsystems, and can 
>> automate nvme-cli to pick the correct existing persistent discovery 
>> controller even if the user does _not_ specify the device name.
> 
> And instead specify the subsysnqn? I am not aware of this reasoning you
> are referring to at all, but I was probably absent from these
> discussions... I thought that this TP was solely related to 8010 stuff
> and also solves an issue that came up with the discovery subsystem and
> the authentication TPs.

True. authentication requires a unique NQN, and hence can't be used 
without TP8013.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Sagi Grimberg]

>>> The current problem we're having (on linux) is that we cannot 
>>> identify discovery controllers. Each discovery controller has the 
>>> same subsystem NQN, and hence that's not sufficient to identify the 
>>> subsystem
>>> Which resulted in linux to always create a new 'nvme_subsystem' 
>>> instance when creating a new nvme discover controller.
>>
>> That is the case regardless of naming... there is no multipathing with
>> discovery subsystems so there will be a subsystem for every discovery
>> controller...
>>
> Well, technically, no, but in practice there is, as one can easily 
> specify more than one discovery port (to the same discovery subsystem).
> And quite some target implementations do.
> Discussion is probably moot anyway as multipathing would only affect 
> I/O, which won't be done anyway.

It is a moot discussion.

>>> Problem now starts when you try to use persistent discovery 
>>> controllers; it's quite easy to _create_ a persistent discovery 
>>> controller, but less easy to _use_ an existing one; how would you 
>>> know which one to pick?
>>
>> You are not supposed to "use" an existing one, the whole point of the
>> persistent discovery controllers is that you don't need to worry about
>> it. We already perfectly handle that with udev using the kernel of
>> the uevent.
>>
> 
> And my point is that you _do_. Yes, udev does work, but only in so far 
> AENs are involved.

Why do you need persistent discovery controllers if you don't want the
AEN?

> When you want to retrieve the discovery log manually 
> _and_ have existing persistent discovery controllers you'll need to know 
> which device to pick.

Please describe the use-case because I still don't understand.
nvme-cli offers 'discover' that gives you the log page and 'connect-all'
that connects to the subsystems it found in the log page.

Do you have foreign udev rules in addition to the ones supplied in
nvme-cli? I am missing the problem statement that you are alluding to.

> Which is getting especially interesting if you have several different 
> subsystems to contend with.

I still don't understand what you are describing.

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Hannes Reinecke]

On 3/15/22 12:15, Sagi Grimberg wrote:
> 
[ .. ]
>> When you want to retrieve the discovery log manually _and_ have 
>> existing persistent discovery controllers you'll need to know which 
>> device to pick.
> 
> Please describe the use-case because I still don't understand.
> nvme-cli offers 'discover' that gives you the log page and 'connect-all'
> that connects to the subsystems it found in the log page.
> 
> Do you have foreign udev rules in addition to the ones supplied in
> nvme-cli? I am missing the problem statement that you are alluding to.
> 

No, not foreign rules; it's the manual call to 'nvme discover' which I 
try to simplify.

But you are right; unique discovery controllers are only important for 
authentication. Everything else is just to make the system 'nicer'.

So leave it for now and I'll come back with a different set of patches, 
as I _still_ see a benefit in being able to configure the discovery 
response.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Sagi Grimberg]

>>> When you want to retrieve the discovery log manually _and_ have 
>>> existing persistent discovery controllers you'll need to know which 
>>> device to pick.
>>
>> Please describe the use-case because I still don't understand.
>> nvme-cli offers 'discover' that gives you the log page and 'connect-all'
>> that connects to the subsystems it found in the log page.
>>
>> Do you have foreign udev rules in addition to the ones supplied in
>> nvme-cli? I am missing the problem statement that you are alluding to.
>>
> 
> No, not foreign rules; it's the manual call to 'nvme discover' which I 
> try to simplify.
> 
> But you are right; unique discovery controllers are only important for 
> authentication. Everything else is just to make the system 'nicer'.

OK, thanks.

> So leave it for now and I'll come back with a different set of patches, 
> as I _still_ see a benefit in being able to configure the discovery 
> response.

Sure

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[John Meneghini]

Sorry I'm late to the party.  Please see my comments below.

On 3/15/22 05:49, Christoph Hellwig wrote:
> On Tue, Mar 15, 2022 at 10:06:26AM +0100, Hannes Reinecke wrote:
>> The core question really is: do we _want_ to expose the discovery subsystem
>> in configfs?
> 
> Well, if you want a freely configurable one we kinda have to, right?
> 
>> Unfortunately, exposing the discovery subsystem and trying to configure it
>> with configfs does _not_ match with the way discovery is implemented today.
>> While we currently only have a single discovery subsystem, it will only
>> ever return the subsystems visible from this particular port.

I don't see why this would need to change. What is it that you want to configure
in the new unique discovery subsystem(s) that would be any different from the
existing well known discovery subsystem?

> Well.  The original Fabrics spec had this concept of that magic discovery
> NQN, which implies that there is one subsystem (or many pretending to be
> one).  And that is what the implementation followed.  The varipus 80?? TPs
> then made a complete mess off that.

I agree that FMDS has made an overly complicated mess of NVMe-oF Discovery
with the new Discovery TPs. However, I am hoping that TP-8013 and TP-8014
could be used to help with some of the problem.

>> Hence this rather simple approach, having the 'normal' discovery subsystem
>> exposed, and let the admin configure it accordingly.
>>
>> I can look at keeping the internal implementation, and only expose unique
>> discovery controller (ie those with a unique subsystem NQN).
>> That would remove the need to having the 'discovery_nqn' attribute, and
>> address Christophs concerns.
> 
> I suspect if we want to support all the new mess from the FMDS group
> (and maybe we need to question the why a little more), then we should
> so something like:
> 
>   (1) keep the existing global NQN-based discovery as-is.

I agree that we need some way to support legacy hosts and legacy controllers
in the same fabric. What ever we do with TP-8010, etc., we need to be sure that
all hosts and all discovery controllers interoperate cleanly.

>   (2) maybe add a per-port known to allow disabling it if people really care
>   (3) allow creating additional discovery subsystems with non-default
>       NQNs that do not automatically get anything added to them and will
>       just be configured as needed through configfs
> 
> But maybe first we should take a step back and figure out what supporting
> TPAR8013 even buys us?

TP-8013 was designed to work with TP-8014. In fact, at one point we talked about
combining these two TPs into a single TP. The idea behind TP-8013 & 8014 was:

  1. All hosts will connect to the existing Discovery Service with the Well Known
     Discovery NQN and retrieve the Discovery Log pages for the HostNQN provided
     in the Fabric Connect command, as it is done today.

     It was assumed that Authenticating with the Well Known Discovery NQN would
     would not be needed or supported because:
     a) The Discovery Controller controls the Authenticate work flow and
        returning AUTHREQ=1 in the connect response would break legacy hosts.
     b) It doesn't make sense to have a Well Known Discovery NQN as a part of a psk.

  2. Discovery Controllers which support Authentication can return Discovery
     Log Page Entries with Subsystem Type (SUBTYPE): 03h - as defined by TP-8014.
     These DLPEs will contain Unique Discovery NQNs - as defined by TP-8013

  3. Hosts that support Authentication can then disconnect from the Well Known
     Discovery Controller and re-connect with the Unique Discovery NQN. These
     hosts should expect an AUTHREQ=1 response.

  4. Hosts that don't want to support Authentication can ignore the SUBTYPE 03h
     Log Page Entries and operate normally. This would include legacy hosts.

Hopefully, with some kind of a design like this, both legacy (non-authenticating)
and new (authenticating) hosts and discovery controllers can interoperate.

/John

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Christoph Hellwig]

On Wed, Mar 23, 2022 at 01:17:32PM -0400, John Meneghini wrote:
>  1. All hosts will connect to the existing Discovery Service with the Well Known
>     Discovery NQN and retrieve the Discovery Log pages for the HostNQN provided
>     in the Fabric Connect command, as it is done today.
>
>     It was assumed that Authenticating with the Well Known Discovery NQN would
>     would not be needed or supported because:
>     a) The Discovery Controller controls the Authenticate work flow and
>        returning AUTHREQ=1 in the connect response would break legacy hosts.
>     b) It doesn't make sense to have a Well Known Discovery NQN as a part of a psk.
>
>  2. Discovery Controllers which support Authentication can return Discovery
>     Log Page Entries with Subsystem Type (SUBTYPE): 03h - as defined by TP-8014.
>     These DLPEs will contain Unique Discovery NQNs - as defined by TP-8013
>
>  3. Hosts that support Authentication can then disconnect from the Well Known
>     Discovery Controller and re-connect with the Unique Discovery NQN. These
>     hosts should expect an AUTHREQ=1 response.
>
>  4. Hosts that don't want to support Authentication can ignore the SUBTYPE 03h
>     Log Page Entries and operate normally. This would include legacy hosts.
>
> Hopefully, with some kind of a design like this, both legacy (non-authenticating)
> and new (authenticating) hosts and discovery controllers can interoperate.

This makes much more sense.  But why would a host that is part of a PSK
setup even use the well known discovery controller at all?  Whatever
mechanism is used to share the key should also distribute the actual
discovery controller to be used and avoid that entire step.

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[John Meneghini]

On 3/23/22 13:23, Christoph Hellwig wrote:
> On Wed, Mar 23, 2022 at 01:17:32PM -0400, John Meneghini wrote:
>>
>> Hopefully, with some kind of a design like this, both legacy (non-authenticating)
>> and new (authenticating) hosts and discovery controllers can interoperate.
> 
> This makes much more sense.  But why would a host that is part of a PSK
> setup even use the well known discovery controller at all? Whatever > mechanism is used to share the key should also distribute the actual
> discovery controller to be used and avoid that entire step.

I agree. But that's not what FMDS is doing. They are basically trying to re-invent
the wheel by adding all kinds of arguably redundant mechanisms to the spec. I think
this is what happens when you let a bunch of storage engineers design a network. Just
read TP-8010 and TP-8009 to see what I mean.

I proposed TP-8014 because I saw the mess coming and I wanted some simple way to make
things work... at least with authentication.

/John

RE: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Knight, Frederick]

Please don't make this assumption:

  3. Hosts that support Authentication can then disconnect from the Well Known
     Discovery Controller and re-connect with the Unique Discovery NQN. These
     hosts should expect an AUTHREQ=1 response.

  4. Hosts that don't want to support Authentication can ignore the SUBTYPE 03h
     Log Page Entries and operate normally. This would include legacy hosts.

There should be NO assumption that subtype 03h requires authentication.  It should use AUTHREQ only.

And, while using authentication with the well-known discovery controller NQN is not recommended (and not very secure), there is nothing that prevents the setting of AUTHREQ=1 when using the well-known discovery controller NQN; so again, I would suggest you do not just assume that is true, and that the host still use AUTHREQ to control authentication no matter what/who you are talking to.  As for targets - they are free to implement what their customers require.

	Fred


> -----Original Message-----
> From: John Meneghini <jmeneghi@redhat.com>
> Sent: Wednesday, March 23, 2022 1:18 PM
> To: Christoph Hellwig <hch@lst.de>; Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>; Keith Busch
> <keith.busch@wdc.com>; linux-nvme@lists.infradead.org; Knight, Frederick
> <Frederick.Knight@netapp.com>; Chris Leech <cleech@redhat.com>
> Subject: Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs
> 
> NetApp Security WARNING: This is an external email. Do not click links or
> open attachments unless you recognize the sender and know the content is
> safe.
> 
> 
> 
> 
> Sorry I'm late to the party.  Please see my comments below.
> 
> On 3/15/22 05:49, Christoph Hellwig wrote:
> > On Tue, Mar 15, 2022 at 10:06:26AM +0100, Hannes Reinecke wrote:
> >> The core question really is: do we _want_ to expose the discovery
> >> subsystem in configfs?
> >
> > Well, if you want a freely configurable one we kinda have to, right?
> >
> >> Unfortunately, exposing the discovery subsystem and trying to
> >> configure it with configfs does _not_ match with the way discovery is
> implemented today.
> >> While we currently only have a single discovery subsystem, it will
> >> only ever return the subsystems visible from this particular port.
> 
> I don't see why this would need to change. What is it that you want to
> configure in the new unique discovery subsystem(s) that would be any
> different from the existing well known discovery subsystem?
> 
> > Well.  The original Fabrics spec had this concept of that magic
> > discovery NQN, which implies that there is one subsystem (or many
> > pretending to be one).  And that is what the implementation followed.
> > The varipus 80?? TPs then made a complete mess off that.
> 
> I agree that FMDS has made an overly complicated mess of NVMe-oF
> Discovery with the new Discovery TPs. However, I am hoping that TP-8013
> and TP-8014 could be used to help with some of the problem.
> 
> >> Hence this rather simple approach, having the 'normal' discovery
> >> subsystem exposed, and let the admin configure it accordingly.
> >>
> >> I can look at keeping the internal implementation, and only expose
> >> unique discovery controller (ie those with a unique subsystem NQN).
> >> That would remove the need to having the 'discovery_nqn' attribute,
> >> and address Christophs concerns.
> >
> > I suspect if we want to support all the new mess from the FMDS group
> > (and maybe we need to question the why a little more), then we should
> > so something like:
> >
> >   (1) keep the existing global NQN-based discovery as-is.
> 
> I agree that we need some way to support legacy hosts and legacy
> controllers in the same fabric. What ever we do with TP-8010, etc., we need
> to be sure that all hosts and all discovery controllers interoperate cleanly.
> 
> >   (2) maybe add a per-port known to allow disabling it if people really care
> >   (3) allow creating additional discovery subsystems with non-default
> >       NQNs that do not automatically get anything added to them and will
> >       just be configured as needed through configfs
> >
> > But maybe first we should take a step back and figure out what
> > supporting
> > TPAR8013 even buys us?
> 
> TP-8013 was designed to work with TP-8014. In fact, at one point we talked
> about combining these two TPs into a single TP. The idea behind TP-8013 &
> 8014 was:
> 
>   1. All hosts will connect to the existing Discovery Service with the Well
> Known
>      Discovery NQN and retrieve the Discovery Log pages for the HostNQN
> provided
>      in the Fabric Connect command, as it is done today.
> 
>      It was assumed that Authenticating with the Well Known Discovery NQN
> would
>      would not be needed or supported because:
>      a) The Discovery Controller controls the Authenticate work flow and
>         returning AUTHREQ=1 in the connect response would break legacy
> hosts.
>      b) It doesn't make sense to have a Well Known Discovery NQN as a part of
> a psk.
> 
>   2. Discovery Controllers which support Authentication can return Discovery
>      Log Page Entries with Subsystem Type (SUBTYPE): 03h - as defined by TP-
> 8014.
>      These DLPEs will contain Unique Discovery NQNs - as defined by TP-8013
> 
>   3. Hosts that support Authentication can then disconnect from the Well
> Known
>      Discovery Controller and re-connect with the Unique Discovery NQN.
> These
>      hosts should expect an AUTHREQ=1 response.
> 
>   4. Hosts that don't want to support Authentication can ignore the SUBTYPE
> 03h
>      Log Page Entries and operate normally. This would include legacy hosts.
> 
> Hopefully, with some kind of a design like this, both legacy (non-
> authenticating) and new (authenticating) hosts and discovery controllers can
> interoperate.
> 
> /John

Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[John Meneghini]

On 3/23/22 13:34, Knight, Frederick wrote:
> Please don't make this assumption:
> 
>    3. Hosts that support Authentication can then disconnect from the Well Known
>       Discovery Controller and re-connect with the Unique Discovery NQN. These
>       hosts should expect an AUTHREQ=1 response.
> 
>    4. Hosts that don't want to support Authentication can ignore the SUBTYPE 03h
>       Log Page Entries and operate normally. This would include legacy hosts.
> 
> There should be NO assumption that subtype 03h requires authentication.  It should use AUTHREQ only.

Well, then maybe we need to figure that out.  I agree that any wire protocol that depends on an assumption is
a broken wire protocol.

> And, while using authentication with the well-known discovery controller NQN is not recommended (and not very secure), 
> there is nothing that prevents the setting of AUTHREQ=1 when using the well-known discovery controller NQN; 

This is true. According to the protocol AUTHREQ=1 can be returned to any Fabric Connect command. But if Discovery Controllers 
start doing this, they will probably break 99% of the running hosts out here.  So that's whats preventing it from happening.

> so again, I would suggest you do not just assume that is true, and that the host still use AUTHREQ to control authentication
> no matter what/who you are talking to.  As for targets - they are free to implement what their customers require.

I don't disagree. I'm just saying, the realities of turning on AUTHREQ=1 with the exiting well known discovery service is:
you will have many host that suddenly fail to interoperate.

/John

RE: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs

[Knight, Frederick]

I agree that a reasonably configured environment will distribute keys and unique NQNs at the same time (as a pair).  A configuration that tries to use a well-known NQN and authentication isn't very secure in the first place, so there isn't much point.  And a well-known discovery controller that REQUIRES authentication, well, its getting what it asked for.

	Fred

> -----Original Message-----
> From: John Meneghini <jmeneghi@redhat.com>
> Sent: Wednesday, March 23, 2022 2:04 PM
> To: Knight, Frederick <Frederick.Knight@netapp.com>; Christoph Hellwig
> <hch@lst.de>; Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>; Keith Busch
> <keith.busch@wdc.com>; linux-nvme@lists.infradead.org; Chris Leech
> <cleech@redhat.com>
> Subject: Re: [PATCH 1/3] nvmet: expose discovery subsystem in sysfs
> 
> NetApp Security WARNING: This is an external email. Do not click links or
> open attachments unless you recognize the sender and know the content is
> safe.
> 
> 
> 
> 
> On 3/23/22 13:34, Knight, Frederick wrote:
> > Please don't make this assumption:
> >
> >    3. Hosts that support Authentication can then disconnect from the Well
> Known
> >       Discovery Controller and re-connect with the Unique Discovery NQN.
> These
> >       hosts should expect an AUTHREQ=1 response.
> >
> >    4. Hosts that don't want to support Authentication can ignore the
> SUBTYPE 03h
> >       Log Page Entries and operate normally. This would include legacy hosts.
> >
> > There should be NO assumption that subtype 03h requires authentication.
> It should use AUTHREQ only.
> 
> Well, then maybe we need to figure that out.  I agree that any wire protocol
> that depends on an assumption is a broken wire protocol.
> 
> > And, while using authentication with the well-known discovery
> > controller NQN is not recommended (and not very secure), there is
> > nothing that prevents the setting of AUTHREQ=1 when using the
> > well-known discovery controller NQN;
> 
> This is true. According to the protocol AUTHREQ=1 can be returned to any
> Fabric Connect command. But if Discovery Controllers start doing this, they
> will probably break 99% of the running hosts out here.  So that's whats
> preventing it from happening.
> 
> > so again, I would suggest you do not just assume that is true, and
> > that the host still use AUTHREQ to control authentication no matter
> what/who you are talking to.  As for targets - they are free to implement
> what their customers require.
> 
> I don't disagree. I'm just saying, the realities of turning on AUTHREQ=1 with
> the exiting well known discovery service is:
> you will have many host that suddenly fail to interoperate.
> 
> /John

[PATCH 2/3] nvmet: restrict setting of discovery_nqn to discovery subsystem

[Hannes Reinecke]

Changing the discovery NQN only makes sense for the discovery subsystem,
as any other subsystem can only have one (fixed) subsystem NQN.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/configfs.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 9b94af2444b2..58124abdbf62 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1310,6 +1310,12 @@ CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 static ssize_t nvmet_subsys_attr_discovery_nqn_show(struct config_item *item,
 			char *page)
 {
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	/* Only the discovery subsystem can have a different subsystem NQN */
+	if (subsys != nvmet_disc_subsys)
+		return -EBUSY;
+
 	return snprintf(page, PAGE_SIZE, "%s\n",
 			nvmet_disc_subsys->subsysnqn);
 }
@@ -1318,9 +1324,17 @@ static ssize_t nvmet_subsys_attr_discovery_nqn_store(struct config_item *item,
 			const char *page, size_t count)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
+	struct nvmet_port *port;
+	struct nvmet_subsys_link *p;
 	char *subsysnqn;
 	int len;
 
+	/*
+	 * Can only change the discovery NQN for the discovery subsystem
+	 */
+	if (subsys != nvmet_disc_subsys)
+		return -EBUSY;
+
 	len = strcspn(page, "\n");
 	if (!len)
 		return -EINVAL;
@@ -1330,13 +1344,21 @@ static ssize_t nvmet_subsys_attr_discovery_nqn_store(struct config_item *item,
 		return -ENOMEM;
 
 	/*
-	 * The discovery NQN must be different from subsystem NQN.
+	 * The discovery NQN must be different from other subsystem NQNs.
+	 *
+	 * This is a tad convoluted, but avoids having to do a
+	 * listdir() from the kernel.
 	 */
-	if (!strcmp(subsysnqn, subsys->subsysnqn)) {
-		kfree(subsysnqn);
-		return -EBUSY;
-	}
 	down_write(&nvmet_config_sem);
+	list_for_each_entry(port, &nvmet_ports_list, global_entry) {
+		list_for_each_entry(p, &port->subsystems, entry) {
+			if (!strcmp(subsysnqn, p->subsys->subsysnqn)) {
+				up_write(&nvmet_config_sem);
+				kfree(subsysnqn);
+				return -EBUSY;
+			}
+		}
+	}
 	kfree(nvmet_disc_subsys->subsysnqn);
 	nvmet_disc_subsys->subsysnqn = subsysnqn;
 	up_write(&nvmet_config_sem);
-- 
2.29.2

Re: [PATCH 2/3] nvmet: restrict setting of discovery_nqn to discovery subsystem

[Christoph Hellwig]

On Mon, Mar 14, 2022 at 11:53:32AM +0100, Hannes Reinecke wrote:
> Changing the discovery NQN only makes sense for the discovery subsystem,
> as any other subsystem can only have one (fixed) subsystem NQN.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/nvme/target/configfs.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 9b94af2444b2..58124abdbf62 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1310,6 +1310,12 @@ CONFIGFS_ATTR(nvmet_subsys_, attr_model);
>  static ssize_t nvmet_subsys_attr_discovery_nqn_show(struct config_item *item,
>  			char *page)
>  {
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	/* Only the discovery subsystem can have a different subsystem NQN */
> +	if (subsys != nvmet_disc_subsys)
> +		return -EBUSY;
> +
>  	return snprintf(page, PAGE_SIZE, "%s\n",
>  			nvmet_disc_subsys->subsysnqn);

This completely breaks the existing ABI.  This attribute is shown on
the "normal subsystems" and in fact only exposed for those so far.

It is a fairly recent addition by you and without nvmetcli support, so
maybe we can revert this entire attribute to redo discovery controller
handling, but as-is this patch seems rather broken.

Re: [PATCH 2/3] nvmet: restrict setting of discovery_nqn to discovery subsystem

[Sagi Grimberg]

>> Changing the discovery NQN only makes sense for the discovery subsystem,
>> as any other subsystem can only have one (fixed) subsystem NQN.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/target/configfs.c | 32 +++++++++++++++++++++++++++-----
>>   1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>> index 9b94af2444b2..58124abdbf62 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -1310,6 +1310,12 @@ CONFIGFS_ATTR(nvmet_subsys_, attr_model);
>>   static ssize_t nvmet_subsys_attr_discovery_nqn_show(struct config_item *item,
>>   			char *page)
>>   {
>> +	struct nvmet_subsys *subsys = to_subsys(item);
>> +
>> +	/* Only the discovery subsystem can have a different subsystem NQN */
>> +	if (subsys != nvmet_disc_subsys)
>> +		return -EBUSY;
>> +
>>   	return snprintf(page, PAGE_SIZE, "%s\n",
>>   			nvmet_disc_subsys->subsysnqn);
> 
> This completely breaks the existing ABI.  This attribute is shown on
> the "normal subsystems" and in fact only exposed for those so far.

I don't think we even want this to be on a per-subsystem at all.

> It is a fairly recent addition by you and without nvmetcli support, so
> maybe we can revert this entire attribute to redo discovery controller
> handling, but as-is this patch seems rather broken.

I think we could, I doubt anyone uses this interface at this point...

Re: [PATCH 2/3] nvmet: restrict setting of discovery_nqn to discovery subsystem

[Christoph Hellwig]

On Tue, Mar 15, 2022 at 10:57:35AM +0200, Sagi Grimberg wrote:
>> It is a fairly recent addition by you and without nvmetcli support, so
>> maybe we can revert this entire attribute to redo discovery controller
>> handling, but as-is this patch seems rather broken.
>
> I think we could, I doubt anyone uses this interface at this point...

Then please send a revert for "nvmet: make discovery NQN configurable"
against the 5.17 tree ASAP.  That way it will only be in 5.16.

Re: [PATCH 2/3] nvmet: restrict setting of discovery_nqn to discovery subsystem

[John Meneghini]

On 3/15/22 05:00, Christoph Hellwig wrote:
> On Tue, Mar 15, 2022 at 10:57:35AM +0200, Sagi Grimberg wrote:
>>> It is a fairly recent addition by you and without nvmetcli support, so
>>> maybe we can revert this entire attribute to redo discovery controller
>>> handling, but as-is this patch seems rather broken.
>>
>> I think we could, I doubt anyone uses this interface at this point...
> 
> Then please send a revert for "nvmet: make discovery NQN configurable"
> against the 5.17 tree ASAP.  That way it will only be in 5.16.

Has this patch been reverted yet?

I don't see the revert.

jmeneghi:centos-stream-9(nvme-5.18) > git logl -200 --grep="nvmet: make discovery NQN configurable"
626851e9225d 2021-10-20 1632292519  nvmet: make discovery NQN configurable [ Christoph Hellwig / hare@suse.de ]
jmeneghi:centos-stream-9(nvme-5.18) > git branch --contains 626851e9225d
   5.17/scsi-queue
   5.17/scsi-staging
   master
   nvme-5.17
* nvme-5.18

Re: [PATCH 2/3] nvmet: restrict setting of discovery_nqn to discovery subsystem

[John Meneghini]

Never mind.  I see it has been reverted.

jmeneghi:centos-stream-9(nvme-5.18) > git showc 0c48645a7f39
0c48645a7f39 2022-03-15T10:39:26+01:00 1647335676  (tag: nvme-5.17-2022-03-16, nvme/nvme-5.17, nvme-5.17) nvmet: revert "nvmet: 
make discovery NQN configurable" [ Christoph Hellwig / hare@suse.de ]


On 3/23/22 20:52, John Meneghini wrote:
> On 3/15/22 05:00, Christoph Hellwig wrote:
>> On Tue, Mar 15, 2022 at 10:57:35AM +0200, Sagi Grimberg wrote:
>>>> It is a fairly recent addition by you and without nvmetcli support, so
>>>> maybe we can revert this entire attribute to redo discovery controller
>>>> handling, but as-is this patch seems rather broken.
>>>
>>> I think we could, I doubt anyone uses this interface at this point...
>>
>> Then please send a revert for "nvmet: make discovery NQN configurable"
>> against the 5.17 tree ASAP.  That way it will only be in 5.16.
> 
> Has this patch been reverted yet?
> 
> I don't see the revert.
> 
> jmeneghi:centos-stream-9(nvme-5.18) > git logl -200 --grep="nvmet: make discovery NQN configurable"
> 626851e9225d 2021-10-20 1632292519  nvmet: make discovery NQN configurable [ Christoph Hellwig / hare@suse.de ]
> jmeneghi:centos-stream-9(nvme-5.18) > git branch --contains 626851e9225d
>    5.17/scsi-queue
>    5.17/scsi-staging
>    master
>    nvme-5.17
> * nvme-5.18
> 
> 
> 
> 
> 

[PATCH 3/3] nvmet: do not allow to create a subsystem with the discovery NQN

[Hannes Reinecke]

configfs will only protect us against duplication of subsystem NQNs,
but discovery subsystems might have two NQNs, the standard discovery NQN
(which will be used as the configfs directory name), and the additional
unique subsystem NQN.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/configfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 58124abdbf62..617ab027ae2f 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1434,6 +1434,9 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (sysfs_streq(name, nvmet_disc_subsys->subsysnqn))
+		return ERR_PTR(-EBUSY);
+
 	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
 	if (IS_ERR(subsys))
 		return ERR_CAST(subsys);
-- 
2.29.2

Re: [PATCH 3/3] nvmet: do not allow to create a subsystem with the discovery NQN

[Christoph Hellwig]

On Mon, Mar 14, 2022 at 11:53:33AM +0100, Hannes Reinecke wrote:
> configfs will only protect us against duplication of subsystem NQNs,
> but discovery subsystems might have two NQNs, the standard discovery NQN
> (which will be used as the configfs directory name), and the additional
> unique subsystem NQN.

This is something that we should probably pick up as fix ASAP.  Can
you add a proper fixes tag and submit it stand-alone?

Re: [PATCH 3/3] nvmet: do not allow to create a subsystem with the discovery NQN

[Hannes Reinecke]

On 3/15/22 09:10, Christoph Hellwig wrote:
> On Mon, Mar 14, 2022 at 11:53:33AM +0100, Hannes Reinecke wrote:
>> configfs will only protect us against duplication of subsystem NQNs,
>> but discovery subsystems might have two NQNs, the standard discovery NQN
>> (which will be used as the configfs directory name), and the additional
>> unique subsystem NQN.
> 
> This is something that we should probably pick up as fix ASAP.  Can
> you add a proper fixes tag and submit it stand-alone?

Will need to rework the patch, as in its current form it relies on the 
discovery subsystem to be exposed in configfs.
But yes, can do.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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