[PATCH 0/8] Set of fixes for expansion

[PATCH 0/8] Set of fixes for expansion

[Adam Kwolek]

Fixes are related to reshape finalization and next array reshape start.
First patch allows to reach checkpointing end of array. This enables
reshape finalization code in set_array_state() (not used so far).

Then array finalization code in mdmon is put in to single place 
and setting array size mechanism after reshape in mdmon is enabled.
This makes setting size in mdadm not necessary.

Some code cleanup (debug strings, put common code in to function)
was introduced also.

BR
Adam

---

Adam Kwolek (8):
      imsm: move common code for array size calculation to function
      imsm: FIX: Debug strings cleanup
      FIX: Do not set array size after reshape in mdadm
      imsm: fix: imsm_num_data_members() can return error
      imsm: FIX: put expansion finalization in to one place
      imsm: FIX: Size is already set in metadata
      imsm: FIX: array size is wrong
      FIX: Last checkpoint is not set


 Grow.c        |   35 --------------
 monitor.c     |   18 +++++++
 super-intel.c |  141 ++++++++++++++++++++++++++-------------------------------
 3 files changed, 82 insertions(+), 112 deletions(-)

-- 
Signature

[PATCH 1/8] FIX: Last checkpoint is not set

[Adam Kwolek]

When reshape is finished monitor has to set last checkpoint
to the array end to allow metatdata for reshape finalization.
Metadata has to know if reshape is finished or it is broken
On reshape finish metadata finalization is required.
When reshape is broken, metadata must remain as is to allow
for reshape restart from checkpoint.

This can be resolved based on reshape_position sysfs entry.
When it is equal to 'none', it means that md finishes work.
In such situation move checkpoint to the end of array.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 monitor.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8d33a5d..7aa0d91 100644
--- a/monitor.c
+++ b/monitor.c
@@ -366,6 +366,24 @@ static int read_and_act(struct active_array *a)
 		 */
 		if (sync_completed != 0)
 			a->last_checkpoint = sync_completed;
+		/* we have to make difference
+		 * becaurse of reshape finish reason.
+		 * if array reshape is really finished:
+		 *        set check point to the end, this allows
+		 *        set_array_state() to finalize reshape in metadata
+		 * if reshape if broken: do not set checkpoint to the end
+		 *        this allows for reshape restart from checkpoint
+		 */
+		if ((a->curr_action != reshape) &&
+		    (a->prev_action == reshape)) {
+			char buf[PATH_MAX];
+			if ((sysfs_get_str(&a->info, NULL,
+					  "reshape_position",
+					  buf,
+					  sizeof(buf)) >= 0) &&
+			     strncmp(buf, "none", 4) == 0)
+				a->last_checkpoint = a->info.component_size;
+		}
 		a->container->ss->set_array_state(a, a->curr_state <= clean);
 		a->last_checkpoint = sync_completed;
 	}

[PATCH 2/8] imsm: FIX: array size is wrong

[Adam Kwolek]

Calculation of size is almost ok, except concept of blocks.
Size for setting in md has to be divided by 2 to be correct.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index ee0d9c4..42f7065 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5187,7 +5187,7 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
 					<< SECT_PER_MB_SHIFT;
 				dev->size_low = __cpu_to_le32((__u32) array_blocks);
 				dev->size_high = __cpu_to_le32((__u32) (array_blocks >> 32));
-				a->info.custom_array_size = array_blocks;
+				a->info.custom_array_size = array_blocks/2;
 				a->check_reshape = 1; /* encourage manager to update
 						       * array size
 						       */

[PATCH 3/8] imsm: FIX: Size is already set in metadata

[Adam Kwolek]

In metadata size is set already during migration initialization.
There is no reason for second /the same/ calculation.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 42f7065..62b13b0 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5185,15 +5185,12 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
 				/* round array size down to closest MB */
 				array_blocks = (array_blocks >> SECT_PER_MB_SHIFT)
 					<< SECT_PER_MB_SHIFT;
-				dev->size_low = __cpu_to_le32((__u32) array_blocks);
-				dev->size_high = __cpu_to_le32((__u32) (array_blocks >> 32));
 				a->info.custom_array_size = array_blocks/2;
 				a->check_reshape = 1; /* encourage manager to update
 						       * array size
 						       */
-				super->updates_pending++;
 				imsm_progress_container_reshape(super);
-			}				
+			}
 		}
 	}
 

[PATCH 4/8] imsm: FIX: put expansion finalization in to one place

[Adam Kwolek]

When a->last_checkpoint variable can reach array end,
reshape finalization can be put in to single place.

