[systemd-devel] [PATCHv2] core: do not spawn jobs or touch other units during coldplugging

[Lennart Poettering]

On Mon, 27.04.15 18:28, Ivan Shapovalov (intelfx100 at gmail.com) wrote:

> On 2015-04-27 at 17:14 +0200, Lennart Poettering wrote:
> > On Sat, 25.04.15 05:48, Ivan Shapovalov (intelfx100 at gmail.com) wrote:
> > 
> > > On 2015-04-25 at 04:00 +0300, Ivan Shapovalov wrote:
> > > > On 2015-04-24 at 16:04 +0200, Lennart Poettering wrote:
> > > > > [...]
> > > > > 
> > > > > Actually, it really is about the UNIT_TRIGGERS dependencies 
> > > > > only,
> > > > > since we don't do the retroactive deps stuff at all when we are
> > > > > coldplugging, it's conditionalized in m->n_reloading <= 0.
> > > > 
> > > > So, I think I understand the problem. We should do this not only 
> > > > for
> > > > UNIT_TRIGGERS, but also for any dependencies which may matter
> > > > when activating that unit. That is, anything which is referenced 
> > > > by
> > > > transaction_add_job_and_dependencies()... recursively.
> > > 
> > > Here is what I have in mind. Don't know whether this is correct, 
> > > but
> > > it fixes the problem for me.
> > > 
> > > From 515d878e526e52fc154874e93a4c97555ebd8cff Mon Sep 17 00:00:00 
> > > 2001
> > > From: Ivan Shapovalov <intelfx100 at gmail.com>
> > > Date: Sat, 25 Apr 2015 04:57:59 +0300
> > > Subject: [PATCH] core: coldplug all units which participate in jobs
> > > 
> > > This is yet another attempt to fix coldplugging order (more 
> > > especially,
> > > the problem which happens when one creates a job during 
> > > coldplugging, and
> > > it references a not-yet-coldplugged unit).
> > > 
> > > Now we forcibly coldplug all units which participate in jobs. This
> > > is a superset of previously implemented handling of the 
> > > UNIT_TRIGGERS
> > > dependencies, so that handling is removed.
> > > ---
> > >  src/core/transaction.c | 6 ++++++
> > >  src/core/unit.c        | 8 --------
> > >  2 files changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/core/transaction.c b/src/core/transaction.c
> > > index 5974b1e..a02c02c 100644
> > > --- a/src/core/transaction.c
> > > +++ b/src/core/transaction.c
> > > @@ -848,6 +848,12 @@ int transaction_add_job_and_dependencies(
> > >          assert(type < _JOB_TYPE_MAX_IN_TRANSACTION);
> > >          assert(unit);
> > >  
> > > +        /* Before adding jobs for this unit, let's ensure that 
> > > its state has been loaded.
> > > +         * This matters when jobs are spawned as part of 
> > > coldplugging itself (see. e. g. path_coldplug().
> > > +         * This way, we "recursively" coldplug units, ensuring 
> > > that we do not look at state of
> > > +         * not-yet-coldplugged units. */
> > > +        unit_coldplug(unit);
> > 
> > I like the simplicity of this patch actually, but it's unfortunately
> > too simple: coldplugging is to be applied only for services that are
> > around at the time we come back from a reload. If you start a service
> > during runtime, without any reloading anywhere around, we should not
> > coldplug at all.
> > 
> > I figure we need a "coldplugging" bool or so in Manager, which we set
> > while coldplugging and can then check here.
> 
> Yeah, right, I've fixed it locally but forgot to send a follow-up mail.
> Actually, isn't it "unit->manager->n_reloading > 0"?

Yes, indeed, that should suffice.

Lennart

-- 
Lennart Poettering, Red Hat