Re: [PATCH 00/35] RFC: add "nvme monitor" subcommand

From: Martin Wilck <redacted>

(Cover letter copied from https://github.com/linux-nvme/nvme-cli/pull/877)

This patch set adds a new subcommand **nvme monitor**. In this mode,
**nvme-cli** runs continuously, monitors events (currently, uevents) relevant
for discovery, and optionally autoconnects to newly discovered subsystems.

The monitor mode is suitable to be run in a systemd service. An appropriate
unit file is provided. As such, **nvme monitor** can be used as an alternative
to the current auto-connection mechanism based on udev rules and systemd
template units. If `--autoconnect` is active, **nvme monitor** masks the
respective udev rules in order to prevent simultaneous connection attempts
from udev and itself.

I think that a monitor daemon is a good path forward.

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?

  It keeps track of persistent discovery

    controllers it created, and tears them down on exit. When run in
    `--persistent --autoconnect` mode *in the initial ramfs*, it will keep
    discovery controllers alive, so that a new instance started after switching
    root can simply re-use them.

Nice.

  * 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.

  * 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?

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

Which events?

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?

The main process just waits for events using `epoll()`. When an event is
received that necessitates a new discovery, a child is forked. This makes it
possible to fill in the configuration parameters for `do_discover()` without
interfering with the main process or other discovery tasks running in
parallel. The program tracks *transport addresses* (called "connections" in
the code) rather than NVMe controllers. In `--persistent` mode, it tries to
maintain exactly one persistent discovery connection per transport address.

Using `epoll()` may look over-engineered at this stage. I hope the better
flexibility over `poll()` (in particular, the ability to add new event sources
while waiting) will simplify future extensions and improvements.

### Todo

  * Referrals are not handled perfectly yet. They will be handled by
    `do_discover()` just as it would when called from **nvme connect-all**, but
    it would be better to pass referrals back to the main process to make it
    aware of the additional discovery controller rather than using
    recursion. The main process would e.g. know if a discovery is already
    running for the transport address in the referrral.

Agree, this way you can also keep new referrals persistently, which I
don't know if we do today.

  *  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...

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

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

  * 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?)

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