[systemd-devel] [PATCH, v3 3/3] find_symlinks: adds a cache of enabled unit symbolic link state

[Lennart Poettering]

On Thu, 23.10.14 09:24, David Timothy Strauss (david at davidstrauss.net) wrote:

> On Thu, Oct 23, 2014 at 5:15 AM, Lennart Poettering
> <lennart at poettering.net> wrote:
> > With your patch you generate a system-wide cache for that, but when do
> > you flush it precisely? What's the logic there?
> 
> It updates on daemon-reload or daemon-reexec, consistent with how we

Hm, it does? How so? Am I missing something? Don't see it in the
patch?

BTW, I noticed that the context is currently only used for
unit_file_add_dependency(), unit_file_get_state() and
unit_file_get_list(). Why just these three? What about the other ones,
for example unit_file_enable()?

> Aside from the small amount of additional memory required (which we
> can easily reduce to be proportional to the number of enabled units,
> if we're not doing that already), I don't see much downside to keeping
> the cache around. It's used every time someone runs "systemctl status
> <unit>", even if the calls are not one-after-the-other. Running

Hmm, the "systemctl status" thing is a good point.

> "systemctl" alone hits the cache once for every unit. It's also an
> optimization for the critical path of getting PID 1 back to its event
> loop, which is key for minimizing socket activation latency.
> 
> That said, if it's a blocker, I can work with Ken to make the cache
> more ephemeral.

Hmm, so there are two things I'd like to see added, to make this
safer:

a) the two hashmaps should have a size limit. It can be high, but it
   must be there; we need to put limits on all resources we load from
   external sources.

b) given that we iterate through a variety of dirs, it would be good
   to keep track of the newest mtime of all dirs. Since creating or
   removing a symlink upadtes the mtime on the next use of the cache
   we could check the dirs again, and if something changed flush the
   cache and read again. 

Item a) above removes my doubts about the memory usage. Item b)
removes my doubts about cache flushes. Of course, it comes at the
price of having to stat() all unit files every now and then, but that
should still be a ton cheaper then iterating through the dirs
fully... 

I hope that makes sense?

Lennart

-- 
Lennart Poettering, Red Hat