[PATCH V1] md:Fix name of raid thread when raid takeovered.

[PATCH V1] md:Fix name of raid thread when raid takeovered.

[majianpeng]

When raid takeovered, it used old personality to set task->comm[].
So we add judgement and set corrected task->comm[].

Signed-off-by: majianpeng <majianpeng@gmail.com>
---
 drivers/md/md.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1c2f904..6b3ce6e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6738,11 +6738,25 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
 				 const char *name)
 {
 	struct md_thread *thread;
+	struct md_personality *pers = NULL;
 
 	thread = kzalloc(sizeof(struct md_thread), GFP_KERNEL);
 	if (!thread)
 		return NULL;
 
+	/*
+	 *When raid takeovered,the mddev->pers was old,so the
+	 *thread_name is wrong.
+	 */
+	if (name == NULL && mddev->level != mddev->new_level) {
+		spin_lock(&pers_lock);
+		list_for_each_entry(pers, &pers_list, list)
+			if (pers->level == mddev->new_level) {
+				name = pers->name;
+				break;
+			}
+		spin_unlock(&pers_lock);
+	}
 	init_waitqueue_head(&thread->wqueue);
 
 	thread->run = run;
-- 
1.7.5.4

Re: [PATCH V1] md:Fix name of raid thread when raid takeovered.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <majianpeng@gmail.com> wrote:

> When raid takeovered, it used old personality to set task->comm[].
> So we add judgement and set corrected task->comm[].
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/md.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 1c2f904..6b3ce6e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6738,11 +6738,25 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
>  				 const char *name)
>  {
>  	struct md_thread *thread;
> +	struct md_personality *pers = NULL;
>  
>  	thread = kzalloc(sizeof(struct md_thread), GFP_KERNEL);
>  	if (!thread)
>  		return NULL;
>  
> +	/*
> +	 *When raid takeovered,the mddev->pers was old,so the
> +	 *thread_name is wrong.
> +	 */
> +	if (name == NULL && mddev->level != mddev->new_level) {
> +		spin_lock(&pers_lock);
> +		list_for_each_entry(pers, &pers_list, list)
> +			if (pers->level == mddev->new_level) {
> +				name = pers->name;
> +				break;
> +			}
> +		spin_unlock(&pers_lock);
> +	}
>  	init_waitqueue_head(&thread->wqueue);
>  
>  	thread->run = run;

No, I really don't like that.  It is to focussed on fixing the particular
symptom rather that making the code generally cleaner.

Was there a reason you didn't just change the last arg of md_register_thread
to be the level name instead of NULL??

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Re: [PATCH V1] md:Fix name of raid thread when raid takeovered.

[majianpeng]

On  Mon, 4 Jun 2012 22:49:02 -0700, neil wrote:
>On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
>> When raid takeovered, it used old personality to set task->comm[].
>> So we add judgement and set corrected task->comm[].
>> 
>> Signed-off-by: majianpeng <majianpeng@gmail.com>
>> ---
>>  drivers/md/md.c |   14 ++++++++++++++
>>  1 files changed, 14 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 1c2f904..6b3ce6e 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -6738,11 +6738,25 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
>>  				 const char *name)
>>  {
>>  	struct md_thread *thread;
>> +	struct md_personality *pers = NULL;
>>  
>>  	thread = kzalloc(sizeof(struct md_thread), GFP_KERNEL);
>>  	if (!thread)
>>  		return NULL;
>>  
>> +	/*
>> +	 *When raid takeovered,the mddev->pers was old,so the
>> +	 *thread_name is wrong.
>> +	 */
>> +	if (name == NULL && mddev->level != mddev->new_level) {
>> +		spin_lock(&pers_lock);
>> +		list_for_each_entry(pers, &pers_list, list)
>> +			if (pers->level == mddev->new_level) {
>> +				name = pers->name;
>> +				break;
>> +			}
>> +		spin_unlock(&pers_lock);
>> +	}
>>  	init_waitqueue_head(&thread->wqueue);
>>  
>>  	thread->run = run;
>
>No, I really don't like that.  It is to focussed on fixing the particular
>symptom rather that making the code generally cleaner.
>

>Was there a reason you didn't just change the last arg of md_register_thread
>to be the level name instead of NULL??
>
No.I first wanted to corrected in which call md_register_thread.But I found it will take some codes.
So i think corrected this function may better.
>NeilBrown
>
>


--------------
majianpeng

Re: [PATCH V1] md:Fix name of raid thread when raid takeovered.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4733 bytes --]

On Tue, 5 Jun 2012 14:06:06 +0800 majianpeng <majianpeng@gmail.com> wrote:

