[PATCH] nvme-cli: nvmf-connect@.service: Remove matching from default syntax

[PATCH] nvme-cli: nvmf-connect@.service: Remove matching from default syntax

[James Smart]

commit 53aab69a0add added the "--matching" argument to the systemd
connect script that issues connect-all to a discovery controller. When
this argument is used, only discovery log entries whose target port
traddr's match the traddr of the discovery controller will be connected
to.  This eliminates the ability to do referrals by the discovery
controller.

Revert the commit so that the "--matching" argument is not default
behavior.

Signed-off-by: James Smart <jsmart2021@gmail.com>
CC: Martin Wilck <mwilck@suse.com>
---
 nvmf-autoconnect/systemd/nvmf-connect@.service | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nvmf-autoconnect/systemd/nvmf-connect@.service b/nvmf-autoconnect/systemd/nvmf-connect@.service
index 5fbf081..c60f146 100644
--- a/nvmf-autoconnect/systemd/nvmf-connect@.service
+++ b/nvmf-autoconnect/systemd/nvmf-connect@.service
@@ -11,4 +11,4 @@ Requires=nvmf-connect.target
 [Service]
 Type=simple
 Environment="CONNECT_ARGS=%i"
-ExecStart=/bin/sh -c "nvme connect-all --matching --quiet `/bin/echo -e '${CONNECT_ARGS}'`"
+ExecStart=/bin/sh -c "nvme connect-all --quiet `/bin/echo -e '${CONNECT_ARGS}'`"
-- 
2.26.2

Re: [PATCH] nvme-cli: nvmf-connect@.service: Remove matching from default syntax

[Martin Wilck]

On Fri, 2021-12-17 at 11:32 -0800, James Smart wrote:
> commit 53aab69a0add added the "--matching" argument to the systemd
> connect script that issues connect-all to a discovery controller.
> When
> this argument is used, only discovery log entries whose target port
> traddr's match the traddr of the discovery controller will be
> connected
> to.  This eliminates the ability to do referrals by the discovery
> controller.
> 
> Revert the commit so that the "--matching" argument is not default
> behavior.

Hm, I guess you have to do revert this if it breaks referrals.

The intention of my patch was not to suppress referrals. Unless I'm
mistaken, a referral is a log page entry that lists another discovery
controller, on which then another discovery is carried out. The "--
matching" parameter was intended to ignore log page entries listing
ordinary (non-discovery) subsystems on if their traddr didn't match the
discovery controller's traddr.

I still think connecting to those entries is wrong more often than not.
In the environment I was testing in back then, IIRC there were 4
discovery controllers, each listing every subsystem on every
controller. The host would try to connect to every subsystem 4 times,
resulting in error messages 3 out of 4 times. With --matching, these
errors could be avoided.

I believe the correct solution would be to fix the way referrals are
handled with --matching. If a discovery subsystem D1 refers to another
discovery subsystem D2, then on D2 those entries that match the traddr
of D2 should be considered "matching", and so on with additional
referrals. Obviously, "--matching" would need to be ignored for the
referral entries themselves.

But as long as we haven't fixed this logic, I'm fine with your patch.

Martin

