[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?
> 
> 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.

Then maybe we can just "remember" (save somewhere) all new jobs as we
coldplug the units, and actually add them only after we've coldplugged
everything? This way, the transaction machinery will observe the
dependencies' state at the time it is already restored, and no extra
jobs will be created.

-- 
Ivan Shapovalov / intelfx /
-------------- 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/20150214/422cd7d3/attachment.sig>