Re: [PATCH 1/3] Remove race for starting container devices.

[NeilBrown <neilb@suse.de>]

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

On Fri, 21 Oct 2011 15:33:18 +0200 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> This moves the lock handling out of Incremental_container() and relies
> on the caller holding the lock. This prevents conflict with a
> follow-on mdadm comment which may try and launch the device in
> parallel.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi Jes,

 I have applied the 7 patches you sent.

However I have question mark over part of this one:


> @@ -451,9 +456,10 @@ int Incremental(char *devname, int verbose, int runstop,
>  			devnum = fd2devnum(mdfd);
>  		close(mdfd);
>  		sysfs_free(sra);
> -		rv = Incremental(chosen_name, verbose, runstop,
> -				 NULL, homehost, require_homehost, autof,
> -				 freeze_reshape);
> +		rv = Incremental_container(st, chosen_name, homehost,
> +					   verbose, runstop, autof,
> +					   freeze_reshape);
> +		map_unlock(&map);
>  		if (rv == 1)
>  			/* Don't fail the whole -I if a subarray didn't
>  			 * have enough devices to start yet

You have replaced a call to Incremental with a call to Incremental_container.
I feel that is significant enough that I would have liked to have seen it
discussed in the changelog comment.

You seem have have open-coded Incremental and then removed all the bits that
you don't need in this case - which is that vast majority of it.
But you didn't include the call to ->load_content().

So I have put it back in because I'm sure it must be harmless (Because it was
there already) but it may not be needed.

If you have thoughts on this, please let me know.

Thanks,
NeilBrown


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