Re: [PATCH] nfs lockd reclaimer: Convert to kthread API

2007-04-21 Thread Eric W. Biederman
Trond Myklebust <[EMAIL PROTECTED]> writes:

> On Thu, 2007-04-19 at 14:40 -0700, Andrew Morton wrote:
>> Using signals to communicate with kernel threads is fairly unpleasant, IMO.
>> We have much simpler, faster and more idiomatic ways of communicating
>> between threads in-kernel and there are better ways in which userspace can
>> communicate with the kernel - system calls, for example...
>> 
>> So I think generally any move which gets us away from using signals in
>> kernel threads is moving in a good direction.
>
> I have yet to see a proposal which did. Eric's patch was eliminating
> signals in kernel threads that used them without proposing any
> replacement mechanism or showing that he had plans to do so. That is a
> good reason for a veto.

Possibly I just hadn't looked close enough.  The signals looked like
a redundant mechanism.

>> > > With pid namespaces all kernel threads will disappear so how do
>> > > we cope with the problem when the sysadmin can not see the kernel
>> > > threads?
>> > 
>> > Then you have a usability problem. How does the sysadmin reboot the
>> > system if there is no way to shut down the processes that are hanging on
>> > an unresponsive filesystem?
>> 
>> Where's the hang?  A user process is stuck on h_rwsem?
>> 
>> If so, would it be appropriate to convert the user process to use
>> down_foo_interruptible(), so that the operator can just kill the user
>> process as expected, rather than having to futz around killing kernel
>> threads?
>
> If an NFS server reboots, then the locks held by user processes on the
> client need to be re-established by when it comes up again. Otherwise,
> the processes that thought they were holding locks will suddenly fail.
> This recovery job is currently the done by a kernel thread.
>
> The question is then what to do if the server crashes again while the
> kernel thread is re-establishing the locks. Particularly if it never
> comes back again.
> Currently, the administrator can intervene by killing anything that has
> open files on that volume and kill the recovery kernel thread.
> You'll also note that lockd_down(), nfsd_down() etc all use signals to
> inform lockd(), nfsd() etc that they should be shutting down. Since the
> reclaimer thread is started by the lockd() thread using CLONE_SIGHAND,
> this means that we also automatically kill any lingering recovery
> threads whenever we shutdown lockd().

Maybe I'm missing something but I think you are referring to the semantics
of do_group_exit in the presence of CLONE_THREAD.  All sharing a
sighand should do is cause the sharing of the signal handler.  Causing
allow_signal and disallow_signal to act on a group of threads instead
of a single thread.   I don't recall clone_sighand having any
other effects.

> These mechanisms need to be replaced _before_ we start shooting down
> sigallow() etc in the kernel.

Reasonable if these mechanisms are not redundant.

Thinking it through because everything having to do with nfs mounting and
unmounting is behind the privileged mount operation this is not going to
become an issue until we start allowing unprivileged nfs mounts.  Because
we cannot delegate control of nfs mount and unmount operations until then.

Since signals do not pose a immediate barrier to forward progress like
daemonize and kernel_thread we can leave things as is until we can
sort this out.

Eric
-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-21 Thread Eric W. Biederman
Dave Hansen <[EMAIL PROTECTED]> writes:

> On Thu, 2007-04-19 at 17:19 -0400, Trond Myklebust wrote:
>> > With pid namespaces all kernel threads will disappear so how do
>> > we cope with the problem when the sysadmin can not see the kernel
>> > threads?
>
> Do they actually always disappear, or do we keep them in the
> init_pid_namespace?

In the init pid namespace but not in any of it's children.

Eric
-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-21 Thread Eric W. Biederman
Dave Hansen [EMAIL PROTECTED] writes:

 On Thu, 2007-04-19 at 17:19 -0400, Trond Myklebust wrote:
  With pid namespaces all kernel threads will disappear so how do
  we cope with the problem when the sysadmin can not see the kernel
  threads?

 Do they actually always disappear, or do we keep them in the
 init_pid_namespace?

In the init pid namespace but not in any of it's children.

