[PATCH 1/3] FIX: Do not allow for multiple reshape_array() execution during reshape_container() call

[PATCH 1/3] FIX: Do not allow for multiple reshape_array() execution during reshape_container() call

[Adam Kwolek]

It can happen during reshape restart that reshape_array() can exit without
error (e.g. Grow.c:1915) and reshape is not moved to next array.
reshape_array() is called again for the same device.
Do not allow for such execution and check if last reshaped array is not
the current one.
This patch can be treat not as solution, but it allows for such errors
detection.

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 184a973..1828f83 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2462,6 +2462,7 @@ int reshape_container(char *container, char *devname,
 {
 	struct mdinfo *cc = NULL;
 	int rv = restart;
+	int last_devnum = -1;
 
 	/* component_size is not meaningful for a container,
 	 * so pass '-1' meaning 'no change'
@@ -2546,6 +2547,17 @@ int reshape_container(char *container, char *devname,
 		if (!adev)
 			adev = content->text_version;
 
+		if (last_devnum == mdstat->devnum) {
+			/* do not allow for reentry reshape_array()
+			 * for the same device. It can happen when resahpe_array
+			 */
+			printf(Name ": Multiple reshape execution detected for "
+			       "device  %s.", adev);
+			close(fd);
+			break;
+		}
+		last_devnum = mdstat->devnum;
+
 		sysfs_init(content, fd, mdstat->devnum);
 
 		rv = reshape_array(container, fd, adev, st,

[PATCH 2/3] FIX: Add error message in container_reshape()

[Adam Kwolek]

Add proper error message for container reshape when device cannot be opened.
fd variable operation is moved down to display information what particular
device cannot be opened.

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

 Grow.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Grow.c b/Grow.c
index 1828f83..97f6973 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2538,15 +2538,19 @@ int reshape_container(char *container, char *devname,
 		if (!content)
 			break;
 
-		fd = open_dev(mdstat->devnum);
-		if (fd < 0)
-			break;
 		adev = map_dev(dev2major(mdstat->devnum),
 			       dev2minor(mdstat->devnum),
 			       0);
 		if (!adev)
 			adev = content->text_version;
 
+		fd = open_dev(mdstat->devnum);
+		if (fd < 0) {
+			printf(Name ": Device %s cannot be opened for reshape.",
+			       adev);
+			break;
+		}
+
 		if (last_devnum == mdstat->devnum) {
 			/* do not allow for reentry reshape_array()
 			 * for the same device. It can happen when resahpe_array

[PATCH 3/3] imsm: FIX: return correct status from load_imsm_migr_rec()

[Adam Kwolek]

load_imsm_migr_rec() should see difference between no migration record due
to no migration in progress and loading migration record error.
Additional return value (-2) was introduced to this function.

Using new status load_super_imsm_all() can correctly check loading
migration record status.

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

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

diff --git a/super-intel.c b/super-intel.c
index e8e21f4..3a34f5a 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2120,6 +2120,7 @@ static struct imsm_dev *imsm_get_device_during_migration(
  * Returns:
  *	 0 : success
  *	-1 : fail
+ *	-2 : no migration in progress
  ******************************************************************************/
 static int load_imsm_migr_rec(struct intel_super *super, struct mdinfo *info)
 {
@@ -2137,7 +2138,7 @@ static int load_imsm_migr_rec(struct intel_super *super, struct mdinfo *info)
 	/* nothing to load,no migration in progress?
 	*/
 	if (dev == NULL)
-		return 0;
+		return -2;
 	map = get_imsm_map(dev, MAP_0);
 
 	if (info) {
@@ -4079,13 +4080,16 @@ static int load_super_imsm_all(struct supertype *st, int fd, void **sbp,
 
 	/* load migration record */
 	err = load_imsm_migr_rec(super, NULL);
-	if (err) {
+	if (err == -1) {
+		/* migration is in progress,
+		 * but migr_rec cannot be loaded,
+		 */
 		err = 4;
 		goto error;
 	}
 
 	/* Check migration compatibility */
-	if (check_mpb_migr_compatibility(super) != 0) {
+	if ((err == 0) && (check_mpb_migr_compatibility(super) != 0)) {
 		fprintf(stderr, Name ": Unsupported migration detected");
 		if (devname)
 			fprintf(stderr, " on %s\n", devname);

Re: [PATCH 1/3] FIX: Do not allow for multiple reshape_array() execution during reshape_container() call

[NeilBrown]

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

On Tue, 13 Dec 2011 11:12:10 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> It can happen during reshape restart that reshape_array() can exit without
> error (e.g. Grow.c:1915) and reshape is not moved to next array.
> reshape_array() is called again for the same device.
> Do not allow for such execution and check if last reshaped array is not
> the current one.
> This patch can be treat not as solution, but it allows for such errors
> detection.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>

Hi Adam.
 I'm afraid I understand the problem that requires this patch.
Could you please outline how it can happen that "reshape_array() can exit
without error (e.g. Grow.c:1915) and reshape is not moved to next array."

Also the comment in the code seems to be incomplete.  Could you complete it
please?
Thanks.

The other 2 patches in the series are fine.

Thanks,
NeilBrown

> ---
> 
>  Grow.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 184a973..1828f83 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -2462,6 +2462,7 @@ int reshape_container(char *container, char *devname,
>  {
>  	struct mdinfo *cc = NULL;
>  	int rv = restart;
> +	int last_devnum = -1;
>  
>  	/* component_size is not meaningful for a container,
>  	 * so pass '-1' meaning 'no change'
> @@ -2546,6 +2547,17 @@ int reshape_container(char *container, char *devname,
>  		if (!adev)
>  			adev = content->text_version;
>  
> +		if (last_devnum == mdstat->devnum) {
> +			/* do not allow for reentry reshape_array()
> +			 * for the same device. It can happen when resahpe_array
This one.  Should there be more to this sentence?

NB


> +			 */
> +			printf(Name ": Multiple reshape execution detected for "
> +			       "device  %s.", adev);
> +			close(fd);
> +			break;
> +		}
> +		last_devnum = mdstat->devnum;
> +
>  		sysfs_init(content, fd, mdstat->devnum);
>  
>  		rv = reshape_array(container, fd, adev, st,


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

RE: [PATCH 1/3] FIX: Do not allow for multiple reshape_array() execution during reshape_container() call

[Kwolek, Adam]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of NeilBrown
> Sent: Wednesday, December 14, 2011 9:09 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Ciechanowski, Ed; Labun, Marcin; Williams,
> Dan J
> Subject: Re: [PATCH 1/3] FIX: Do not allow for multiple reshape_array()
> execution during reshape_container() call
> 
> On Tue, 13 Dec 2011 11:12:10 +0100 Adam Kwolek
> <adam.kwolek@intel.com> wrote:
> 
> > It can happen during reshape restart that reshape_array() can exit
> > without error (e.g. Grow.c:1915) and reshape is not moved to next array.
> > reshape_array() is called again for the same device.
> > Do not allow for such execution and check if last reshaped array is
> > not the current one.
> > This patch can be treat not as solution, but it allows for such errors
> > detection.
> >
> > Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> 
> Hi Adam.
>  I'm afraid I understand the problem that requires this patch.
> Could you please outline how it can happen that "reshape_array() can exit
> without error (e.g. Grow.c:1915) and reshape is not moved to next array."

It happens (Lukasz Dorau saw it), when mdadm assembles array in initramfs and even mdadm is directed to assemble array only (--freeze-reshape is used),
filesystetem pivot occurs too early and array is in not stable state. During reshape continuation analyse_change() returns 0 in backup_blocks and reshape_container calls reshape_array() again.
This time restart variable is cleaned and mdadm finishes with error. For now I wanted to put better error description to help Lukasz in problem investigation.
At this moment I do not know more details, but probably when I finish checking raid0 md problem (using patch you sent) I'll go in to tihis.

> 
> Also the comment in the code seems to be incomplete.  Could you complete
> it please?
> Thanks.
> 
> The other 2 patches in the series are fine.
> 
> Thanks,
> NeilBrown
> 
> > ---
> >
> >  Grow.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 184a973..1828f83 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -2462,6 +2462,7 @@ int reshape_container(char *container, char
> > *devname,  {
> >  	struct mdinfo *cc = NULL;
> >  	int rv = restart;
> > +	int last_devnum = -1;
> >
> >  	/* component_size is not meaningful for a container,
> >  	 * so pass '-1' meaning 'no change'
> > @@ -2546,6 +2547,17 @@ int reshape_container(char *container, char
> *devname,
> >  		if (!adev)
> >  			adev = content->text_version;
> >
> > +		if (last_devnum == mdstat->devnum) {
> > +			/* do not allow for reentry reshape_array()
> > +			 * for the same device. It can happen when
> resahpe_array
> This one.  Should there be more to this sentence?


You are right, I've missed it. I'll correct comment and resend this patch.

BR
Adam


> 
> NB
> 
> 
> > +			 */
> > +			printf(Name ": Multiple reshape execution detected
> for "
> > +			       "device  %s.", adev);
> > +			close(fd);
> > +			break;
> > +		}
> > +		last_devnum = mdstat->devnum;
> > +
> >  		sysfs_init(content, fd, mdstat->devnum);
> >
> >  		rv = reshape_array(container, fd, adev, st,