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

[Ivan Shapovalov]

On 2015-01-28 at 20:15 +0100, Lennart Poettering wrote:
> 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?

The .path unit from CUPS is triggered and starts its .service unit
*while the service unit's state has not yet been coldplugged*.
Actually, almost nothing is coldplugged at that point. Thus the
basic.target is effectively re-started with all its dependencies,
ignoring all existing state (because it is not yet coldplugged). This
is the actual bug, and it is the root cause of the reported bug.

Reported bug, on the other hand, happens due to
1) the above-described bug (not related to alsa) takes place;
2) alsa-restore.service is Type=oneshot, RemainAfterExit=false and
   WantedBy=basic.target.

Hope it makes things clearer...

-- 
Ivan Shapovalov / intelfx /

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150128/2b727702/attachment.sig>