There is no need to reset migration variables.
imsm_set_disk() will call end_migration() and this sets
all migration variables to required values.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 62b13b0..d6c2756 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5175,10 +5175,7 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
 			if (a->last_checkpoint >= a->info.component_size) {
 				unsigned long long array_blocks;
 				int used_disks;
-				/* it seems the reshape is all done */
-				dev->vol.migr_state = 0;
-				dev->vol.migr_type = 0;
-				dev->vol.curr_migr_unit = 0;
+				struct mdinfo *mdi;
 
 				used_disks = imsm_num_data_members(dev, -1);
 				array_blocks = map->blocks_per_member * used_disks;
@@ -5189,6 +5186,13 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
 				a->check_reshape = 1; /* encourage manager to update
 						       * array size
 						       */
+
+				/* finalize online capacity expansion/reshape */
+				for (mdi = a->info.devs; mdi; mdi = mdi->next)
+					imsm_set_disk(a,
+						      mdi->disk.raid_disk,
+						      mdi->curr_state);
+
 				imsm_progress_container_reshape(super);
 			}
 		}
@@ -5255,19 +5259,6 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
 		super->updates_pending++;
 	}
 
-	/* manage online capacity expansion/reshape */
-	if ((a->curr_action != reshape) &&
-	    (a->prev_action == reshape)) {
-		struct mdinfo *mdi;
-
-		/* finalize online capacity expansion/reshape */
-		for (mdi = a->info.devs; mdi; mdi = mdi->next)
-			imsm_set_disk(a, mdi->disk.raid_disk, mdi->curr_state);
-
-		/* check next volume reshape */
-		imsm_progress_container_reshape(super);
-	}
-
 	return consistent;
 }
 

[PATCH 5/8] imsm: fix: imsm_num_data_members() can return error

[Adam Kwolek]

imsm_num_data_members() can indicate error by returning 0 value
In such case size cannot be set based on 0 value.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index d6c2756..f604589 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5178,15 +5178,22 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
 				struct mdinfo *mdi;
 
 				used_disks = imsm_num_data_members(dev, -1);
-				array_blocks = map->blocks_per_member * used_disks;
-				/* round array size down to closest MB */
-				array_blocks = (array_blocks >> SECT_PER_MB_SHIFT)
-					<< SECT_PER_MB_SHIFT;
-				a->info.custom_array_size = array_blocks/2;
-				a->check_reshape = 1; /* encourage manager to update
-						       * array size
-						       */
-
+				if (used_disks > 0) {
+					array_blocks =
+						map->blocks_per_member *
+						used_disks;
+					/* round array size down to closest MB
+					*/
+					array_blocks = (array_blocks
+							>> SECT_PER_MB_SHIFT)
+							<< SECT_PER_MB_SHIFT;
+					a->info.custom_array_size =
+						array_blocks/2;
+					/* encourage manager to update array
+					 * size
+					 */
+					a->check_reshape = 1;
+				}
 				/* finalize online capacity expansion/reshape */
 				for (mdi = a->info.devs; mdi; mdi = mdi->next)
 					imsm_set_disk(a,

[PATCH 6/8] FIX: Do not set array size after reshape in mdadm

[Adam Kwolek]

Do not set array size after reshape in mdadm,
as it is up to mdmon metadata handler (set_array_state()) now.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 Grow.c |   35 -----------------------------------
 1 files changed, 0 insertions(+), 35 deletions(-)

diff --git a/Grow.c b/Grow.c
index 8229b4d..a74da89 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2045,41 +2045,6 @@ started:
 		}
 	}
 
