[systemd-devel] Deadlocks with reloading jobs which are part of current transaction [was: [PATCH] Avoid reloading services when shutting down]

[Martin Pitt]

Lennart Poettering [2015-02-04 13:27 +0100]:
> On Wed, 04.02.15 08:56, Martin Pitt (martin.pitt at ubuntu.com) wrote:
> 
> > Lennart Poettering [2015-02-03 21:40 +0100]:
> > > It's really about synchronous waiting on jobs. If you synchronously
> > > wait for completion of jobs that are ordered against the job your are
> > > part of yourself, then things will deadlock.
> > 
> > Indeed. The problem is that if you reload e. g. postfix from a DHCP or
> > "network up/down" hook, such a script doesn't have the slightest idea
> > whether it was run because the network changed at runtime (i. e. udev
> > event or the user just selected a new network) or whether it happens
> > as part of a systemd transaction (boot/shutdown). In the former case
> > you do want to block, in the latter case you mustn't.
> 
> I don't see why you'd want to block in either case. Networking is
> asynchronous anyway, there's really no point in blocking here. What
> does this give you?

So far, if a hook or shell script calls e. g. "service foo restart",
it can count on the service actually being reloaded after it finishes,
and thus you can then interact with it immediately. That's not
important for many scripts, but we can't just always make "service foo
reload" async by using --no-block, as that would break compatibility
with scripts which do rely on that behaviour. So we do need to
conditionalize it to avoid the deadlocks under systemd.

> Modern code, that talks directly to systemctl, that uses hook scripts,
> should always use --no-block for these cases.

Yeah, for sure. We just don't have that luxury easily, as first these
scripts don't call systemctl but "service" (for the Debianists: or
invoke-rc.d, but same difference), it will take some time to find
all these occurrences, and people will hate us for having to add stuff
like

  if (running under systemd)
       systemctl --no-block reload foo
  else
       service foo reload

as that special case appears rather pointless for someone who hasn't
dealt with the details of systemd job transactions. That's not to say
that it shouldn't happen (perhaps adding a --no-block option to
service), but that takes time.

> I mean, you must have some code in Debian that forwards old sysv
> restart/reload requests to systemctl. That's probably going to be in
> /usr/sbin/service and maybe a few other places. Why can't you just add
> the --no-block logic there, conditionalized to shutdown/reboot?

Well, we can; it's just as much an unreliable hack as the two existing
patches :-) and I was wondering if we can't solve this in a more
generic/distro agnostic way.

> >  - Don't enqueue a reload if the service to be reloaded isn't running.
> >    E. g. postfix.service "inactive/dead" in
> >    https://bugs.debian.org/635777 or smbd.service "start/waiting" in
> >    https://launchpad.net/bugs/1417010.  This would completely avoid
> >    the deadlock in most situations already, and doesn't change the
> >    semantics for working use cases either, so this should even be
> >    applicable for upstream?
> 
> No, this would open up the door for races. The guarantee we give
> around blocking operations, is that by the time they return the
> operation or an equivalent has been executed. More specifically, if
> you start a service, and it is in "starting", and then issue a
> "reload" or "restart", and it returns you *know* that the
> configuration that was on disk at the time you issued the "reload" or
> "restart" -- or a newer one -- is in place. If you'd suppress the
> reload/restart in this case, then you will not get that guarantee,
> because the configuration ultimately loaded might be the one from the
> time the daemon was first put into starting mode at.

Hm, I don't quite understand this. If you reload a service which isn't
currently running, systemctl will fail. It's just currently going to
enqueue the reload request, and executing the job will fail due to the
"not active" check in unit_reload(). The problem with that is just
that systemctl doesn't fail immediately with "the unit is not active",
but blocks on the queued request. So if you don't have pending jobs,
the net result is the same, but with pending jobs you can easily get a
deadlock.

> Or in other words: if a script does this:
> 
>    # sed -i -e 's/something/somethingelse/' /etc/mydaemon.conf
>    # systemctl restart mydaemon.service
> 
> Then we *must* give the guarantee that when the second command returns
> the change just made to /etc/mydaemon.conf is in place, and not a
> previous config loaded.

Yes, that's fine. I only said "reload" above, as that's what
DHCP/network scripts usually do.

> (Now, this example is about restarts, but reload is pretty much the
> same

reload on an inactive unit will fail, restart will start it.

> Also: *why* for heaven's sake? Just add --no-block when you are
> running from these contexts, and all is good. I really don't get why
> you don't want to do that.

Well, I do, but special-casing that on boot/shutdown isn't enough (you
can have other bigger transactions which lock up); and it's rather
expensive to do this via multiple CLI tools and D-BUS calls rather
than in the manager itself. So I was pondering how to get this effect
much more efficiently in manager.c instead of the shell wrappers
calling systemctl.

> On Fedora, these hook scripts use --no-block correctly, and I am
> completely puzzled why you don't want this on Debian!

Mostly because we don't currently have the luxury of having only
systemd :-)

Anyway, I think your stanza on this is clear. We'll keep the hacks on
the Debian side, if that's not a problem for other distros.

Thanks for the discussion and the hint with --no-block!

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)