> 
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> CC: Martin Wilck <mwilck@suse.com>
> ---
>  nvmf-autoconnect/systemd/nvmf-connect@.service | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nvmf-autoconnect/systemd/nvmf-connect@.service b/nvmf-
> autoconnect/systemd/nvmf-connect@.service
> index 5fbf081..c60f146 100644
> --- a/nvmf-autoconnect/systemd/nvmf-connect@.service
> +++ b/nvmf-autoconnect/systemd/nvmf-connect@.service
> @@ -11,4 +11,4 @@ Requires=nvmf-connect.target
>  [Service]
>  Type=simple
>  Environment="CONNECT_ARGS=%i"
> -ExecStart=/bin/sh -c "nvme connect-all --matching --quiet `/bin/echo
> -e '${CONNECT_ARGS}'`"
> +ExecStart=/bin/sh -c "nvme connect-all --quiet `/bin/echo -e
> '${CONNECT_ARGS}'`"

Re: [PATCH] nvme-cli: nvmf-connect@.service: Remove matching from default syntax

[James Smart]

On 12/17/2021 12:43 PM, Martin Wilck wrote:
> On Fri, 2021-12-17 at 11:32 -0800, James Smart wrote:
>> commit 53aab69a0add added the "--matching" argument to the systemd
>> connect script that issues connect-all to a discovery controller.
>> When
>> this argument is used, only discovery log entries whose target port
>> traddr's match the traddr of the discovery controller will be
>> connected
>> to.  This eliminates the ability to do referrals by the discovery
>> controller.
>>
>> Revert the commit so that the "--matching" argument is not default
>> behavior.
> 
> Hm, I guess you have to do revert this if it breaks referrals.
> 
> The intention of my patch was not to suppress referrals. Unless I'm
> mistaken, a referral is a log page entry that lists another discovery
> controller, on which then another discovery is carried out. The "--
> matching" parameter was intended to ignore log page entries listing
> ordinary (non-discovery) subsystems on if their traddr didn't match the
> discovery controller's traddr.

Yes - I guess that is the definition of a referral. But there is no rule 
that state subsystems returned by a discovery controller must be behind 
the same transport target port (quite the contrary if one is not on fc).
So that's a rather larger restriction.

> 
> I still think connecting to those entries is wrong more often than not.
> In the environment I was testing in back then, IIRC there were 4
> discovery controllers, each listing every subsystem on every
> controller. The host would try to connect to every subsystem 4 times,
> resulting in error messages 3 out of 4 times. With --matching, these
> errors could be avoided.
> 
> I believe the correct solution would be to fix the way referrals are
> handled with --matching. If a discovery subsystem D1 refers to another
> discovery subsystem D2, then on D2 those entries that match the traddr
> of D2 should be considered "matching", and so on with additional
> referrals. Obviously, "--matching" would need to be ignored for the
> referral entries themselves.

I believe the devices are acting appropriately, and note, that is one 
particular implementation.

What is rudimentary is the linux implementation. Perhaps nvme-cli should 
be smarter about investigating, via sysfs, for the subsystem before 
initiating the connect request. It would need to look for subnqn and the 
target port address tuple data (more than just traddr, perhaps with 
per-transport rules).   Another thing to do is "lessen" the error 
notification. The corrects checks are in place in the kernel thus the 
messages, but perhaps we ought to make systemd less chatty by the style 
of error (or success) we return.

I'm ok with the matching argument - just not on by default. I also think 
this is yet another motivator for making transport/address/nqn vendor 
specific connect options table. This would allow us to turn on the 
option the vendor that was very verbose.

> 
> But as long as we haven't fixed this logic, I'm fine with your patch.
> 
> Martin
> 

Thanks

-- james

Re: [PATCH] nvme-cli: nvmf-connect@.service: Remove matching from default syntax

[Hannes Reinecke]

On 12/17/21 9:43 PM, Martin Wilck wrote:
> On Fri, 2021-12-17 at 11:32 -0800, James Smart wrote:
>> commit 53aab69a0add added the "--matching" argument to the systemd
>> connect script that issues connect-all to a discovery controller.
>> When
>> this argument is used, only discovery log entries whose target port
>> traddr's match the traddr of the discovery controller will be
>> connected
>> to.  This eliminates the ability to do referrals by the discovery
>> controller.
>>
>> Revert the commit so that the "--matching" argument is not default
>> behavior.
> 
> Hm, I guess you have to do revert this if it breaks referrals.
> 
> The intention of my patch was not to suppress referrals. Unless I'm
> mistaken, a referral is a log page entry that lists another discovery
> controller, on which then another discovery is carried out. The "--
> matching" parameter was intended to ignore log page entries listing
> ordinary (non-discovery) subsystems on if their traddr didn't match the
> discovery controller's traddr.
> 
But this does assume that each I/O controller will share the port the 
discovery controller.
Which is not mandated by the spec (even if most arrays currently 
implement it that way), and it's certainly not true for TPAR 8010 CDCs.

So this is not the right way of approaching this 'problem'.

> I still think connecting to those entries is wrong more often than not.
> In the environment I was testing in back then, IIRC there were 4
> discovery controllers, each listing every subsystem on every
> controller. The host would try to connect to every subsystem 4 times,
> resulting in error messages 3 out of 4 times. With --matching, these
> errors could be avoided.
> 
This issue is a direct result of the NVMe spec not putting any 
restriction on what the discovery log pages can contain.
They might even contain entries not reachable from the current host port.

So again, the concept of 'matching' ports is wrong, and should be 
replaced by a concept of 'reachable' ports.
We can easily figure that out for FC, and it should be doable for TCP, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH] nvme-cli: nvmf-connect@.service: Remove matching from default syntax

[Daniel Wagner]

On Fri, 17 Dec 2021 11:32:32 -0800, James Smart wrote:
> commit 53aab69a0add added the "--matching" argument to the systemd
> connect script that issues connect-all to a discovery controller. When
> this argument is used, only discovery log entries whose target port
> traddr's match the traddr of the discovery controller will be connected
> to.  This eliminates the ability to do referrals by the discovery
> controller.
> 
> [...]

Applied, thanks!

[1/1] nvme-cli: nvmf-connect@.service: Remove matching from default syntax
      commit: 58c23ceb12d16756b2222a55d1d9dc5f34bb4fa4

Best regards,
-- 
Daniel Wagner <dwagner@suse.de>