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

Steve French smfrench at gmail.com
Fri May 9 18:08:19 GMT 2008


Were there any updates to the patch as you describe at the bottom?

On Sun, May 4, 2008 at 2:34 PM, Jeff Layton <jlayton at redhat.com> wrote:

> On Sun, 4 May 2008 10:46:00 -0500
> "Shirish Pargaonkar" <shirishpargaonkar at gmail.com> wrote:
>
> > 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?
>
> It would loop until umount, I suppose. I don't think it would waste
> much in resources -- just a bit of memory, really CPU time would be
> pretty much nox-existent (it would just stay asleep).
>
> >
> > 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!
> >
>
> Hmm...I'm not sure that this is actually possible. I thought
> kernel_recvmsg only returned -EINTR when signaled, but I could be wrong
> here.
>
> If, however, we make the assumption that it is possible, then it seems
> like allowing the thread to go to sleep is a reasonable thing to do. The
> current situation is that the thread just exits. You could hit the same
> panic that you've already hit if the thread exiting happened to race
> with a umount.
>
> If this is a concern, one possibility is to just have the thread not
> break out of the main loop when kernel_recvmsg returns -EINTR. It could
> just msleep(1) and continue. We're not flushing the pending signal, so I
> don't think we'll get stuck in kernel_recvmsg.
>
> --
> Jeff Layton <jlayton at redhat.com>
>



-- 
Thanks,

Steve
-------------- next part --------------
HTML attachment scrubbed and removed


More information about the linux-cifs-client mailing list