Skip to contentSkip to conversation

md:Fix name of raid thread when raid takeovered.

PATCH Applied majianpeng 2 participants 5 messages Jun 5, 2012 – Jun 27, 2012 MD RAID

lore
Share & tools
lore

majianpeng

author
#0 lore raw

PATCH 1/1 md:Fix name of raid thread when raid takeovered.

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

Signed-off-by majianpeng <[email protected]>
---
 drivers/md/md.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
drivers/md/md.c +14 -0
index 1c2f904..6b3ce6e 100644
@@ -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;
Signature · 1 line
1.7.5.4
#1 lore raw
Quoted 28 lines · On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <[email protected]> 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 <[email protected]>
---
 drivers/md/md.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
drivers/md/md.c +7 -0
index 1c2f904..6b3ce6e 100644
@@ -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);

drivers/md/md.c struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev · line 6762

Quoted 10 lines
drivers/md/md.c +7 -0
+		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

author
#2 lore raw
Quoted 39 lines · neil
Quoted 37 lines · On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <[email protected]> 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 <[email protected]>
---
 drivers/md/md.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
drivers/md/md.c +14 -0
index 1c2f904..6b3ce6e 100644
@@ -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);
Quoted 9 lines

drivers/md/md.c struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev · line 6762

Quoted 1 line
drivers/md/md.c +0 -0
 	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.

Quoted 3 lines

NeilBrown

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

#3 lore raw
Quoted 46 lines · On Tue, 5 Jun 2012 14:06:06 +0800 majianpeng <[email protected]> wrote:
Quoted 44 lines · neil
Quoted 28 lines · On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <[email protected]> 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 <[email protected]>
---
 drivers/md/md.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
drivers/md/md.c +7 -0
index 1c2f904..6b3ce6e 100644
@@ -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);

drivers/md/md.c struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev · line 6762

Quoted 10 lines
drivers/md/md.c +7 -0
+		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.

Quoted 5 lines
Quoted 3 lines

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

drivers/md/md.c +1 -1
index 1c2f904..2e70768 100644
@@ -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;
drivers/md/multipath.c +2 -1
index 9339e67..61a1833 100644
@@ -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));
drivers/md/raid1.c +1 -1
index a9c7981..39b2a8a 100644
@@ -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",
drivers/md/raid10.c +1 -1
index 99ae606..994ac9a 100644
@@ -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;
drivers/md/raid5.c +3 -1
index d267672..92c164f 100644
@@ -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",
Applied
#4 lore raw
In reply to majianpeng
Quoted 46 lines · On Tue, 5 Jun 2012 14:06:06 +0800 majianpeng <[email protected]> wrote:
Quoted 44 lines · neil
Quoted 28 lines · On Tue, 5 Jun 2012 13:37:15 +0800 majianpeng <[email protected]> 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 <[email protected]>
---
 drivers/md/md.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
drivers/md/md.c +7 -0
index 1c2f904..6b3ce6e 100644
@@ -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);

drivers/md/md.c struct md_thread *md_register_thread(void (*run) (struct mddev *), struct mddev · line 6762

Quoted 10 lines
drivers/md/md.c +7 -0
+		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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 1 line

NeilBrown

I've applied this patch to fix the problem.

Thanks,
NeilBrown

From 5fc056ff25485cecb8464a6fe09d3d486fd99d6a Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
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 <[email protected]>
Signed-off-by NeilBrown <[email protected]>
drivers/md/md.c +1 -1
index 973aa84..c601c4b 100644
@@ -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;
drivers/md/multipath.c +2 -1
index 9339e67..61a1833 100644
@@ -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));
drivers/md/raid1.c +1 -1
index a9c7981..39b2a8a 100644
@@ -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",
drivers/md/raid10.c +1 -1
index bcf6ea8..0ee7602 100644
@@ -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;
drivers/md/raid5.c +3 -1
index 62b6b3a..a5135e5 100644
@@ -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",