Eric
-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-21 Thread Eric W. Biederman
Trond Myklebust [EMAIL PROTECTED] writes:

 On Thu, 2007-04-19 at 14:40 -0700, Andrew Morton wrote:
 Using signals to communicate with kernel threads is fairly unpleasant, IMO.
 We have much simpler, faster and more idiomatic ways of communicating
 between threads in-kernel and there are better ways in which userspace can
 communicate with the kernel - system calls, for example...
 
 So I think generally any move which gets us away from using signals in
 kernel threads is moving in a good direction.

 I have yet to see a proposal which did. Eric's patch was eliminating
 signals in kernel threads that used them without proposing any
 replacement mechanism or showing that he had plans to do so. That is a
 good reason for a veto.

Possibly I just hadn't looked close enough.  The signals looked like
a redundant mechanism.

   With pid namespaces all kernel threads will disappear so how do
   we cope with the problem when the sysadmin can not see the kernel
   threads?
  
  Then you have a usability problem. How does the sysadmin reboot the
  system if there is no way to shut down the processes that are hanging on
  an unresponsive filesystem?
 
 Where's the hang?  A user process is stuck on h_rwsem?
 
 If so, would it be appropriate to convert the user process to use
 down_foo_interruptible(), so that the operator can just kill the user
 process as expected, rather than having to futz around killing kernel
 threads?

 If an NFS server reboots, then the locks held by user processes on the
 client need to be re-established by when it comes up again. Otherwise,
 the processes that thought they were holding locks will suddenly fail.
 This recovery job is currently the done by a kernel thread.

 The question is then what to do if the server crashes again while the
 kernel thread is re-establishing the locks. Particularly if it never
 comes back again.
 Currently, the administrator can intervene by killing anything that has
 open files on that volume and kill the recovery kernel thread.
 You'll also note that lockd_down(), nfsd_down() etc all use signals to
 inform lockd(), nfsd() etc that they should be shutting down. Since the
 reclaimer thread is started by the lockd() thread using CLONE_SIGHAND,
 this means that we also automatically kill any lingering recovery
 threads whenever we shutdown lockd().

Maybe I'm missing something but I think you are referring to the semantics
of do_group_exit in the presence of CLONE_THREAD.  All sharing a
sighand should do is cause the sharing of the signal handler.  Causing
allow_signal and disallow_signal to act on a group of threads instead
of a single thread.   I don't recall clone_sighand having any
other effects.

 These mechanisms need to be replaced _before_ we start shooting down
 sigallow() etc in the kernel.

Reasonable if these mechanisms are not redundant.

Thinking it through because everything having to do with nfs mounting and
unmounting is behind the privileged mount operation this is not going to
become an issue until we start allowing unprivileged nfs mounts.  Because
we cannot delegate control of nfs mount and unmount operations until then.

Since signals do not pose a immediate barrier to forward progress like
daemonize and kernel_thread we can leave things as is until we can
sort this out.

Eric
-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Trond Myklebust
On Thu, 2007-04-19 at 14:40 -0700, Andrew Morton wrote:
> Using signals to communicate with kernel threads is fairly unpleasant, IMO.
> We have much simpler, faster and more idiomatic ways of communicating
> between threads in-kernel and there are better ways in which userspace can
> communicate with the kernel - system calls, for example...
> 
> So I think generally any move which gets us away from using signals in
> kernel threads is moving in a good direction.

I have yet to see a proposal which did. Eric's patch was eliminating
signals in kernel threads that used them without proposing any
replacement mechanism or showing that he had plans to do so. That is a
good reason for a veto.

> > > With pid namespaces all kernel threads will disappear so how do
> > > we cope with the problem when the sysadmin can not see the kernel
> > > threads?
> > 
> > Then you have a usability problem. How does the sysadmin reboot the
> > system if there is no way to shut down the processes that are hanging on
> > an unresponsive filesystem?
> 
> Where's the hang?  A user process is stuck on h_rwsem?
> 
> If so, would it be appropriate to convert the user process to use
> down_foo_interruptible(), so that the operator can just kill the user
> process as expected, rather than having to futz around killing kernel
> threads?

If an NFS server reboots, then the locks held by user processes on the
client need to be re-established by when it comes up again. Otherwise,
the processes that thought they were holding locks will suddenly fail.
This recovery job is currently the done by a kernel thread.

