Jeff,
Not seeing any objections to your revised approach (to not allowing
signals for cifsd kernel thread), I just merged something similar to
your patch to the cifs-2.6.git tree (also fixed some nearby lines that
went past 80 columns).
Thanks
Signed-off-by: Jeff Layton <[email protected]>
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index f4e9266..27c1ebe 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -348,7 +348,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
int isMultiRsp;
int reconnect;
- allow_signal(SIGKILL);
current->flags |= PF_MEMALLOC;
server->tsk = current; /* save process info to wake at shutdown */
cFYI(1, ("Demultiplex PID: %d", current->pid));
@@ -2074,7 +2073,7 @@ cifs_mount(struct super_block *sb, struct
cifs_sb_info *cifs_sb,
always wake up processes blocked in
tcp in recv_mesg then we could remove the
send_sig call */
- send_sig(SIGKILL,srvTcp->tsk,1);
+ force_sig(SIGKILL,srvTcp->tsk);
tsk = srvTcp->tsk;
if(tsk)
kthread_stop(tsk);
@@ -2093,7 +2092,7 @@ 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;
- send_sig(SIGKILL,pSesInfo->server->tsk,1);
+ force_sig(SIGKILL,pSesInfo->server->tsk);
tsk = pSesInfo->server->tsk;
if (tsk)
kthread_stop(tsk);
@@ -3345,7 +3344,7 @@ cifs_umount(struct super_block *sb, struct
cifs_sb_info *cifs_sb)
} else if (rc == -ESHUTDOWN) {
cFYI(1,("Waking up socket by sending it signal"));
if (cifsd_task) {
- send_sig(SIGKILL,cifsd_task,1);
+ force_sig(SIGKILL,cifsd_task);
kthread_stop(cifsd_task);
}
rc = 0;
--
Thanks,
Steve
On Mon, Jun 25, 2007 at 05:25:00PM -0500, Steve French wrote:
> Jeff,
> Not seeing any objections to your revised approach (to not allowing
> signals for cifsd kernel thread), I just merged something similar to
> your patch to the cifs-2.6.git tree (also fixed some nearby lines that
> went past 80 columns).
Yes, I still object to it. I'll come back to the implementation discussion
once I get a little more time.
On Mon, Jun 25, 2007 at 05:25:00PM -0500, Steve French wrote:
> Jeff,
> Not seeing any objections to your revised approach (to not allowing
> signals for cifsd kernel thread), I just merged something similar to
> your patch to the cifs-2.6.git tree (also fixed some nearby lines that
> went past 80 columns).
Ok, I'm back to this.
As I said mixing force_sig with the kthread infrastructure is a bad idea.
The proper short-term (aka 2.6.22) fix is to revert the kthread conversion
for this particular thread. Just go back to what worked before.
Now the right fix is a lot more complicated and involved:
Stop using blocking recvmsg (or read) in kernel threads!
If you look at what the other consumers of networking reads from kernel
threads do is they either use tcp_read_sock and hooks into the sk_ callbacks
which would be nice for high performance reads in cifs aswell, but probably
not the demultiplexer thread, or they use MSG_DONTWAIT to avoid this problems
and deal with the blocking behaviour on a higher level.
On Sat, 30 Jun 2007 09:42:09 +0100
Christoph Hellwig <[email protected]> wrote:
> On Mon, Jun 25, 2007 at 05:25:00PM -0500, Steve French wrote:
> > Jeff,
> > Not seeing any objections to your revised approach (to not allowing
> > signals for cifsd kernel thread), I just merged something similar to
> > your patch to the cifs-2.6.git tree (also fixed some nearby lines that
> > went past 80 columns).
>
> Ok, I'm back to this.
>
> As I said mixing force_sig with the kthread infrastructure is a bad idea.
> The proper short-term (aka 2.6.22) fix is to revert the kthread conversion
> for this particular thread. Just go back to what worked before.
Could you clarify why this is? It looks like kthreads and signalling
should be more or less orthogonal. Or is it just an issue of the
complexity added when you mix signalling into kthreads?
Note that the problem of insulation from userspace signals predates the
conversion to using the kthreads interface for cifsd. So even if we
revert the switch of the demultiplexer thread to kthreads in the near
term, I'd like to keep the recent change to block all signals from
userspace and use force_sig in lieu of send_sig.
Does that sound reasonable?
>
> Now the right fix is a lot more complicated and involved:
>
> Stop using blocking recvmsg (or read) in kernel threads!
>
> If you look at what the other consumers of networking reads from kernel
> threads do is they either use tcp_read_sock and hooks into the sk_ callbacks
> which would be nice for high performance reads in cifs aswell, but probably
> not the demultiplexer thread, or they use MSG_DONTWAIT to avoid this problems
> and deal with the blocking behaviour on a higher level.
--
Jeff Layton <[email protected]>
The reason that cifs switched from wait_for_completion to the kthread
call to cifs_demultiplex_thread in the first place is because without
use of kthread it won't work with a linux-vserver. See the thread:
http://marc.info/?l=linux-cifs-client&m=117552761703381&w=2
If we take out the kthread call, we break those guys.
I agree that using sk_callbacks is worth looking into - I only found
ocfs2 and SunRPC (NFS) though that used it. Is there a better
example though? The NFS socket handling code is huge
(net/sunrpc/xprtsck.c) - something seems wrong when replacing a few
lines of code with a new 1675 line file. There must be a better
example of doing what you suggest...
I am tempted to drop the socket timeout (which cifs sets to 7 seconds)
to a smaller number and not use signals at all rather than add that
much complexity
On 6/30/07, Jeff Layton <[email protected]> wrote:
> On Sat, 30 Jun 2007 09:42:09 +0100
> Christoph Hellwig <[email protected]> wrote:
>
> > On Mon, Jun 25, 2007 at 05:25:00PM -0500, Steve French wrote:
> > > Jeff,
> > > Not seeing any objections to your revised approach (to not allowing
> > > signals for cifsd kernel thread), I just merged something similar to
> > > your patch to the cifs-2.6.git tree (also fixed some nearby lines that
> > > went past 80 columns).
> >
> > Ok, I'm back to this.
> >
> > As I said mixing force_sig with the kthread infrastructure is a bad idea.
> > The proper short-term (aka 2.6.22) fix is to revert the kthread conversion
> > for this particular thread. Just go back to what worked before.
>
> Could you clarify why this is? It looks like kthreads and signalling
> should be more or less orthogonal. Or is it just an issue of the
> complexity added when you mix signalling into kthreads?
>
> Note that the problem of insulation from userspace signals predates the
> conversion to using the kthreads interface for cifsd. So even if we
> revert the switch of the demultiplexer thread to kthreads in the near
> term, I'd like to keep the recent change to block all signals from
> userspace and use force_sig in lieu of send_sig.
>
> Does that sound reasonable?
>
> >
> > Now the right fix is a lot more complicated and involved:
> >
> > Stop using blocking recvmsg (or read) in kernel threads!
> >
> > If you look at what the other consumers of networking reads from kernel
> > threads do is they either use tcp_read_sock and hooks into the sk_ callbacks
> > which would be nice for high performance reads in cifs aswell, but probably
> > not the demultiplexer thread, or they use MSG_DONTWAIT to avoid this problems
> > and deal with the blocking behaviour on a higher level.
>
> --
> Jeff Layton <[email protected]>
>
--
Thanks,
Steve
[ Trimmed Cc: list ]
On 6/30/07, Steve French <[email protected]> wrote:
> The reason that cifs switched from wait_for_completion to the kthread
> call to cifs_demultiplex_thread in the first place is because without
> use of kthread it won't work with a linux-vserver. See the thread:
>
> http://marc.info/?l=linux-cifs-client&m=117552761703381&w=2
>
> If we take out the kthread call, we break those guys.
>
> I agree that using sk_callbacks is worth looking into - I only found
> ocfs2 and SunRPC (NFS) though that used it. Is there a better
> example though? The NFS socket handling code is huge
> (net/sunrpc/xprtsck.c) - something seems wrong when replacing a few
> lines of code with a new 1675 line file. There must be a better
> example of doing what you suggest...
You're correct. "Right" / "elegant" solutions are rarely (if ever?) complex
and involved. Simplicity _is_ good. I see no point in converting 5 good
lines of maintainable, readable, solid code with 1000 lines of kludge :-)
just to work-around this kthreads limitation. But then, of course, the call
is yours.
> I am tempted to drop the socket timeout (which cifs sets to 7 seconds)
> to a smaller number and not use signals at all rather than add that
> much complexity
Timeout too low => CPU wastage => power wastage. [ Think laptop
batteries, with say 5 cifsd kthreads waking up once every second ... ]
Timeout too high => umount(2) hangs, annoys user, user takes
drastic actions ... so think of some good "magic number" :-)
I don't quite think of all these suggestions as solutions at all -- they
are workarounds at best, IMHO (for kthread's limitation in dealing with
kernel threads that want to block -- I still don't see any fundamental
reasoning / logic behind why kthreads should be banned from doing
blocking recv's -- if there is, please let me know too).
Don't have much else to say than what I already have on the two
threads discussing this.
Satyam