-	/* set new array size if required customer_array_size is used
-	 * by this metadata.
-	 */
-	if (reshape.before.data_disks !=
-	    reshape.after.data_disks &&
-	    info->custom_array_size) {
-		struct mdinfo *info2;
-		char *subarray = strchr(info->text_version+1, '/')+1;
-
-		info2 = st->ss->container_content(st, subarray);
-		if (info2) {
-			unsigned long long current_size = 0;
-			unsigned long long new_size =
-				info2->custom_array_size/2;
-
-			if (sysfs_get_ll(sra,
-					 NULL,
-					 "array_size",
-					 &current_size) == 0 &&
-			    new_size > current_size) {
-				if (sysfs_set_num(sra, NULL,
-						  "array_size", new_size)
-				    < 0)
-					dprintf("Error: Cannot"
-						" set array size");
-				else
-					dprintf("Array size "
-						"changed");
-				dprintf(" from %llu to %llu.\n",
-					current_size, new_size);
-			}
-			sysfs_free(info2);
-		}
-	}
-
 	if (info->new_level != reshape.level) {
 
 		c = map_num(pers, info->new_level);

[PATCH 7/8] imsm: FIX: Debug strings cleanup

[Adam Kwolek]

Some debug strings remains as they were introduced,
before code was moved to separate function.
Information displayed by debug information in not all cases
was correct.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index f604589..e523313 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5858,15 +5858,15 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
 	int ret_val = 0;
 	unsigned int dev_id;
 
-	dprintf("imsm: imsm_process_update() for update_reshape\n");
+	dprintf("imsm: apply_reshape_container_disks_update()\n");
 
 	/* enable spares to use in array */
 	for (i = 0; i < delta_disks; i++) {
 		new_disk = get_disk_super(super,
 					  major(u->new_disks[i]),
 					  minor(u->new_disks[i]));
-		dprintf("imsm: imsm_process_update(): new disk "
-			"for reshape is: %i:%i (%p, index = %i)\n",
+		dprintf("imsm: new disk for reshape is: %i:%i "
+			"(%p, index = %i)\n",
 			major(u->new_disks[i]), minor(u->new_disks[i]),
 			new_disk, new_disk->index);
 		if ((new_disk == NULL) ||
@@ -5882,8 +5882,8 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
 		new_disk->disk.status &= ~SPARE_DISK;
 	}
 
-	dprintf("imsm: process_update(): update_reshape: volume set"
-		" mpb->num_raid_devs = %i\n", mpb->num_raid_devs);
+	dprintf("imsm: volume set mpb->num_raid_devs = %i\n",
+		mpb->num_raid_devs);
 	/* manage changes in volume
 	 */
 	for (dev_id = 0; dev_id < mpb->num_raid_devs; dev_id++) {
@@ -5912,8 +5912,8 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
 		if (devices_to_reshape) {
 			int used_disks;
 
-			dprintf("process_update(): modifying "
-				"subdev: %i\n", id->index);
+			dprintf("imsm: modifying subdev: %i\n",
+				id->index);
 			devices_to_reshape--;
 			newdev->vol.migr_state = 1;
 			newdev->vol.curr_migr_unit = 0;

[PATCH 8/8] imsm: move common code for array size calculation to function

[Adam Kwolek]

Array size calculation is made in the same way in few places in code.
Make function imsm_set_device_size() for this common code.

Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
---

 super-intel.c |   74 +++++++++++++++++++++++++--------------------------------
 1 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index e523313..c70d44b 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5052,6 +5052,37 @@ static void handle_missing(struct intel_super *super, struct imsm_dev *dev)
 	super->updates_pending++;
 }
 
+static unsigned long long imsm_set_array_size(struct imsm_dev *dev)
+{
+	int used_disks = imsm_num_data_members(dev, 0);
+	unsigned long long array_blocks;
+	struct imsm_map *map;
+
+	if (used_disks == 0) {
+		/* when problems occures
+		 * return current array_blocks value
+		 */
+		array_blocks = __le32_to_cpu(dev->size_high);
+		array_blocks = array_blocks << 32;
+		array_blocks += __le32_to_cpu(dev->size_low);
+
+		return array_blocks;
+	}
+
+	/* set array size in metadata
+	 */
+	map = get_imsm_map(dev, 0);
+	array_blocks = map->blocks_per_member * used_disks;
+
+	/* round array size down to closest MB
+	 */
+	array_blocks = (array_blocks >> SECT_PER_MB_SHIFT) << SECT_PER_MB_SHIFT;
+	dev->size_low = __cpu_to_le32((__u32)array_blocks);
+	dev->size_high = __cpu_to_le32((__u32)(array_blocks >> 32));
+
+	return array_blocks;
+}
+
 static void imsm_set_disk(struct active_array *a, int n, int state);
 
 static void imsm_progress_container_reshape(struct intel_super *super)
@@ -5071,7 +5102,6 @@ static void imsm_progress_container_reshape(struct intel_super *super)
 		struct imsm_map *map = get_imsm_map(dev, 0);
 		struct imsm_map *map2;
 		int prev_num_members;
-		int used_disks;
 
 		if (dev->vol.migr_state)
 			return;
@@ -5099,26 +5129,7 @@ static void imsm_progress_container_reshape(struct intel_super *super)
 		memcpy(map2, map, copy_map_size);
 		map2->num_members = prev_num_members;
 
-		/* calculate new size
-		 */
-		used_disks = imsm_num_data_members(dev, 0);
-		if (used_disks) {
-			unsigned long long array_blocks;
-
-			array_blocks =
-				map->blocks_per_member
-				* used_disks;
-			/* round array size down to closest MB
-			 */
-			array_blocks = (array_blocks
-					>> SECT_PER_MB_SHIFT)
-				<< SECT_PER_MB_SHIFT;
-			dev->size_low =
-				__cpu_to_le32((__u32)array_blocks);
-			dev->size_high =
-				__cpu_to_le32(
-					(__u32)(array_blocks >> 32));
-		}
+		imsm_set_array_size(dev);
 		super->updates_pending++;
 	}
 }
@@ -5910,8 +5921,6 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
 		/* update one device only
 		 */
 		if (devices_to_reshape) {
-			int used_disks;
-
 			dprintf("imsm: modifying subdev: %i\n",
 				id->index);
 			devices_to_reshape--;
@@ -5929,24 +5938,7 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
 			newmap = get_imsm_map(newdev, 1);
 			memcpy(newmap, oldmap, sizeof_imsm_map(oldmap));
 
-			/* calculate new size
-			 */
-			used_disks = imsm_num_data_members(newdev, 0);
-			if (used_disks) {
-				unsigned long long array_blocks;
-
-				array_blocks =
-					newmap->blocks_per_member * used_disks;
-				/* round array size down to closest MB
-				 */
-				array_blocks = (array_blocks
-						>> SECT_PER_MB_SHIFT)
-					<< SECT_PER_MB_SHIFT;
-				newdev->size_low =
-					__cpu_to_le32((__u32)array_blocks);
-				newdev->size_high =
-					__cpu_to_le32((__u32)(array_blocks >> 32));
-			}
+			imsm_set_array_size(newdev);
 		}
 
 		sp = (void **)id->dev;

Re: [PATCH 2/8] imsm: FIX: array size is wrong

[NeilBrown]

On Tue, 01 Feb 2011 14:49:12 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> Calculation of size is almost ok, except concept of blocks.
> Size for setting in md has to be divided by 2 to be correct.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  super-intel.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index ee0d9c4..42f7065 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5187,7 +5187,7 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
>  					<< SECT_PER_MB_SHIFT;
>  				dev->size_low = __cpu_to_le32((__u32) array_blocks);
>  				dev->size_high = __cpu_to_le32((__u32) (array_blocks >> 32));
> -				a->info.custom_array_size = array_blocks;
> +				a->info.custom_array_size = array_blocks/2;
>  				a->check_reshape = 1; /* encourage manager to update
>  						       * array size
>  						       */
> 


I think the original code is correct.
custom_array_size is measured in sectors.
In grow.c we divide by 2 before comparing with the "array_size" sysfs
attribute which is measured in kilobytes.
Also in getinfo_super_imsm_volume, custom_array_size is set from
the same size_low/size_high fields as used here.

However in manage_member custom_array_size is compared directly
with "array_size" which is wrong and is probably causing whatever
problem you experienced.

So I'll change this patch to fix that up instead - see below.

Thanks,
NeilBrown

commit 02eedb57aa0c6f7fdeda1b612f35545ab4eee58f
Author: Adam Kwolek <adam.kwolek@intel.com>
Date:   Thu Feb 3 17:40:18 2011 +1100

    imsm: FIX: array size is wrong
    
    Calculation of size is almost ok, except concept of blocks.
    Size for setting in md has to be divided by 2 to be correct.
    
    Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/managemon.c b/managemon.c
index 63c9705..6001f6a 100644
--- a/managemon.c
+++ b/managemon.c
@@ -549,9 +549,9 @@ static void manage_member(struct mdstat_ent *mdstat,
 			disk_init_and_add(newd, d, newa);
 		}
 		if (sysfs_get_ll(info, NULL, "array_size", &array_size) == 0
-		    && a->info.custom_array_size > array_size) {
+		    && a->info.custom_array_size > array_size*2) {
 			sysfs_set_num(info, NULL, "array_size",
-				      a->info.custom_array_size);
+				      a->info.custom_array_size/2);
 		}
 	out2:
 		sysfs_free(info);

Re: [PATCH 3/8] imsm: FIX: Size is already set in metadata

[NeilBrown]

On Tue, 01 Feb 2011 14:49:20 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> In metadata size is set already during migration initialization.
> There is no reason for second /the same/ calculation.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  super-intel.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index 42f7065..62b13b0 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5185,15 +5185,12 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
>  				/* round array size down to closest MB */
>  				array_blocks = (array_blocks >> SECT_PER_MB_SHIFT)
>  					<< SECT_PER_MB_SHIFT;
> -				dev->size_low = __cpu_to_le32((__u32) array_blocks);
> -				dev->size_high = __cpu_to_le32((__u32) (array_blocks >> 32));
>  				a->info.custom_array_size = array_blocks/2;
>  				a->check_reshape = 1; /* encourage manager to update
>  						       * array size
>  						       */
> -				super->updates_pending++;
>  				imsm_progress_container_reshape(super);
> -			}				
> +			}
>  		}
>  	}
>  
> 

