problem with kthread changes that went into cifs_demultiplex_thread

problem with kthread changes that went into cifs_demultiplex_thread

[Steve French]

In working on the smb2 client implementation, I noticed that there is
a problem with the revised kthread logic which takes down cifsd
(cifs_demultiplex_thread) on umount or on abnormal socket termination
during protocol negotiation.    I noticed this while looking at SMB2
since the Samba 4 SMB2 server was not handling the new SMB2 Negotiate
protocol request (and unlike Windows Server 2008) and was closing the
tcp session immediately after receiving the negprot request from the
client.  An unexpected error on negprot which causes the TCP session
to close will cause cifsd to exit, but now  this blocks forever at
kthread_should_stop just after the main loop in
cifs_demultiplex_thread.    The task doing mount is blocked in
wait_event on the response_q but will never be woken up by cifsd (even
though it would timeout if some one did wake it up once the 15 second
timeout was expired).   umount will never call kthread_stop because
mount never succeeded - we do not have a tree connection yet (so tcon
is null and we can't get to tcon->ses->server) and the failed mount
won't call kthread_stop because it is stuck in wait_event with no one
to wake up response_q.

Adding a call to wake_up_all(&server->response_q) right after we set
server->tcpStatus = CifsExiting at the end of the loop in
cifs_demultiplex_thread but before we wait for kthread_stop works in
my testing.

-- 
Thanks,

Steve

Re: problem with kthread changes that went into cifs_demultiplex_thread

[Jeff Layton]

On Tue, 10 Jun 2008 10:41:29 -0500
"Steve French" <smfrench@gmail.com> wrote:

> In working on the smb2 client implementation, I noticed that there is
> a problem with the revised kthread logic which takes down cifsd
> (cifs_demultiplex_thread) on umount or on abnormal socket termination
> during protocol negotiation.    I noticed this while looking at SMB2
> since the Samba 4 SMB2 server was not handling the new SMB2 Negotiate
> protocol request (and unlike Windows Server 2008) and was closing the
> tcp session immediately after receiving the negprot request from the
> client.  An unexpected error on negprot which causes the TCP session
> to close will cause cifsd to exit, but now  this blocks forever at
> kthread_should_stop just after the main loop in
> cifs_demultiplex_thread. 

Ok, I think I see what you're talking about. *sigh* -- and I just posted
the patch to make the kthread go to sleep to our internal mailing lists
this morning.

> The task doing mount is blocked in
> wait_event on the response_q but will never be woken up by cifsd (even
> though it would timeout if some one did wake it up once the 15 second
> timeout was expired).   umount will never call kthread_stop because
> mount never succeeded - we do not have a tree connection yet (so tcon
> is null and we can't get to tcon->ses->server) and the failed mount
> won't call kthread_stop because it is stuck in wait_event with no one
> to wake up response_q.
> 
> Adding a call to wake_up_all(&server->response_q) right after we set
> server->tcpStatus = CifsExiting at the end of the loop in
> cifs_demultiplex_thread but before we wait for kthread_stop works in
> my testing.
> 

The solution sounds reasonable at first glance. Will you be pushing a
patch to your git tree for this soon?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: problem with kthread changes that went into cifs_demultiplex_thread

[Steve French]

OK - have added the patch for the negotiate problem (waiting on
kthread_stop) and the fix from Marcin for the oops when mount helper
missing and DFS enabled and no mount data.   The only other fixes in
cifs-2.6.git now are:

The following changes since commit cbff290491cd97bcd449b14f672d98992ddad5cb:
  Linus Torvalds (1):
        Merge branch 'merge' of git://git.kernel.org/.../paulus/powerpc

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6.git
..BRANCH.NOT.VERIFIED..

Jeff Layton (4):
      on non-posix shares, clear write bits in mode when ATTR_READONLY is set
      when creating new inodes, use file_mode/dir_mode exclusively on
mount without unix extensions
      silently ignore ownership changes unless unix extensions are
enabled or we're faking uid changes
      disable most mode changes on non-unix/non-cifsacl mounts

Steve French (6):
      [CIFS] remove unused variables
      [CIFS] remove trailing whitespace
      [CIFS] warn if both dynperm and cifsacl mount options specified
      [CIFS] Correct incorrect obscure open flag
      [CIFS] Fix hang in mount when negprot causes server to kill tcp session
      [CIFS] cifs: fix oops on mount when CONFIG_CIFS_DFS_UPCALL is enabled

 fs/cifs/CHANGES    |    5 ++
 fs/cifs/cifsfs.c   |   21 ++++----
 fs/cifs/cifsglob.h |    3 +-
 fs/cifs/cifspdu.h  |   23 ++++++--
 fs/cifs/cifssmb.c  |    6 +--
 fs/cifs/connect.c  |    5 ++
 fs/cifs/dir.c      |    4 +-
 fs/cifs/file.c     |    7 ---
 fs/cifs/inode.c    |  148 ++++++++++++++++++++++++++++++---------------------
 fs/cifs/misc.c     |    3 +-
 fs/cifs/readdir.c  |   77 ++++++++++++++-------------
 11 files changed, 172 insertions(+), 130 deletions(-)

I will request a merge to mainline from cifs-2.6.git when you have had
a chance to look at them and double check (since I want to make sure
that you still want this subset of the dynperm changes)

-- 
Thanks,

Steve