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

[Ivan Shapovalov]

On 2015-02-05 at 02:03 +0300, Ivan Shapovalov wrote:
> On 2015-01-28 at 22:29 +0300, Ivan Shapovalov wrote:
> > 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...
> 
> Ping? Lennart, do you understand what's going on, or should I try to
> describe things more carefully?
> 
> // I would like to get this fixed before v219 ;)

Or, at worst, before v220...

-- 
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/20150222/b411277e/attachment.sig>