Doing the same calculation in two placed does seem wrong, but are you
sure this is the right one to remove?

The available space in the array does not change until the reshape
completes.  So changing dev->size_{low,high} during initialisation
seems wrong.

Are you able to confirm what the windows driver does?

If we do set the size early, then we have to be careful to reduce to the
original size when assembling an array that is in the middle of reshape.

So I won't apply this patch now, and will wait for you to confirm when the
windows driver updates dev->size_{low,high}

Thanks,
NeilBrown

Re: [PATCH 6/8] FIX: Do not set array size after reshape in mdadm

[NeilBrown]

On Tue, 01 Feb 2011 14:49:44 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> Do not set array size after reshape in mdadm,
> as it is up to mdmon metadata handler (set_array_state()) now.

I'm not sure about this...

I agree that it is probably more appropriate for mdmon to impose the new
array_size rather than for mdadm to do it.
In general, if the kernel does something for native metadata, then mdmon
should probably do it for external metadata (though there might be exceptions
to this).

However it is not the metadata handler which should do this (and it
currently doesn't, which is good). The metadata handler should set
custom_array_array, and the core code of mdmon should use this number
to set array_size.
And I don't see that mdmon does this at the moment.  It sets the array size
when the reshape starts (which is possibly wrong) but it does not set
it when the reshape finishes (which is the right time).

Could you please review all of this and either point out to me where
I am wrong, or fix up the code to do the right thing, thanks.

I won't apply this patch now, but am happy to apply it once I'm sure
mdmon is performing this task.

NeilBrown


> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Grow.c |   35 -----------------------------------
>  1 files changed, 0 insertions(+), 35 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 8229b4d..a74da89 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -2045,41 +2045,6 @@ started:
>  		}
>  	}
>  
> -	/* set new array size if required customer_array_size is used
> -	 * by this metadata.
> -	 */
> -	if (reshape.before.data_disks !=
> -	    reshape.after.data_disks &&
> -	    info->custom_array_size) {
> -		struct mdinfo *info2;
> -		char *subarray = strchr(info->text_version+1, '/')+1;
> -
> -		info2 = st->ss->container_content(st, subarray);
> -		if (info2) {
> -			unsigned long long current_size = 0;
> -			unsigned long long new_size =
> -				info2->custom_array_size/2;
> -
> -			if (sysfs_get_ll(sra,
> -					 NULL,
> -					 "array_size",
> -					 &current_size) == 0 &&
> -			    new_size > current_size) {
> -				if (sysfs_set_num(sra, NULL,
> -						  "array_size", new_size)
> -				    < 0)
> -					dprintf("Error: Cannot"
> -						" set array size");
> -				else
> -					dprintf("Array size "
> -						"changed");
> -				dprintf(" from %llu to %llu.\n",
> -					current_size, new_size);
> -			}
> -			sysfs_free(info2);
> -		}
> -	}
> -
>  	if (info->new_level != reshape.level) {
>  
>  		c = map_num(pers, info->new_level);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] Set of fixes for expansion

