[RFC: 2.6 patch] simplify drivers/md/md.c:update_size()

[RFC: 2.6 patch] simplify drivers/md/md.c:update_size()

[Adrian Bunk]

While looking at commit 8ddeeae51f2f197b4fafcba117ee8191b49d843e,
I got the impression that this commit couldn't fix anything, since the 
"size" variable can't be changed before "fit" gets used.

Is there any big thinko, or is the patch below that slightly simplifies 
update_size() semantically equivalent to the current code?

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

 drivers/md/md.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- linux-2.6.19-mm1/drivers/md/md.c.old	2006-12-15 00:57:05.000000000 +0100
+++ linux-2.6.19-mm1/drivers/md/md.c	2006-12-15 00:57:42.000000000 +0100
@@ -4039,57 +4039,56 @@
 	 * Generate a 128 bit UUID
 	 */
 	get_random_bytes(mddev->uuid, 16);
 
 	mddev->new_level = mddev->level;
 	mddev->new_chunk = mddev->chunk_size;
 	mddev->new_layout = mddev->layout;
 	mddev->delta_disks = 0;
 
 	mddev->dead = 0;
 	return 0;
 }
 
 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;
 	/* The "size" is the amount of each device that is used.
 	 * This can only make sense for arrays with redundancy.
 	 * linear and raid0 always use whatever space is available
 	 * We can only consider changing the size if no resync
 	 * or reconstruction is happening, and if the new size
 	 * is acceptable. It must fit before the sb_offset or,
 	 * if that is <data_offset, it must fit before the
 	 * size of each device.
 	 * If size is zero, we find the largest size that fits.
 	 */
 	if (mddev->sync_thread)
 		return -EBUSY;
 	ITERATE_RDEV(mddev,rdev,tmp) {
 		sector_t avail;
 		avail = rdev->size * 2;
 
-		if (fit && (size == 0 || size > avail/2))
+		if (size == 0)
 			size = avail/2;
 		if (avail < ((sector_t)size << 1))
 			return -ENOSPC;
 	}
 	rv = mddev->pers->resize(mddev, (sector_t)size *2);
 	if (!rv) {
 		struct block_device *bdev;
 
 		bdev = bdget_disk(mddev->gendisk, 0);
 		if (bdev) {
 			mutex_lock(&bdev->bd_inode->i_mutex);
 			i_size_write(bdev->bd_inode, (loff_t)mddev->array_size << 10);
 			mutex_unlock(&bdev->bd_inode->i_mutex);
 			bdput(bdev);
 		}
 	}
 	return rv;
 }

Re: [RFC: 2.6 patch] simplify drivers/md/md.c:update_size()

[Doug Ledford]

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

On Fri, 2006-12-15 at 01:19 +0100, Adrian Bunk wrote:
> While looking at commit 8ddeeae51f2f197b4fafcba117ee8191b49d843e,
> I got the impression that this commit couldn't fix anything, since the 
> "size" variable can't be changed before "fit" gets used.
> 
> Is there any big thinko, or is the patch below that slightly simplifies 
> update_size() semantically equivalent to the current code?

No, this patch is broken.  Where it fails is specifically the case where
you want to autofit the largest possible size, you have different size
devices, and the first device is not the smallest.  When you hit the
first device, you will set size, then as you repeat the ITERATE_RDEV
loop, when you hit the smaller device, size will be non-0 and you'll
then trigger the later if and return -ENOSPC.  In the case of autofit,
you have to preserve the fit variable instead of looking at size so you
know whether or not to modify the size when you hit a smaller device
later in the list.

> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> ---
> 
>  drivers/md/md.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- linux-2.6.19-mm1/drivers/md/md.c.old	2006-12-15 00:57:05.000000000 +0100
> +++ linux-2.6.19-mm1/drivers/md/md.c	2006-12-15 00:57:42.000000000 +0100
> @@ -4039,57 +4039,56 @@
>  	 * Generate a 128 bit UUID
>  	 */
>  	get_random_bytes(mddev->uuid, 16);
>  
>  	mddev->new_level = mddev->level;
>  	mddev->new_chunk = mddev->chunk_size;
>  	mddev->new_layout = mddev->layout;
>  	mddev->delta_disks = 0;
>  
>  	mddev->dead = 0;
>  	return 0;
>  }
>  
>  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;
>  	/* The "size" is the amount of each device that is used.
>  	 * This can only make sense for arrays with redundancy.
>  	 * linear and raid0 always use whatever space is available
>  	 * We can only consider changing the size if no resync
>  	 * or reconstruction is happening, and if the new size
>  	 * is acceptable. It must fit before the sb_offset or,
>  	 * if that is <data_offset, it must fit before the
>  	 * size of each device.
>  	 * If size is zero, we find the largest size that fits.
>  	 */
>  	if (mddev->sync_thread)
>  		return -EBUSY;
>  	ITERATE_RDEV(mddev,rdev,tmp) {
>  		sector_t avail;
>  		avail = rdev->size * 2;
>  
> -		if (fit && (size == 0 || size > avail/2))
> +		if (size == 0)
>  			size = avail/2;
>  		if (avail < ((sector_t)size << 1))
>  			return -ENOSPC;
>  	}
>  	rv = mddev->pers->resize(mddev, (sector_t)size *2);
>  	if (!rv) {
>  		struct block_device *bdev;
>  
>  		bdev = bdget_disk(mddev->gendisk, 0);
>  		if (bdev) {
>  			mutex_lock(&bdev->bd_inode->i_mutex);
>  			i_size_write(bdev->bd_inode, (loff_t)mddev->array_size << 10);
>  			mutex_unlock(&bdev->bd_inode->i_mutex);
>  			bdput(bdev);
>  		}
>  	}
>  	return rv;
>  }
-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC: 2.6 patch] simplify drivers/md/md.c:update_size()

[Adrian Bunk]

On Thu, Dec 14, 2006 at 07:36:35PM -0500, Doug Ledford wrote:
> On Fri, 2006-12-15 at 01:19 +0100, Adrian Bunk wrote:
> > While looking at commit 8ddeeae51f2f197b4fafcba117ee8191b49d843e,
> > I got the impression that this commit couldn't fix anything, since the 
> > "size" variable can't be changed before "fit" gets used.
> > 
> > Is there any big thinko, or is the patch below that slightly simplifies 
> > update_size() semantically equivalent to the current code?
> 
> No, this patch is broken.  Where it fails is specifically the case where
> you want to autofit the largest possible size, you have different size
> devices, and the first device is not the smallest.  When you hit the
> first device, you will set size, then as you repeat the ITERATE_RDEV
> loop, when you hit the smaller device, size will be non-0 and you'll
> then trigger the later if and return -ENOSPC.  In the case of autofit,
> you have to preserve the fit variable instead of looking at size so you
> know whether or not to modify the size when you hit a smaller device
> later in the list.
>...

OK, sorry, I've got my thinko:

ITERATE_RDEV() is a loop.

That's what I missed.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed