Re: [PATCH 3/8] Do not restart reshape if it is started already

[NeilBrown <neilb@suse.de>]

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

On Tue, 27 Sep 2011 14:04:57 +0200 Adam Kwolek <adam.kwolek@intel.com> wrote:

> When reshape was invoked during initrd start-up stage array is pushed
> in to reshape state already, so read only state cannot be set again during
> reshape continuation. Set previously reshape state has to be reused
> during reshape continuation.
> 
> Signed-off-by: Adam Kwolek <adam.kwolek@intel.com>
> ---
> 
>  Grow.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 768fc86..d9c2817 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3773,11 +3773,17 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
>  {
>  	char buf[40];
>  	char *container = NULL;
> -	int err;
> +	int err = 0;
>  
> -	err = sysfs_set_str(info, NULL, "array_state", "readonly");
> +	/* set read only array state when there is no reshape
> +	 * in progress already
> +	 */
> +	if ((sysfs_get_str(info, NULL, "sync_action", buf, 40) != 8) &&
> +	    (strncmp(buf, "reshape", 7) != 0))
> +		err = sysfs_set_str(info, NULL, "array_state", "readonly");
>  	if (err)
>  		return err;
> +
>  	if (st->ss->external) {
>  		fmt_devname(buf, st->container_dev);
>  		container = buf;


This is wrong.
For a start the '&&' should be '||' or it doesn't make sense.

The reason were are setting readonly here is that the array hasn't been
activated at all, so it isn't possible to freeze anything.
So we set the array to readonly so that no reshape/resync/etc can start, then
activate the array in reshape_array, then freeze, and allow it to be
read-write.

So Grow_continue really assumes that the array hasn't been started yet.
You are using it in different situation, where it has been started.
For that to be reasonably, you really need to tell Grow_continue that the
array hasn't started, not let it try to figure out for itself.

However I think it would be easier to just copy the code from Grow_continue
into the place where you called it in Grow_continue_command and then remove
the bits that you don't need.
You don't need the readonly setting and you don't need the start_mdmon and I
don't think you need the freeze(), so it becomes:

if (st->ss->external && info->reshape_active == 2) {
  		int cfd = open_dev(st->container_dev);
		if (cfd < 0)
			return 1;
		st->ss->load_container(st, cfd, container);
		close(cfd);
		ret_val = reshape_container(container, NULL,
					 st, info, 0, backup_file,
					 0, 1, freeze_reshape);
	}
} else
	ret_val = reshape_array(container, mdfd, "array", st, info, 1,
			     NULL, backup_file, 0, 0, 1, freeze_reshape);

which is a better result I think.

So I won't apply this patch.  Please consider the above and submit a revised
version if you agree.

Thanks,
NeilBrown

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