The question is then what to do if the server crashes again while the
kernel thread is re-establishing the locks. Particularly if it never
comes back again.
Currently, the administrator can intervene by killing anything that has
open files on that volume and kill the recovery kernel thread.
You'll also note that lockd_down(), nfsd_down() etc all use signals to
inform lockd(), nfsd() etc that they should be shutting down. Since the
reclaimer thread is started by the lockd() thread using CLONE_SIGHAND,
this means that we also automatically kill any lingering recovery
threads whenever we shutdown lockd().

These mechanisms need to be replaced _before_ we start shooting down
sigallow() etc in the kernel.

  Trond

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Andrew Morton
On Thu, 19 Apr 2007 17:19:24 -0400
Trond Myklebust <[EMAIL PROTECTED]> wrote:

> > Regardless kernel threads should be an implementation detail
> > not a part of the user interface.  If kernel threads are part
> > of the user interface it makes them very hard to change.
> > 
> > So it isn't that it doesn't make sense to me it is that it looks
> > fundamentally broken and like a maintenance nightmare.
> > 
> > I would rather kill kernel threads then try and simulate them
> > when the kernel implementation has changed and kernel threads
> > are not visible.
> > 
> > If I could be convinced that signal handling in kernel threads
> > is not something that will impede code modifications and refactoring
> > I would have less of a problem, and might not care.
> 
> Tough. You're the one proposing to change existing code.

Using signals to communicate with kernel threads is fairly unpleasant, IMO.
We have much simpler, faster and more idiomatic ways of communicating
between threads in-kernel and there are better ways in which userspace can
communicate with the kernel - system calls, for example...

So I think generally any move which gets us away from using signals in
kernel threads is moving in a good direction.

> > With pid namespaces all kernel threads will disappear so how do
> > we cope with the problem when the sysadmin can not see the kernel
> > threads?
> 
> Then you have a usability problem. How does the sysadmin reboot the
> system if there is no way to shut down the processes that are hanging on
> an unresponsive filesystem?

Where's the hang?  A user process is stuck on h_rwsem?

If so, would it be appropriate to convert the user process to use
down_foo_interruptible(), so that the operator can just kill the user
process as expected, rather than having to futz around killing kernel
threads?

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Dave Hansen
On Thu, 2007-04-19 at 17:19 -0400, Trond Myklebust wrote:
> > With pid namespaces all kernel threads will disappear so how do
> > we cope with the problem when the sysadmin can not see the kernel
> > threads?

Do they actually always disappear, or do we keep them in the
init_pid_namespace?

-- Dave

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Trond Myklebust
On Thu, 2007-04-19 at 13:20 -0600, Eric W. Biederman wrote:
> Trond Myklebust <[EMAIL PROTECTED]> writes:
> 
> > On Thu, 2007-04-19 at 01:58 -0600, Eric W. Biederman wrote:
> >> From: Eric W. Biederman <[EMAIL PROTECTED]>
> >> 
> >> Start the reclaimer thread using kthread_run instead
> >> of a combination of kernel_thread and daemonize.
> >> The small amount of signal handling code is also removed
> >> as it makes no sense and is a maintenance problem to handle
> >> signals in kernel threads.
> >
> > Vetoed. Removing stuff just because it doesn't make sense to you is not
> > acceptable.
> >
> > Signal handling in reclaimer threads is there in order to allow
> > administrators to deal with the case where the server never comes up
> > again.
> 
> Doesn't unmount handle that?

On a pinned filesystem?

> Regardless kernel threads should be an implementation detail
> not a part of the user interface.  If kernel threads are part
> of the user interface it makes them very hard to change.
> 
> So it isn't that it doesn't make sense to me it is that it looks
> fundamentally broken and like a maintenance nightmare.
> 
> I would rather kill kernel threads then try and simulate them
> when the kernel implementation has changed and kernel threads
> are not visible.
> 
> If I could be convinced that signal handling in kernel threads
> is not something that will impede code modifications and refactoring
> I would have less of a problem, and might not care.

Tough. You're the one proposing to change existing code.

> With pid namespaces all kernel threads will disappear so how do
> we cope with the problem when the sysadmin can not see the kernel
> threads?

Then you have a usability problem. How does the sysadmin reboot the
system if there is no way to shut down the processes that are hanging on
an unresponsive filesystem?

Trond

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Eric W. Biederman
Trond Myklebust <[EMAIL PROTECTED]> writes:

