[linux-cifs-client] [PATCH 4/4] [CIFS] don't allow demultiplex
thread to exit until kthread_stop is called
Jeff Layton
jlayton at redhat.com
Fri May 9 19:10:15 GMT 2008
On Fri, 9 May 2008 13:08:19 -0500
"Steve French" <smfrench at gmail.com> wrote:
> Were there any updates to the patch as you describe at the bottom?
>
No. I don't think kernel_recvmsg will exit with -EINTR unless the task
is signaled. Even if it somehow could, it seems definitely preferable
to just have the process sleep until umount instead of possibly hitting
an oops.
Frankly, if we're going to make the change I was talking about, we
might as well go all the way and make it so that the thread *only*
exits the main loop when kthread_stop() is called. Then we won't
need to put the thread to sleep like this at all.
I'm a little more leery of this though -- it's a bigger change to the
thread logic, and I'm not sure if there's any danger in continuing to
loop when we hit the "tcp session abend after SMBnegprot" cases.
> 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>
> >
>
>
>
--
Jeff Layton <jlayton at redhat.com>
More information about the linux-cifs-client
mailing list