[PATCH 0/4] Reshape problems during double raid0 processing

[PATCH 0/4] Reshape problems during double raid0 processing

[Adam Kwolek]

The following series fixes problems found during double raid0 container reshape.
1. Second raid0 reshape doesn't start
2. Second raid0 array reshape is not finalized in metadata

BR
Adam

---

Adam Kwolek (4):
      FIX: Typo error in fprint command
      FIX: mdmon check in reshape_container() can cause a problem
      FIX: External metadata sometimes is not updated
      FIX: Takeover mdmon when grow is continued from command line


 Grow.c  |   11 ++++-------
 mdadm.h |    1 +
 util.c  |   53 ++++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 49 insertions(+), 16 deletions(-)

-- 
Signature

[PATCH 1/4] FIX: Takeover mdmon when grow is continued from command line

[Adam Kwolek]

When reshape with '--continue' option is executed mdadm doesn't know,
if mdmon is switched to using '--takeover' option to current file system
context. To resolve this, on reshape continuation when mdmon is run,
execute mdmon takeover.

Lack of takeovered mdmon can cause e.g.:
- multivolume reshape is broken when reshaped volume is changed
- size of reshaped volume dowsn't increase

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

 Grow.c  |    2 ++
 mdadm.h |    1 +
 util.c  |   53 ++++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/Grow.c b/Grow.c
index b2c1360..f0422d2 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3876,6 +3876,8 @@ int Grow_continue_command(char *devname, int fd,
 		 */
 		if (!mdmon_running(container_dev))
 			start_mdmon(container_dev);
+		else
+			takeover_mdmon(container_dev);
 		ping_monitor(buf);
 
 		if (mdmon_running(container_dev))
diff --git a/mdadm.h b/mdadm.h
index 381ef86..f274ae7 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1223,6 +1223,7 @@ extern int mdmon_pid(int devnum);
 extern int check_env(char *name);
 extern __u32 random32(void);
 extern int start_mdmon(int devnum);
+extern int takeover_mdmon(int devnum);
 
 extern int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
 			 struct supertype *st, unsigned long stripes,
diff --git a/util.c b/util.c
index 6985a70..e95a366 100644
--- a/util.c
+++ b/util.c
@@ -1581,11 +1581,13 @@ int mdmon_running(int devnum)
 	return 0;
 }
 
-int start_mdmon(int devnum)
+#define MDMON_TAKEOVER_WAIT_COUNTER 15
+int execute_mdmon(int devnum, int takeover)
 {
 	int i, skipped;
 	int len;
-	pid_t pid;	
+	pid_t pid;
+	pid_t pid_org;
 	int status;
 	char pathbuf[1024];
 	char *paths[4] = {
@@ -1611,6 +1613,12 @@ int start_mdmon(int devnum)
 	} else
 		pathbuf[0] = '\0';
 
+	if (takeover) {
+		pid_org = mdmon_pid(devnum);
+		if (pid_org < 0)
+			takeover = 0;
+	}
+
 	switch(fork()) {
 	case 0:
 		/* FIXME yuk. CLOSE_EXEC?? */
@@ -1620,24 +1628,51 @@ int start_mdmon(int devnum)
 				skipped++;
 			else
 				skipped = 0;
-
-		for (i=0; paths[i]; i++)
-			if (paths[i][0])
-				execl(paths[i], "mdmon",
-				      devnum2devname(devnum),
-				      NULL);
+		for (i = 0; paths[i]; i++) {
+			if (paths[i][0]) {
+				if (!takeover)
+					execl(paths[i], "mdmon",
+					      devnum2devname(devnum),
+					      NULL);
+				else
+					execl(paths[i], "mdmon",
+					      "--takeover",
+					      devnum2devname(devnum),
+					      NULL);
+			}
+		}
 		exit(1);
 	case -1: fprintf(stderr, Name ": cannot run mdmon. "
 			 "Array remains readonly\n");
 		return -1;
 	default: /* parent - good */
-		pid = wait(&status);
+		i = 0;
+		do {
+			if (i)
+				sleep(1);
+			pid = wait(&status);
+			if (takeover && (pid == pid_org))
+				pid = -1;
+			i++;
+		} while (takeover &&
+			 (i < MDMON_TAKEOVER_WAIT_COUNTER) &&
+			 (pid < 0));
 		if (pid < 0 || status != 0)
 			return -1;
 	}
 	return 0;
 }
 