> On Thu, 2007-04-19 at 01:58 -0600, Eric W. Biederman wrote:
>> From: Eric W. Biederman <[EMAIL PROTECTED]>
>> 
>> Start the reclaimer thread using kthread_run instead
>> of a combination of kernel_thread and daemonize.
>> The small amount of signal handling code is also removed
>> as it makes no sense and is a maintenance problem to handle
>> signals in kernel threads.
>
> Vetoed. Removing stuff just because it doesn't make sense to you is not
> acceptable.
>
> Signal handling in reclaimer threads is there in order to allow
> administrators to deal with the case where the server never comes up
> again.

Doesn't unmount handle that?

Regardless kernel threads should be an implementation detail
not a part of the user interface.  If kernel threads are part
of the user interface it makes them very hard to change.

So it isn't that it doesn't make sense to me it is that it looks
fundamentally broken and like a maintenance nightmare.

I would rather kill kernel threads then try and simulate them
when the kernel implementation has changed and kernel threads
are not visible.

If I could be convinced that signal handling in kernel threads
is not something that will impede code modifications and refactoring
I would have less of a problem, and might not care.

With pid namespaces all kernel threads will disappear so how do
we cope with the problem when the sysadmin can not see the kernel
threads?

Eric
-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Trond Myklebust
On Thu, 2007-04-19 at 01:58 -0600, Eric W. Biederman wrote:
> From: Eric W. Biederman <[EMAIL PROTECTED]>
> 
> Start the reclaimer thread using kthread_run instead
> of a combination of kernel_thread and daemonize.
> The small amount of signal handling code is also removed
> as it makes no sense and is a maintenance problem to handle
> signals in kernel threads.

Vetoed. Removing stuff just because it doesn't make sense to you is not
acceptable.

Signal handling in reclaimer threads is there in order to allow
administrators to deal with the case where the server never comes up
again.

Trond

-
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/


[PATCH] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Eric W. Biederman
From: Eric W. Biederman <[EMAIL PROTECTED]>

Start the reclaimer thread using kthread_run instead
of a combination of kernel_thread and daemonize.
The small amount of signal handling code is also removed
as it makes no sense and is a maintenance problem to handle
signals in kernel threads.

Cc: Neil Brown <[EMAIL PROTECTED]>
Cc: Trond Myklebust <[EMAIL PROTECTED]>
Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
---
 fs/lockd/clntlock.c |8 ++--
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index f4d45d4..83591f6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -153,7 +154,7 @@ nlmclnt_recovery(struct nlm_host *host)
if (!host->h_reclaiming++) {
nlm_get_host(host);
__module_get(THIS_MODULE);
-   if (kernel_thread(reclaimer, host, CLONE_KERNEL) < 0)
+   if (IS_ERR(kthread_run(reclaimer, host, "%s-reclaim", 
host->h_name)))
module_put(THIS_MODULE);
}
 }
@@ -166,9 +167,6 @@ reclaimer(void *ptr)
struct file_lock *fl, *next;
u32 nsmstate;
 
-   daemonize("%s-reclaim", host->h_name);
-   allow_signal(SIGKILL);
-
down_write(>h_rwsem);
 
/* This one ensures that our parent doesn't terminate while the
@@ -193,8 +191,6 @@ restart:
list_del_init(>fl_u.nfs_fl.list);
 
/* Why are we leaking memory here? --okir */
-   if (signalled())
-   continue;
if (nlmclnt_reclaim(host, fl) != 0)
continue;
list_add_tail(>fl_u.nfs_fl.list, >h_granted);
-- 
1.5.0.g53756

-
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/


[PATCH] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Eric W. Biederman
From: Eric W. Biederman <[EMAIL PROTECTED]> - unquoted

Start the reclaimer thread using kthread_run instead
of a combination of kernel_thread and daemonize.
The small amount of signal handling code is also removed
as it makes no sense and is a maintenance problem to handle
signals in kernel threads.

