Re: [PATCH] CIFS: make cifsd (more)
[ 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=117552761703381=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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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=117552761703381=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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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-clientm=117552761703381w=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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
[ 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-clientm=117552761703381w=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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] CIFS: make cifsd (more)
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-cifs-client] Re: [PATCH] CIFS: make cifsd (more) signal-safe
On Wed, 6 Jun 2007 09:55:50 +0100
Christoph Hellwig <[EMAIL PROTECTED]> wrote:
> On Tue, Jun 05, 2007 at 03:23:40PM -0400, Jeff Layton wrote:
> > I recently sent a similar, smaller patch for this problem. After some
> > discussion with Steve and Shaggy, I think I better understand why cifsd
> > allows signals through, and I realize that my earlier patch wasn't
> > comprehensive enough
> >
> > The mount and unmount calls will send a KILL signal to cifsd to wake it
> > up if it happens to be blocked in kernel_recvmsg. The problem is that
> > it doesn't distinguish between "legitimate" signals sent for this
> > reason and spurious signals sent by a userspace process (for instance).
> > While this is definitely a "don't do that" sort of situation, we might
> > as well try to have cifsd be as signal-safe as possible.
> >
> > The following patch does this by making sure that we set tcpStatus to
> > CifsExiting before sending cifsd a signal, and having cifsd check for
> > that when it sees that it's been signalled. If the tcpStatus is not set
> > correctly, it ignores it, flushes signals and moves on.
> >
> > I've tested a similar backported version of this on an earlier kernel,
> > but have not tested this particular patch as of yet.
>
> The right way to fix this is to stop sending signals at all and have
> a kernel-internal way to get out of kernel_recvmsg. Uses of signals by
> kernel thread generally are bugs.
>
I've looked at different ways to do this and haven't seen a clean way
to do this. I made an initial stab at having tcp_recvmsg check
kthread_stop and break out of the loop if it sees it. Herbert Xu stated
that he didn't think that was right -- after all, why should
tcp_recvmsg care about khthreads?
As I see it we're left with using signals, or setting up some other
signal-type infrastructure that's just available in the kernel. That
seems redundant to me, and I'm not clear as to why use of signals by
kthreads is a bad thing.
So, here's a second attempt at this. This changes cifsd to ignore all
signals and changes the cifs mount/umount code to use force_sig()
instead of send_sig(). I don't think this is any worse than what we're
doing today and it insulates cifsd from signals sent from userspace.
This should apply to the current cifs-2.6 git tree.
Seem reasonable?
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;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-cifs-client] Re: [PATCH] CIFS: make cifsd (more) signal-safe
On Wed, 6 Jun 2007 09:55:50 +0100
Christoph Hellwig [EMAIL PROTECTED] wrote:
On Tue, Jun 05, 2007 at 03:23:40PM -0400, Jeff Layton wrote:
I recently sent a similar, smaller patch for this problem. After some
discussion with Steve and Shaggy, I think I better understand why cifsd
allows signals through, and I realize that my earlier patch wasn't
comprehensive enough
The mount and unmount calls will send a KILL signal to cifsd to wake it
up if it happens to be blocked in kernel_recvmsg. The problem is that
it doesn't distinguish between legitimate signals sent for this
reason and spurious signals sent by a userspace process (for instance).
While this is definitely a don't do that sort of situation, we might
as well try to have cifsd be as signal-safe as possible.
The following patch does this by making sure that we set tcpStatus to
CifsExiting before sending cifsd a signal, and having cifsd check for
that when it sees that it's been signalled. If the tcpStatus is not set
correctly, it ignores it, flushes signals and moves on.
I've tested a similar backported version of this on an earlier kernel,
but have not tested this particular patch as of yet.
The right way to fix this is to stop sending signals at all and have
a kernel-internal way to get out of kernel_recvmsg. Uses of signals by
kernel thread generally are bugs.
I've looked at different ways to do this and haven't seen a clean way
to do this. I made an initial stab at having tcp_recvmsg check
kthread_stop and break out of the loop if it sees it. Herbert Xu stated
that he didn't think that was right -- after all, why should
tcp_recvmsg care about khthreads?
As I see it we're left with using signals, or setting up some other
signal-type infrastructure that's just available in the kernel. That
seems redundant to me, and I'm not clear as to why use of signals by
kthreads is a bad thing.
So, here's a second attempt at this. This changes cifsd to ignore all
signals and changes the cifs mount/umount code to use force_sig()
instead of send_sig(). I don't think this is any worse than what we're
doing today and it insulates cifsd from signals sent from userspace.
This should apply to the current cifs-2.6 git tree.
Seem reasonable?
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;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
