Remaining straight forward kthread API conversions...
Subsystems (22)
PATCH 1/1 md: Remove broken SIGKILL support
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Currently md_thread calls allow_signal so it can receive a SIGKILL but then does nothing with it except flush the sigkill so that it not can use an interruptible sleep.
This whole dance is silly so remove the unnecessary and broken signal handling logic.
--- drivers/md/md.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
drivers/md/md.c +0 -6
@@ -4542,17 +4542,11 @@ static int md_thread(void * arg)*/ current->flags |=PF_NOFREEZE ; -allow_signal (SIGKILL );while (!kthread_should_stop ()) {/* We need to wait INTERRUPTIBLE so that * we don't add to the load-average. -* That means we need to be sure no signals are -* pending */ -if (signal_pending (current)) -flush_signals (current); -wait_event_interruptible_timeout (thread->wqueue, test_bit(THREAD_WAKEUP, &thread->flags)
Signature · 1 line
1.5.0.g53756
Re: [PATCH] md: Remove broken SIGKILL support
Quoted 8 lines · On Thursday April 19, [email protected] wrote:
From: Eric W. Biederman <[email protected]>
Currently
md_threadcallsallow_signalso it can receive a SIGKILL but then does nothing with it except flush the sigkill so that it not can use an interruptible sleep.This whole dance is silly so remove the unnecessary and broken signal handling logic.
(sorry of the delay in replying)
You missed some related code which should help you see that it is - maybe - not completely 'silly' (though I confess it might be slightly broken).
In md_check_recovery:
if (signal_pending(current)) {
if (mddev->pers->sync_request) {
printk(KERN_INFO "md: %s in immediate safe mode\n",
mdname(mddev));
mddev->safemode = 2;
}
flush_signals(current);
}The idea is that alt-sysrq-K will send SIGKILL to all processes including the md support threads, which will cause them to enter "immediate safe mode" so that the metadata will be marked clean immediately at every opportunity. That way you can use alt-sysrq:
sync,unmount,kill,reboot
and be fairly sure that you md array will be shut down cleanly.
I'd be just as happy to link this into Unmount (aka do_emergency_remount), but that doesn't seem at all straight forward, and in any case should be done before the current code is ripped out.
While we do have a reboot_notifier which tries to stop all arrays, I've never been comfortable with that. A reboot really should just reboot...
What I would REALLY like is for the block device to know whether it is open read-only or read-write. Then I could mark it clean when it becomes read-only as would happen when do_emergency_remount remounts it read-only.
I might see how hard that would be...
NeilBrown
Re: [PATCH] md: Remove broken SIGKILL support
Quoted 17 lines · Neil Brown
Quoted 8 lines · On Thursday April 19, [email protected] wrote:
From: Eric W. Biederman <[email protected]>
Currently
md_threadcallsallow_signalso it can receive a SIGKILL but then does nothing with it except flush the sigkill so that it not can use an interruptible sleep.This whole dance is silly so remove the unnecessary and broken signal handling logic.
(sorry of the delay in replying)
You missed some related code which should help you see that it is - maybe - not completely 'silly' (though I confess it might be slightly broken).
Inmd_check_recovery:
Quoted 8 lines
if (signal_pending(current)) {
if (mddev->pers->sync_request) {
printk(KERN_INFO "md: %s in immediate safe mode\n",
mdname(mddev));
mddev->safemode = 2;
}
flush_signals(current);
}Thanks.
Quoted 15 lines
The idea is that alt-sysrq-K will send SIGKILL to all processes including the md support threads, which will cause them to enter "immediate safe mode" so that the metadata will be marked clean immediately at every opportunity. That way you can use alt-sysrq:
sync,unmount,kill,reboot
and be fairly sure that you md array will be shut down cleanly.I'd be just as happy to link this into Unmount (aka
do_emergency_remount), but that doesn't seem at all straight forward, and in any case should be done before the current code is ripped out.While we do have a
reboot_notifierwhich tries to stop all arrays, I've never been comfortable with that. A reboot really should just reboot...
Quoted 6 lines
What I would REALLY like is for the block device to know whether it is open read-only or read-write. Then I could mark it clean when it becomes read-only as would happen when
do_emergency_remountremounts it read-only.I might see how hard that would be...
My goal to get signals to kernel threads out of the user space interface especially for non-privileged processes, so everything that we do with kernel threads can just be an unimportant implementation detail to user space.
Eric
PATCH 1/1 nfs4state reclaimer: Remove unnecessary allow_signal
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
--- fs/nfs/nfs4state.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
fs/nfs/nfs4state.c +0 -2
@@ -775,8 +775,6 @@ static int reclaimer(void *ptr)struct rpc_cred *cred;int status =0 ; -allow_signal (SIGKILL ); -/* Ensure exclusive access to NFSv4 state */ lock_kernel ();down_write (&clp->cl_sem);
Signature · 1 line
1.5.0.g53756
Re: [PATCH] nfs4state reclaimer: Remove unnecessary allow_signal
Quoted 14 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Cc Neil Brown <[email protected]>Cc Trond Myklebust <[email protected]>Signed-off-by Eric W. Biederman <[email protected]>--- fs/nfs/nfs4state.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)fs/nfs/nfs4state.c +0
-0
fs/nfs/nfs4state.c static int reclaimer(void *ptr) · line 780
Quoted 8 lines
fs/nfs/nfs4state.c +0
-2@@ -775,8 +775,6 @@ static int reclaimer(void *ptr)struct rpc_cred *cred;int status =0 ; -allow_signal (SIGKILL ); -/* Ensure exclusive access to NFSv4 state */ lock_kernel ();down_write (&clp->cl_sem);
Ditto...
Looks like you were missing at least the pcie hotplug driver. Another one of the horrible thread abuses in drivers/pci/hotpug.
- full conversion to kthread infrastructure
- use wake_up_process to wake the thread up
Like most pci hotplug drivers it still uses very race non-atomic variable assignment to communicated with the thread, but that's something the maintainers should look into.
drivers/pci/hotplug/pciehp_ctrl.c +21 -33
@@ -32,14 +32,13 @@#include <linux/types.h> #include <linux/smp_lock.h> #include <linux/pci.h> +#include <linux/kthread.h> #include "../pci.h" #include "pciehp.h" static void interrupt_event_handler (struct controller *ctrl); -static struct semaphore event_semaphore;/* mutex for process loop (up if something to process) */ -static struct semaphore event_exit;/* guard ensure thread has exited before calling it quits */ -static int event_finished; +static struct task_struct *pciehpd_event_thread;static unsigned long pushbutton_pending;/* = 0 */ static unsigned long surprise_rm_pending;/* = 0 */
@@ -93,8 +92,9 @@ u8 pciehp_handle_attention_button(u8 hp_info ("Button ignore on Slot(%s)\n" ,slot_name (p_slot)); } +/* signal event thread that new event is posted */ if (rc) -up (&event_semaphore);/* signal event thread that new event is posted */ +wake_up_process (pciehpd_event_thread);return 0 ;
@@ -135,8 +135,9 @@ u8 pciehp_handle_switch_change(u8 hp_slo taskInfo->event_type =INT_SWITCH_CLOSE ; } +/* signal event thread that new event is posted */ if (rc) -up (&event_semaphore);/* signal event thread that new event is posted */ +wake_up_process (pciehpd_event_thread);return rc; }
@@ -178,8 +179,9 @@ u8 pciehp_handle_presence_change(u8 hp_s taskInfo->event_type =INT_PRESENCE_OFF ; } +/* signal event thread that new event is posted */ if (rc) -up (&event_semaphore);/* signal event thread that new event is posted */ +wake_up_process (pciehpd_event_thread);return rc; }
@@ -217,8 +219,10 @@ u8 pciehp_handle_power_fault(u8 hp_slot, taskInfo->event_type =INT_POWER_FAULT ;info ("power fault bit %x set\n" , hp_slot); } + +/* signal event thread that new event is posted */ if (rc) -up (&event_semaphore);/* signal event thread that new event is posted */ +wake_up_process (pciehpd_event_thread);return rc; }
@@ -362,7 +366,7 @@ static void pushbutton_helper_thread(uns { pushbutton_pending = data; -up (&event_semaphore); +wake_up_process (pciehpd_event_thread); }/**
@@ -452,19 +456,14 @@ static void pciehp_surprise_rm_thread(un/* this is the main worker thread */ -static int event_thread (void * data) +static int event_thread (void *data) {struct controller *ctrl; -lock_kernel (); -daemonize ("pciehpd_event" ); - -unlock_kernel ();while (1 ) { -dbg ("!!!!event_thread sleeping\n" ); -down_interruptible (&event_semaphore); -dbg ("event_thread woken finished = %d\n" , event_finished); -if (event_finished ||signal_pending (current)) +set_current_state (TASK_INTERRUPTIBLE ); +schedule (); +if (kthread_should_stop ())break ;/* Do stuff here */ if (pushbutton_pending)
@@ -476,24 +475,15 @@ static int event_thread(void* data)interrupt_event_handler (ctrl); }dbg ("event_thread signals exit\n" ); -up (&event_exit);return 0 ; }int pciehp_event_start_thread (void ) { -int pid; - -/* initialize our semaphores */ -init_MUTEX_LOCKED (&event_exit); - event_finished=0 ; - -init_MUTEX_LOCKED (&event_semaphore); - pid =kernel_thread (event_thread,NULL ,0 ); - -if (pid <0 ) { + pciehpd_event_thread =kthread_run (event_thread,NULL ,"pciehpd_event" ); +if (IS_ERR (pciehpd_event_thread)) {err ("Can't start up our event thread\n" ); -return -1 ; +return PTR_ERR (pciehpd_event_thread); }return 0 ; }
@@ -501,9 +491,7 @@ int pciehp_event_start_thread(void)void pciehp_event_stop_thread (void ) { - event_finished =1 ; -up (&event_semaphore); -down (&event_exit); +kthread_stop (pciehpd_event_thread); }
@@ -624,7 +612,7 @@ static void interrupt_event_handler(strudbg ("Surprise Removal\n" );if (p_slot) { surprise_rm_pending = (unsigned long ) p_slot; -up (&event_semaphore); +wake_up_process (pciehpd_event_thread);update_slot_info (p_slot); } }