quoted

quoted

This method for discovery and autodetection has some advantages
over the
current udev-rule based approach:

   * By using the `--persistent` option, users can easily control
whether
     persistent discovery controllers for discovered transport
addresses should
     be created and monitored for AEN events. **nvme monitor**
watches known
     transport addresses, creates discovery controllers as required,
and re-uses
     existing ones if possible.

What does that mean?

In general, if the monitor detects a new host_traddr/traddr/trsvcid
tuple, it runs a discovery on it, and keeps the discovery controller
open if --persistent was given. On startup, it scans existing
controllers, and if it finds already existing discovery controllers,
re-uses them. These will not be shut down when the monitor exits.

And if it doesn't run with --persistent it deletes them? even if these
weren't created by it?

And, if none exist where does it get new discovery controller details?
Today we use discovery.conf for that.

This allows users fine-grained control about what discovery controllers
to operate persistently. Users who want all discovery controllers to be
persistent just use --persistent. Others can set up those that they
want to have (manually or with a script), and not use --persistent.

The background is that hosts may not need every detected discovery
controller to be persistent. In multipath scenarios, you may see more
discovery subsystems than anything else, and not everyone likes that.
That's a generic issue and unrelated to the monitor, but running the
monitor with --persistent creates discovery controllers that would
otherwise not be visible.

Hope this clarifies it.

Well, How do people expect to know if stuff change without a persistent
discovery controller? Not sure if people may see more discovery
controllers this is a real issue given what they are getting from it.

But OK.

quoted

quoted

   * In certain situations, the systemd-based approach may miss
events due to
     race conditions. This can happen e.g. if an FC remote port is
detected, but
     shortly after it's detection an FC relogin procedure is
necessary e.g. due to
     an RSCN. In this case, an `fc_udev_device` uevent will be
received on the
     first detection and handled by an `nvme connect-all` command
run from
     `nvmf-connect@.service`. The connection attempt to the rport in
question will
     fail with "no such device" because of the simultaneous FC
     relogin. `nvmf-connect@.service` may not terminate immediately,
because it
     attempts to establish other connections listed in the Discovery
Log page it
     retrieved. When the FC relogin eventually finishes, a new
uevent will be
     received, and `nvmf-connect@` will be started again, but *this
has no effect*
     if the previous `nvmf-connect@` service hasn't finished yet.
This is the
     general semantics of systemd services, no easy workaround
exists.  **nvme
     monitor** doesn't suffer from this problem. If it sees an
uevent for a
     transport address for which a discovery is already running, it
will queue the
     handling of this event up and restart the discovery after it's
finished.

While I understand the issue, this reason alone is an overkill for
doing
this.

I agree. But it's not easy to fix the issue otherwise. In the customer
problem where we observed it, I worked around it by adding the udev
seqnum to the "instance name" of the systemd service, thus allowing
several "nvme connect-all" processes to run for the same transport
address simultaneously. But I don't think that would scale well;
the monitor can handle it more cleanly.

Still changing the entire thing for a corner case...

quoted

quoted

   * Resource consumption for handling uevents is lower. Instead of
running an
     udev worker, executing the rules, executing `systemctl start`
from the
     worker, starting a systemd service, and starting a separate
**nvme-cli**
     instance, only a single `fork()` operation is necessary. Of
course, on the
     back side, the monitor itself consumes resources while it's
running and
     waiting for events. On my system with 8 persistent discovery
controllers,
     its RSS is ~3MB. CPU consumption is zero as long as no events
occur.

What is the baseline with what we have today?

A meaningful comparsion is difficult and should be done when the
monitor functionality is finalized. I made this statement only to
provide a rough idea of the resource usage, not more.

You mention that the utilization is lower than what we do today, hence
my question is by how much?

quoted

quoted

   * **nvme monitor** could be easily extended to handle events for
non-FC
     transports.

Which events?

Network discovery, mDNS or the like. I haven't digged into the details
yet.

Yes, that is possible, would probably be easier to do this in a higher
level language but libavahi can also do this...

quoted

quoted

I've tested `fc_udev_device` handling for NVMeoFC with an Ontap
target, and
AEN handling for RDMA using a Linux **nvmet** target.

### Implementation notes

I've tried to change the exisiting **nvme-cli** code as little as
possible
while reusing the code from `fabrics.c`. The majority of changes in
the
existing code exports formerly static functions and variables, so
that they
are usable from the monitor code.

General comment, can you please separate out fixes/cleanups that are
not
related to the goal of this patchset?

Which ones are you referring to? 09 and 19? While these are minor
improvements to the existing code, I wouldn't say they qualify as fixes
or cleanups. They aren't necessary without adding the monitor code.

Patches that are not directly related to the goal of this should
be splitted out to ease the review.

But yes, I can post all changes to existing code separately.

quoted

quoted

   *  When "add" uevents for nvme controller devices are received,
the
      controller is consistently not in `live` state yet, and
attempting to read
      the `subsysnqn` sysfs attribute returns `(efault)`. While this
should
      arguably be fixed in the kernel, it could be worked around in
user space
      by using timers or polling the `state` sysfs attribute for
changes.

This is a bug, what in the code causes this? nothing in controller
state
should prevent from this sysfs read from executing correctly...

I think it can be fixed by making nvme_sysfs_show_subsysnqn() fall back
to ctrl->opts->subsysnqn if ctrl->subsys is NULL.

I'll send a patch. Anyway, it'll take time until this is fixed
everywhere.

Thanks.

quoted

quoted

   * Parse and handle `discovery.conf` on startup.

This is a must I think, where do you get the known transport
addresses
on startup today?

There's a systemd service that runs "nvme connect-all" once during
boot. That exists today. I'm not sure if it should be integrated in the
monitor, perhaps it's good to keep these separate. People who don't
need the monitor can still run the existing service only, whereas for
others, the two would play together just fine.

Then doesn't this service need to run after it?

quoted

quoted

   * Implement support for RDMA and TCP protocols.

What is needed for supporting them? Not sure I follow (I thought
you mentioned that you tested against linux nvmet-rdma?)

AENs over existing discovery controllers are supported for all
transports. But there's no support for discovery of new transports
except for NVMeoFC's "fc_udev_device" mechanism.

I see. thanks.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme