Re: [PATCH rdma-core 5/5] srp_daemon.service: Add support for hot-plugging

[Bart Van Assche <Bart.VanAssche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>]

On Mon, 2017-05-15 at 17:25 -0600, Jason Gunthorpe wrote:
> On Mon, May 15, 2017 at 03:47:33PM -0700, Bart Van Assche wrote:
> > srp_daemon.service is modified such that instead of starting
> > /usr/sbin/srp_daemon.sh that it does not start any process. A new
> > template service is added, namely srp_daemon_port@.service. This
> > service replaces srp_daemon.sh and controls /usr/sbin/srp_daemon
> > for a single port. A udev rule is added that instantiates the
> > srp_daemon_port@.service every time an RDMA port is hot-added.
> > Since the per-port service depends on the srp_daemon service,
> > starting or stopping the srp_daemon service affects all per-port
> > services.
> 
> This looks pretty good to me ... You are happy with how the user experience works?

Yes, except for one aspect, namely that starting srp_daemon does not start
the per-port services. I have addressed that as follows in srp_daemon.service:

ExecStart=.../srp_daemon_start_all.sh

where srp_daemon_start_all.sh is the following script:

#!/bin/sh
for p in /sys/class/infiniband/*/ports/*; do
    [ -e "$p" ] || continue
    p=${p#/sys/class/infiniband/}
    nohup /usr/bin/systemctl start "srp_daemon_port@${p/\/ports\//:}" </dev/null >&/dev/null &
done

> A few minor thoughts
> 
> > +++ b/srp_daemon/90-srp-daemon.rules
> > @@ -0,0 +1 @@
> > +ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", TAG+="systemd", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"
> 
> I think we should have a main rdma udev rules file that always creates
> .device targets inside systemd for umad, unconditionally. This should
> be useful for other things like opensm/etc.
> 
> So we'd have a line like:
> 
> ACTION=="add", SUBSYSTEM=="infiniband_mad", TAG+="systemd", ENV{SYSTEMD_ALIAS}="/dev/infiniband/umad/$attr{ibdev}:$attr{port}"
> 
> So we get (unescaped)
> 
> /dev/infiniband/umad0.device
> /dev/infiniband/umad/mlx4_0:0.device
> 
> Then the srp-daemon.rule would just be the wants:
> 
> ACTION=="add", SUBSYSTEM=="infiniband_mad", PROGRAM:="/usr/bin/systemctl show srp_daemon -p UnitFileState", RESULT=="UnitFileState=enabled", ENV{SYSTEMD_WANTS}+="srp_daemon_port@$attr{ibdev}:$attr{port}.service"
> 
> > index 00000000..666400ab
> > +++ b/srp_daemon/srp_daemon_port@.service
> > @@ -0,0 +1,18 @@
> > +[Unit]
> > +Description=SRP daemon that monitors port %i
> > +Documentation=man:srp_daemon file:/etc/rdma/rdma.conf file:/etc/srp_daemon.conf
> > +DefaultDependencies=false
> > +Conflicts=emergency.target emergency.service
> > +Requires=rdma.service
> > +Wants=opensm.service
> > +After=rdma.service opensm.service srp_daemon.service
> > +After=network.target
> > +Before=remote-fs-pre.target
> > +BindsTo=srp_daemon.service
> 
> Using the new .device, we can add a requires:
> 
> Requires: -dev-infiniband-umad0-%i.device
> 
> Now, systemd will never start the port service until udev says that
> the umad is available for that port. It will actually wait until udev
> says that umad is present if something requires it.

All of this has been addressed in the following updated pull request:
https://github.com/linux-rdma/rdma-core/pull/135

> I also wonder if the systemd activation protocol should be used by
> srp_daemon, this should defer mount until srp_daemon has done the
> right stuff, but that might be no good if the 'right stuff' requires a
> link up?
>
> However, if the admin is using the above .mount unit Requires then
> they would want to wait until the link is up and the SM has been
> contacted before attempting the first mount? systemd obnoxiously does
> not have mount retries so everything should be perfect before mount is
> done.

I think that systemd mount units should have a dependency on /dev/disk/by-id
or /dev/disk/by-uuid instead of srp_daemon. Even after an srp_daemon_port@
service has been started it can take some time
before login occurs. Using a
dependency on /dev/disk/by-*id avoids that the mount service gets started
before the path it needs is available.

> Does
>   systemctl mask srp_daemon_port@mlx4_0:1
> 
> Work to inhibit the daemon on a specific port? If yes that should be
> mentioned in here I think. Otherwise, we probably still need a way to
> do that.

Information about masking has been added to the srp_daemon_port@ man page.

> The final bits would be to see if any security sandboxing options are
> appropriate for the .unit file, since srp_daemon is network
> facing. Drop caps, ro filesystems, etc. It should also dump root.

Sorry but since srp_daemon writes into /sys/class/infiniband_srp/*/add_target
I don't think it's possible to drop root privileges for the srp_daemon_port@
service.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html