Cc: Neil Brown <[EMAIL PROTECTED]>
Cc: Trond Myklebust <[EMAIL PROTECTED]>
Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
---
 fs/lockd/clntlock.c |8 ++--
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index f4d45d4..83591f6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -153,7 +154,7 @@ nlmclnt_recovery(struct nlm_host *host)
if (!host->h_reclaiming++) {
nlm_get_host(host);
__module_get(THIS_MODULE);
-   if (kernel_thread(reclaimer, host, CLONE_KERNEL) < 0)
+   if (IS_ERR(kthread_run(reclaimer, host, "%s-reclaim", 
host->h_name)))
module_put(THIS_MODULE);
}
 }
@@ -166,9 +167,6 @@ reclaimer(void *ptr)
struct file_lock *fl, *next;
u32 nsmstate;
 
-   daemonize("%s-reclaim", host->h_name);
-   allow_signal(SIGKILL);
-
down_write(>h_rwsem);
 
/* This one ensures that our parent doesn't terminate while the
@@ -193,8 +191,6 @@ restart:
list_del_init(>fl_u.nfs_fl.list);
 
/* Why are we leaking memory here? --okir */
-   if (signalled())
-   continue;
if (nlmclnt_reclaim(host, fl) != 0)
continue;
list_add_tail(>fl_u.nfs_fl.list, >h_granted);
-- 
1.5.0.g53756

-
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/


[PATCH] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Eric W. Biederman
From: Eric W. Biederman [EMAIL PROTECTED] - unquoted

Start the reclaimer thread using kthread_run instead
of a combination of kernel_thread and daemonize.
The small amount of signal handling code is also removed
as it makes no sense and is a maintenance problem to handle
signals in kernel threads.

Cc: Neil Brown [EMAIL PROTECTED]
Cc: Trond Myklebust [EMAIL PROTECTED]
Signed-off-by: Eric W. Biederman [EMAIL PROTECTED]
---
 fs/lockd/clntlock.c |8 ++--
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index f4d45d4..83591f6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -9,6 +9,7 @@
 #include linux/module.h
 #include linux/types.h
 #include linux/time.h
+#include linux/kthread.h
 #include linux/nfs_fs.h
 #include linux/sunrpc/clnt.h
 #include linux/sunrpc/svc.h
@@ -153,7 +154,7 @@ nlmclnt_recovery(struct nlm_host *host)
if (!host-h_reclaiming++) {
nlm_get_host(host);
__module_get(THIS_MODULE);
-   if (kernel_thread(reclaimer, host, CLONE_KERNEL)  0)
+   if (IS_ERR(kthread_run(reclaimer, host, %s-reclaim, 
host-h_name)))
module_put(THIS_MODULE);
}
 }
@@ -166,9 +167,6 @@ reclaimer(void *ptr)
struct file_lock *fl, *next;
u32 nsmstate;
 
-   daemonize(%s-reclaim, host-h_name);
-   allow_signal(SIGKILL);
-
down_write(host-h_rwsem);
 
/* This one ensures that our parent doesn't terminate while the
@@ -193,8 +191,6 @@ restart:
list_del_init(fl-fl_u.nfs_fl.list);
 
/* Why are we leaking memory here? --okir */
-   if (signalled())
-   continue;
if (nlmclnt_reclaim(host, fl) != 0)
continue;
list_add_tail(fl-fl_u.nfs_fl.list, host-h_granted);
-- 
1.5.0.g53756

-
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/


[PATCH] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Eric W. Biederman
From: Eric W. Biederman [EMAIL PROTECTED]

Start the reclaimer thread using kthread_run instead
of a combination of kernel_thread and daemonize.
The small amount of signal handling code is also removed
as it makes no sense and is a maintenance problem to handle
signals in kernel threads.

Cc: Neil Brown [EMAIL PROTECTED]
Cc: Trond Myklebust [EMAIL PROTECTED]
Signed-off-by: Eric W. Biederman [EMAIL PROTECTED]
---
 fs/lockd/clntlock.c |8 ++--
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index f4d45d4..83591f6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -9,6 +9,7 @@
 #include linux/module.h
 #include linux/types.h
 #include linux/time.h
+#include linux/kthread.h
 #include linux/nfs_fs.h
 #include linux/sunrpc/clnt.h
 #include linux/sunrpc/svc.h