+int start_mdmon(int devnum)
+{
+	return execute_mdmon(devnum, 0);
+}
+
+int takeover_mdmon(int devnum)
+{
+	return execute_mdmon(devnum, 1);
+}
+
 int check_env(char *name)
 {
 	char *val = getenv(name);

Re: [PATCH 1/4] FIX: Takeover mdmon when grow is continued from command line

[NeilBrown]

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

On Thu, 12 Jan 2012 08:12:31 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> When reshape with '--continue' option is executed mdadm doesn't know,
> if mdmon is switched to using '--takeover' option to current file system
> context. To resolve this, on reshape continuation when mdmon is run,
> execute mdmon takeover.
> 
> Lack of takeovered mdmon can cause e.g.:
> - multivolume reshape is broken when reshaped volume is changed
> - size of reshaped volume dowsn't increase

I don't think this is right.

In particular, the way we seem to be going with systemd means that we might
not end up using '--takeover' at all.  So performing a --takeover
transparently without being explicitly asked to is not a good idea.

--continue is only used once the final root is mounted and all the early boot
complications are out of the way.
So can't we simple require that --takeover - if it is being used - be
performed before any --continue??

i.e. is there a reason we cannot ensure this works properly simply by making
sure boot scripts do the right things in the right order.

NeilBrown


> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Grow.c  |    2 ++
>  mdadm.h |    1 +
>  util.c  |   53 ++++++++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index b2c1360..f0422d2 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3876,6 +3876,8 @@ int Grow_continue_command(char *devname, int fd,
>  		 */
>  		if (!mdmon_running(container_dev))
>  			start_mdmon(container_dev);
> +		else
> +			takeover_mdmon(container_dev);
>  		ping_monitor(buf);
>  
>  		if (mdmon_running(container_dev))
> diff --git a/mdadm.h b/mdadm.h
> index 381ef86..f274ae7 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1223,6 +1223,7 @@ extern int mdmon_pid(int devnum);
>  extern int check_env(char *name);
>  extern __u32 random32(void);
>  extern int start_mdmon(int devnum);
> +extern int takeover_mdmon(int devnum);
>  
>  extern int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
>  			 struct supertype *st, unsigned long stripes,
> diff --git a/util.c b/util.c
> index 6985a70..e95a366 100644
> --- a/util.c
> +++ b/util.c
> @@ -1581,11 +1581,13 @@ int mdmon_running(int devnum)
>  	return 0;
>  }
>  
> -int start_mdmon(int devnum)
> +#define MDMON_TAKEOVER_WAIT_COUNTER 15
> +int execute_mdmon(int devnum, int takeover)
>  {
>  	int i, skipped;
>  	int len;
> -	pid_t pid;	
> +	pid_t pid;
> +	pid_t pid_org;
>  	int status;
>  	char pathbuf[1024];
>  	char *paths[4] = {
> @@ -1611,6 +1613,12 @@ int start_mdmon(int devnum)
>  	} else
>  		pathbuf[0] = '\0';
>  
> +	if (takeover) {
> +		pid_org = mdmon_pid(devnum);
> +		if (pid_org < 0)
> +			takeover = 0;
> +	}
> +
>  	switch(fork()) {
>  	case 0:
>  		/* FIXME yuk. CLOSE_EXEC?? */
> @@ -1620,24 +1628,51 @@ int start_mdmon(int devnum)
>  				skipped++;
>  			else
>  				skipped = 0;
> -
> -		for (i=0; paths[i]; i++)
> -			if (paths[i][0])
> -				execl(paths[i], "mdmon",
> -				      devnum2devname(devnum),
> -				      NULL);
> +		for (i = 0; paths[i]; i++) {
> +			if (paths[i][0]) {
> +				if (!takeover)
> +					execl(paths[i], "mdmon",
> +					      devnum2devname(devnum),
> +					      NULL);
> +				else
> +					execl(paths[i], "mdmon",
> +					      "--takeover",
> +					      devnum2devname(devnum),
> +					      NULL);
> +			}
> +		}
>  		exit(1);
>  	case -1: fprintf(stderr, Name ": cannot run mdmon. "
>  			 "Array remains readonly\n");
>  		return -1;
>  	default: /* parent - good */
> -		pid = wait(&status);
> +		i = 0;
> +		do {
> +			if (i)
> +				sleep(1);
> +			pid = wait(&status);
> +			if (takeover && (pid == pid_org))
> +				pid = -1;
> +			i++;
> +		} while (takeover &&
> +			 (i < MDMON_TAKEOVER_WAIT_COUNTER) &&
> +			 (pid < 0));
>  		if (pid < 0 || status != 0)
>  			return -1;
>  	}
>  	return 0;
>  }
>  
> +int start_mdmon(int devnum)
> +{
> +	return execute_mdmon(devnum, 0);
> +}
> +
> +int takeover_mdmon(int devnum)
> +{
> +	return execute_mdmon(devnum, 1);
> +}
> +
>  int check_env(char *name)
>  {
>  	char *val = getenv(name);


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

RE: [PATCH 1/4] FIX: Takeover mdmon when grow is continued from command line

[Kwolek, Adam]

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Monday, January 30, 2012 1:24 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan
> J
> Subject: Re: [PATCH 1/4] FIX: Takeover mdmon when grow is continued from
> command line
> 
> On Thu, 12 Jan 2012 08:12:31 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
> 
> > When reshape with '--continue' option is executed mdadm doesn't know,
> > if mdmon is switched to using '--takeover' option to current file
> > system context. To resolve this, on reshape continuation when mdmon is
> > run, execute mdmon takeover.
> >
> > Lack of takeovered mdmon can cause e.g.:
> > - multivolume reshape is broken when reshaped volume is changed
> > - size of reshaped volume dowsn't increase
> 
> I don't think this is right.
> 
> In particular, the way we seem to be going with systemd means that we might
> not end up using '--takeover' at all.  So performing a --takeover transparently
> without being explicitly asked to is not a good idea.
> 
> --continue is only used once the final root is mounted and all the early boot
> complications are out of the way.
> So can't we simple require that --takeover - if it is being used - be performed
> before any --continue??
> 
> i.e. is there a reason we cannot ensure this works properly simply by making sure
> boot scripts do the right things in the right order.
> 
> NeilBrown

This particular patch was written for process automation only. 
What about remaining patches from this series?

BR
Adam


> 
> 
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> > ---
> >
> >  Grow.c  |    2 ++
> >  mdadm.h |    1 +
> >  util.c  |   53 ++++++++++++++++++++++++++++++++++++++++++++---------
> >  3 files changed, 47 insertions(+), 9 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index b2c1360..f0422d2 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -3876,6 +3876,8 @@ int Grow_continue_command(char *devname, int
> fd,
> >  		 */
> >  		if (!mdmon_running(container_dev))
> >  			start_mdmon(container_dev);
> > +		else
> > +			takeover_mdmon(container_dev);
> >  		ping_monitor(buf);
> >
> >  		if (mdmon_running(container_dev))
> > diff --git a/mdadm.h b/mdadm.h
> > index 381ef86..f274ae7 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -1223,6 +1223,7 @@ extern int mdmon_pid(int devnum);  extern int
> > check_env(char *name);  extern __u32 random32(void);  extern int
> > start_mdmon(int devnum);
> > +extern int takeover_mdmon(int devnum);
> >
> >  extern int child_monitor(int afd, struct mdinfo *sra, struct reshape *reshape,
> >  			 struct supertype *st, unsigned long stripes, diff --git
> a/util.c
> > b/util.c index 6985a70..e95a366 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -1581,11 +1581,13 @@ int mdmon_running(int devnum)
> >  	return 0;
> >  }
> >
> > -int start_mdmon(int devnum)
> > +#define MDMON_TAKEOVER_WAIT_COUNTER 15 int execute_mdmon(int
> devnum,
> > +int takeover)
> >  {
> >  	int i, skipped;
> >  	int len;
> > -	pid_t pid;
> > +	pid_t pid;
> > +	pid_t pid_org;
> >  	int status;
> >  	char pathbuf[1024];
> >  	char *paths[4] = {
> > @@ -1611,6 +1613,12 @@ int start_mdmon(int devnum)
> >  	} else
> >  		pathbuf[0] = '\0';
> >
> > +	if (takeover) {
> > +		pid_org = mdmon_pid(devnum);
> > +		if (pid_org < 0)
> > +			takeover = 0;
> > +	}
> > +
> >  	switch(fork()) {
> >  	case 0:
> >  		/* FIXME yuk. CLOSE_EXEC?? */
> > @@ -1620,24 +1628,51 @@ int start_mdmon(int devnum)
> >  				skipped++;
> >  			else
> >  				skipped = 0;
> > -
> > -		for (i=0; paths[i]; i++)
> > -			if (paths[i][0])
> > -				execl(paths[i], "mdmon",
> > -				      devnum2devname(devnum),
> > -				      NULL);
> > +		for (i = 0; paths[i]; i++) {
> > +			if (paths[i][0]) {
> > +				if (!takeover)
> > +					execl(paths[i], "mdmon",
> > +					      devnum2devname(devnum),
> > +					      NULL);
> > +				else
> > +					execl(paths[i], "mdmon",
> > +					      "--takeover",
> > +					      devnum2devname(devnum),
> > +					      NULL);
> > +			}
> > +		}
> >  		exit(1);
> >  	case -1: fprintf(stderr, Name ": cannot run mdmon. "
> >  			 "Array remains readonly\n");
> >  		return -1;
> >  	default: /* parent - good */
> > -		pid = wait(&status);
> > +		i = 0;
> > +		do {
> > +			if (i)
> > +				sleep(1);
> > +			pid = wait(&status);
> > +			if (takeover && (pid == pid_org))
> > +				pid = -1;
> > +			i++;
> > +		} while (takeover &&
> > +			 (i < MDMON_TAKEOVER_WAIT_COUNTER) &&
> > +			 (pid < 0));
> >  		if (pid < 0 || status != 0)
> >  			return -1;
> >  	}
> >  	return 0;
> >  }
> >
> > +int start_mdmon(int devnum)
> > +{
> > +	return execute_mdmon(devnum, 0);
> > +}
> > +
> > +int takeover_mdmon(int devnum)
> > +{
> > +	return execute_mdmon(devnum, 1);
> > +}
> > +
> >  int check_env(char *name)
> >  {
> >  	char *val = getenv(name);

[PATCH 2/4] FIX: External metadata sometimes is not updated

[Adam Kwolek]

External metadata sometimes is not updated.
It can be observed during 2 raid0 arrays Capacity Expansion.
New array size is not set, because metadata is not updated and on the reshape
end mdadm doesn't read new array size from metadata.
This happens when mdmon finishes his work (due to takeover to raid0),
before all metadata updates are processed.

Make sure that all updates are flushed to disk before executing takeover.

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

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

diff --git a/Grow.c b/Grow.c
index f0422d2..061ddf1 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2396,6 +2396,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);
 			st->ss->free_super(st);
 			st->ss->load_container(st, cfd, container);

[PATCH 3/4] FIX: mdmon check in reshape_container() can cause a problem

[Adam Kwolek]

When raid0 reshape is executed mdmon can dissappear due to raid level
takeover operation. If this happen before mdmon check, mdadm would treat
it as error condition. It is not true for this case.

Remove mdmon check from reshape_container() function.
Error condition check will remain using reshape_array() reentry test
for the same array (line 2577).

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

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

diff --git a/Grow.c b/Grow.c
index 061ddf1..b52d34b 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2608,12 +2608,6 @@ int reshape_container(char *container, char *devname,
 		restart = 0;
 		if (rv)
 			break;
-		rv = !mdmon_running(devname2devnum(container));
-		if (rv) {
-			printf(Name ": Mdmon is not found. "
-			       "Cannot continue container reshape.\n");
-			break;
-		}
 	}
 	if (!rv)
 		unfreeze(st);

[PATCH 4/4] FIX: Typo error in fprint command

[Adam Kwolek]

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

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

diff --git a/Grow.c b/Grow.c
index b52d34b..5dab37c 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2323,7 +2323,7 @@ started:
 		free(offsets);
 		sysfs_free(sra);
 		fprintf(stderr, Name ": Reshape has to be continued from"
-			" location %llu when root fileststem has been mounted\n",
+			" location %llu when root filesystem has been mounted.\n",
 			sra->reshape_progress);
 		return 1;
 	}

RE: [PATCH 0/4] Reshape problems during double raid0 processing

[Kwolek, Adam]

Hello Neil,

Can you have a look at this series?

Thank you
Adam



-----Original Message-----
From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-owner@vger.kernel.org] On Behalf Of Adam Kwolek
Sent: Thursday, January 12, 2012 8:12 AM
To: neilb@suse.de
Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams, Dan J
Subject: [PATCH 0/4] Reshape problems during double raid0 processing

