Re: [PATCH 9/9] Create: set array status to frozen until monitoring starts

[NeilBrown <neilb@suse.de>]

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

On Tue,  6 Aug 2013 23:38:03 +0200 mwilck@arcor.de wrote:

> When we create an array while mdmon is working on an event
> (e.g. disk failure), the meta data on disk may not be up-to-date.
> 
> Patch "DDF: ddf_open_new: check device status for new subarray"
> added some checks for in the monitor for that situation - in particular,
> to handle a freshly created array with faulty disks. The remaining
> problem is that the kernel may start syncing the disks before this
> situation is detected. This patch delays recovery until mdmon finished
> checking.
> 
> tests/10ddf-fail-create-race should succeed reliably with this patch
> and "DDF: ddf_open_new: check device status for new subarray". Without,
> it will fail sporadically.
> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>
> ---
>  Create.c    |    8 ++++++++
>  managemon.c |    6 ++++++
>  2 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/Create.c b/Create.c
> index ac22f77..f9b7db2 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -993,6 +993,14 @@ int Create(struct supertype *st, char *mddev,
>  				need_mdmon = 0;
>  				break;
>  			default:
> +				/*
> +				 * The meta data we saw on disk may not be
> +				 * up-to-date. The monitor will check and
> +				 * possibly fail. Avoid a resync happening
> +				 * in the kernel before that.
> +				 */
> +				sysfs_set_str(&info, NULL, "sync_action",
> +					      "frozen");
>  				err = sysfs_set_str(&info, NULL, "array_state",
>  						    "readonly");
>  				break;
> diff --git a/managemon.c b/managemon.c
> index f40bbdb..5bc54da 100644
> --- a/managemon.c
> +++ b/managemon.c
> @@ -744,6 +744,12 @@ static void manage_new(struct mdstat_ent *mdstat,
>  		new->container = NULL;
>  		free_aa(new);
>  	} else {
> +		/*
> +		 * Create() set this to frozen.
> +		 * This relies on the kernel clear FROZEN status
> +		 * if an invalid value is written to sync_action.
> +		 */
> +		sysfs_set_str(&new->info, NULL, "sync_action", "");
>  		replace_array(container, victim, new);
>  		if (failed) {
>  			new->check_degraded = 1;


I don't think that this patch should be necessary.
If you find it makes a difference, then I'm missing something.

Recovery never starts while the array is readonly - the kernel doesn't allow
it.
So mdmon should be in complete control as it is the only one that is allowed
to change the array from readonly.

Whether it quite does the right thing in this case I can't promise, but it
should be able to do the right thing without any extra help from mdadm.

And writing an empty string to 'sync_action' will not do anything useful.
You probably mean to write "idle".

I've applied and push out the previous patches but not this one.

Thanks,
NeilBrown


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