[PATCH md 000 of 2] Introduction

[PATCH md 000 of 2] Introduction

[NeilBrown]

Two patches against 2.6.12-rc2 for md.
(patches apply to 2.6.12-rc2-mm1 with various offsets).

The first closes an extreme small race in md thread deregistration.
This race was actually lost by someone who, due to a different bug
which has since been fixed, has md threads being created and destroyed
at a very high rate.

The second removes a number of MD_BUG calls that that are either
completely pointless, or misleading (i.e. the report a bug when it is
just an unusual condition that is handled correctly).

NeilBrown


 [PATCH md 001 of 2] Close a small race in md thread deregistration
 [PATCH md 002 of 2] Remove a number of misleading calls to MD_BUG

[PATCH md 001 of 2] Close a small race in md thread deregistration

[NeilBrown]

There is a tiny race when de-registering an MD thread, in that the
thread could disappear before it is set a SIGKILL, causing
send_sig to have problems.  
This is most easily closed by holding tasklist_lock between
enabling the thread to exit (setting ->run to NULL) and telling 
it to exit.

Signed-off-by: Neil Brown <neilb@cse.unsw.edu.au>

### Diffstat output
 ./drivers/md/md.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff ./drivers/md/md.c~current~ ./drivers/md/md.c
--- ./drivers/md/md.c~current~	2005-04-08 11:36:35.000000000 +1000
+++ ./drivers/md/md.c	2005-04-08 11:36:35.000000000 +1000
@@ -2840,16 +2840,6 @@ mdk_thread_t *md_register_thread(void (*
 	return thread;
 }
 
-static void md_interrupt_thread(mdk_thread_t *thread)
-{
-	if (!thread->tsk) {
-		MD_BUG();
-		return;
-	}
-	dprintk("interrupting MD-thread pid %d\n", thread->tsk->pid);
-	send_sig(SIGKILL, thread->tsk, 1);
-}
-
 void md_unregister_thread(mdk_thread_t *thread)
 {
 	struct completion event;
@@ -2857,9 +2847,15 @@ void md_unregister_thread(mdk_thread_t *
 	init_completion(&event);
 
 	thread->event = &event;
+
+	/* As soon as ->run is set to NULL, the task could disappear,
+	 * so we need to hold tasklist_lock until we have sent the signal
+	 */
+	dprintk("interrupting MD-thread pid %d\n", thread->tsk->pid);
+	read_lock(&tasklist_lock);
 	thread->run = NULL;
-	thread->name = NULL;
-	md_interrupt_thread(thread);
+	send_sig(SIGKILL, thread->tsk, 1);
+	read_unlock(&tasklist_lock);
 	wait_for_completion(&event);
 	kfree(thread);
 }

Re: [PATCH md 001 of 2] Close a small race in md thread deregistration

[Andrew Morton]

NeilBrown <neilb@cse.unsw.edu.au> wrote:
>
>  There is a tiny race when de-registering an MD thread, in that the
>  thread could disappear before it is set a SIGKILL, causing
>  send_sig to have problems.  
>  This is most easily closed by holding tasklist_lock between
>  enabling the thread to exit (setting ->run to NULL) and telling 
>  it to exit.

That code all seems a bit crufty to me.  Sometime it would be good to stop
using signals in-kernel and to use the kthread API for thread startup and
shutdown.

Re: [PATCH md 001 of 2] Close a small race in md thread deregistration

[Neil Brown]

On Thursday April 7, akpm@osdl.org wrote:
> 
> That code all seems a bit crufty to me.  Sometime it would be good to stop
> using signals in-kernel and to use the kthread API for thread startup and
> shutdown.

I've just added that to my TODO list... thanks for the suggestion.

NeilBrown

[PATCH md 002 of 2] Remove a number of misleading calls to MD_BUG

[NeilBrown]

The conditions that cause these calls to MD_BUG are
not kernel bugs, just oddities in what userspace is
asking for.

Also convert analyze_sbs to return void, and the value
it returned was always 0.


Signed-off-by: Neil Brown <neilb@cse.unsw.edu.au>

### Diffstat output
 ./drivers/md/md.c |   30 ++++++++----------------------
 1 files changed, 8 insertions(+), 22 deletions(-)

diff ./drivers/md/md.c~current~ ./drivers/md/md.c
--- ./drivers/md/md.c~current~	2005-04-08 11:36:35.000000000 +1000
+++ ./drivers/md/md.c	2005-04-08 11:36:47.000000000 +1000
@@ -1387,7 +1387,7 @@ abort_free:
  */
 
 
-static int analyze_sbs(mddev_t * mddev)
+static void analyze_sbs(mddev_t * mddev)
 {
 	int i;
 	struct list_head *tmp;
@@ -1441,7 +1441,6 @@ static int analyze_sbs(mddev_t * mddev)
 		       " -- starting background reconstruction\n",
 		       mdname(mddev));
 
-	return 0;
 }
 
 int mdp_major = 0;
@@ -1508,10 +1507,9 @@ static int do_md_run(mddev_t * mddev)
 	struct gendisk *disk;
 	char b[BDEVNAME_SIZE];
 
-	if (list_empty(&mddev->disks)) {
-		MD_BUG();
+	if (list_empty(&mddev->disks))
+		/* cannot run an array with no devices.. */
 		return -EINVAL;
-	}
 
 	if (mddev->pers)
 		return -EBUSY;
@@ -1519,10 +1517,8 @@ static int do_md_run(mddev_t * mddev)
 	/*
 	 * Analyze all RAID superblock(s)
 	 */
-	if (!mddev->raid_disks && analyze_sbs(mddev)) {
-		MD_BUG();
-		return -EINVAL;
-	}
+	if (!mddev->raid_disks)
+		analyze_sbs(mddev);
 
 	chunk_size = mddev->chunk_size;
 	pnum = level_to_pers(mddev->level);
@@ -1548,7 +1544,7 @@ static int do_md_run(mddev_t * mddev)
 		 * chunk-size has to be a power of 2 and multiples of PAGE_SIZE
 		 */
 		if ( (1 << ffz(~chunk_size)) != chunk_size) {
-			MD_BUG();
+			printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
 			return -EINVAL;
 		}
 		if (chunk_size < PAGE_SIZE) {
@@ -1573,11 +1569,6 @@ static int do_md_run(mddev_t * mddev)
 		}
 	}
 
-	if (pnum >= MAX_PERSONALITY) {
-		MD_BUG();
-		return -EINVAL;
-	}
-
 #ifdef CONFIG_KMOD
 	if (!pers[pnum])
 	{
@@ -1762,10 +1753,8 @@ static void autorun_array(mddev_t *mddev
 	struct list_head *tmp;
 	int err;
 
-	if (list_empty(&mddev->disks)) {
-		MD_BUG();
+	if (list_empty(&mddev->disks))
 		return;
-	}
 
 	printk(KERN_INFO "md: running: ");
 
@@ -3128,7 +3117,6 @@ int register_md_personality(int pnum, md
 	spin_lock(&pers_lock);
 	if (pers[pnum]) {
 		spin_unlock(&pers_lock);
-		MD_BUG();
 		return -EBUSY;
 	}
 
@@ -3140,10 +3128,8 @@ int register_md_personality(int pnum, md
 
 int unregister_md_personality(int pnum)
 {
-	if (pnum >= MAX_PERSONALITY) {
-		MD_BUG();
+	if (pnum >= MAX_PERSONALITY)
 		return -EINVAL;
-	}
 
 	printk(KERN_INFO "md: %s personality unregistered\n", pers[pnum]->name);
 	spin_lock(&pers_lock);