[systemd-devel] [Patch 0/2] logind: make sure that closed sessions will be cleaned

[Colin Guthrie]

'Twas brillig, and Djalal Harouni at 22/01/14 09:24 did gyre and gimble:
> On Thu, Jan 16, 2014 at 11:19:08AM +0100, Djalal Harouni wrote:
>> On Thu, Jan 16, 2014 at 06:01:58AM +0100, Zbigniew Jędrzejewski-Szmek wrote:
>>> Indeed, in a container (without your patches), sessions remain in
>>> "closing" state. But with your patches, systemd --user instance is
>>> started and killed immediately during login. Not good either :)
>> Ok, ouch!
> It seems that the systemd --user instance is killed without my patches!
> 
> Systemd git will confirm it.
> 
> Ok, after some thoughts and source code analysis, I came to the
> conclusion that in order to correctly clean session and user state
> files, we need:
> 
> a) Improve session_check_gc() and user_check_gc() by using the session
> and user states. This implies b)
> 
> b) Make sure that the state of the session and the user is always
> correctly set.

FWIW, I have some backported patches in Mageia that are not (yet) in the
fedora package. As you mentioned Fedora earlier I figure that's what you
are using:

Namely a partial backport of cc377381

Patch is here:

http://svnweb.mageia.org/packages/cauldron/systemd/current/SOURCES/0511-logind-Partial-backport-of-cc377381.patch?revision=566087&view=markup&pathrev=566087

It might be useful to apply to Fedora too. EDIT: Perhaps not, see below.

> I'll discuss some notes about a) and follow with patches that will try
> to honore b).
> 
> I'm sending this to have some feedback before spending more time on it,
> ahh yes debugging this beast is hard!
> 
> Please note that I did not take a look at seats logic, only sessions and
> users using getty and ssh logins.
> 
> Please feel free to correct me!
> 
> 
> a) Improve session_check_gc() and user_check_gc()
> 
> * user_check_gc() has an  "if (u->sessions)" check! this should be
>   updated to check if there are sessions that are not in the "closing"
>   state.
>   
>   If *all* the sessions are in the "closing" state then check each
>   session to see if there are still running processes in the background:
>   screen,...

If the session is closing, it has to, by definition, still have
processes. If not, then the session wouldn't be closing, but actually
fully closed and disappeared...

That said, it's perhaps this check here that actually *prevents* this
final state from being reached due to GC not being run! So this could
fix the chicken+egg problem (I don't know the code, so can only really
talk in generalities!)



>   If so then abort the user_check_gc() or depend on a per-session
>   function that will perhaps do the "kill-session-processes".

> * session_check_gc() should check if the session is in the "closing"
>   state and if there are still running processes in the background, then
>   decide to abort or force the "kill"session-processes"
> 
>   But it should not do the following check:
>   "if (s->scope && manager_unit_is_active(s->manager, s->scope))"
> 
>   Since the scope will always be around, the same was applied in the
>   commit 63966da86 for user_check_gc(), do not check the user manager!


Interesting. It seems that 63966da86 suggests my patch above shouldn't
be needed and in older code, the old "if (u->slice_job ||
u->service_job)" check should just be dropped.


> b) Make sure that the state of the session and the user is always
> correctly set. Not to mention that some logind clients depend on it.
>  
> So to make a) we must honor that b) is stable, and after an extensive
> source code analysis, it seems that the state of users and sessions is
> not always correct. I'll list some notes/bugs here:
> 
> * It seems that in systemd v204 after a logout from a session where
>   processes are still running like "screen" the state files will show:
> 
>   systemd 204
>   session state file:			user state file:
>   Active = 1				State = active
>   State = closing			ACTIVE_SESSIONS = X
> 
>   Where for systemd git:
>   session state file:			user state file:
>   Active = 0				State = closing
>   State = closing			ACTIVE_SESSIONS =
> 
>   Not to mention that for systemd git, the session and user state files
>   will show the same even if there are no background processes and they
>   will not be cleaned  (using getty or ssh logins) which is is the object
>   of this thread!
> 
>   This is a bug, not fixed yet.

So basically the problem is that the gc is simply not triggered often
enough to tidy up these old "closing" sessions with no processes left?

> * To track the state of sessions the fifo_fd should be used since the
>   event manager will call session_dispatch_fifo() to remove it on eof.
> 
>   Using the session_is_active() *before* the fifo_fd check is not stable
>   since it will return true before the fifo_fd was even created! and
>   also before the session scope and user serivce are created, the
>   session->seat->active will be set earlier before the fifo_fd creation
>   and will be the last one to be unset after fifo_fd removal.
> 
>   And currently this affects src/login/logind-user.c:user_get_state()
>   logic since it checks session_is_active() before calling
>   session_get_state() which performs the fifo_fd check before the
>   session_is_active()
>  
>   So user_get_state() and session_get_state() results might differ!
> 
>   This is a bug, not fixed yet, user_get_state() should count on
>   session_get_state().
> 
> 
> * It seems there are other problems with the session and user states,
>   different variables are used to check the state and later these
>   variables are updated at different places!
> 
>   The following patches will describe the problem and try to make the
>   state more stable.
> 
>   The design was do not add extra flags to 'User' and 'Session' structs
>   unless this is the only way. The logic is already there, and it needs
>   a littel synchronization.


I'll let someone else comment on these last points. They seem sensible,
but I'm not familiar with the code here so can't really make educated
comments without a bit more research and am sadly a bit too busy right now.

Cheers!

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/