[systemd-devel] bug 88401 / daemon-reload causes Type=oneshot services to be re-started / path_coldplug() is non-deserialization-aware

[Lennart Poettering]

On Sun, 18.01.15 04:21, Ivan Shapovalov (intelfx100 at gmail.com) wrote:

> Hi folks,
> 
> I'm trying to fix this bug:
> https://bugs.freedesktop.org/show_bug.cgi?id=88401
> 
> The initial problem (as reported) looks following: performing a reload
> (maybe implicitly) re-starts alsa-restore.service if it is enabled.
> 
> With a bit of debugging I've apparently found a root cause. Explanation
> following.
> 
> Suppose we have CUPS installed and org.cups.cupsd.{path,service} are
> started.
> 
> We enter manager_reload(), units are serialized, reset, re-read,
> deserialized and then cold-plugged. (Note that e. g. unit_notify() has
> special "protection" to avoid spawning jobs when a reload is in
> progress.)
> 
> So, if org.cups.cupsd.path is started, *it is almost first to be
> cold-plugged*. The call chain is:
> 
> unit_coldplug()
> path_coldplug()
> path_enter_waiting() // recheck == true
> path_check_good() // returns true
> path_enter_running()
> manager_add_job() // at this point we're fucked up
> 
> So, a job is enqueued for org.cups.cupsd.service. This is already wrong,
> because a reload should never enqueue any jobs (IIUC). So, the job is
> enqueued... remember that almost all units are inactive by now. Thus we
> end up re-starting half a system (the whole basic.target) as
> dependencies.
> 
> Questions:
> - is my analysis correct?
> - if yes, then how to fix this? Maybe add a similar
>   "if (UNIT(p)->manager->n_reloading <= 0)" check to
>   path_enter_running() to avoid calling manager_add_job() during
>   reloading?

Hmm, how does this relate to the ALSA case? I mean, the alsa units
don't use .path units, do they?

So, in general I think .path units should trigger things on reload,
but only those things that ar bound to "level", not those to "edge",
if you follow what I mean. More specifically, I think that cups.path
should trigger things, since its job is to make sure that cups.service
is running as long as there's a file in /var/spool/cups/. 

PathExists=/PathExistsGlob=/DirectoryNotEmpty= are all of the "level"
kind. As long as the condition they express is true they should ensure
their service is running.  PathChanged= and PathModified= OTOH are
"edge" triggers, and they should only trigger something when a file is
changed while we look at it, but not during coldplugging.

During a reload, I hence believe it is OK if trigger units trigger new
jobs. 

Anyway, I am not really sure I grok the relation to
alsa-restore... Can you elaborate?

Lennart

-- 
Lennart Poettering, Red Hat