[PATCH 00/12] Problems during restart container reshape

[PATCH 00/12] Problems during restart container reshape

[Adam Kwolek]

This series corrects problems occurred during restart multi arrays reshape:
 1. Assembled incrementally array during reshape is assembled degraded
 2. Reshape cannot be continued after reboot. '--continue' gives  an error
 3. When raid0 arrays are reshaped, first array is reshaped only
 4. Size doesn't change on second raid0 array
Please note that patch 'imsm: FIX: No new missing disks are allowed during general migration' can be easily extended by:
 - Narrow it to incremental assembly only
 - Array can be pushed to be assembled by using e.g. 'force' option during incremental assembly.

Other issue addressed in this series is chunk size change from 128k to 4k problem.

BR
Adam

---

Adam Kwolek (12):
      FIX: Do not try to (continue) reshape using inactive array
      FIX: restart reshape when reshape process is stopped just between 2 reshapes
      imsm: FIX: Clear migration record when migration switches to next volume.
      imsm: FIX: use md position to reshape restart
      FIX: use md position to reshape restart
      imsm: FIX: Chunk size migration problem
      Flush mdmon before next reshape step during container operation
      Fix: Sometimes mdmon throws core dump during reshape
      imsm: FIX: imsm_get_allowed_degradation() doesn't count degradation for raid1
      FIX: Array is not run when expansion disks are added
      imsm: FIX: No new missing disks are allowed during general migration
      FIX: NULL pointer to strdup() can be passed


 Assemble.c    |    2 -
 Grow.c        |  124 ++++++++++++++++++++++++++++++++++++++++++++-------------
 managemon.c   |    6 +++
 msg.c         |   10 +++++
 msg.h         |    1 
 super-intel.c |   47 +++++++++++++++++++---
 util.c        |    5 +-
 7 files changed, 159 insertions(+), 36 deletions(-)

-- 
Signature

[PATCH 01/12] FIX: NULL pointer to strdup() can be passed

[Adam Kwolek]

When result from strchr() is NULL and it is assigned to subarray,
NULL pointer can be passed to strdup() function and coredump file
is generated.

Subarray is checked for NULL pointer, so it is assumed that it can
be NULL at this moment.

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

 util.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/util.c b/util.c
index e5f7a20..7abbff7 100644
--- a/util.c
+++ b/util.c
@@ -966,9 +966,10 @@ struct supertype *super_by_fd(int fd, char **subarrayp)
 		char *dev = verstr+1;
 
 		subarray = strchr(dev, '/');
-		if (subarray)
+		if (subarray) {
 			*subarray++ = '\0';
-		subarray = strdup(subarray);
+			subarray = strdup(subarray);
+		}
 		container = devname2devnum(dev);
 		if (sra)
 			sysfs_free(sra);

[PATCH 02/12] imsm: FIX: No new missing disks are allowed during general migration

[Adam Kwolek]

When during incremental assembly general migration is in progress,
starting degraded array causes that no more disks (even present)
can be added later as array is already started.

Request all previously present disks during general migration for assembly.

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

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

diff --git a/super-intel.c b/super-intel.c
index eba11d6..17034bb 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2683,7 +2683,17 @@ static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info, char *
 			enough = 0;
 		else /* we're normal, or already degraded */
 			enough = 1;
-
+		if (is_gen_migration(dev) && missing) {
+			/* during general migration we need all disks
+			 * that process is running on.
+			 * No new missing disk is allowed.
+			 */
+			max_enough = -1;
+			enough = -1;
+			/* no more checks necessary
+			 */
+			break;
+		}
 		/* in the missing/failed disk case check to see
 		 * if at least one array is runnable
 		 */

[PATCH 03/12] FIX: Array is not run when expansion disks are added

[Adam Kwolek]

When added disk is disk added by expansion and this is last disk added
to array, assemble_container_content() will not even try to run such array.

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

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

diff --git a/Assemble.c b/Assemble.c
index ad4eb9c..13adfc3 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -1557,7 +1557,7 @@ int assemble_container_content(struct supertype *st, int mdfd,
 				working++;
 		} else if (errno == EEXIST)
 			preexist++;
