[PATCH] Grow: Do not use grow-continue unit file if reshape is starting

[PATCH] Grow: Do not use grow-continue unit file if reshape is starting

[Baldysiak, Pawel]

Mdadm should use mdadm-grow-continue unit file only if
reshape is going to be continued. Otherwise, array specific
reshape with IMSM metadata will fail to start, due to
missing information about ongoing migration -
grow-continue will try to start again the reshape process.

Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
---
 Grow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index a2f4f14..0cd9442 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3272,7 +3272,7 @@ started:
 		return 1;
 	}
 
-	if (!forked && !check_env("MDADM_NO_SYSTEMCTL"))
+	if (restart && !forked && !check_env("MDADM_NO_SYSTEMCTL"))
 		if (continue_via_systemd(container ?: sra->sys_name)) {
 			free(fdlist);
 			free(offsets);
-- 
1.9.3

Re: [PATCH] Grow: Do not use grow-continue unit file if reshape is starting

[NeilBrown]

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

On Fri, 4 Jul 2014 08:59:00 +0000 "Baldysiak, Pawel"
<pawel.baldysiak@intel.com> wrote:

> Mdadm should use mdadm-grow-continue unit file only if
> reshape is going to be continued. Otherwise, array specific
> reshape with IMSM metadata will fail to start, due to
> missing information about ongoing migration -
> grow-continue will try to start again the reshape process.

I don't think I agree.  I think mdadm should always use mdadm-grow-continue
unit file if it is available.

Please explain in more detail what problem you are seeing.

NeilBrown

> 
> Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> ---
>  Grow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Grow.c b/Grow.c
> index a2f4f14..0cd9442 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3272,7 +3272,7 @@ started:
>  		return 1;
>  	}
>  
> -	if (!forked && !check_env("MDADM_NO_SYSTEMCTL"))
> +	if (restart && !forked && !check_env("MDADM_NO_SYSTEMCTL"))
>  		if (continue_via_systemd(container ?: sra->sys_name)) {
>  			free(fdlist);
>  			free(offsets);


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

RE: [PATCH] Grow: Do not use grow-continue unit file if reshape is starting

[Baldysiak, Pawel]

> On Monday, July 07, 2014 3:06 AM Neil Brown wrote:
> To: Baldysiak, Pawel
> Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur
> Subject: Re: [PATCH] Grow: Do not use grow-continue unit file if reshape is
> starting
> 
> On Fri, 4 Jul 2014 08:59:00 +0000 "Baldysiak, Pawel"
> <pawel.baldysiak@intel.com> wrote:
> 
> > Mdadm should use mdadm-grow-continue unit file only if reshape is
> > going to be continued. Otherwise, array specific reshape with IMSM
> > metadata will fail to start, due to missing information about ongoing
> > migration - grow-continue will try to start again the reshape process.
> 
> I don't think I agree.  I think mdadm should always use mdadm-grow-
> continue unit file if it is available.
> 
> Please explain in more detail what problem you are seeing.
> 
> NeilBrown
> 

Hi Neil

If we try to do array-specific reshape (e.g. change from level 0 to 5) 
mdadm will impose all changes to metadata and start reshape - before running grow-continue.
After that  grow-continue will see that reshape progress is '0', so will overwrite "restart" by setting it to =0.
Then it will try to impose all changes again and fail - with error:
"Cannot set device shape for /dev/md/vol" - It will be unable to write changes to sysfs,
due to already started reshape (which freeze at '0' progress).

We can try to fix it by moving part of code that runs grow-continue in reshape_array earlier,
 so "start_reshape() " will be executed by grow-continue (I am testing that kind of patch now), 
or forbid running grow-continue when reshape is about to start (patch is tested, attach in my prev mail).

What do you think about that?
Do you have any other idea how to fix this issue?

Thanks
Pawel Baldysiak

> >
> > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> > ---
> >  Grow.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index a2f4f14..0cd9442 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -3272,7 +3272,7 @@ started:
> >  		return 1;
> >  	}
> >
> > -	if (!forked && !check_env("MDADM_NO_SYSTEMCTL"))
> > +	if (restart && !forked && !check_env("MDADM_NO_SYSTEMCTL"))
> >  		if (continue_via_systemd(container ?: sra->sys_name)) {
> >  			free(fdlist);
> >  			free(offsets);

Re: [PATCH] Grow: Do not use grow-continue unit file if reshape is starting

[NeilBrown]

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

On Mon, 7 Jul 2014 14:18:40 +0000 "Baldysiak, Pawel"
<pawel.baldysiak@intel.com> wrote:

> > On Monday, July 07, 2014 3:06 AM Neil Brown wrote:
> > To: Baldysiak, Pawel
> > Cc: linux-raid@vger.kernel.org; Paszkiewicz, Artur
> > Subject: Re: [PATCH] Grow: Do not use grow-continue unit file if reshape is
> > starting
> > 
> > On Fri, 4 Jul 2014 08:59:00 +0000 "Baldysiak, Pawel"
> > <pawel.baldysiak@intel.com> wrote:
> > 
> > > Mdadm should use mdadm-grow-continue unit file only if reshape is
> > > going to be continued. Otherwise, array specific reshape with IMSM
> > > metadata will fail to start, due to missing information about ongoing
> > > migration - grow-continue will try to start again the reshape process.
> > 
> > I don't think I agree.  I think mdadm should always use mdadm-grow-
> > continue unit file if it is available.
> > 
> > Please explain in more detail what problem you are seeing.
> > 
> > NeilBrown
> > 
> 
> Hi Neil
> 
> If we try to do array-specific reshape (e.g. change from level 0 to 5) 
> mdadm will impose all changes to metadata and start reshape - before running grow-continue.
> After that  grow-continue will see that reshape progress is '0', so will overwrite "restart" by setting it to =0.

I assume you are referring to this code:
	if (st->ss->external && restart && (info->reshape_progress == 0)) {
          .....
		if ((verify_reshape_position(info, reshape.level) >= 0) &&
		    (info->reshape_progress == 0))
			restart = 0;
	}

in reshape_array().

> Then it will try to impose all changes again and fail - with error:
> "Cannot set device shape for /dev/md/vol" - It will be unable to write changes to sysfs,
> due to already started reshape (which freeze at '0' progress).
> 
> We can try to fix it by moving part of code that runs grow-continue in reshape_array earlier,
>  so "start_reshape() " will be executed by grow-continue (I am testing that kind of patch now), 
> or forbid running grow-continue when reshape is about to start (patch is tested, attach in my prev mail).
> 
> What do you think about that?
> Do you have any other idea how to fix this issue?

I suspect that the real problem is in the code above.
Maybe it should read the "sync_action" attribute and if it is "reshape", then
this is obviously a restart and it shouldn't set "restart" to 0.

Can you try something like that?

NeilBrown


> 
> Thanks
> Pawel Baldysiak
> 
> > >
> > > Signed-off-by: Pawel Baldysiak <pawel.baldysiak@intel.com>
> > > ---
> > >  Grow.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index a2f4f14..0cd9442 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -3272,7 +3272,7 @@ started:
> > >  		return 1;
> > >  	}
> > >
> > > -	if (!forked && !check_env("MDADM_NO_SYSTEMCTL"))
> > > +	if (restart && !forked && !check_env("MDADM_NO_SYSTEMCTL"))
> > >  		if (continue_via_systemd(container ?: sra->sys_name)) {
> > >  			free(fdlist);
> > >  			free(offsets);



-- 

-- 

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