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

From: Kwolek, Adam <hidden>
Date: 2012-02-09 10:23:56

-----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" [off-list ref]
wrote:

quoted

-----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"
[off-list ref]
wrote:

quoted

-----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
[off-list ref]
wrote:

quoted

When reshape is broken, it can occur that metadata is not saved

properly.

quoted

This can cause that reshape process is farther in md than metadata

states.

quoted

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

error.

quoted

Signed-off-by: Adam Kwolek <redacted>
---

 Grow.c |   86 +++++++++++++++++++++++++++++++++++++++++++++--

----

quoted

-----

quoted

--------

quoted

 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

quoted

+ *		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);

quoted

+		if (!(ep == buf || (*ep != 0 && *ep != '\n' && *ep != ' ')))

quoted

+			position *= get_data_disks(level,
+						   info->new_layout,
+						   info-

array.raid_disks);

quoted

+			if (info->reshape_progress < position) {
+				dprintf("Corrected reshape progress

(%llu) to "

quoted

+					"md position (%llu)\n",
+					info->reshape_progress,

position);

quoted

+				info->reshape_progress = position;
+				ret_val = 1;
+			} else if (info->reshape_progress > position) {
+				fprintf(stderr, Name ": Fatal error:

array "

quoted

quoted
quoted
quoted
quoted
+					"reshape was not properly

frozen "

quoted

quoted
quoted
quoted
quoted
+					"(expected reshape position is

%llu, "

quoted

quoted
quoted
quoted
quoted
+					"but reshape progress is

%llu.\n",

quoted

quoted
quoted
quoted
+					position, info-

reshape_progress);

quoted

+				ret_val = -1;
+			} else {
+				dprintf("Reshape position in md and

metadata "

quoted

+					"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

quoted

+2300,16 @@

quoted

started:

 	sra->new_chunk = info->new_chunk;

-	if (restart)
+	if (restart) {
+		/* for external metadata checkpoint saved by mdmon

can be

quoted

lost

quoted

+		 * or missed /due to e.g. crash/. Check if md is not

during

quoted

+		 * 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)

quoted

 			/* start from the end of the new array */ @@ -

3765,8

quoted

+3821,6 @@

quoted

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,

quoted

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

quoted

-		fprintf(stderr, Name
-			": cannot open verify reshape progress for %s

(%i)\n",

quoted

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

quoted

^^

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

quoted

 		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.

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help