The following series fixes problems found during double raid0 container reshape.
1. Second raid0 reshape doesn't start
2. Second raid0 array reshape is not finalized in metadata

BR
Adam

---

Adam Kwolek (4):
      FIX: Typo error in fprint command
      FIX: mdmon check in reshape_container() can cause a problem
      FIX: External metadata sometimes is not updated
      FIX: Takeover mdmon when grow is continued from command line


 Grow.c  |   11 ++++-------
 mdadm.h |    1 +
 util.c  |   53 ++++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 49 insertions(+), 16 deletions(-)

--
Signature
--
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/4] Reshape problems during double raid0 processing

[NeilBrown]

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

On Thu, 12 Jan 2012 08:12:24 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> The following series fixes problems found during double raid0 container reshape.
> 1. Second raid0 reshape doesn't start
> 2. Second raid0 array reshape is not finalized in metadata
> 
> BR
> Adam
> 
> ---
> 
> Adam Kwolek (4):
>       FIX: Typo error in fprint command
>       FIX: mdmon check in reshape_container() can cause a problem
>       FIX: External metadata sometimes is not updated

Above 3 applied, thanks.

>       FIX: Takeover mdmon when grow is continued from command line

Not applied and discussed in separate email.


Thanks,
NeilBrown


> 
> 
>  Grow.c  |   11 ++++-------
>  mdadm.h |    1 +
>  util.c  |   53 ++++++++++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 49 insertions(+), 16 deletions(-)
> 


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