[PATCH 0/8] md: More patches for md.c

[PATCH 0/8] md: More patches for md.c

[Andre Noll]

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
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[PATCH 1/8] md: Simplify uuid_equal().

[Andre Noll]

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2580ac1..c559b9e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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;
-- 
1.5.3.8

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

[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 <maan@systemlinux.org>
---
 drivers/md/md.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index c559b9e..58762dd 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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);
-- 
1.5.3.8

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

[Neil Brown]

On Sunday March 23, maan@systemlinux.org 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 <maan@systemlinux.org>
> ---
>  drivers/md/md.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index c559b9e..58762dd 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -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

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

[Andre Noll]

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

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 58762dd..7943df1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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;
-- 
1.5.3.8

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

[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 <maan@systemlinux.org>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 7943df1..bf1499c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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);
-- 
1.5.3.8

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

[Neil Brown]

On Tuesday March 25, maan@systemlinux.org 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 <maan@systemlinux.org>
> ---
>  drivers/md/md.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 7943df1..bf1499c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -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

[PATCH 5/8] md: Simplify restart_array().

[Andre Noll]

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |   46 ++++++++++++++++------------------------------
 1 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index bf1499c..90b6d1d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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
-- 
1.5.3.8

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

[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 <maan@systemlinux.org>
---
 drivers/md/md.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 90b6d1d..3f22ec7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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);
-- 
1.5.3.8

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

[Andre Noll]

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/md.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3f22ec7..98a024c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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;
-- 
1.5.3.8

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

[Neil Brown]

On Saturday April 5, maan@systemlinux.org wrote:
> 
> Signed-off-by: Andre Noll <maan@systemlinux.org>
> ---
>  drivers/md/md.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 3f22ec7..98a024c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -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;

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: <maan@skl-net.de>
Subject: [PATCH 7/8] md: update_size(): Remove useless variable "fit".
Date:	Sat, 5 Apr 2008 15:54:49 +0200

Thanks,
NeilBrown

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

[Andre Noll]

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

On 09:08, Neil Brown wrote:
> On Saturday April 5, maan@systemlinux.org wrote:
> > 
> > Signed-off-by: Andre Noll <maan@systemlinux.org>
> > ---
> >  drivers/md/md.c |    3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 3f22ec7..98a024c 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -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;
> 
> 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.

> 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.

> 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
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Neil Brown]

On Wednesday July 9, maan@systemlinux.org wrote:
> > 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":"");

> 
> 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.

> 
> I will have a look at it, but I might need some further advice.
> 
> > 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 linux-raid@vger.kernel.org ...
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

[PATCH 8/8] md: Simplify update_size().

[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 <maan@systemlinux.org>
---
 drivers/md/md.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 98a024c..f4087bb 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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;
-- 
1.5.3.8