[NeilBrown]

On Tue, 01 Feb 2011 14:48:56 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> Fixes are related to reshape finalization and next array reshape start.
> First patch allows to reach checkpointing end of array. This enables
> reshape finalization code in set_array_state() (not used so far).
> 
> Then array finalization code in mdmon is put in to single place 
> and setting array size mechanism after reshape in mdmon is enabled.
> This makes setting size in mdadm not necessary.
> 
> Some code cleanup (debug strings, put common code in to function)
> was introduced also.
> 
> BR
> Adam
> 
> ---
> 
> Adam Kwolek (8):
>       imsm: move common code for array size calculation to function
>       imsm: FIX: Debug strings cleanup
>       FIX: Do not set array size after reshape in mdadm
>       imsm: fix: imsm_num_data_members() can return error
>       imsm: FIX: put expansion finalization in to one place
>       imsm: FIX: Size is already set in metadata
>       imsm: FIX: array size is wrong
>       FIX: Last checkpoint is not set
> 
> 
>  Grow.c        |   35 --------------
>  monitor.c     |   18 +++++++
>  super-intel.c |  141 ++++++++++++++++++++++++++-------------------------------
>  3 files changed, 82 insertions(+), 112 deletions(-)
> 


Thanks.
I have applied these, except for the few that I commented on directly.

NeilBrown

RE: [PATCH 3/8] imsm: FIX: Size is already set in metadata

[Kwolek, Adam]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of NeilBrown
> Sent: Thursday, February 03, 2011 7:45 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 3/8] imsm: FIX: Size is already set in metadata
> 
> On Tue, 01 Feb 2011 14:49:20 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > In metadata size is set already during migration initialization.
> > There is no reason for second /the same/ calculation.
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  super-intel.c |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 42f7065..62b13b0 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5185,15 +5185,12 @@ static int imsm_set_array_state(struct
> active_array *a, int consistent)
> >  				/* round array size down to closest MB */
> >  				array_blocks = (array_blocks >>
> SECT_PER_MB_SHIFT)
> >  					<< SECT_PER_MB_SHIFT;
> > -				dev->size_low = __cpu_to_le32((__u32)
> array_blocks);
> > -				dev->size_high = __cpu_to_le32((__u32)
> (array_blocks >> 32));
> >  				a->info.custom_array_size = array_blocks/2;
> >  				a->check_reshape = 1; /* encourage manager to
> update
> >  						       * array size
> >  						       */
> > -				super->updates_pending++;
> >  				imsm_progress_container_reshape(super);
> > -			}
> > +			}
> >  		}
> >  	}
> >
> >
> 
> Doing the same calculation in two placed does seem wrong, but are you
> sure this is the right one to remove?
> 
> The available space in the array does not change until the reshape
> completes.  So changing dev->size_{low,high} during initialisation
> seems wrong.
> 
> Are you able to confirm what the windows driver does?