@@ -153,7 +154,7 @@ nlmclnt_recovery(struct nlm_host *host)
if (!host-h_reclaiming++) {
nlm_get_host(host);
__module_get(THIS_MODULE);
-   if (kernel_thread(reclaimer, host, CLONE_KERNEL)  0)
+   if (IS_ERR(kthread_run(reclaimer, host, %s-reclaim, 
host-h_name)))
module_put(THIS_MODULE);
}
 }
@@ -166,9 +167,6 @@ reclaimer(void *ptr)
struct file_lock *fl, *next;
u32 nsmstate;
 
-   daemonize(%s-reclaim, host-h_name);
-   allow_signal(SIGKILL);
-
down_write(host-h_rwsem);
 
/* This one ensures that our parent doesn't terminate while the
@@ -193,8 +191,6 @@ restart:
list_del_init(fl-fl_u.nfs_fl.list);
 
/* Why are we leaking memory here? --okir */
-   if (signalled())
-   continue;
if (nlmclnt_reclaim(host, fl) != 0)
continue;
list_add_tail(fl-fl_u.nfs_fl.list, host-h_granted);
-- 
1.5.0.g53756

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Trond Myklebust
On Thu, 2007-04-19 at 01:58 -0600, Eric W. Biederman wrote:
 From: Eric W. Biederman [EMAIL PROTECTED]
 
 Start the reclaimer thread using kthread_run instead
 of a combination of kernel_thread and daemonize.
 The small amount of signal handling code is also removed
 as it makes no sense and is a maintenance problem to handle
 signals in kernel threads.

Vetoed. Removing stuff just because it doesn't make sense to you is not
acceptable.

Signal handling in reclaimer threads is there in order to allow
administrators to deal with the case where the server never comes up
again.

Trond

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Eric W. Biederman
Trond Myklebust [EMAIL PROTECTED] writes:

 On Thu, 2007-04-19 at 01:58 -0600, Eric W. Biederman wrote:
 From: Eric W. Biederman [EMAIL PROTECTED]
 
 Start the reclaimer thread using kthread_run instead
 of a combination of kernel_thread and daemonize.
 The small amount of signal handling code is also removed
 as it makes no sense and is a maintenance problem to handle
 signals in kernel threads.

 Vetoed. Removing stuff just because it doesn't make sense to you is not
 acceptable.

 Signal handling in reclaimer threads is there in order to allow
 administrators to deal with the case where the server never comes up
 again.

Doesn't unmount handle that?

Regardless kernel threads should be an implementation detail
not a part of the user interface.  If kernel threads are part
of the user interface it makes them very hard to change.

So it isn't that it doesn't make sense to me it is that it looks
fundamentally broken and like a maintenance nightmare.

I would rather kill kernel threads then try and simulate them
when the kernel implementation has changed and kernel threads
are not visible.

If I could be convinced that signal handling in kernel threads
is not something that will impede code modifications and refactoring
I would have less of a problem, and might not care.

With pid namespaces all kernel threads will disappear so how do
we cope with the problem when the sysadmin can not see the kernel
threads?

Eric
-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Trond Myklebust
On Thu, 2007-04-19 at 13:20 -0600, Eric W. Biederman wrote:
 Trond Myklebust [EMAIL PROTECTED] writes:
 
  On Thu, 2007-04-19 at 01:58 -0600, Eric W. Biederman wrote:
  From: Eric W. Biederman [EMAIL PROTECTED]
  
  Start the reclaimer thread using kthread_run instead
  of a combination of kernel_thread and daemonize.
  The small amount of signal handling code is also removed
  as it makes no sense and is a maintenance problem to handle
  signals in kernel threads.
 
  Vetoed. Removing stuff just because it doesn't make sense to you is not
  acceptable.
 
  Signal handling in reclaimer threads is there in order to allow
  administrators to deal with the case where the server never comes up
  again.
 
 Doesn't unmount handle that?

On a pinned filesystem?

 Regardless kernel threads should be an implementation detail
 not a part of the user interface.  If kernel threads are part
 of the user interface it makes them very hard to change.
 
 So it isn't that it doesn't make sense to me it is that it looks
 fundamentally broken and like a maintenance nightmare.
 
 I would rather kill kernel threads then try and simulate them
 when the kernel implementation has changed and kernel threads
 are not visible.
 
 If I could be convinced that signal handling in kernel threads
 is not something that will impede code modifications and refactoring
 I would have less of a problem, and might not care.