> On  Mon, 4 Jun 2012 22:49:02 -0700, neil wrote:
> >On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <majianpeng@gmail.com> wrote:
> >
> >> When raid takeovered, it used old personality to set task->comm[].
> >> So we add judgement and set corrected task->comm[].
> >> 
> >> Signed-off-by: majianpeng <majianpeng@gmail.com>
> >> ---
> >>  drivers/md/md.c |   14 ++++++++++++++
> >>  1 files changed, 14 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 1c2f904..6b3ce6e 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -6738,11 +6738,25 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
> >>  				 const char *name)
> >>  {
> >>  	struct md_thread *thread;
> >> +	struct md_personality *pers = NULL;
> >>  
> >>  	thread = kzalloc(sizeof(struct md_thread), GFP_KERNEL);
> >>  	if (!thread)
> >>  		return NULL;
> >>  
> >> +	/*
> >> +	 *When raid takeovered,the mddev->pers was old,so the
> >> +	 *thread_name is wrong.
> >> +	 */
> >> +	if (name == NULL && mddev->level != mddev->new_level) {
> >> +		spin_lock(&pers_lock);
> >> +		list_for_each_entry(pers, &pers_list, list)
> >> +			if (pers->level == mddev->new_level) {
> >> +				name = pers->name;
> >> +				break;
> >> +			}
> >> +		spin_unlock(&pers_lock);
> >> +	}
> >>  	init_waitqueue_head(&thread->wqueue);
> >>  
> >>  	thread->run = run;
> >
> >No, I really don't like that.  It is to focussed on fixing the particular
> >symptom rather that making the code generally cleaner.
> >
> 
> >Was there a reason you didn't just change the last arg of md_register_thread
> >to be the level name instead of NULL??
> >
> No.I first wanted to corrected in which call md_register_thread.But I found it will take some codes.
> So i think corrected this function may better.

I don't think it is that much code.  Just this:

NeilBrown

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1c2f904..2e70768 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6751,7 +6751,7 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
 	thread->tsk = kthread_run(md_thread, thread,
 				  "%s_%s",
 				  mdname(thread->mddev),
-				  name ?: mddev->pers->name);
+				  name);
 	if (IS_ERR(thread->tsk)) {
 		kfree(thread);
 		return NULL;
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index 9339e67..61a1833 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -474,7 +474,8 @@ static int multipath_run (struct mddev *mddev)
 	}
 
 	{
-		mddev->thread = md_register_thread(multipathd, mddev, NULL);
+		mddev->thread = md_register_thread(multipathd, mddev,
+						   "multipath");
 		if (!mddev->thread) {
 			printk(KERN_ERR "multipath: couldn't allocate thread"
 				" for %s\n", mdname(mddev));
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index a9c7981..39b2a8a 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2621,7 +2621,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 		goto abort;
 	}
 	err = -ENOMEM;
-	conf->thread = md_register_thread(raid1d, mddev, NULL);
+	conf->thread = md_register_thread(raid1d, mddev, "raid1");
 	if (!conf->thread) {
 		printk(KERN_ERR
 		       "md/raid1:%s: couldn't allocate thread\n",
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 99ae606..994ac9a 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3421,7 +3421,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 	spin_lock_init(&conf->resync_lock);
 	init_waitqueue_head(&conf->wait_barrier);
 
-	conf->thread = md_register_thread(raid10d, mddev, NULL);
+	conf->thread = md_register_thread(raid10d, mddev, "raid10");
 	if (!conf->thread)
 		goto out;
 
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d267672..92c164f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -4823,6 +4823,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 	int raid_disk, memory, max_disks;
 	struct md_rdev *rdev;
 	struct disk_info *disk;
+	char pers_name[6];
 
 	if (mddev->new_level != 5
 	    && mddev->new_level != 4
@@ -4946,7 +4947,8 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 		printk(KERN_INFO "md/raid:%s: allocated %dkB\n",
 		       mdname(mddev), memory);
 
-	conf->thread = md_register_thread(raid5d, mddev, NULL);
+	sprintf(pers_name, "raid%d", mddev->new_level);
+	conf->thread = md_register_thread(raid5d, mddev, pers_name);
 	if (!conf->thread) {
 		printk(KERN_ERR
 		       "md/raid:%s: couldn't allocate thread.\n",

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH V1] md:Fix name of raid thread when raid takeovered.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5397 bytes --]

On Tue, 5 Jun 2012 14:06:06 +0800 majianpeng <majianpeng@gmail.com> wrote:

> On  Mon, 4 Jun 2012 22:49:02 -0700, neil wrote:
> >On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <majianpeng@gmail.com> wrote:
> >
> >> When raid takeovered, it used old personality to set task->comm[].
> >> So we add judgement and set corrected task->comm[].
> >> 
> >> Signed-off-by: majianpeng <majianpeng@gmail.com>
> >> ---
> >>  drivers/md/md.c |   14 ++++++++++++++
> >>  1 files changed, 14 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 1c2f904..6b3ce6e 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -6738,11 +6738,25 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
> >>  				 const char *name)
> >>  {
> >>  	struct md_thread *thread;
> >> +	struct md_personality *pers = NULL;
> >>  
> >>  	thread = kzalloc(sizeof(struct md_thread), GFP_KERNEL);
> >>  	if (!thread)
> >>  		return NULL;
> >>  
> >> +	/*
> >> +	 *When raid takeovered,the mddev->pers was old,so the
> >> +	 *thread_name is wrong.
> >> +	 */
> >> +	if (name == NULL && mddev->level != mddev->new_level) {
> >> +		spin_lock(&pers_lock);
> >> +		list_for_each_entry(pers, &pers_list, list)
> >> +			if (pers->level == mddev->new_level) {
> >> +				name = pers->name;
> >> +				break;
> >> +			}
> >> +		spin_unlock(&pers_lock);
> >> +	}
> >>  	init_waitqueue_head(&thread->wqueue);
> >>  
> >>  	thread->run = run;
> >
> >No, I really don't like that.  It is to focussed on fixing the particular
> >symptom rather that making the code generally cleaner.
> >
> 
> >Was there a reason you didn't just change the last arg of md_register_thread
> >to be the level name instead of NULL??
> >
> No.I first wanted to corrected in which call md_register_thread.But I found it will take some codes.
> So i think corrected this function may better.
> >NeilBrown

I've applied this patch to fix the problem.

Thanks,
NeilBrown

From 5fc056ff25485cecb8464a6fe09d3d486fd99d6a Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Wed, 27 Jun 2012 13:53:52 +1000
Subject: [PATCH] md: make 'name' arg to md_register_thread non-optional.

Having the 'name' arg optional and defaulting to the current
personality name is no necessary and leads to errors, as when
changing the level of an array we can end up using the
name of the old level instead of the new one.

So make it non-optional and always explicitly pass the name
of the level that the array will be.

Reported-by: majianpeng <majianpeng@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 973aa84..c601c4b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6751,7 +6751,7 @@ struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev
 	thread->tsk = kthread_run(md_thread, thread,
 				  "%s_%s",
 				  mdname(thread->mddev),
-				  name ?: mddev->pers->name);
+				  name);
 	if (IS_ERR(thread->tsk)) {
 		kfree(thread);
 		return NULL;
diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index 9339e67..61a1833 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -474,7 +474,8 @@ static int multipath_run (struct mddev *mddev)
 	}
 
 	{
-		mddev->thread = md_register_thread(multipathd, mddev, NULL);
+		mddev->thread = md_register_thread(multipathd, mddev,
+						   "multipath");
 		if (!mddev->thread) {
 			printk(KERN_ERR "multipath: couldn't allocate thread"
 				" for %s\n", mdname(mddev));
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index a9c7981..39b2a8a 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2621,7 +2621,7 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 		goto abort;
 	}
 	err = -ENOMEM;
-	conf->thread = md_register_thread(raid1d, mddev, NULL);
+	conf->thread = md_register_thread(raid1d, mddev, "raid1");
 	if (!conf->thread) {
 		printk(KERN_ERR
 		       "md/raid1:%s: couldn't allocate thread\n",
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index bcf6ea8..0ee7602 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3427,7 +3427,7 @@ static struct r10conf *setup_conf(struct mddev *mddev)
 	spin_lock_init(&conf->resync_lock);
 	init_waitqueue_head(&conf->wait_barrier);
 
-	conf->thread = md_register_thread(raid10d, mddev, NULL);
+	conf->thread = md_register_thread(raid10d, mddev, "raid10");
 	if (!conf->thread)
 		goto out;
 
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 62b6b3a..a5135e5 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -4840,6 +4840,7 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 	int raid_disk, memory, max_disks;
 	struct md_rdev *rdev;
 	struct disk_info *disk;
+	char pers_name[6];
 
 	if (mddev->new_level != 5
 	    && mddev->new_level != 4
@@ -4963,7 +4964,8 @@ static struct r5conf *setup_conf(struct mddev *mddev)
 		printk(KERN_INFO "md/raid:%s: allocated %dkB\n",
 		       mdname(mddev), memory);
 
-	conf->thread = md_register_thread(raid5d, mddev, NULL);
+	sprintf(pers_name, "raid%d", mddev->new_level);
+	conf->thread = md_register_thread(raid5d, mddev, pers_name);
 	if (!conf->thread) {
 		printk(KERN_ERR
 		       "md/raid:%s: couldn't allocate thread.\n",


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]