headers
Christoph Hellwig | 1 May 2008 18:29
Favicon

Re: [PATCH 1/3] [CIFS] don't allow demultiplex thread to exit until kthread_stop is called

Looks good.  Thanks for sorting out that messy thread handling.
Permalink | Reply | 1 comment |
headers
Christoph Hellwig | 1 May 2008 18:30
Favicon

Re: [PATCH 2/3] [CIFS] convert cifs_demultiplex_thread to use nonblocking sockets

On Thu, Apr 17, 2008 at 04:59:05PM -0400, Jeff Layton wrote:
> Currently, cifs_demultiplex_thread uses blocking calls to
> kernel_recvmsg to receive data. These calls do not return immediately
> when kthread_stop tries to wake the thread, so cifs_mount and
> cifs_umount also signal the thread to make it break out of kernel_recvmsg.
> 
> This patch converts cifs_demultiplex_thread to repeatedly check the
> socket with a TIOCINQ ioctl to see if there is data waiting. If there
> isn't then it sleeps a bit and polls again. If there is data then
> we call kernel_recvmsg in non-blocking mode to get it. Since we're
> polling like this, we can check kthread_should_stop on each poll and
> no longer need to signal when tearing down the thread.

Wouldn't it be better if we do real event driver non-blocking I/O,
like an poll or epoll loop in userspace.  I don't think this has
been done in kernelspace before, but I don't see any fundamental reason
why we can't do it.
Permalink | Reply | 1 comment |
headers
Christoph Hellwig | 1 May 2008 18:31
Favicon

Re: [PATCH 3/3] [CIFS] remove unneeded half-second sleep from cifs_umount

On Thu, Apr 17, 2008 at 04:59:06PM -0400, Jeff Layton wrote:
> I can't see any reason for this sleep, and it slows down umounts.

Looks odd to me aswell.  Maybe Steve has an idea?
Permalink | Reply | 1 comment |
headers
Jeff Layton | 1 May 2008 18:33
Picon
Favicon

Re: [PATCH 1/3] [CIFS] don't allow demultiplex thread to exit until kthread_stop is called

On Thu, 1 May 2008 12:29:26 -0400
Christoph Hellwig <hch <at> infradead.org> wrote:

> Looks good.  Thanks for sorting out that messy thread handling.
> 

Thanks Christoph,

I'll have a slight revision to this set in the near future. The basic
idea is still the same, but there are some small changes that need to
be made to make sure the thread always comes down on failed mount
attempts. It generally does now, but there could be corner cases
with some oddball servers.

Cheers,
--

-- 
Jeff Layton <jlayton <at> redhat.com>
Permalink | Reply | 0 comments |
headers
Jeff Layton | 1 May 2008 20:05
Picon
Favicon

Re: [PATCH 2/3] [CIFS] convert cifs_demultiplex_thread to use nonblocking sockets

On Thu, 1 May 2008 12:30:43 -0400
Christoph Hellwig <hch <at> infradead.org> wrote:

> On Thu, Apr 17, 2008 at 04:59:05PM -0400, Jeff Layton wrote:
> > Currently, cifs_demultiplex_thread uses blocking calls to
> > kernel_recvmsg to receive data. These calls do not return immediately
> > when kthread_stop tries to wake the thread, so cifs_mount and
> > cifs_umount also signal the thread to make it break out of kernel_recvmsg.
> > 
> > This patch converts cifs_demultiplex_thread to repeatedly check the
> > socket with a TIOCINQ ioctl to see if there is data waiting. If there
> > isn't then it sleeps a bit and polls again. If there is data then
> > we call kernel_recvmsg in non-blocking mode to get it. Since we're
> > polling like this, we can check kthread_should_stop on each poll and
> > no longer need to signal when tearing down the thread.
> 
> Wouldn't it be better if we do real event driver non-blocking I/O,
> like an poll or epoll loop in userspace. I don't think this has
> been done in kernelspace before, but I don't see any fundamental reason
> why we can't do it. 
> 

I had a quick look over what poll() does and this doesn't look
trivial. It sounds interesting though and is something we should
probably consider for the future.

I think it would probably be best to start with something like this
ioctl() polling since that is used elsewhere in kernel and we know it
works. Once we have that in place and feel good about it we can
consider converting that to use something similar to what poll() calls
(Continue reading)

Permalink | Reply | 0 comments |
headers
Jeff Layton | 1 May 2008 20:35
Picon
Favicon

Re: [PATCH 3/3] [CIFS] remove unneeded half-second sleep from cifs_umount

On Thu, 1 May 2008 12:31:06 -0400
Christoph Hellwig <hch <at> infradead.org> wrote:

> On Thu, Apr 17, 2008 at 04:59:06PM -0400, Jeff Layton wrote:
> > I can't see any reason for this sleep, and it slows down umounts.
> 
> Looks odd to me aswell.  Maybe Steve has an idea?
> 

The only thing I can figure is that it was intended to give cifsd time
to come down before freeing the cifsSesInfo struct. Sounds like a racy
proposition though :-)

Now that cifsd is kthread based, we know that it's down once
kthread_stop returns. If that was the original reason, then we
definitely don't need it anymore.

--

-- 
Jeff Layton <jlayton <at> redhat.com>
Permalink | Reply | 0 comments |
headers
Guenter Kukkukk | 2 May 2008 05:54

Re: SMB_COM_CLOSE and LastWriteTime

