core: cleanup unit's dropin directories from global cache#40845
core: cleanup unit's dropin directories from global cache#40845msekletar wants to merge 1 commit intosystemd:mainfrom
Conversation
7678007 to
d0b6ff5
Compare
|
Btw, leak is easily reproducible. Starting a transient unit and calling systemd set-property afterwards is enough to trigger it. Put it in the loop and keep running for a day and systemd can eat up 100s of MiBs of memory. |
|
@yuwata PTAL. |
| free(u->source_path); | ||
|
|
||
| /* Maybe we have put unit specific dropin dir into unit_path_cache. */ | ||
| STRV_FOREACH(dropin, u->dropin_paths) { |
There was a problem hiding this comment.
this should only apply to transient units, no? i.e. should also be done in unit_remove_transient. Otherwise if a static unit gets GC'ed and then loaded again we suddenly ignore all the drop-ins?
There was a problem hiding this comment.
I think the issue is quite generic, we put dropin directory into cache in unit_write_settings() which can be called via bus_cgroup_set_property() for non-transient units.
Regarding ignoring dropins, I don't think that is the case on top of that if the unit is GCed and then loaded again as part of loading all dropins should be freshly resolved.
There was a problem hiding this comment.
I think the issue is quite generic, we put dropin directory into cache in unit_write_settings() which can be called via bus_cgroup_set_property() for non-transient units.
Sure, but those drop-ins stick around. We really shouldn't willy-nilly discard data we initially cache...
There was a problem hiding this comment.
Regarding ignoring dropins, I don't think that is the case on top of that if the unit is GCed and then loaded again as part of loading all dropins should be freshly resolved.
This does have an impact btw. We only GC unit objects, not cache built during unit load. That's the whole point of the cache after all, so that we don't have to look into whether certain files/dirs exist if we need to load a unit back in again...
There was a problem hiding this comment.
Sure, but those drop-ins stick around. We really shouldn't willy-nilly discard data we initially cache...
Well, it is not willy-nilly per say, unit is gone and we free drop-in paths because drop-in paths are lifecycle bound to the unit object, directories those drop-in reside in should be deleted too. Btw, notice that I am deleting only unit specific drop-in directories not global ones like .../service.d/
There was a problem hiding this comment.
Well, it is not willy-nilly per say, unit is gone and we free drop-in paths because drop-in paths are lifecycle bound to the unit object, directories those drop-in reside in should be deleted too.
They really aren't. Again this is what unit path cache is about after all... To flush out old records for static unit files you must issue a daemon-reload.
See unit_file_find_dirs() why this actually matters. We'd skip the drop-in dir if the cache is supplied and doesn't contain the dir.
There was a problem hiding this comment.
Yeah, I agree with @YHNdnzj. I think it is better to do that in unit_remove_transient().
BTW, CIs are red...
When user creates dropin files via API (e.g. systemctl set-property ...) we put the dropin directory path into unit_path_cache. Drop those directories from the cache in unit_free() and prevent memory leak. Follow-up for fce94c5.
d0b6ff5 to
8bf76cb
Compare
When user creates dropin files via API (e.g. systemctl set-property ...) we put the dropin directory path into unit_path_cache. Drop those directories from the cache in unit_free() and prevent memory leak.
Follow-up for fce94c5.