Tough. You're the one proposing to change existing code.

 With pid namespaces all kernel threads will disappear so how do
 we cope with the problem when the sysadmin can not see the kernel
 threads?

Then you have a usability problem. How does the sysadmin reboot the
system if there is no way to shut down the processes that are hanging on
an unresponsive filesystem?

Trond

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Dave Hansen
On Thu, 2007-04-19 at 17:19 -0400, Trond Myklebust wrote:
  With pid namespaces all kernel threads will disappear so how do
  we cope with the problem when the sysadmin can not see the kernel
  threads?

Do they actually always disappear, or do we keep them in the
init_pid_namespace?

-- Dave

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Andrew Morton
On Thu, 19 Apr 2007 17:19:24 -0400
Trond Myklebust [EMAIL PROTECTED] wrote:

  Regardless kernel threads should be an implementation detail
  not a part of the user interface.  If kernel threads are part
  of the user interface it makes them very hard to change.
  
  So it isn't that it doesn't make sense to me it is that it looks
  fundamentally broken and like a maintenance nightmare.
  
  I would rather kill kernel threads then try and simulate them
  when the kernel implementation has changed and kernel threads
  are not visible.
  
  If I could be convinced that signal handling in kernel threads
  is not something that will impede code modifications and refactoring
  I would have less of a problem, and might not care.
 
 Tough. You're the one proposing to change existing code.

Using signals to communicate with kernel threads is fairly unpleasant, IMO.
We have much simpler, faster and more idiomatic ways of communicating
between threads in-kernel and there are better ways in which userspace can
communicate with the kernel - system calls, for example...

So I think generally any move which gets us away from using signals in
kernel threads is moving in a good direction.

  With pid namespaces all kernel threads will disappear so how do
  we cope with the problem when the sysadmin can not see the kernel
  threads?
 
 Then you have a usability problem. How does the sysadmin reboot the
 system if there is no way to shut down the processes that are hanging on
 an unresponsive filesystem?

Where's the hang?  A user process is stuck on h_rwsem?

If so, would it be appropriate to convert the user process to use
down_foo_interruptible(), so that the operator can just kill the user
process as expected, rather than having to futz around killing kernel
threads?

-
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] nfs lockd reclaimer: Convert to kthread API

2007-04-19 Thread Trond Myklebust
On Thu, 2007-04-19 at 14:40 -0700, Andrew Morton wrote:
 Using signals to communicate with kernel threads is fairly unpleasant, IMO.
 We have much simpler, faster and more idiomatic ways of communicating
 between threads in-kernel and there are better ways in which userspace can
 communicate with the kernel - system calls, for example...
 
 So I think generally any move which gets us away from using signals in
 kernel threads is moving in a good direction.

I have yet to see a proposal which did. Eric's patch was eliminating
signals in kernel threads that used them without proposing any
replacement mechanism or showing that he had plans to do so. That is a
good reason for a veto.

   With pid namespaces all kernel threads will disappear so how do
   we cope with the problem when the sysadmin can not see the kernel
   threads?
  
  Then you have a usability problem. How does the sysadmin reboot the
  system if there is no way to shut down the processes that are hanging on
  an unresponsive filesystem?
 
 Where's the hang?  A user process is stuck on h_rwsem?
 
 If so, would it be appropriate to convert the user process to use
 down_foo_interruptible(), so that the operator can just kill the user
 process as expected, rather than having to futz around killing kernel
 threads?

If an NFS server reboots, then the locks held by user processes on the
client need to be re-established by when it comes up again. Otherwise,
the processes that thought they were holding locks will suddenly fail.
This recovery job is currently the done by a kernel thread.

The question is then what to do if the server crashes again while the
kernel thread is re-establishing the locks. Particularly if it never
comes back again.
Currently, the administrator can intervene by killing anything that has
open files on that volume and kill the recovery kernel thread.
You'll also note that lockd_down(), nfsd_down() etc all use signals to
inform lockd(), nfsd() etc that they should be shutting down. Since the
reclaimer thread is started by the lockd() thread using CLONE_SIGHAND,
this means that we also automatically kill any lingering recovery
threads whenever we shutdown lockd().

These mechanisms need to be replaced _before_ we start shooting down
sigallow() etc in the kernel.

  Trond

-
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/