Skip to contentSkip to conversation

md: More patches for md.c

PATCH In review Andre Noll 2 participants 14 messages Mar 23, 2008 – Jul 9, 2008 MD RAID

lore
Share & tools
lore

Andre Noll

author
#0 lore raw

cover md: More patches for md.c

Hi

here are some more patches to md.c. These are a bit more involved than the first batch but still fairly simple. Please review.

Andre

Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
author
#1 lore raw

PATCH 1/8 md: Simplify uuid_equal().

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)
drivers/md/md.c +4 -9
index 2580ac1..c559b9e 100644
@@ -543,17 +543,12 @@ fail:
 
 static int uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 {
-	if (	(sb1->set_uuid0 == sb2->set_uuid0) &&
-		(sb1->set_uuid1 == sb2->set_uuid1) &&
-		(sb1->set_uuid2 == sb2->set_uuid2) &&
-		(sb1->set_uuid3 == sb2->set_uuid3))
-
-		return 1;
-
-	return 0;
+	return 	sb1->set_uuid0 == sb2->set_uuid0 &&
+		sb1->set_uuid1 == sb2->set_uuid1 &&
+		sb1->set_uuid2 == sb2->set_uuid2 &&
+		sb1->set_uuid3 == sb2->set_uuid3;
 }
 
-
 static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 {
 	int ret;
Signature · 1 line
1.5.3.8
author
#2 lore raw

PATCH 2/8 md: Simplify sb_equal().

In reply to Andre Noll

The only caller of sb_equal() tests the return value against zero, so it's OK to return the negated return value of memcmp().

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)
drivers/md/md.c +1 -5
index c559b9e..58762dd 100644
@@ -572,11 +572,7 @@ static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 	tmp1->nr_disks = 0;
 	tmp2->nr_disks = 0;
 
-	if (memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * 4))
-		ret = 0;
-	else
-		ret = 1;
-
+	ret = !memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * 4);
 abort:
 	kfree(tmp1);
 	kfree(tmp2);
Signature · 1 line
1.5.3.8
#3 lore raw

Re: [PATCH 2/8] md: Simplify sb_equal().

Quoted 13 lines · On Sunday March 23, [email protected] wrote:

The only caller of sb_equal() tests the return value against zero, so it's OK to return the negated return value of memcmp().

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)
drivers/md/md.c +0 -0
index c559b9e..58762dd 100644

drivers/md/md.c static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2) · line 575

Quoted 9 lines
drivers/md/md.c +1 -5
@@ -572,11 +572,7 @@ static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
 	tmp1->nr_disks = 0;
 	tmp2->nr_disks = 0;
 
-	if (memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * 4))
-		ret = 0;
-	else
-		ret = 1;
-
+	ret = !memcmp(tmp1, tmp2, MD_SB_GENERIC_CONSTANT_WORDS * 4);

One of my pet hates is "!strcmp" and similarly "!memcmp". memcmp doesn't return a "boolean", so testing it or its inverse doesn't read well. I always compare the result with '0'. The comparison operator used matches exactly how I want to compare the two values.
So I've changed this to

        ret = (memcmp(....) == 0).

NeilBrown

author
#4 lore raw

PATCH 3/8 md: alloc_disk_sb(): Return proper error value.

In reply to Andre Noll

If alloc_page() fails, ENOMEM is a more suitable error value than EINVAL.

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/md/md.c +1 -1
index 58762dd..7943df1 100644
@@ -372,7 +372,7 @@ static int alloc_disk_sb(mdk_rdev_t * rdev)
 	rdev->sb_page = alloc_page(GFP_KERNEL);
 	if (!rdev->sb_page) {
 		printk(KERN_ALERT "md: out of memory.\n");
-		return -EINVAL;
+		return -ENOMEM;
 	}
 
 	return 0;
Signature · 1 line
1.5.3.8
author
#5 lore raw

PATCH 4/8 md: analyze_sbs(): Fix potential NULL-pointer dereference.

In reply to Andre Noll

If no device in the array contains a valid super block, "freshest" will be NULL, but we happily dereference that pointer in the subsequent call to validate_super().

