Re: [PATCH 05/27] DDF: Implement store_super_ddf

[NeilBrown <neilb@suse.de>]

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

On Wed,  3 Jul 2013 22:27:45 +0200 mwilck@arcor.de wrote:

> This patch implements the previously unsupported case where
> store_super_ddf is called with a non-empty superblock.
> 
> For DDF, writing meta data to just one disk makes no sense.
> We would run the risk of writing inconsistent meta data
> to the devices. So just call __write_init_super_ddf and
> write to all devices, including the one passed by the caller.
> 
> This patch assumes that the device to store the superblock on
> has already been added to the DDF structure. Otherwise, an
> error message will be emitted.
> 
> Signed-off-by: Martin Wilck <mwilck@arcor.de>

I'm not sure I really like this.  If mdadm is calling ->store_super, then it
wants to write to just one device.  It will quite possibly call
->store_super on all devices, and with your change that will write to all
devices multiple times.
So maybe a different interface is needed?

What is the big-picture effect of this patch?  What mdadm function now works
that didn't work before?

Thanks,
NeilBrown



> ---
>  super-ddf.c |   41 +++++++++++++++++++++++++++++++++++------
>  1 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index b806949..b3c846d 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -3471,15 +3471,44 @@ static int store_super_ddf(struct supertype *st, int fd)
>  	if (!ddf)
>  		return 1;
>  
> -	/* ->dlist and ->conflist will be set for updates, currently not
> -	 * supported
> -	 */
> -	if (ddf->dlist || ddf->conflist)
> -		return 1;
> -
>  	if (!get_dev_size(fd, NULL, &dsize))
>  		return 1;
>  
> +	if (ddf->dlist || ddf->conflist) {
> +		struct stat sta;
> +		struct dl *dl;
> +		int ofd, ret;
> +
> +		if (fstat(fd, &sta) == -1 || !S_ISBLK(sta.st_mode)) {
> +			pr_err("%s: file descriptor for invalid device\n",
> +			       __func__);
> +			return 1;
> +		}
> +		for (dl = ddf->dlist; dl; dl = dl->next)
> +			if (dl->major == (int)major(sta.st_rdev) &&
> +			    dl->minor == (int)minor(sta.st_rdev))
> +				break;
> +		if (!dl) {
> +			pr_err("%s: couldn't find disk %d/%d\n", __func__,
> +			       (int)major(sta.st_rdev),
> +			       (int)minor(sta.st_rdev));
> +			return 1;
> +		}
> +		/*
> +		   For DDF, writing to just one disk makes no sense.
> +		   We would run the risk of writing inconsistent meta data
> +		   to the devices. So just call __write_init_super_ddf and
> +		   write to all devices, including this one.
> +		   Use the fd passed to this function, just in case dl->fd
> +		   is invalid.
> +		 */
> +		ofd = dl->fd;
> +		dl->fd = fd;
> +		ret =  __write_init_super_ddf(st);
> +		dl->fd = ofd;
> +		return ret;
> +	}
> +
>  	if (posix_memalign(&buf, 512, 512) != 0)
>  		return 1;
>  	memset(buf, 0, 512);


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