Yes (look below).

> 
> If we do set the size early, then we have to be careful to reduce to
> the
> original size when assembling an array that is in the middle of
> reshape.

Yes (as you describes in your roadmap, it will be part of 3.2.1 code changes/investigation).
Size for array with reshape in progress process, should be calculated based on second map information.

> 
> So I won't apply this patch now, and will wait for you to confirm when
> the
> windows driver updates dev->size_{low,high}
> 
> Thanks,
> NeilBrown

I've (double) checked Windows driver behavior. Array size is set during expansion initialization (first metadata update).
This means, that code for size changes on reshape end has to be removed as it was proposed.
For volume expansion, size in imsm metadata has to be set in 2 places/cases:
1. in metadata update during reshape update processing for first array(it is implemented)
2. In initialization reshape by set_array_state on second array (it is implemented)

BR
Adam

> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/8] imsm: FIX: array size is wrong

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, February 03, 2011 7:41 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 2/8] imsm: FIX: array size is wrong
> 
> On Tue, 01 Feb 2011 14:49:12 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > Calculation of size is almost ok, except concept of blocks.
> > Size for setting in md has to be divided by 2 to be correct.
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  super-intel.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index ee0d9c4..42f7065 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5187,7 +5187,7 @@ static int imsm_set_array_state(struct
> active_array *a, int consistent)
> >  					<< SECT_PER_MB_SHIFT;
> >  				dev->size_low = __cpu_to_le32((__u32)
> array_blocks);
> >  				dev->size_high = __cpu_to_le32((__u32)
> (array_blocks >> 32));
> > -				a->info.custom_array_size = array_blocks;
> > +				a->info.custom_array_size = array_blocks/2;
> >  				a->check_reshape = 1; /* encourage manager to
> update
> >  						       * array size
> >  						       */
> >
> 
> 
> I think the original code is correct.
> custom_array_size is measured in sectors.
> In grow.c we divide by 2 before comparing with the "array_size" sysfs
> attribute which is measured in kilobytes.
> Also in getinfo_super_imsm_volume, custom_array_size is set from
> the same size_low/size_high fields as used here.
> 
> However in manage_member custom_array_size is compared directly
> with "array_size" which is wrong and is probably causing whatever
> problem you experienced.
> 
> So I'll change this patch to fix that up instead - see below.
> 
> Thanks,
> NeilBrown

Thanks 
Adam

> 
> commit 02eedb57aa0c6f7fdeda1b612f35545ab4eee58f
> Author: Adam Kwolek <adam.kwolek@intel.com>
> Date:   Thu Feb 3 17:40:18 2011 +1100
> 
>     imsm: FIX: array size is wrong
> 
>     Calculation of size is almost ok, except concept of blocks.
>     Size for setting in md has to be divided by 2 to be correct.
> 
>     Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
>     Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/managemon.c b/managemon.c
> index 63c9705..6001f6a 100644
> --- a/managemon.c
> +++ b/managemon.c
> @@ -549,9 +549,9 @@ static void manage_member(struct mdstat_ent
> *mdstat,
>  			disk_init_and_add(newd, d, newa);
>  		}
>  		if (sysfs_get_ll(info, NULL, "array_size", &array_size) ==
> 0
> -		    && a->info.custom_array_size > array_size) {
> +		    && a->info.custom_array_size > array_size*2) {
>  			sysfs_set_num(info, NULL, "array_size",
> -				      a->info.custom_array_size);
> +				      a->info.custom_array_size/2);
>  		}
>  	out2:
>  		sysfs_free(info);

