[linux-cifs-client] [PATCH 4/4] [CIFS] don't allow demultiplex thread to exit until kthread_stop is called

[Shirish Pargaonkar]

On 5/2/08, Jeff Layton <jlayton at redhat.com> wrote:
> cifs_demultiplex_thread can exit under several conditions:
>
> 1) if it's signaled
> 2) if there's a problem with session setup
> 3) if kthread_stop is called on it
>
> The first two are problems. If kthread_stop is called on the thread,
> there is no guarantee that it will still be up. We need to have the
> thread stay up until kthread_stop is called on it.
>
> One option would be to not even try to tear things down until after
> kthread_stop is called. However, in the case where there is a problem
> setting up the session, there's no real reason to try continuing the
> loop.
>
> This patch allows the thread to clean up and prepare for exit under all
> three conditions, but it has the thread go to sleep until kthread_stop
> is called. This allows us to simplify the shutdown code somewhat since
> we can be reasonably sure that the thread won't exit after being
> signaled but before kthread_stop is called.
>
> It also removes the places where the thread itself set the tsk variable
> since it appeared that it could have a potential race where the thread
> might never be shut down.
>
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> ---
>  fs/cifs/connect.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4c93c2a..f904713 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -348,7 +348,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
>        int reconnect;
>
>        current->flags |= PF_MEMALLOC;
> -       server->tsk = current;  /* save process info to wake at shutdown */
>        cFYI(1, ("Demultiplex PID: %d", task_pid_nr(current)));
>
>        length = atomic_inc_return(&tcpSesAllocCount);
> @@ -649,10 +648,20 @@ multi_t2_fnd:
>
>        spin_lock(&GlobalMid_Lock);
>        server->tcpStatus = CifsExiting;
> -       server->tsk = NULL;
> +       spin_unlock(&GlobalMid_Lock);
> +
> +       /* don't exit until kthread_stop is called */
> +       set_current_state(TASK_UNINTERRUPTIBLE);
> +       while (!kthread_should_stop()) {
> +               schedule();
> +               set_current_state(TASK_UNINTERRUPTIBLE);
> +       }
> +       set_current_state(TASK_RUNNING);
> +
>        /* check if we have blocked requests that need to free */
>        /* Note that cifs_max_pending is normally 50, but
>        can be set at module install time to as little as two */
> +       spin_lock(&GlobalMid_Lock);
>        if (atomic_read(&server->inFlight) >= cifs_max_pending)
>                atomic_set(&server->inFlight, cifs_max_pending - 1);
>        /* We do not want to set the max_pending too low or we
> @@ -2182,15 +2191,12 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                        srvTcp->tcpStatus = CifsExiting;
>                        spin_unlock(&GlobalMid_Lock);
>                        if (srvTcp->tsk) {
> -                               struct task_struct *tsk;
>                                /* If we could verify that kthread_stop would
>                                   always wake up processes blocked in
>                                   tcp in recv_mesg then we could remove the
>                                   send_sig call */
>                                force_sig(SIGKILL, srvTcp->tsk);
> -                               tsk = srvTcp->tsk;
> -                               if (tsk)
> -                                       kthread_stop(tsk);
> +                               kthread_stop(srvTcp->tsk);
>                        }
>                }
>                 /* If find_unc succeeded then rc == 0 so we can not end */
> @@ -2206,23 +2212,17 @@ cifs_mount(struct super_block *sb, struct cifs_sb_info *cifs_sb,
>                                        if ((temp_rc == -ESHUTDOWN) &&
>                                            (pSesInfo->server) &&
>                                            (pSesInfo->server->tsk)) {
> -                                               struct task_struct *tsk;
>                                                force_sig(SIGKILL,
>                                                        pSesInfo->server->tsk);
> -                                               tsk = pSesInfo->server->tsk;
> -                                               if (tsk)
> -                                                       kthread_stop(tsk);
> +                                               kthread_stop(pSesInfo->server->tsk);
>                                        }
>                                } else {
>                                        cFYI(1, ("No session or bad tcon"));
>                                        if ((pSesInfo->server) &&
>                                            (pSesInfo->server->tsk)) {
> -                                               struct task_struct *tsk;
>                                                force_sig(SIGKILL,
>                                                        pSesInfo->server->tsk);
> -                                               tsk = pSesInfo->server->tsk;
> -                                               if (tsk)
> -                                                       kthread_stop(tsk);
> +                                               kthread_stop(pSesInfo->server->tsk);
>                                        }
>                                }
>                                sesInfoFree(pSesInfo);
> --
> 1.5.3.6
>
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client at lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>

Jeff,

Is there a possibility that kthread_stop would never get called, in which
case cifsd would loop forever or for a long long time i.e. cifsd has become
useless but would not go away, sort of wasting system resources however
miniscule?

I think this can only happen if kernel_recvmsg returns with return code EINTR
not because it was sig(kill)naled by mount or unmount code but for some
other reason!

Regards,

Shirish