Fix it by returning early in this case.

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/md/md.c +4 -0
index 7943df1..bf1499c 100644
@@ -2249,6 +2249,10 @@ static void analyze_sbs(mddev_t * mddev)
 			kick_rdev_from_array(rdev);
 		}
 
+	if (!freshest) {
+		printk(KERN_ERR "md: no valid devices found\n");
+		return;
+	}
 
 	super_types[mddev->major_version].
 		validate_super(mddev, freshest);
Signature · 1 line
1.5.3.8
#6 lore raw

Re: [PATCH 4/8] md: analyze_sbs(): Fix potential NULL-pointer dereference.

Quoted 16 lines · On Tuesday March 25, [email protected] wrote:

If no device in the array contains a valid super block, "freshest" will be NULL, but we happily dereference that pointer in the subsequent call to validate_super().

Fix it by returning early in this case.

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/md/md.c +0 -0
index 7943df1..bf1499c 100644

drivers/md/md.c static void analyze_sbs(mddev_t * mddev) · line 2258

Quoted 10 lines
drivers/md/md.c +4 -0
@@ -2249,6 +2249,10 @@ static void analyze_sbs(mddev_t * mddev)
 			kick_rdev_from_array(rdev);
 		}
 
+	if (!freshest) {
+		printk(KERN_ERR "md: no valid devices found\n");
+		return;
+	}
 
 	super_types[mddev->major_version].
 		validate_super(mddev, freshest);

This shouldn't actually be possible. But I've got to rush off just now. I'll have a deeper look later.

Thanks,
NeilBrown

author
#7 lore raw

PATCH 5/8 md: Simplify restart_array().

In reply to Andre Noll
Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |   46 ++++++++++++++++------------------------------
 1 files changed, 16 insertions(+), 30 deletions(-)