RE: [PATCH 6/8] FIX: Do not set array size after reshape in mdadm

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, February 03, 2011 7:57 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 6/8] FIX: Do not set array size after reshape in
> mdadm
> 
> On Tue, 01 Feb 2011 14:49:44 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > Do not set array size after reshape in mdadm,
> > as it is up to mdmon metadata handler (set_array_state()) now.
> 
> I'm not sure about this...
> 
> I agree that it is probably more appropriate for mdmon to impose the
> new
> array_size rather than for mdadm to do it.
> In general, if the kernel does something for native metadata, then
> mdmon
> should probably do it for external metadata (though there might be
> exceptions
> to this).
> 
> However it is not the metadata handler which should do this (and it
> currently doesn't, which is good). The metadata handler should set
> custom_array_array, and the core code of mdmon should use this number
> to set array_size.
> And I don't see that mdmon does this at the moment.  It sets the array
> size
> when the reshape starts (which is possibly wrong) but it does not set
> it when the reshape finishes (which is the right time).
> 
> Could you please review all of this and either point out to me where
> I am wrong, or fix up the code to do the right thing, thanks.
> 
> I won't apply this patch now, but am happy to apply it once I'm sure
> mdmon is performing this task.
> 
> NeilBrown

I think mdmon takes carry about size now.
If set_array_state() sets a->check_reshape variable to 1 (super-intel.c:near5206) on reshape end,
and a->info.custom_array_size will be set to bigger value Managemon will set it in to md (manage_member():551)
(the only problem I faced was seze value to set '/2' in one of previous path that you commented already)

In my tests mdmon does his job for size changes :).

BR
Adam



> 
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  Grow.c |   35 -----------------------------------
> >  1 files changed, 0 insertions(+), 35 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 8229b4d..a74da89 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -2045,41 +2045,6 @@ started:
> >  		}
> >  	}
> >
> > -	/* set new array size if required customer_array_size is used
> > -	 * by this metadata.
> > -	 */
> > -	if (reshape.before.data_disks !=
> > -	    reshape.after.data_disks &&
> > -	    info->custom_array_size) {
> > -		struct mdinfo *info2;
> > -		char *subarray = strchr(info->text_version+1, '/')+1;
> > -
> > -		info2 = st->ss->container_content(st, subarray);
> > -		if (info2) {
> > -			unsigned long long current_size = 0;
> > -			unsigned long long new_size =
> > -				info2->custom_array_size/2;
> > -
> > -			if (sysfs_get_ll(sra,
> > -					 NULL,
> > -					 "array_size",
> > -					 &current_size) == 0 &&
> > -			    new_size > current_size) {
> > -				if (sysfs_set_num(sra, NULL,
> > -						  "array_size", new_size)
> > -				    < 0)
> > -					dprintf("Error: Cannot"
> > -						" set array size");
> > -				else
> > -					dprintf("Array size "
> > -						"changed");
> > -				dprintf(" from %llu to %llu.\n",
> > -					current_size, new_size);
> > -			}
> > -			sysfs_free(info2);
> > -		}
> > -	}
> > -
> >  	if (info->new_level != reshape.level) {
> >
> >  		c = map_num(pers, info->new_level);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] imsm: FIX: Size is already set in metadata

[NeilBrown]

On Thu, 3 Feb 2011 09:52:46 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> > -----Original Message-----
> > From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> > owner@vger.kernel.org] On Behalf Of NeilBrown
> > Sent: Thursday, February 03, 2011 7:45 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 3/8] imsm: FIX: Size is already set in metadata
> > 
> > On Tue, 01 Feb 2011 14:49:20 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> > 
> > > In metadata size is set already during migration initialization.
> > > There is no reason for second /the same/ calculation.
> > >
> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > ---
> > >
> > >  super-intel.c |    5 +----
> > >  1 files changed, 1 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 42f7065..62b13b0 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -5185,15 +5185,12 @@ static int imsm_set_array_state(struct
> > active_array *a, int consistent)
> > >  				/* round array size down to closest MB */
> > >  				array_blocks = (array_blocks >>
> > SECT_PER_MB_SHIFT)
> > >  					<< SECT_PER_MB_SHIFT;
> > > -				dev->size_low = __cpu_to_le32((__u32)
> > array_blocks);
> > > -				dev->size_high = __cpu_to_le32((__u32)
> > (array_blocks >> 32));
> > >  				a->info.custom_array_size = array_blocks/2;
> > >  				a->check_reshape = 1; /* encourage manager to
> > update
> > >  						       * array size
> > >  						       */
> > > -				super->updates_pending++;
> > >  				imsm_progress_container_reshape(super);
> > > -			}
> > > +			}
> > >  		}
> > >  	}
> > >
> > >
> > 
> > Doing the same calculation in two placed does seem wrong, but are you
> > sure this is the right one to remove?
> > 
> > The available space in the array does not change until the reshape
> > completes.  So changing dev->size_{low,high} during initialisation
> > seems wrong.
> > 
> > Are you able to confirm what the windows driver does?
> 
> Yes (look below).
> 
> > 
> > If we do set the size early, then we have to be careful to reduce to
> > the
> > original size when assembling an array that is in the middle of
> > reshape.
> 
> Yes (as you describes in your roadmap, it will be part of 3.2.1 code changes/investigation).
> Size for array with reshape in progress process, should be calculated based on second map information.
> 
> > 
> > So I won't apply this patch now, and will wait for you to confirm when
> > the
> > windows driver updates dev->size_{low,high}
> > 
> > Thanks,
> > NeilBrown
> 
> I've (double) checked Windows driver behavior. Array size is set during expansion initialization (first metadata update).