Am Freitag, 25. April 2008 schrieb Jeff Layton:
> On Fri, 25 Apr 2008 05:27:23 +0200
> Guenter Kukkukk <linux <at> kukkukk.com> wrote:
> 
> > Am Donnerstag, 24. April 2008 schrieb Jeff Layton:
> > > On Thu, 24 Apr 2008 17:26:58 +0200
> > > Guenter Kukkukk <linux <at> kukkukk.com> wrote:
> > > 
> > > > Am Donnerstag, 24. April 2008 schrieb Jeff Layton:
> > > > > On Thu, 24 Apr 2008 16:13:46 +0200
> > > > > Guenter Kukkukk <linux <at> kukkukk.com> wrote:
> > > > > 
> > > > > > Am Donnerstag, 24. April 2008 schrieb Guenter Kukkukk:
> > > > > > 
> > > > > > > An additional note:
> > > > > > > i get the following, when i use
> > > > > > > cp -p foo /mnt/linux/cifs
> > > > > > > cp: preserving permissions for `/mnt/linux/cifs/settime.c': Input/output error
> > > > > > > I must specify the "noacl" mount option to avoid this.
> > > > > > > Happens when mounting recent 3.0.x or 3.2 samba server.
> > > > > > 
> > > > > > Ouch - should read  
> > > > > > 
> > > > > > cp -p settime.c /mnt/linux/cifs
> > > > > > cp: preserving permissions for `/mnt/linux/cifs/settime.c': Input/output error
> > > > > > 
> > > > > > We can leave this one alone atm - could also be a wrong server setup.
> > > > > > Cheers, Günter
> > > > > 
> > > > >
(Continue reading)

Permalink | Reply | 1 comment |
headers
Jeff Layton | 2 May 2008 21:25
Picon
Favicon

[PATCH 3/4] [CIFS] remove locking around tcpSesAllocCount atomic variable

The global tcpSesAllocCount variable is an atomic already and doesn't
really need the extra locking around it. Remove the locking and just use
the atomic_inc_return and atomic_dec_return functions to make sure we
access it correctly.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/cifs/connect.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 2e156f6..4c93c2a 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
 <at>  <at>  -350,11 +350,9  <at>  <at>  cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	current->flags |= PF_MEMALLOC;
 	server->tsk = current;	/* save process info to wake at shutdown */
 	cFYI(1, ("Demultiplex PID: %d", task_pid_nr(current)));
-	write_lock(&GlobalSMBSeslock);
-	atomic_inc(&tcpSesAllocCount);
-	length = tcpSesAllocCount.counter;
-	write_unlock(&GlobalSMBSeslock);
-	if (length  > 1)
+
+	length = atomic_inc_return(&tcpSesAllocCount);
+	if (length > 1)
 		mempool_resize(cifs_req_poolp, length + cifs_min_rcv,
 				GFP_KERNEL);

 <at>  <at>  -733,14 +731,11  <at>  <at>  multi_t2_fnd:
(Continue reading)

Permalink | Reply | 6 comments |
headers
Jeff Layton | 2 May 2008 21:25
Picon
Favicon

[PATCH 2/4] [CIFS] remove cifsd_complete completion variable

I think this was a holdover from the old kernel_thread based cifsd
code. We needed to know that the thread had set the task variable
before proceeding. Now that kthread_run returns the new task, this
doesn't appear to be needed anymore.

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
---
 fs/cifs/connect.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f4a668b..2e156f6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
 <at>  <at>  -49,8 +49,6  <at>  <at> 
 #define CIFS_PORT 445
 #define RFC1001_PORT 139

-static DECLARE_COMPLETION(cifsd_complete);
-
 extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
 			 unsigned char *p24);

 <at>  <at>  -356,7 +354,6  <at>  <at>  cifs_demultiplex_thread(struct TCP_Server_Info *server)
 	atomic_inc(&tcpSesAllocCount);
 	length = tcpSesAllocCount.counter;
 	write_unlock(&GlobalSMBSeslock);
-	complete(&cifsd_complete);
 	if (length  > 1)
 		mempool_resize(cifs_req_poolp, length + cifs_min_rcv,
(Continue reading)

Permalink | Reply | 8 comments |
headers
Jeff Layton | 2 May 2008 21:25
Picon
Favicon

[PATCH 0/4] [CIFS] clean up cifsd thread startup and shutdown

Hi Steve,

Shirish ran across a problem when testing a RHEL4.7 beta kernel where
kthread_stop was called on a cifsd that had already exited. Looking at
the code, it looks like this is actually a problem in mainline. There is
no guarantee that cifsd will actually be up when we call kthread_stop on
it.

This patchset is intended to try and close this race and also to do some
other small cleanups to the startup/shutdown of cifs_demultiplex_thread.
For now, I've dropped the patch that makes cifsd use non-blocking I/O.
It works and I still would like to see us do it, but I think we should
probably treat that piece independently of the rest of this set.

While this is being sent as a set, the patches are actually relatively
independent of one another. The first 3 patches are cleanup patches.
The 4th patch is what actually closes the race.

I've tested this primarily by running iozone tests on it with the -U
option to mount and unmount the filesystems between each test, and
it seems to be OK.

As always, comments and suggestions are appreciated...

Signed-off-by: Jeff Layton <jlayton <at> redhat.com>
Permalink | Reply | 11 comments |

Gmane