drivers/md/md.c +16 -30
index bf1499c..90b6d1d 100644
@@ -3610,38 +3610,24 @@ static int do_md_run(mddev_t * mddev)
 static int restart_array(mddev_t *mddev)
 {
 	struct gendisk *disk = mddev->gendisk;
-	int err;
 
-	/*
-	 * Complain if it has no devices
-	 */
-	err = -ENXIO;
+	/* Complain if it has no devices */
 	if (list_empty(&mddev->disks))
-		goto out;
-
-	if (mddev->pers) {
-		err = -EBUSY;
-		if (!mddev->ro)
-			goto out;
-
-		mddev->safemode = 0;
-		mddev->ro = 0;
-		set_disk_ro(disk, 0);
-
-		printk(KERN_INFO "md: %s switched to read-write mode.\n",
-			mdname(mddev));
-		/*
-		 * Kick recovery or resync if necessary
-		 */
-		set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
-		md_wakeup_thread(mddev->thread);
-		md_wakeup_thread(mddev->sync_thread);
-		err = 0;
-	} else
-		err = -EINVAL;
-
-out:
-	return err;
+		return -ENXIO;
+	if (!mddev->pers)
+		return -EINVAL;
+	if (!mddev->ro)
+		return -EBUSY;
+	mddev->safemode = 0;
+	mddev->ro = 0;
+	set_disk_ro(disk, 0);
+	printk(KERN_INFO "md: %s switched to read-write mode.\n",
+		mdname(mddev));
+	/* Kick recovery or resync if necessary */
+	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
+	md_wakeup_thread(mddev->thread);
+	md_wakeup_thread(mddev->sync_thread);
+	return 0;
 }
 
 /* similar to deny_write_access, but accounts for our holding a reference
Signature · 1 line
1.5.3.8
author
#8 lore raw

PATCH 6/8 md: get_disk_info(): Don't convert between signed and unsigned and back.

In reply to Andre Noll

The current code copies a signed int from user space, converts it to unsigned and passes the unsigned value to find_rdev_nr() which expects a signed value. Simply pass the signed value from user space directly.

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)
drivers/md/md.c +1 -4
index 90b6d1d..3f22ec7 100644
@@ -4025,15 +4025,12 @@ out:
 static int get_disk_info(mddev_t * mddev, void __user * arg)
 {
 	mdu_disk_info_t info;
-	unsigned int nr;
 	mdk_rdev_t *rdev;
 
 	if (copy_from_user(&info, arg, sizeof(info)))
 		return -EFAULT;
 
-	nr = info.number;
-
-	rdev = find_rdev_nr(mddev, nr);
+	rdev = find_rdev_nr(mddev, info.number);
 	if (rdev) {
 		info.major = MAJOR(rdev->bdev->bd_dev);
 		info.minor = MINOR(rdev->bdev->bd_dev);
Signature · 1 line
1.5.3.8
author
#9 lore raw

PATCH 7/8 md: update_size(): Remove useless variable "fit".

In reply to Andre Noll
Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/md/md.c +1 -2
index 3f22ec7..98a024c 100644
@@ -4459,7 +4459,6 @@ static int update_size(mddev_t *mddev, unsigned long size)
 	mdk_rdev_t * rdev;
 	int rv;
 	struct list_head *tmp;
-	int fit = (size == 0);
 
 	if (mddev->pers->resize == NULL)
 		return -EINVAL;

@@ -4479,7 +4478,7 @@ static int update_size(mddev_t *mddev, unsigned long size)
 		sector_t avail;
 		avail = rdev->size * 2;
 
-		if (fit && (size == 0 || size > avail/2))
+		if (size == 0 || size > avail/2)
 			size = avail/2;
 		if (avail < ((sector_t)size << 1))
 			return -ENOSPC;
Signature · 1 line
1.5.3.8
#10 lore raw

Re: [PATCH 7/8] md: update_size(): Remove useless variable "fit".

Quoted 19 lines · On Saturday April 5, [email protected] wrote:
Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/md/md.c +0 -1
index 3f22ec7..98a024c 100644
@@ -4459,7 +4459,6 @@ static int update_size(mddev_t *mddev, unsigned long size)
 	mdk_rdev_t * rdev;
 	int rv;
 	struct list_head *tmp;
-	int fit = (size == 0);
 
 	if (mddev->pers->resize == NULL)
 		return -EINVAL;

drivers/md/md.c static int update_size(mddev_t *mddev, unsigned long size) · line 4484

Quoted 8 lines
drivers/md/md.c +1 -1
@@ -4479,7 +4478,7 @@ static int update_size(mddev_t *mddev, unsigned long size)
 		sector_t avail;
 		avail = rdev->size * 2;
 
-		if (fit && (size == 0 || size > avail/2))
+		if (size == 0 || size > avail/2)
 			size = avail/2;
 		if (avail < ((sector_t)size << 1))
 			return -ENOSPC;

This is wrong.
The old code tested for "size == 0" once outside a loop. The new code does the test inside a loop which potentially changes size. So after the first time through the loop, it is doing a different test.
Thus patch 8/8 isn't appropriate either.

If you are keen to do some more tidying up, I would really love it if all internal values that are currently stored as 'K' would instead be stored as sectors.
So for example with update_size, we could make the value that is passed in be a number of sectors, get rid of those divisions. Then if we change rdev->size to be stored in sectors, that will get rid of even more mess.

BTW your patches arrive without a valid "To" field. The Date is odd too.

To: unlisted-recipients: ;(no To-header on input)
Apparently-To: <[email protected]>
Subject: [PATCH 7/8] md: update_size(): Remove useless variable "fit".
Date: Sat, 5 Apr 2008 15:54:49 +0200

Thanks,
NeilBrown

author
#11 lore raw

Re: [PATCH 7/8] md: update_size(): Remove useless variable "fit".

Quoted 23 lines · Neil Brown
Quoted 22 lines · On Saturday April 5, [email protected] wrote:
Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/md/md.c +0 -1
index 3f22ec7..98a024c 100644
@@ -4459,7 +4459,6 @@ static int update_size(mddev_t *mddev, unsigned long size)
 	mdk_rdev_t * rdev;
 	int rv;
 	struct list_head *tmp;
-	int fit = (size == 0);
 
 	if (mddev->pers->resize == NULL)
 		return -EINVAL;

@@ -4479,7 +4478,7 @@ static int update_size(mddev_t *mddev, unsigned long size)
 		sector_t avail;
 		avail = rdev->size * 2;
Quoted 10 lines

drivers/md/md.c static int update_size(mddev_t *mddev, unsigned long size) · line 4484

Quoted 5 lines
drivers/md/md.c +1 -1
-		if (fit && (size == 0 || size > avail/2))
+		if (size == 0 || size > avail/2)
 			size = avail/2;
 		if (avail < ((sector_t)size << 1))
 			return -ENOSPC;

This is wrong.
The old code tested for "size == 0" once outside a loop. The new code does the test inside a loop which potentially changes size.

Oops, right you are. Please drop.

Quoted 5 lines

If you are keen to do some more tidying up, I would really love it if all internal values that are currently stored as 'K' would instead be stored as sectors.
So for example with update_size, we could make the value that is passed in be a number of sectors, get rid of those divisions.

Of course the size unit exported to user space programs via sysfs must not change. So if we change update_size() as you propose, the division has to be done in the size_store() instead, which calls update_size() with a value obtained from sysfs.

Therefore I think it's impossible to completely get rid of the divisions/multiplications without breaking user space, but it might still be worth to change the internal representations.

I will have a look at it, but I might need some further advice.

Quoted 2 lines

BTW your patches arrive without a valid "To" field. The Date is odd too.

They were generated by git-format-patch and then bounced to the list. The date is "correct" in the sense that I checked them in back then. I'll see that subsequent patches have a proper date and a valid "To" field.

Thanks
Andre

Signature · 1 line
The only person who always got his work done by Friday was Robinson Crusoe
#12 lore raw

Re: [PATCH 7/8] md: update_size(): Remove useless variable "fit".

Quoted 10 lines · On Wednesday July 9, [email protected] wrote:
Quoted 5 lines

If you are keen to do some more tidying up, I would really love it if all internal values that are currently stored as 'K' would instead be stored as sectors.
So for example with update_size, we could make the value that is passed in be a number of sectors, get rid of those divisions.

Of course the size unit exported to user space programs via sysfs must not change. So if we change update_size() as you propose, the division has to be done in the size_store() instead, which calls update_size() with a value obtained from sysfs.

Correct - the unit exported to userspace must not change. We could conceivably make it fractional though.

    100.5
for 201 sectors.
size_store would do a multiplication:
       err = update_size(mddev, size*2);
size_show would do a division:
	return sprintf(page, "%llu\n", mddev->size/2);
or conceivably
	return sprintf(page, "%llu%s\n", mddev->size/2,
			(mddev->size&1) ? ".5":"");
Quoted 4 lines

Therefore I think it's impossible to completely get rid of the divisions/multiplications without breaking user space, but it might still be worth to change the internal representations.

Exactly how I see it.

Quoted 10 lines

I will have a look at it, but I might need some further advice.

Quoted 2 lines

BTW your patches arrive without a valid "To" field. The Date is odd too.

They were generated by git-format-patch and then bounced to the list. The date is "correct" in the sense that I checked them in back then. I'll see that subsequent patches have a proper date and a valid "To" field.

The date wasn't a problem, it just seemed odd. Your explanation makes perfect sense.
I think you are "supposed" to use
git-send-mail --to [email protected] ...
to send the mail. That sticks in a "To:". Without the "To:" is have to edit the headers to reply to the list (which isn't a big problem).

Thanks,
NeilBrown

author
#13 lore raw

PATCH 8/8 md: Simplify update_size().

In reply to Andre Noll

The condition avail < ((sector_t)size << 1) is equivalent to avail/2 < size and is thus never true because in this case we set size = avail/2 in the line preceeding this test.

Signed-off-by Andre Noll <[email protected]>
---
 drivers/md/md.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)
drivers/md/md.c +3 -9
index 98a024c..f4087bb 100644
@@ -4474,15 +4474,9 @@ static int update_size(mddev_t *mddev, unsigned long size)
 	 */
 	if (mddev->sync_thread)
 		return -EBUSY;
-	rdev_for_each(rdev, tmp, mddev) {
-		sector_t avail;
-		avail = rdev->size * 2;
-
-		if (size == 0 || size > avail/2)
-			size = avail/2;
-		if (avail < ((sector_t)size << 1))
-			return -ENOSPC;
-	}
+	rdev_for_each(rdev, tmp, mddev)
+		if (size == 0 || size > rdev->size)
+			size = rdev->size;
 	rv = mddev->pers->resize(mddev, (sector_t)size *2);
 	if (!rv) {
 		struct block_device *bdev;
Signature · 1 line
1.5.3.8