That sounds like a weird design decision, but I guess we have to work with
it.  I've applied that patch now.
These patches are still going in to my devel-3.2 branch.

Thanks,
NeilBrown


> This means, that code for size changes on reshape end has to be removed as it was proposed.
> For volume expansion, size in imsm metadata has to be set in 2 places/cases:
> 1. in metadata update during reshape update processing for first array(it is implemented)
> 2. In initialization reshape by set_array_state on second array (it is implemented)
> 
> BR
> Adam
> 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid"
> > in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] FIX: Do not set array size after reshape in mdadm

[NeilBrown]

On Thu, 3 Feb 2011 10:08:12 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Thursday, February 03, 2011 7:57 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 6/8] FIX: Do not set array size after reshape in
> > mdadm
> > 
> > On Tue, 01 Feb 2011 14:49:44 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> > 
> > > Do not set array size after reshape in mdadm,
> > > as it is up to mdmon metadata handler (set_array_state()) now.
> > 
> > I'm not sure about this...
> > 
> > I agree that it is probably more appropriate for mdmon to impose the
> > new
> > array_size rather than for mdadm to do it.
> > In general, if the kernel does something for native metadata, then
> > mdmon
> > should probably do it for external metadata (though there might be
> > exceptions
> > to this).
> > 
> > However it is not the metadata handler which should do this (and it
> > currently doesn't, which is good). The metadata handler should set
> > custom_array_array, and the core code of mdmon should use this number
> > to set array_size.
> > And I don't see that mdmon does this at the moment.  It sets the array
> > size
> > when the reshape starts (which is possibly wrong) but it does not set
> > it when the reshape finishes (which is the right time).
> > 
> > Could you please review all of this and either point out to me where
> > I am wrong, or fix up the code to do the right thing, thanks.
> > 
> > I won't apply this patch now, but am happy to apply it once I'm sure
> > mdmon is performing this task.
> > 
> > NeilBrown
> 
> I think mdmon takes carry about size now.
> If set_array_state() sets a->check_reshape variable to 1 (super-intel.c:near5206) on reshape end,
> and a->info.custom_array_size will be set to bigger value Managemon will set it in to md (manage_member():551)
> (the only problem I faced was seze value to set '/2' in one of previous path that you commented already)
> 
> In my tests mdmon does his job for size changes :).

I see ... yes.....

I'm not sure I like ->set_array_state changing ->check_reshape.
Maybe it is OK, but the intention for ->check_reshape was that it was
a reshape starting not ending.
Maybe that doesn't matter, but maybe it does.

I'll have a read through the code again and see what I think.

Thanks for the explanation.

NeilBrown


> 
> BR
> Adam
> 
> 
> 
> > 
> > >
> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > ---
> > >
> > >  Grow.c |   35 -----------------------------------
> > >  1 files changed, 0 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index 8229b4d..a74da89 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -2045,41 +2045,6 @@ started:
> > >  		}
> > >  	}
> > >
> > > -	/* set new array size if required customer_array_size is used
> > > -	 * by this metadata.
> > > -	 */
> > > -	if (reshape.before.data_disks !=
> > > -	    reshape.after.data_disks &&
> > > -	    info->custom_array_size) {
> > > -		struct mdinfo *info2;
> > > -		char *subarray = strchr(info->text_version+1, '/')+1;
> > > -
> > > -		info2 = st->ss->container_content(st, subarray);
> > > -		if (info2) {
> > > -			unsigned long long current_size = 0;
> > > -			unsigned long long new_size =
> > > -				info2->custom_array_size/2;
> > > -
> > > -			if (sysfs_get_ll(sra,
> > > -					 NULL,
> > > -					 "array_size",
> > > -					 &current_size) == 0 &&
> > > -			    new_size > current_size) {
> > > -				if (sysfs_set_num(sra, NULL,
> > > -						  "array_size", new_size)
> > > -				    < 0)
> > > -					dprintf("Error: Cannot"
> > > -						" set array size");
> > > -				else
> > > -					dprintf("Array size "
> > > -						"changed");
> > > -				dprintf(" from %llu to %llu.\n",
> > > -					current_size, new_size);
> > > -			}
> > > -			sysfs_free(info2);
> > > -		}
> > > -	}
> > > -
> > >  	if (info->new_level != reshape.level) {
> > >
> > >  		c = map_num(pers, info->new_level);
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-raid"
> > in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html