-	if (working == 0)
+	if (working + expansion == 0)
 		return 1;/* Nothing new, don't try to start */
 
 	map_update(&map, fd2devnum(mdfd),

[PATCH 04/12] imsm: FIX: imsm_get_allowed_degradation() doesn't count degradation for raid1

[Adam Kwolek]

Missing case raid1 added to function.

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

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

diff --git a/super-intel.c b/super-intel.c
index 17034bb..19a2c84 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -8771,6 +8771,7 @@ static int imsm_get_allowed_degradation(int level, int raid_disks,
 					struct imsm_dev *dev)
 {
 	switch (level) {
+	case 1:
 	case 10:{
 		int ret_val = 0;
 		struct imsm_map *map;

[PATCH 05/12] Fix: Sometimes mdmon throws core dump during reshape

[Adam Kwolek]

Problem was found during reshaping 2 volumes /raid0 and raid5/ in container.
Sometimes mdmon throws core dump due to NULL pointer exception.

Problem occurs in scenario:
- managemon: is about spare activation (degraded raid4 volume == raid0 under takeover)
- managemon: detect level change and signals monitor (manage_member() calls replace_array())
- monitor: detects transition raid4/5->raid0 and sets a->container to NULL
           to indicate array deactivation
- managemon : continues his work and tries to activate spare (a->check_degraded is set).
              NULL pointer is passed to metadata handler activate_spare()
              Core dump is generated.

To resolve this situation managemon (after monitor kick) checks again
a->container pointer to learn if current array is not to be deactivated.

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

 managemon.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/managemon.c b/managemon.c
index cde0d8b..6c21ecb 100644
--- a/managemon.c
+++ b/managemon.c
@@ -486,6 +486,12 @@ static void manage_member(struct mdstat_ent *mdstat,
 		}
 	}
 
+	/* we are after monitor kick,
+	 * so container field can be cleared - check it again
+	 */
+	if (a->container == NULL)
+		return;
+
 	/* We don't check the array while any update is pending, as it
 	 * might container a change (such as a spare assignment) which
 	 * could affect our decisions.

[PATCH 06/12] Flush mdmon before next reshape step during container operation

[Adam Kwolek]

Using takeover operation for grow purposes, mdadm has to be sure
that mdmon processes all updates, and if necessary it will be closed
at takeover to raid0 operation. If mdmon is late, next array in container
is processed and due to race condition mdmon closes itself instead to monitor
next reshape operation.

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

 Grow.c |   12 ++++++++++--
 msg.c  |   10 ++++++++++
 msg.h  |    1 +
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Grow.c b/Grow.c
index 36a1de7..70bdee1 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2003,6 +2003,9 @@ static int reshape_array(char *container, int fd, char *devname,
 
 		if (reshape.level > 0 && st->ss->external) {
 			/* make sure mdmon is aware of the new level */
+			if (mdmon_running(st->container_dev))
+				flush_mdmon(container);
+
 			if (!mdmon_running(st->container_dev))
 				start_mdmon(st->container_dev);
 			ping_monitor(container);
@@ -2396,8 +2399,7 @@ started:
 		/* Re-load the metadata as much could have changed */
 		int cfd = open_dev(st->container_dev);
 		if (cfd >= 0) {
-			ping_manager(container);
-			ping_monitor(container);
+			flush_mdmon(container);
 			st->ss->free_super(st);
 			st->ss->load_container(st, cfd, container);
 			close(cfd);
@@ -2594,6 +2596,9 @@ int reshape_container(char *container, char *devname,
 
 		sysfs_init(content, fd, mdstat->devnum);
 
+		if (mdmon_running(devname2devnum(container)))
+			flush_mdmon(container);
+
 		rv = reshape_array(container, fd, adev, st,
 				   content, force, NULL,
 				   backup_file, quiet, 1, restart,
@@ -2608,6 +2613,9 @@ int reshape_container(char *container, char *devname,
 		restart = 0;
 		if (rv)
 			break;
+
+		if (mdmon_running(devname2devnum(container)))
+			flush_mdmon(container);
 	}
 	if (!rv)
 		unfreeze(st);
diff --git a/msg.c b/msg.c
index dc780b3..44aad1f 100644
--- a/msg.c
+++ b/msg.c
@@ -487,3 +487,13 @@ int ping_manager(char *devname)
 	close(sfd);
 	return err;
 }
+
+/* using takeover operation for grow purposes, mdadm has to be sure
+ * that mdmon processes all updates, and if necessary it will be closed
+ * at takeover to raid0 operation
+  */
+void flush_mdmon(char *container)
+{
+	ping_manager(container);
+	ping_monitor(container);
+}
diff --git a/msg.h b/msg.h
index c6d037d..eefa649 100644
--- a/msg.h
+++ b/msg.h
@@ -34,5 +34,6 @@ extern int block_monitor(char *container, const int freeze);
 extern void unblock_monitor(char *container, const int unfreeze);
 extern int fping_monitor(int sock);
 extern int ping_manager(char *devname);
+extern void flush_mdmon(char *container);
 
 #define MSG_MAX_LEN (4*1024*1024)

[PATCH 07/12] imsm: FIX: Chunk size migration problem

[Adam Kwolek]

When chunk size migration occurs (e.g. 128k->4k) first checkpoint cannot
be set in md due to too small step. Correct migration record initialization
to allow whole copy area usage and increase migration checkpoint step.

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

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

diff --git a/super-intel.c b/super-intel.c
index 19a2c84..f5762d8 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -8913,7 +8913,8 @@ void init_migr_record_imsm(struct supertype *st, struct imsm_dev *dev,
 
 	migr_rec->dest_depth_per_unit = GEN_MIGR_AREA_SIZE /
 		max(map_dest->blocks_per_strip, map_src->blocks_per_strip);
-	migr_rec->dest_depth_per_unit *= map_dest->blocks_per_strip;
+	migr_rec->dest_depth_per_unit *=
+		max(map_dest->blocks_per_strip, map_src->blocks_per_strip);
 	new_data_disks = imsm_num_data_members(dev, MAP_0);
 	migr_rec->blocks_per_unit =
 		__cpu_to_le32(migr_rec->dest_depth_per_unit * new_data_disks);

[PATCH 08/12] FIX: use md position to reshape restart

[Adam Kwolek]

When reshape is broken, it can occur that metadata is not saved properly.
This can cause that reshape process is farther in md than metadata states.

On reshape restart use md position as start position, if it is farther than
position specified in metadata. Opposite situation treat as error.

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

 Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/Grow.c b/Grow.c
index 70bdee1..0668a16 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1862,6 +1862,55 @@ release:
 	return rv;
 }
 
+/* verify_reshape_position()
+ *	Function checks if reshape position in metadata is not farther
+ *	than position in md.
+ * Return value:
+ *	 0 : not valid sysfs entry
+ *		it can be caused by not started reshape, it should be started
+ *		by reshape array or raid0 array is before takeover
+ *	-1 :	error, reshape position is obviously wrong
+ *	 1 :	success, reshape progress correct or updated
+*/
+static int verify_reshape_position(struct mdinfo *info, int level)
+{
+	int ret_val = 0;
+	char buf[40];
+
+	/* read sync_max, failure can mean raid0 array */
+	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
+		char *ep;
+		unsigned long long position = strtoull(buf, &ep, 0);
+
+		dprintf(Name": Read sync_max sysfs entry is: %s\n", buf);
+		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))) {
+			position *= get_data_disks(level,
+						   info->new_layout,
+						   info->array.raid_disks);
+			if (info->reshape_progress < position) {
+				dprintf("Corrected reshape progress (%llu) to "
+					"md position (%llu)\n",
+					info->reshape_progress, position);
+				info->reshape_progress = position;
+				ret_val = 1;
+			} else if (info->reshape_progress > position) {
+				fprintf(stderr, Name ": Fatal error: array "
+					"reshape was not properly frozen "
+					"(expected reshape position is %llu, "
+					"but reshape progress is %llu.\n",
+					position, info->reshape_progress);
+				ret_val = -1;
+			} else {
+				dprintf("Reshape position in md and metadata "
+					"are the same;");
+				ret_val = 1;
+			}
+		}
+	}
+
+	return ret_val;
+}
+
 static int reshape_array(char *container, int fd, char *devname,
 			 struct supertype *st, struct mdinfo *info,
 			 int force, struct mddev_dev *devlist,
@@ -2251,9 +2300,16 @@ started:
 
 	sra->new_chunk = info->new_chunk;
 
-	if (restart)
+	if (restart) {
+		/* for external metadata checkpoint saved by mdmon can be lost
+		 * or missed /due to e.g. crash/. Check if md is not during
+		 * restart farther than metadata points to.
+		 * If so, this means metadata information is obsolete.
+		 */
+		if (st->ss->external)
+			verify_reshape_position(info, reshape.level);
 		sra->reshape_progress = info->reshape_progress;
-	else {
+	} else {
 		sra->reshape_progress = 0;
 		if (reshape.after.data_disks < reshape.before.data_disks)
 			/* start from the end of the new array */
@@ -3765,8 +3821,6 @@ int Grow_continue_command(char *devname, int fd,
 	char buf[40];
 	int cfd = -1;
 	int fd2 = -1;
-	char *ep;
-	unsigned long long position;
 
 	dprintf("Grow continue from command line called for %s\n",
 		devname);
@@ -3894,28 +3948,8 @@ int Grow_continue_command(char *devname, int fd,
 	/* verify that array under reshape is started from
 	 * correct position
 	 */
-	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
-	if (ret_val <= 0) {
-		fprintf(stderr, Name
-			": cannot open verify reshape progress for %s (%i)\n",
-			content->sys_name, ret_val);
-		ret_val = 1;
-		goto Grow_continue_command_exit;
-	}
-	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
-	position = strtoull(buf, &ep, 0);
-	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
-		fprintf(stderr, Name ": Fatal error: array reshape was"
-			" not properly frozen\n");
-		ret_val = 1;
-		goto Grow_continue_command_exit;
-	}
-	position *= get_data_disks(map_name(pers, mdstat->level),
-				   content->new_layout,
-				   content->array.raid_disks);
-	if (position != content->reshape_progress) {
-		fprintf(stderr, Name ": Fatal error: array reshape was"
-			" not properly frozen.\n");
+	if (verify_reshape_position(content,
+				    map_name(pers, mdstat->level)) < 0) {
 		ret_val = 1;
 		goto Grow_continue_command_exit;
 	}

Re: [PATCH 08/12] FIX: use md position to reshape restart

[NeilBrown]

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

On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> When reshape is broken, it can occur that metadata is not saved properly.
> This can cause that reshape process is farther in md than metadata states.
> 
> On reshape restart use md position as start position, if it is farther than
> position specified in metadata. Opposite situation treat as error.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 60 insertions(+), 26 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 70bdee1..0668a16 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1862,6 +1862,55 @@ release:
>  	return rv;
>  }
>  
> +/* verify_reshape_position()
> + *	Function checks if reshape position in metadata is not farther
> + *	than position in md.
> + * Return value:
> + *	 0 : not valid sysfs entry
> + *		it can be caused by not started reshape, it should be started
> + *		by reshape array or raid0 array is before takeover
> + *	-1 :	error, reshape position is obviously wrong
> + *	 1 :	success, reshape progress correct or updated
> +*/
> +static int verify_reshape_position(struct mdinfo *info, int level)
> +{
> +	int ret_val = 0;
> +	char buf[40];
> +
> +	/* read sync_max, failure can mean raid0 array */
> +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> +		char *ep;
> +		unsigned long long position = strtoull(buf, &ep, 0);
> +
> +		dprintf(Name": Read sync_max sysfs entry is: %s\n", buf);
> +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))) {
> +			position *= get_data_disks(level,
> +						   info->new_layout,
> +						   info->array.raid_disks);
> +			if (info->reshape_progress < position) {
> +				dprintf("Corrected reshape progress (%llu) to "
> +					"md position (%llu)\n",
> +					info->reshape_progress, position);
> +				info->reshape_progress = position;
> +				ret_val = 1;
> +			} else if (info->reshape_progress > position) {
> +				fprintf(stderr, Name ": Fatal error: array "
> +					"reshape was not properly frozen "
> +					"(expected reshape position is %llu, "
> +					"but reshape progress is %llu.\n",
> +					position, info->reshape_progress);
> +				ret_val = -1;
> +			} else {
> +				dprintf("Reshape position in md and metadata "
> +					"are the same;");
> +				ret_val = 1;
> +			}
> +		}
> +	}
> +
> +	return ret_val;
> +}
> +
>  static int reshape_array(char *container, int fd, char *devname,
>  			 struct supertype *st, struct mdinfo *info,
>  			 int force, struct mddev_dev *devlist,
> @@ -2251,9 +2300,16 @@ started:
>  
>  	sra->new_chunk = info->new_chunk;
>  
> -	if (restart)
> +	if (restart) {
> +		/* for external metadata checkpoint saved by mdmon can be lost
> +		 * or missed /due to e.g. crash/. Check if md is not during
> +		 * restart farther than metadata points to.
> +		 * If so, this means metadata information is obsolete.
> +		 */
> +		if (st->ss->external)
> +			verify_reshape_position(info, reshape.level);
>  		sra->reshape_progress = info->reshape_progress;
> -	else {
> +	} else {
>  		sra->reshape_progress = 0;
>  		if (reshape.after.data_disks < reshape.before.data_disks)
>  			/* start from the end of the new array */
> @@ -3765,8 +3821,6 @@ int Grow_continue_command(char *devname, int fd,
>  	char buf[40];
>  	int cfd = -1;
>  	int fd2 = -1;
> -	char *ep;
> -	unsigned long long position;
>  
>  	dprintf("Grow continue from command line called for %s\n",
>  		devname);
> @@ -3894,28 +3948,8 @@ int Grow_continue_command(char *devname, int fd,
>  	/* verify that array under reshape is started from
>  	 * correct position
>  	 */
> -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> -	if (ret_val <= 0) {
                    ^^^

This test is '<=' however ...


> -		fprintf(stderr, Name
> -			": cannot open verify reshape progress for %s (%i)\n",
> -			content->sys_name, ret_val);
> -		ret_val = 1;
> -		goto Grow_continue_command_exit;
> -	}
> -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> -	position = strtoull(buf, &ep, 0);
> -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> -		fprintf(stderr, Name ": Fatal error: array reshape was"
> -			" not properly frozen\n");
> -		ret_val = 1;
> -		goto Grow_continue_command_exit;
> -	}
> -	position *= get_data_disks(map_name(pers, mdstat->level),
> -				   content->new_layout,
> -				   content->array.raid_disks);
> -	if (position != content->reshape_progress) {
> -		fprintf(stderr, Name ": Fatal error: array reshape was"
> -			" not properly frozen.\n");
> +	if (verify_reshape_position(content,
> +				    map_name(pers, mdstat->level)) < 0) {
                                                                   ^^

this one is just '<'.  However it looks like it should be the same.

Was there a reason for the change, or was it an oversight?

Thanks,
NeilBrown




>  		ret_val = 1;
>  		goto Grow_continue_command_exit;
>  	}


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH 08/12] FIX: use md position to reshape restart

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, February 09, 2012 02:36
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> J; Dorau, Lukasz
> Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> 
> On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > When reshape is broken, it can occur that metadata is not saved properly.
> > This can cause that reshape process is farther in md than metadata states.
> >
> > On reshape restart use md position as start position, if it is farther
> > than position specified in metadata. Opposite situation treat as error.
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++-----------
> --------
> >  1 files changed, 60 insertions(+), 26 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 70bdee1..0668a16 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -1862,6 +1862,55 @@ release:
> >  	return rv;
> >  }
> >
> > +/* verify_reshape_position()
> > + *	Function checks if reshape position in metadata is not farther
> > + *	than position in md.
> > + * Return value:
> > + *	 0 : not valid sysfs entry
> > + *		it can be caused by not started reshape, it should be started
> > + *		by reshape array or raid0 array is before takeover
> > + *	-1 :	error, reshape position is obviously wrong
> > + *	 1 :	success, reshape progress correct or updated
> > +*/
> > +static int verify_reshape_position(struct mdinfo *info, int level) {
> > +	int ret_val = 0;
> > +	char buf[40];
> > +
> > +	/* read sync_max, failure can mean raid0 array */
> > +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> > +		char *ep;
> > +		unsigned long long position = strtoull(buf, &ep, 0);
> > +
> > +		dprintf(Name": Read sync_max sysfs entry is: %s\n", buf);
> > +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))) {
> > +			position *= get_data_disks(level,
> > +						   info->new_layout,
> > +						   info->array.raid_disks);
> > +			if (info->reshape_progress < position) {
> > +				dprintf("Corrected reshape progress (%llu) to "
> > +					"md position (%llu)\n",
> > +					info->reshape_progress, position);
> > +				info->reshape_progress = position;
> > +				ret_val = 1;
> > +			} else if (info->reshape_progress > position) {
> > +				fprintf(stderr, Name ": Fatal error: array "
> > +					"reshape was not properly frozen "
> > +					"(expected reshape position is %llu, "
> > +					"but reshape progress is %llu.\n",
> > +					position, info->reshape_progress);
> > +				ret_val = -1;
> > +			} else {
> > +				dprintf("Reshape position in md and metadata "
> > +					"are the same;");
> > +				ret_val = 1;
> > +			}
> > +		}
> > +	}
> > +
> > +	return ret_val;
> > +}
> > +
> >  static int reshape_array(char *container, int fd, char *devname,
> >  			 struct supertype *st, struct mdinfo *info,
> >  			 int force, struct mddev_dev *devlist, @@ -2251,9
> +2300,16 @@
> > started:
> >
> >  	sra->new_chunk = info->new_chunk;
> >
> > -	if (restart)
> > +	if (restart) {
> > +		/* for external metadata checkpoint saved by mdmon can be
> lost
> > +		 * or missed /due to e.g. crash/. Check if md is not during
> > +		 * restart farther than metadata points to.
> > +		 * If so, this means metadata information is obsolete.
> > +		 */
> > +		if (st->ss->external)
> > +			verify_reshape_position(info, reshape.level);
> >  		sra->reshape_progress = info->reshape_progress;
> > -	else {
> > +	} else {
> >  		sra->reshape_progress = 0;
> >  		if (reshape.after.data_disks < reshape.before.data_disks)
> >  			/* start from the end of the new array */ @@ -3765,8
> +3821,6 @@
> > int Grow_continue_command(char *devname, int fd,
> >  	char buf[40];
> >  	int cfd = -1;
> >  	int fd2 = -1;
> > -	char *ep;
> > -	unsigned long long position;
> >
> >  	dprintf("Grow continue from command line called for %s\n",
> >  		devname);
> > @@ -3894,28 +3948,8 @@ int Grow_continue_command(char *devname, int
> fd,
> >  	/* verify that array under reshape is started from
> >  	 * correct position
> >  	 */
> > -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> > -	if (ret_val <= 0) {
>                     ^^^
> 
> This test is '<=' however ...
> 
> 
> > -		fprintf(stderr, Name
> > -			": cannot open verify reshape progress for %s (%i)\n",
> > -			content->sys_name, ret_val);
> > -		ret_val = 1;
> > -		goto Grow_continue_command_exit;
> > -	}
> > -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> > -	position = strtoull(buf, &ep, 0);
> > -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > -			" not properly frozen\n");
> > -		ret_val = 1;
> > -		goto Grow_continue_command_exit;
> > -	}
> > -	position *= get_data_disks(map_name(pers, mdstat->level),
> > -				   content->new_layout,
> > -				   content->array.raid_disks);
> > -	if (position != content->reshape_progress) {
> > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > -			" not properly frozen.\n");
> > +	if (verify_reshape_position(content,
> > +				    map_name(pers, mdstat->level)) < 0) {
>                                                                    ^^
> 
> this one is just '<'.  However it looks like it should be the same.
> 
> Was there a reason for the change, or was it an oversight?
> 
> Thanks,
> NeilBrown
> 
> 
> 
> 
> >  		ret_val = 1;
> >  		goto Grow_continue_command_exit;
> >  	}


Removed code is put in to function verify_reshape_position() for use in current location and reshape_array(),
as calculated new position in Grow_continue_command() cannot be passed to reshape_array(), it has to be calculated again.

This is what you are mention above?

BR
Adam

Re: [PATCH 08/12] FIX: use md position to reshape restart

[NeilBrown]

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

On Thu, 9 Feb 2012 08:16:16 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Thursday, February 09, 2012 02:36
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> > J; Dorau, Lukasz
> > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> > 
> > On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> > 
> > > When reshape is broken, it can occur that metadata is not saved properly.
> > > This can cause that reshape process is farther in md than metadata states.
> > >
> > > On reshape restart use md position as start position, if it is farther
> > > than position specified in metadata. Opposite situation treat as error.
> > >
> > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > ---
> > >
> > >  Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++-----------
> > --------
> > >  1 files changed, 60 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index 70bdee1..0668a16 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -1862,6 +1862,55 @@ release:
> > >  	return rv;
> > >  }
> > >
> > > +/* verify_reshape_position()
> > > + *	Function checks if reshape position in metadata is not farther
> > > + *	than position in md.
> > > + * Return value:
> > > + *	 0 : not valid sysfs entry
> > > + *		it can be caused by not started reshape, it should be started
> > > + *		by reshape array or raid0 array is before takeover
> > > + *	-1 :	error, reshape position is obviously wrong
> > > + *	 1 :	success, reshape progress correct or updated
> > > +*/
> > > +static int verify_reshape_position(struct mdinfo *info, int level) {
> > > +	int ret_val = 0;
> > > +	char buf[40];
> > > +
> > > +	/* read sync_max, failure can mean raid0 array */
> > > +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> > > +		char *ep;
> > > +		unsigned long long position = strtoull(buf, &ep, 0);
> > > +
> > > +		dprintf(Name": Read sync_max sysfs entry is: %s\n", buf);
> > > +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))) {
> > > +			position *= get_data_disks(level,
> > > +						   info->new_layout,
> > > +						   info->array.raid_disks);
> > > +			if (info->reshape_progress < position) {
> > > +				dprintf("Corrected reshape progress (%llu) to "
> > > +					"md position (%llu)\n",
> > > +					info->reshape_progress, position);
> > > +				info->reshape_progress = position;
> > > +				ret_val = 1;
> > > +			} else if (info->reshape_progress > position) {
> > > +				fprintf(stderr, Name ": Fatal error: array "
> > > +					"reshape was not properly frozen "
> > > +					"(expected reshape position is %llu, "
> > > +					"but reshape progress is %llu.\n",
> > > +					position, info->reshape_progress);
> > > +				ret_val = -1;
> > > +			} else {
> > > +				dprintf("Reshape position in md and metadata "
> > > +					"are the same;");
> > > +				ret_val = 1;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > > +	return ret_val;
> > > +}
> > > +
> > >  static int reshape_array(char *container, int fd, char *devname,
> > >  			 struct supertype *st, struct mdinfo *info,
> > >  			 int force, struct mddev_dev *devlist, @@ -2251,9
> > +2300,16 @@
> > > started:
> > >
> > >  	sra->new_chunk = info->new_chunk;
> > >
> > > -	if (restart)
> > > +	if (restart) {
> > > +		/* for external metadata checkpoint saved by mdmon can be
> > lost
> > > +		 * or missed /due to e.g. crash/. Check if md is not during
> > > +		 * restart farther than metadata points to.
> > > +		 * If so, this means metadata information is obsolete.
> > > +		 */
> > > +		if (st->ss->external)
> > > +			verify_reshape_position(info, reshape.level);
> > >  		sra->reshape_progress = info->reshape_progress;
> > > -	else {
> > > +	} else {
> > >  		sra->reshape_progress = 0;
> > >  		if (reshape.after.data_disks < reshape.before.data_disks)
> > >  			/* start from the end of the new array */ @@ -3765,8
> > +3821,6 @@
> > > int Grow_continue_command(char *devname, int fd,
> > >  	char buf[40];
> > >  	int cfd = -1;
> > >  	int fd2 = -1;
> > > -	char *ep;
> > > -	unsigned long long position;
> > >
> > >  	dprintf("Grow continue from command line called for %s\n",
> > >  		devname);
> > > @@ -3894,28 +3948,8 @@ int Grow_continue_command(char *devname, int
> > fd,
> > >  	/* verify that array under reshape is started from
> > >  	 * correct position
> > >  	 */
> > > -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> > > -	if (ret_val <= 0) {
> >                     ^^^
> > 
> > This test is '<=' however ...
> > 
> > 
> > > -		fprintf(stderr, Name
> > > -			": cannot open verify reshape progress for %s (%i)\n",
> > > -			content->sys_name, ret_val);
> > > -		ret_val = 1;
> > > -		goto Grow_continue_command_exit;
> > > -	}
> > > -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> > > -	position = strtoull(buf, &ep, 0);
> > > -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > -			" not properly frozen\n");
> > > -		ret_val = 1;
> > > -		goto Grow_continue_command_exit;
> > > -	}
> > > -	position *= get_data_disks(map_name(pers, mdstat->level),
> > > -				   content->new_layout,
> > > -				   content->array.raid_disks);
> > > -	if (position != content->reshape_progress) {
> > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > -			" not properly frozen.\n");
> > > +	if (verify_reshape_position(content,
> > > +				    map_name(pers, mdstat->level)) < 0) {
> >                                                                    ^^
> > 
> > this one is just '<'.  However it looks like it should be the same.
> > 
> > Was there a reason for the change, or was it an oversight?
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > 
> > 
> > >  		ret_val = 1;
> > >  		goto Grow_continue_command_exit;
> > >  	}
> 
> 
> Removed code is put in to function verify_reshape_position() for use in current location and reshape_array(),
> as calculated new position in Grow_continue_command() cannot be passed to reshape_array(), it has to be calculated again.
> 
> This is what you are mention above?

Not exactly.

The original code would print an error if
      sysfs_get_str(content, NULL, "sync_max", buf, 40);
returns 0 or negative.
The new code will not print an error if that returns zero.

i.e. you changed a '<=' into a '<'.  I wondered if there was some reason for
that - something that I was missing.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH 08/12] FIX: use md position to reshape restart

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, February 09, 2012 10:19
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> J; Dorau, Lukasz
> Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> 
> On Thu, 9 Feb 2012 08:16:16 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Thursday, February 09, 2012 02:36
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > > Williams, Dan J; Dorau, Lukasz
> > > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> > >
> > > On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek
> > > <adam.kwolek@intel.com>
> > > wrote:
> > >
> > > > When reshape is broken, it can occur that metadata is not saved properly.
> > > > This can cause that reshape process is farther in md than metadata states.
> > > >
> > > > On reshape restart use md position as start position, if it is
> > > > farther than position specified in metadata. Opposite situation treat as
> error.
> > > >
> > > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > > ---
> > > >
> > > >  Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++------
> -----
> > > --------
> > > >  1 files changed, 60 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/Grow.c b/Grow.c
> > > > index 70bdee1..0668a16 100644
> > > > --- a/Grow.c
> > > > +++ b/Grow.c
> > > > @@ -1862,6 +1862,55 @@ release:
> > > >  	return rv;
> > > >  }
> > > >
> > > > +/* verify_reshape_position()
> > > > + *	Function checks if reshape position in metadata is not farther
> > > > + *	than position in md.
> > > > + * Return value:
> > > > + *	 0 : not valid sysfs entry
> > > > + *		it can be caused by not started reshape, it should be
> started
> > > > + *		by reshape array or raid0 array is before takeover
> > > > + *	-1 :	error, reshape position is obviously wrong
> > > > + *	 1 :	success, reshape progress correct or updated
> > > > +*/
> > > > +static int verify_reshape_position(struct mdinfo *info, int level) {
> > > > +	int ret_val = 0;
> > > > +	char buf[40];
> > > > +
> > > > +	/* read sync_max, failure can mean raid0 array */
> > > > +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> > > > +		char *ep;
> > > > +		unsigned long long position = strtoull(buf, &ep, 0);
> > > > +
> > > > +		dprintf(Name": Read sync_max sysfs entry is: %s\n", buf);
> > > > +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))) {
> > > > +			position *= get_data_disks(level,
> > > > +						   info->new_layout,
> > > > +						   info->array.raid_disks);
> > > > +			if (info->reshape_progress < position) {
> > > > +				dprintf("Corrected reshape progress (%llu) to "
> > > > +					"md position (%llu)\n",
> > > > +					info->reshape_progress, position);
> > > > +				info->reshape_progress = position;
> > > > +				ret_val = 1;
> > > > +			} else if (info->reshape_progress > position) {
> > > > +				fprintf(stderr, Name ": Fatal error: array "
> > > > +					"reshape was not properly frozen "
> > > > +					"(expected reshape position is %llu, "
> > > > +					"but reshape progress is %llu.\n",
> > > > +					position, info->reshape_progress);
> > > > +				ret_val = -1;
> > > > +			} else {
> > > > +				dprintf("Reshape position in md and metadata "
> > > > +					"are the same;");
> > > > +				ret_val = 1;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return ret_val;
> > > > +}
> > > > +
> > > >  static int reshape_array(char *container, int fd, char *devname,
> > > >  			 struct supertype *st, struct mdinfo *info,
> > > >  			 int force, struct mddev_dev *devlist, @@ -2251,9
> > > +2300,16 @@
> > > > started:
> > > >
> > > >  	sra->new_chunk = info->new_chunk;
> > > >
> > > > -	if (restart)
> > > > +	if (restart) {
> > > > +		/* for external metadata checkpoint saved by mdmon can be
> > > lost
> > > > +		 * or missed /due to e.g. crash/. Check if md is not during
> > > > +		 * restart farther than metadata points to.
> > > > +		 * If so, this means metadata information is obsolete.
> > > > +		 */
> > > > +		if (st->ss->external)
> > > > +			verify_reshape_position(info, reshape.level);
> > > >  		sra->reshape_progress = info->reshape_progress;
> > > > -	else {
> > > > +	} else {
> > > >  		sra->reshape_progress = 0;
> > > >  		if (reshape.after.data_disks < reshape.before.data_disks)
> > > >  			/* start from the end of the new array */ @@ -3765,8
> > > +3821,6 @@
> > > > int Grow_continue_command(char *devname, int fd,
> > > >  	char buf[40];
> > > >  	int cfd = -1;
> > > >  	int fd2 = -1;
> > > > -	char *ep;
> > > > -	unsigned long long position;
> > > >
> > > >  	dprintf("Grow continue from command line called for %s\n",
> > > >  		devname);
> > > > @@ -3894,28 +3948,8 @@ int Grow_continue_command(char *devname,
> > > > int
> > > fd,
> > > >  	/* verify that array under reshape is started from
> > > >  	 * correct position
> > > >  	 */
> > > > -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> > > > -	if (ret_val <= 0) {
> > >                     ^^^
> > >
> > > This test is '<=' however ...
> > >
> > >
> > > > -		fprintf(stderr, Name
> > > > -			": cannot open verify reshape progress for %s (%i)\n",
> > > > -			content->sys_name, ret_val);
> > > > -		ret_val = 1;
> > > > -		goto Grow_continue_command_exit;
> > > > -	}
> > > > -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> > > > -	position = strtoull(buf, &ep, 0);
> > > > -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > -			" not properly frozen\n");
> > > > -		ret_val = 1;
> > > > -		goto Grow_continue_command_exit;
> > > > -	}
> > > > -	position *= get_data_disks(map_name(pers, mdstat->level),
> > > > -				   content->new_layout,
> > > > -				   content->array.raid_disks);
> > > > -	if (position != content->reshape_progress) {
> > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > -			" not properly frozen.\n");
> > > > +	if (verify_reshape_position(content,
> > > > +				    map_name(pers, mdstat->level)) < 0) {
> > >
> > > ^^
> > >
> > > this one is just '<'.  However it looks like it should be the same.
> > >
> > > Was there a reason for the change, or was it an oversight?
> > >
> > > Thanks,
> > > NeilBrown
> > >
> > >
> > >
> > >
> > > >  		ret_val = 1;
> > > >  		goto Grow_continue_command_exit;
> > > >  	}
> >
> >
> > Removed code is put in to function verify_reshape_position() for use
> > in current location and reshape_array(), as calculated new position in
> Grow_continue_command() cannot be passed to reshape_array(), it has to be
> calculated again.
> >
> > This is what you are mention above?
> 
> Not exactly.
> 
> The original code would print an error if
>       sysfs_get_str(content, NULL, "sync_max", buf, 40); returns 0 or negative.
> The new code will not print an error if that returns zero.
> 
> i.e. you changed a '<=' into a '<'.  I wondered if there was some reason for that -
> something that I was missing.
> 
> NeilBrown

Yes, you are right. I've missed '0' case during code reorganization.
You would fix this or you want me to post additional patch?

BR
Adam

Re: [PATCH 08/12] FIX: use md position to reshape restart

[NeilBrown]

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

On Thu, 9 Feb 2012 09:49:01 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:

> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Thursday, February 09, 2012 10:19
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> > J; Dorau, Lukasz
> > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> > 
> > On Thu, 9 Feb 2012 08:16:16 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
> > wrote:
> > 
> > >
> > >
> > > > -----Original Message-----
> > > > From: NeilBrown [mailto:neilb@suse.de]
> > > > Sent: Thursday, February 09, 2012 02:36
> > > > To: Kwolek, Adam
> > > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > > > Williams, Dan J; Dorau, Lukasz
> > > > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> > > >
> > > > On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek
> > > > <adam.kwolek@intel.com>
> > > > wrote:
> > > >
> > > > > When reshape is broken, it can occur that metadata is not saved properly.
> > > > > This can cause that reshape process is farther in md than metadata states.
> > > > >
> > > > > On reshape restart use md position as start position, if it is
> > > > > farther than position specified in metadata. Opposite situation treat as
> > error.
> > > > >
> > > > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > > > ---
> > > > >
> > > > >  Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++------
> > -----
> > > > --------
> > > > >  1 files changed, 60 insertions(+), 26 deletions(-)
> > > > >
> > > > > diff --git a/Grow.c b/Grow.c
> > > > > index 70bdee1..0668a16 100644
> > > > > --- a/Grow.c
> > > > > +++ b/Grow.c
> > > > > @@ -1862,6 +1862,55 @@ release:
> > > > >  	return rv;
> > > > >  }
> > > > >
> > > > > +/* verify_reshape_position()
> > > > > + *	Function checks if reshape position in metadata is not farther
> > > > > + *	than position in md.
> > > > > + * Return value:
> > > > > + *	 0 : not valid sysfs entry
> > > > > + *		it can be caused by not started reshape, it should be
> > started
> > > > > + *		by reshape array or raid0 array is before takeover
> > > > > + *	-1 :	error, reshape position is obviously wrong
> > > > > + *	 1 :	success, reshape progress correct or updated
> > > > > +*/
> > > > > +static int verify_reshape_position(struct mdinfo *info, int level) {
> > > > > +	int ret_val = 0;
> > > > > +	char buf[40];
> > > > > +
> > > > > +	/* read sync_max, failure can mean raid0 array */
> > > > > +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> > > > > +		char *ep;
> > > > > +		unsigned long long position = strtoull(buf, &ep, 0);
> > > > > +
> > > > > +		dprintf(Name": Read sync_max sysfs entry is: %s\n", buf);
> > > > > +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' '))) {
> > > > > +			position *= get_data_disks(level,
> > > > > +						   info->new_layout,
> > > > > +						   info->array.raid_disks);
> > > > > +			if (info->reshape_progress < position) {
> > > > > +				dprintf("Corrected reshape progress (%llu) to "
> > > > > +					"md position (%llu)\n",
> > > > > +					info->reshape_progress, position);
> > > > > +				info->reshape_progress = position;
> > > > > +				ret_val = 1;
> > > > > +			} else if (info->reshape_progress > position) {
> > > > > +				fprintf(stderr, Name ": Fatal error: array "
> > > > > +					"reshape was not properly frozen "
> > > > > +					"(expected reshape position is %llu, "
> > > > > +					"but reshape progress is %llu.\n",
> > > > > +					position, info->reshape_progress);
> > > > > +				ret_val = -1;
> > > > > +			} else {
> > > > > +				dprintf("Reshape position in md and metadata "
> > > > > +					"are the same;");
> > > > > +				ret_val = 1;
> > > > > +			}
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return ret_val;
> > > > > +}
> > > > > +
> > > > >  static int reshape_array(char *container, int fd, char *devname,
> > > > >  			 struct supertype *st, struct mdinfo *info,
> > > > >  			 int force, struct mddev_dev *devlist, @@ -2251,9
> > > > +2300,16 @@
> > > > > started:
> > > > >
> > > > >  	sra->new_chunk = info->new_chunk;
> > > > >
> > > > > -	if (restart)
> > > > > +	if (restart) {
> > > > > +		/* for external metadata checkpoint saved by mdmon can be
> > > > lost
> > > > > +		 * or missed /due to e.g. crash/. Check if md is not during
> > > > > +		 * restart farther than metadata points to.
> > > > > +		 * If so, this means metadata information is obsolete.
> > > > > +		 */
> > > > > +		if (st->ss->external)
> > > > > +			verify_reshape_position(info, reshape.level);
> > > > >  		sra->reshape_progress = info->reshape_progress;
> > > > > -	else {
> > > > > +	} else {
> > > > >  		sra->reshape_progress = 0;
> > > > >  		if (reshape.after.data_disks < reshape.before.data_disks)
> > > > >  			/* start from the end of the new array */ @@ -3765,8
> > > > +3821,6 @@
> > > > > int Grow_continue_command(char *devname, int fd,
> > > > >  	char buf[40];
> > > > >  	int cfd = -1;
> > > > >  	int fd2 = -1;
> > > > > -	char *ep;
> > > > > -	unsigned long long position;
> > > > >
> > > > >  	dprintf("Grow continue from command line called for %s\n",
> > > > >  		devname);
> > > > > @@ -3894,28 +3948,8 @@ int Grow_continue_command(char *devname,
> > > > > int
> > > > fd,
> > > > >  	/* verify that array under reshape is started from
> > > > >  	 * correct position
> > > > >  	 */
> > > > > -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> > > > > -	if (ret_val <= 0) {
> > > >                     ^^^
> > > >
> > > > This test is '<=' however ...
> > > >
> > > >
> > > > > -		fprintf(stderr, Name
> > > > > -			": cannot open verify reshape progress for %s (%i)\n",
> > > > > -			content->sys_name, ret_val);
> > > > > -		ret_val = 1;
> > > > > -		goto Grow_continue_command_exit;
> > > > > -	}
> > > > > -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> > > > > -	position = strtoull(buf, &ep, 0);
> > > > > -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> > > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > > -			" not properly frozen\n");
> > > > > -		ret_val = 1;
> > > > > -		goto Grow_continue_command_exit;
> > > > > -	}
> > > > > -	position *= get_data_disks(map_name(pers, mdstat->level),
> > > > > -				   content->new_layout,
> > > > > -				   content->array.raid_disks);
> > > > > -	if (position != content->reshape_progress) {
> > > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > > -			" not properly frozen.\n");
> > > > > +	if (verify_reshape_position(content,
> > > > > +				    map_name(pers, mdstat->level)) < 0) {
> > > >
> > > > ^^
> > > >
> > > > this one is just '<'.  However it looks like it should be the same.
> > > >
> > > > Was there a reason for the change, or was it an oversight?
> > > >
> > > > Thanks,
> > > > NeilBrown
> > > >
> > > >
> > > >
> > > >
> > > > >  		ret_val = 1;
> > > > >  		goto Grow_continue_command_exit;
> > > > >  	}
> > >
> > >
> > > Removed code is put in to function verify_reshape_position() for use
> > > in current location and reshape_array(), as calculated new position in
> > Grow_continue_command() cannot be passed to reshape_array(), it has to be
> > calculated again.
> > >
> > > This is what you are mention above?
> > 
> > Not exactly.
> > 
> > The original code would print an error if
> >       sysfs_get_str(content, NULL, "sync_max", buf, 40); returns 0 or negative.
> > The new code will not print an error if that returns zero.
> > 
> > i.e. you changed a '<=' into a '<'.  I wondered if there was some reason for that -
> > something that I was missing.
> > 
> > NeilBrown
> 
> Yes, you are right. I've missed '0' case during code reorganization.
> You would fix this or you want me to post additional patch?

Thanks.
I already fixed it - I've just pushed my current tree with all your patches
out.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH 08/12] FIX: use md position to reshape restart

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Thursday, February 09, 2012 11:20
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> J; Dorau, Lukasz
> Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> 
> On Thu, 9 Feb 2012 09:49:01 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Thursday, February 09, 2012 10:19
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > > Williams, Dan J; Dorau, Lukasz
> > > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> > >
> > > On Thu, 9 Feb 2012 08:16:16 +0000 "Kwolek, Adam"
> > > <adam.kwolek@intel.com>
> > > wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: NeilBrown [mailto:neilb@suse.de]
> > > > > Sent: Thursday, February 09, 2012 02:36
> > > > > To: Kwolek, Adam
> > > > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > > > > Williams, Dan J; Dorau, Lukasz
> > > > > Subject: Re: [PATCH 08/12] FIX: use md position to reshape
> > > > > restart
> > > > >
> > > > > On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek
> > > > > <adam.kwolek@intel.com>
> > > > > wrote:
> > > > >
> > > > > > When reshape is broken, it can occur that metadata is not saved
> properly.
> > > > > > This can cause that reshape process is farther in md than metadata
> states.
> > > > > >
> > > > > > On reshape restart use md position as start position, if it is
> > > > > > farther than position specified in metadata. Opposite
> > > > > > situation treat as
> > > error.
> > > > > >
> > > > > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > > > > ---
> > > > > >
> > > > > >  Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++--
> ----
> > > -----
> > > > > --------
> > > > > >  1 files changed, 60 insertions(+), 26 deletions(-)
> > > > > >
> > > > > > diff --git a/Grow.c b/Grow.c
> > > > > > index 70bdee1..0668a16 100644
> > > > > > --- a/Grow.c
> > > > > > +++ b/Grow.c
> > > > > > @@ -1862,6 +1862,55 @@ release:
> > > > > >  	return rv;
> > > > > >  }
> > > > > >
> > > > > > +/* verify_reshape_position()
> > > > > > + *	Function checks if reshape position in metadata is not farther
> > > > > > + *	than position in md.
> > > > > > + * Return value:
> > > > > > + *	 0 : not valid sysfs entry
> > > > > > + *		it can be caused by not started reshape, it should be
> > > started
> > > > > > + *		by reshape array or raid0 array is before takeover
> > > > > > + *	-1 :	error, reshape position is obviously wrong
> > > > > > + *	 1 :	success, reshape progress correct or updated
> > > > > > +*/
> > > > > > +static int verify_reshape_position(struct mdinfo *info, int level) {
> > > > > > +	int ret_val = 0;
> > > > > > +	char buf[40];
> > > > > > +
> > > > > > +	/* read sync_max, failure can mean raid0 array */
> > > > > > +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> > > > > > +		char *ep;
> > > > > > +		unsigned long long position = strtoull(buf, &ep, 0);
> > > > > > +
> > > > > > +		dprintf(Name": Read sync_max sysfs entry is: %s\n",
> buf);
> > > > > > +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')))
> {
> > > > > > +			position *= get_data_disks(level,
> > > > > > +						   info->new_layout,
> > > > > > +						   info-
> >array.raid_disks);
> > > > > > +			if (info->reshape_progress < position) {
> > > > > > +				dprintf("Corrected reshape progress
> (%llu) to "
> > > > > > +					"md position (%llu)\n",
> > > > > > +					info->reshape_progress,
> position);
> > > > > > +				info->reshape_progress = position;
> > > > > > +				ret_val = 1;
> > > > > > +			} else if (info->reshape_progress > position) {
> > > > > > +				fprintf(stderr, Name ": Fatal error:
> array "
> > > > > > +					"reshape was not properly
> frozen "
> > > > > > +					"(expected reshape position is
> %llu, "
> > > > > > +					"but reshape progress is
> %llu.\n",
> > > > > > +					position, info-
> >reshape_progress);
> > > > > > +				ret_val = -1;
> > > > > > +			} else {
> > > > > > +				dprintf("Reshape position in md and
> metadata "
> > > > > > +					"are the same;");
> > > > > > +				ret_val = 1;
> > > > > > +			}
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	return ret_val;
> > > > > > +}
> > > > > > +
> > > > > >  static int reshape_array(char *container, int fd, char *devname,
> > > > > >  			 struct supertype *st, struct mdinfo *info,
> > > > > >  			 int force, struct mddev_dev *devlist, @@ -
> 2251,9
> > > > > +2300,16 @@
> > > > > > started:
> > > > > >
> > > > > >  	sra->new_chunk = info->new_chunk;
> > > > > >
> > > > > > -	if (restart)
> > > > > > +	if (restart) {
> > > > > > +		/* for external metadata checkpoint saved by mdmon
> can be
> > > > > lost
> > > > > > +		 * or missed /due to e.g. crash/. Check if md is not
> during
> > > > > > +		 * restart farther than metadata points to.
> > > > > > +		 * If so, this means metadata information is obsolete.
> > > > > > +		 */
> > > > > > +		if (st->ss->external)
> > > > > > +			verify_reshape_position(info, reshape.level);
> > > > > >  		sra->reshape_progress = info->reshape_progress;
> > > > > > -	else {
> > > > > > +	} else {
> > > > > >  		sra->reshape_progress = 0;
> > > > > >  		if (reshape.after.data_disks <
> reshape.before.data_disks)
> > > > > >  			/* start from the end of the new array */ @@ -
> 3765,8
> > > > > +3821,6 @@
> > > > > > int Grow_continue_command(char *devname, int fd,
> > > > > >  	char buf[40];
> > > > > >  	int cfd = -1;
> > > > > >  	int fd2 = -1;
> > > > > > -	char *ep;
> > > > > > -	unsigned long long position;
> > > > > >
> > > > > >  	dprintf("Grow continue from command line called for %s\n",
> > > > > >  		devname);
> > > > > > @@ -3894,28 +3948,8 @@ int Grow_continue_command(char
> > > > > > *devname, int
> > > > > fd,
> > > > > >  	/* verify that array under reshape is started from
> > > > > >  	 * correct position
> > > > > >  	 */
> > > > > > -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> > > > > > -	if (ret_val <= 0) {
> > > > >                     ^^^
> > > > >
> > > > > This test is '<=' however ...
> > > > >
> > > > >
> > > > > > -		fprintf(stderr, Name
> > > > > > -			": cannot open verify reshape progress for %s
> (%i)\n",
> > > > > > -			content->sys_name, ret_val);
> > > > > > -		ret_val = 1;
> > > > > > -		goto Grow_continue_command_exit;
> > > > > > -	}
> > > > > > -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> > > > > > -	position = strtoull(buf, &ep, 0);
> > > > > > -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> > > > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > > > -			" not properly frozen\n");
> > > > > > -		ret_val = 1;
> > > > > > -		goto Grow_continue_command_exit;
> > > > > > -	}
> > > > > > -	position *= get_data_disks(map_name(pers, mdstat->level),
> > > > > > -				   content->new_layout,
> > > > > > -				   content->array.raid_disks);
> > > > > > -	if (position != content->reshape_progress) {
> > > > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > > > -			" not properly frozen.\n");
> > > > > > +	if (verify_reshape_position(content,
> > > > > > +				    map_name(pers, mdstat->level)) < 0)
> {
> > > > >
> > > > > ^^
> > > > >
> > > > > this one is just '<'.  However it looks like it should be the same.
> > > > >
> > > > > Was there a reason for the change, or was it an oversight?
> > > > >
> > > > > Thanks,
> > > > > NeilBrown
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >  		ret_val = 1;
> > > > > >  		goto Grow_continue_command_exit;
> > > > > >  	}
> > > >
> > > >
> > > > Removed code is put in to function verify_reshape_position() for
> > > > use in current location and reshape_array(), as calculated new
> > > > position in
> > > Grow_continue_command() cannot be passed to reshape_array(), it has
> > > to be calculated again.
> > > >
> > > > This is what you are mention above?
> > >
> > > Not exactly.
> > >
> > > The original code would print an error if
> > >       sysfs_get_str(content, NULL, "sync_max", buf, 40); returns 0 or negative.
> > > The new code will not print an error if that returns zero.
> > >
> > > i.e. you changed a '<=' into a '<'.  I wondered if there was some
> > > reason for that - something that I was missing.
> > >
> > > NeilBrown
> >
> > Yes, you are right. I've missed '0' case during code reorganization.
> > You would fix this or you want me to post additional patch?
> 
> Thanks.
> I already fixed it - I've just pushed my current tree with all your patches out.
> 
> Thanks,
> NeilBrown

Maybe this is better.
During this quick update, I've removed by mistake 'ret_val' variable initialization in this patch also, this would be not good ;).

BR
Adam

RE: [PATCH 08/12] FIX: use md position to reshape restart

[Kwolek, Adam]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Kwolek, Adam
> Sent: Thursday, February 09, 2012 11:24
> To: NeilBrown
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> J; Dorau, Lukasz
> Subject: RE: [PATCH 08/12] FIX: use md position to reshape restart
> 
> 
> 
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Thursday, February 09, 2012 11:20
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > Williams, Dan J; Dorau, Lukasz
> > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> >
> > On Thu, 9 Feb 2012 09:49:01 +0000 "Kwolek, Adam"
> > <adam.kwolek@intel.com>
> > wrote:
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: NeilBrown [mailto:neilb@suse.de]
> > > > Sent: Thursday, February 09, 2012 10:19
> > > > To: Kwolek, Adam
> > > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin;
> > > > Williams, Dan J; Dorau, Lukasz
> > > > Subject: Re: [PATCH 08/12] FIX: use md position to reshape restart
> > > >
> > > > On Thu, 9 Feb 2012 08:16:16 +0000 "Kwolek, Adam"
> > > > <adam.kwolek@intel.com>
> > > > wrote:
> > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: NeilBrown [mailto:neilb@suse.de]
> > > > > > Sent: Thursday, February 09, 2012 02:36
> > > > > > To: Kwolek, Adam
> > > > > > Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun,
> > > > > > Marcin; Williams, Dan J; Dorau, Lukasz
> > > > > > Subject: Re: [PATCH 08/12] FIX: use md position to reshape
> > > > > > restart
> > > > > >
> > > > > > On Tue, 07 Feb 2012 15:03:59 +0100 Adam Kwolek
> > > > > > <adam.kwolek@intel.com>
> > > > > > wrote:
> > > > > >
> > > > > > > When reshape is broken, it can occur that metadata is not
> > > > > > > saved
> > properly.
> > > > > > > This can cause that reshape process is farther in md than
> > > > > > > metadata
> > states.
> > > > > > >
> > > > > > > On reshape restart use md position as start position, if it
> > > > > > > is farther than position specified in metadata. Opposite
> > > > > > > situation treat as
> > > > error.
> > > > > > >
> > > > > > > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > > > > > > ---
> > > > > > >
> > > > > > >  Grow.c |   86
> +++++++++++++++++++++++++++++++++++++++++++++--
> > ----
> > > > -----
> > > > > > --------
> > > > > > >  1 files changed, 60 insertions(+), 26 deletions(-)
> > > > > > >
> > > > > > > diff --git a/Grow.c b/Grow.c index 70bdee1..0668a16 100644
> > > > > > > --- a/Grow.c
> > > > > > > +++ b/Grow.c
> > > > > > > @@ -1862,6 +1862,55 @@ release:
> > > > > > >  	return rv;
> > > > > > >  }
> > > > > > >
> > > > > > > +/* verify_reshape_position()
> > > > > > > + *	Function checks if reshape position in metadata is not farther
> > > > > > > + *	than position in md.
> > > > > > > + * Return value:
> > > > > > > + *	 0 : not valid sysfs entry
> > > > > > > + *		it can be caused by not started reshape, it should be
> > > > started
> > > > > > > + *		by reshape array or raid0 array is before takeover
> > > > > > > + *	-1 :	error, reshape position is obviously wrong
> > > > > > > + *	 1 :	success, reshape progress correct or updated
> > > > > > > +*/
> > > > > > > +static int verify_reshape_position(struct mdinfo *info, int level) {
> > > > > > > +	int ret_val = 0;
> > > > > > > +	char buf[40];
> > > > > > > +
> > > > > > > +	/* read sync_max, failure can mean raid0 array */
> > > > > > > +	if (sysfs_get_str(info, NULL, "sync_max", buf, 40) > 0) {
> > > > > > > +		char *ep;
> > > > > > > +		unsigned long long position = strtoull(buf, &ep, 0);
> > > > > > > +
> > > > > > > +		dprintf(Name": Read sync_max sysfs entry is: %s\n",
> > buf);
> > > > > > > +		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != '
> > > > > > > +')))
> > {
> > > > > > > +			position *= get_data_disks(level,
> > > > > > > +						   info->new_layout,
> > > > > > > +						   info-
> > >array.raid_disks);
> > > > > > > +			if (info->reshape_progress < position) {
> > > > > > > +				dprintf("Corrected reshape progress
> > (%llu) to "
> > > > > > > +					"md position (%llu)\n",
> > > > > > > +					info->reshape_progress,
> > position);
> > > > > > > +				info->reshape_progress = position;
> > > > > > > +				ret_val = 1;
> > > > > > > +			} else if (info->reshape_progress > position) {
> > > > > > > +				fprintf(stderr, Name ": Fatal error:
> > array "
> > > > > > > +					"reshape was not properly
> > frozen "
> > > > > > > +					"(expected reshape position is
> > %llu, "
> > > > > > > +					"but reshape progress is
> > %llu.\n",
> > > > > > > +					position, info-
> > >reshape_progress);
> > > > > > > +				ret_val = -1;
> > > > > > > +			} else {
> > > > > > > +				dprintf("Reshape position in md and
> > metadata "
> > > > > > > +					"are the same;");
> > > > > > > +				ret_val = 1;
> > > > > > > +			}
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return ret_val;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int reshape_array(char *container, int fd, char *devname,
> > > > > > >  			 struct supertype *st, struct mdinfo *info,
> > > > > > >  			 int force, struct mddev_dev *devlist, @@ -
> > 2251,9
> > > > > > +2300,16 @@
> > > > > > > started:
> > > > > > >
> > > > > > >  	sra->new_chunk = info->new_chunk;
> > > > > > >
> > > > > > > -	if (restart)
> > > > > > > +	if (restart) {
> > > > > > > +		/* for external metadata checkpoint saved by mdmon
> > can be
> > > > > > lost
> > > > > > > +		 * or missed /due to e.g. crash/. Check if md is not
> > during
> > > > > > > +		 * restart farther than metadata points to.
> > > > > > > +		 * If so, this means metadata information is obsolete.
> > > > > > > +		 */
> > > > > > > +		if (st->ss->external)
> > > > > > > +			verify_reshape_position(info, reshape.level);
> > > > > > >  		sra->reshape_progress = info->reshape_progress;
> > > > > > > -	else {
> > > > > > > +	} else {
> > > > > > >  		sra->reshape_progress = 0;
> > > > > > >  		if (reshape.after.data_disks <
> > reshape.before.data_disks)
> > > > > > >  			/* start from the end of the new array */ @@ -
> > 3765,8
> > > > > > +3821,6 @@
> > > > > > > int Grow_continue_command(char *devname, int fd,
> > > > > > >  	char buf[40];
> > > > > > >  	int cfd = -1;
> > > > > > >  	int fd2 = -1;
> > > > > > > -	char *ep;
> > > > > > > -	unsigned long long position;
> > > > > > >
> > > > > > >  	dprintf("Grow continue from command line called for %s\n",
> > > > > > >  		devname);
> > > > > > > @@ -3894,28 +3948,8 @@ int Grow_continue_command(char
> > > > > > > *devname, int
> > > > > > fd,
> > > > > > >  	/* verify that array under reshape is started from
> > > > > > >  	 * correct position
> > > > > > >  	 */
> > > > > > > -	ret_val = sysfs_get_str(content, NULL, "sync_max", buf, 40);
> > > > > > > -	if (ret_val <= 0) {
> > > > > >                     ^^^
> > > > > >
> > > > > > This test is '<=' however ...
> > > > > >
> > > > > >
> > > > > > > -		fprintf(stderr, Name
> > > > > > > -			": cannot open verify reshape progress for %s
> > (%i)\n",
> > > > > > > -			content->sys_name, ret_val);
> > > > > > > -		ret_val = 1;
> > > > > > > -		goto Grow_continue_command_exit;
> > > > > > > -	}
> > > > > > > -	dprintf(Name ": Read sync_max sysfs entry is: %s\n", buf);
> > > > > > > -	position = strtoull(buf, &ep, 0);
> > > > > > > -	if (ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')) {
> > > > > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > > > > -			" not properly frozen\n");
> > > > > > > -		ret_val = 1;
> > > > > > > -		goto Grow_continue_command_exit;
> > > > > > > -	}
> > > > > > > -	position *= get_data_disks(map_name(pers, mdstat->level),
> > > > > > > -				   content->new_layout,
> > > > > > > -				   content->array.raid_disks);
> > > > > > > -	if (position != content->reshape_progress) {
> > > > > > > -		fprintf(stderr, Name ": Fatal error: array reshape was"
> > > > > > > -			" not properly frozen.\n");
> > > > > > > +	if (verify_reshape_position(content,
> > > > > > > +				    map_name(pers, mdstat->level)) < 0)
> > {
> > > > > >
> > > > > > ^^
> > > > > >
> > > > > > this one is just '<'.  However it looks like it should be the same.
> > > > > >
> > > > > > Was there a reason for the change, or was it an oversight?
> > > > > >
> > > > > > Thanks,
> > > > > > NeilBrown
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > >  		ret_val = 1;
> > > > > > >  		goto Grow_continue_command_exit;
> > > > > > >  	}
> > > > >
> > > > >
> > > > > Removed code is put in to function verify_reshape_position() for
> > > > > use in current location and reshape_array(), as calculated new
> > > > > position in
> > > > Grow_continue_command() cannot be passed to reshape_array(), it
> > > > has to be calculated again.
> > > > >
> > > > > This is what you are mention above?
> > > >
> > > > Not exactly.
> > > >
> > > > The original code would print an error if
> > > >       sysfs_get_str(content, NULL, "sync_max", buf, 40); returns 0 or
> negative.
> > > > The new code will not print an error if that returns zero.
> > > >
> > > > i.e. you changed a '<=' into a '<'.  I wondered if there was some
> > > > reason for that - something that I was missing.
> > > >
> > > > NeilBrown
> > >
> > > Yes, you are right. I've missed '0' case during code reorganization.
> > > You would fix this or you want me to post additional patch?
> >
> > Thanks.
> > I already fixed it - I've just pushed my current tree with all your patches out.
> >
> > Thanks,
> > NeilBrown
> 
> Maybe this is better.
> During this quick update, I've removed by mistake 'ret_val' variable initialization
> in this patch also, this would be not good ;).
> 
> BR
> Adam


I've looked at changes you pushed and this is not exactly the same I've intended to be /look my quick patch with ret_val variable initialization caution/.
Reading 0 length reshape position is an error, absence of sysfs entry is not always an error /raid0 case/.

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

[PATCH 09/12] imsm: FIX: use md position to reshape restart

[Adam Kwolek]

When reshape is broken it can occur that metadata is not saved properly.
This can cause that reshape process is farther in md than metadata states.

On restart save checkpoint to store current position /probably farther/
that can be read from md.

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

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

diff --git a/super-intel.c b/super-intel.c
index f5762d8..5f451f3 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -10067,6 +10067,18 @@ static int imsm_manage_reshape(
 				"are present in copy area.\n");
 			goto abort;
 		}
+		/* Save checkpoint to update migration record for current
+		 * reshape position (in md). It can be farther than current
+		 * reshape position in metadata.
+		 */
+		if (save_checkpoint_imsm(st, sra, UNIT_SRC_NORMAL) == 1) {
+			/* ignore error == 2, this can mean end of reshape here
+			 */
+			dprintf("imsm: Cannot write checkpoint to "
+				"migration record (UNIT_SRC_NORMAL, "
+				"initial save)\n");
+			goto abort;
+		}
 	}
 
 	/* size for data */

[PATCH 10/12] imsm: FIX: Clear migration record when migration switches to next volume.

[Adam Kwolek]

When OLCE is in progress, checkpoint steps are getting bigger due to added space during process.
When mdadm fails after saving "max" to sync_max, mdmon will monitor process
and switch reshape to next array. At this moment we have got information
inconsistency between metadata and migration record.
To avoid this, clear migration record by mdmon /exception from the rule
that migration record is maintained by mdadm/ when reshape switches
to next array.

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

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

diff --git a/super-intel.c b/super-intel.c
index 5f451f3..958edb5 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -353,6 +353,9 @@ struct intel_super {
 		void *migr_rec_buf; /* buffer for I/O operations */
 		struct migr_record *migr_rec; /* migration record */
 	};
+	int clean_migration_record_by_mdmon; /* when reshape is switched to next
+		array, it indicates that mdmon is allowed to clean migration
+		record */
 	size_t len; /* size of the 'buf' allocation */
 	void *next_buf; /* for realloc'ing buf from the manager */
 	size_t next_len;
@@ -3465,6 +3468,7 @@ static int load_imsm_mpb(int fd, struct intel_super *super, char *devname)
 		free(super->buf);
 		return 2;
 	}
+	super->clean_migration_record_by_mdmon = 0;
 
 	if (!sectors) {
 		check_sum = __gen_imsm_checksum(super->anchor);
@@ -5029,6 +5033,10 @@ static int write_super_imsm(struct supertype *st, int doclose)
 	sum = __gen_imsm_checksum(mpb);
 	mpb->check_sum = __cpu_to_le32(sum);
 
+	if (super->clean_migration_record_by_mdmon) {
+		clear_migration_record = 1;
+		super->clean_migration_record_by_mdmon = 0;
+	}
 	if (clear_migration_record)
 		memset(super->migr_rec_buf, 0, MIGR_REC_BUF_SIZE);
 
@@ -5036,9 +5044,6 @@ static int write_super_imsm(struct supertype *st, int doclose)
 	for (d = super->disks; d ; d = d->next) {
 		if (d->index < 0 || is_failed(&d->disk))
 			continue;
-		if (store_imsm_mpb(d->fd, mpb))
-			fprintf(stderr, "%s: failed for device %d:%d (fd: %d)%s\n",
-				__func__, d->major, d->minor, d->fd, strerror(errno));
 
 		if (clear_migration_record) {
 			unsigned long long dsize;
@@ -5050,6 +5055,13 @@ static int write_super_imsm(struct supertype *st, int doclose)
 					perror("Write migr_rec failed");
 			}
 		}
+
+		if (store_imsm_mpb(d->fd, mpb))
+			fprintf(stderr,
+				"%s: failed for device %d:%d (fd: %d)%s\n",
+				__func__, d->major, d->minor,
+				d->fd, strerror(errno));
+
 		if (doclose) {
 			close(d->fd);
 			d->fd = -1;
@@ -6928,6 +6940,7 @@ static void imsm_progress_container_reshape(struct intel_super *super)
 		map2->num_members = prev_num_members;
 
 		imsm_set_array_size(dev);
+		super->clean_migration_record_by_mdmon = 1;
 		super->updates_pending++;
 	}
 }

[PATCH 11/12] FIX: restart reshape when reshape process is stopped just between 2 reshapes

[Adam Kwolek]

When reshape is restarted from '0', very begin of array
it is possible that for external metadata reshape and array
configuration doesn't happen.
Check if md has the same opinion, and reshape is restarted
from 0. If so, this is regular reshape start after reshape
switch in metadata to next array only.

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

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

diff --git a/Grow.c b/Grow.c
index 0668a16..553a49e 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1980,6 +1980,18 @@ static int reshape_array(char *container, int fd, char *devname,
 		goto release;
 	}
 
+	if (st->ss->external && restart && (info->reshape_progress == 0)) {
+		/* When reshape is restarted from '0', very begin of array
+		 * it is possible that for external metadata reshape and array
+		 * configuration doesn't happen.
+		 * Check if md has the same opinion, and reshape is restarted
+		 * from 0. If so, this is regular reshape start after reshape
+		 * switch in metadata to next array only.
+		 */
+		if ((verify_reshape_position(info, reshape.level) >= 0) &&
+		    (info->reshape_progress == 0))
+			restart = 0;
+	}
 	if (restart) {
 		/* reshape already started. just skip to monitoring the reshape */
 		if (reshape.backup_blocks == 0)

[PATCH 12/12] FIX: Do not try to (continue) reshape using inactive array

[Adam Kwolek]

When one of arrays is inactive, do not try to continue reshape
on this array. Just skip it.

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

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

diff --git a/Grow.c b/Grow.c
index 553a49e..e295a5f 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2626,6 +2626,13 @@ int reshape_container(char *container, char *devname,
 						  devname2devnum(container));
 			if (!mdstat)
 				continue;
+			if (mdstat->active == 0) {
+				fprintf(stderr, Name ": Skipping inactive "
+					"array md%i.\n", mdstat->devnum);
+				free_mdstat(mdstat);
+				mdstat = NULL;
+				continue;
+			}
 			break;
 		}
 		if (!content)
@@ -3922,6 +3929,13 @@ int Grow_continue_command(char *devname, int fd,
 			mdstat = mdstat_by_subdev(array, container_dev);
 			if (!mdstat)
 				continue;
+			if (mdstat->active == 0) {
+				fprintf(stderr, Name ": Skipping inactive "
+					"array md%i.\n", mdstat->devnum);
+				free_mdstat(mdstat);
+				mdstat = NULL;
+				continue;
+			}
 			break;
 		}
 		if (!content) {

Re: [PATCH 00/12] Problems during restart container reshape

[NeilBrown]

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

On Tue, 07 Feb 2012 15:02:55 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> This series corrects problems occurred during restart multi arrays reshape:
>  1. Assembled incrementally array during reshape is assembled degraded
>  2. Reshape cannot be continued after reboot. '--continue' gives  an error
>  3. When raid0 arrays are reshaped, first array is reshaped only
>  4. Size doesn't change on second raid0 array
> Please note that patch 'imsm: FIX: No new missing disks are allowed during general migration' can be easily extended by:
>  - Narrow it to incremental assembly only
>  - Array can be pushed to be assembled by using e.g. 'force' option during incremental assembly.
> 
> Other issue addressed in this series is chunk size change from 128k to 4k problem.
> 
> BR
> Adam
> 
> ---
> 
> Adam Kwolek (12):
>       FIX: Do not try to (continue) reshape using inactive array
>       FIX: restart reshape when reshape process is stopped just between 2 reshapes
>       imsm: FIX: Clear migration record when migration switches to next volume.
>       imsm: FIX: use md position to reshape restart
>       FIX: use md position to reshape restart
>       imsm: FIX: Chunk size migration problem
>       Flush mdmon before next reshape step during container operation
>       Fix: Sometimes mdmon throws core dump during reshape
>       imsm: FIX: imsm_get_allowed_degradation() doesn't count degradation for raid1
>       FIX: Array is not run when expansion disks are added
>       imsm: FIX: No new missing disks are allowed during general migration
>       FIX: NULL pointer to strdup() can be passed
> 
> 
>  Assemble.c    |    2 -
>  Grow.c        |  124 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  managemon.c   |    6 +++
>  msg.c         |   10 +++++
>  msg.h         |    1 
>  super-intel.c |   47 +++++++++++++++++++---
>  util.c        |    5 +-
>  7 files changed, 159 insertions(+), 36 deletions(-)
> 

Thanks for these.  I have provisionally applied them all however I won't push
them out until I get confirmation about the '<=' or '<' issue that I
mentioned separately.

Thanks,

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]