Re: [PATCH v2] DM RAID: Add support for MD RAID10

[NeilBrown <neilb@suse.de>]

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

On Wed, 11 Jul 2012 20:36:41 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> Neil,
> 
> I've changed the tunables to the way we discussed.  If it becomes
> necessary to have the freedom to have simultaneous near and far copies,
> then I will likely add 'raid10_near|far|offset_copies' to compliment the
> existing 'raid10_copies' arg.  Like you, I doubt they will be necessary
> though.
> 
> I have yet to add the code that allows new devices to replace old/failed
> devices (i.e. handles the 'rebuild' parameter).  That will be a future
> patch.
> 
>  brassow

Looks good, though a couple of comments below.

Alasdair: I guess we should make sure we are in agreement about how patches
to dm-raid.c are funnelled through.  So far both you and I have feed them to
Linus, which doesn't seem to have caused any problems yet.  Are you OK with
us continuing like that, would you rather all dm-raid.c patched went through
you?
I'm happy either way.

> @@ -76,6 +80,7 @@ static struct raid_type {
>  	const unsigned algorithm;	/* RAID algorithm. */
>  } raid_types[] = {
>  	{"raid1",    "RAID1 (mirroring)",               0, 2, 1, 0 /* NONE */},
> +	{"raid10",   "RAID10 (striped mirrors)",        0, 2, 10, -1 /* Varies */},
>  	{"raid4",    "RAID4 (dedicated parity disk)",	1, 2, 5, ALGORITHM_PARITY_0},
>  	{"raid5_la", "RAID5 (left asymmetric)",		1, 2, 5, ALGORITHM_LEFT_ASYMMETRIC},
>  	{"raid5_ra", "RAID5 (right asymmetric)",	1, 2, 5, ALGORITHM_RIGHT_ASYMMETRIC},

Initialising the "unsigned" algorithm to "-1" looks like it is asking for
trouble.

> @@ -493,7 +554,7 @@ static int parse_raid_params(struct raid
>  			 */
>  			value /= 2;
>  
> -			if (rs->raid_type->level < 5) {
> +			if (rs->raid_type->level != 5) {
>  				rs->ti->error = "Inappropriate argument: stripe_cache";
>  				return -EINVAL;
>  			}

This leaves RAID6 out in the cold.  Maybe
  level < 5 || level > 6
or      !=5          !=6
or a switch statement?


> @@ -536,9 +605,25 @@ static int parse_raid_params(struct raid
>  	if (dm_set_target_max_io_len(rs->ti, max_io_len))
>  		return -EINVAL;
>  
> -	if ((rs->raid_type->level > 1) &&
> -	    sector_div(sectors_per_dev, (rs->md.raid_disks - rs->raid_type->parity_devs))) {
> +	if (rs->raid_type->level == 10) {
> +		/* (Len * Stripes) / Mirrors */
> +		sectors_per_dev *= rs->md.raid_disks;
> +		if (sector_div(sectors_per_dev, raid10_copies)) {
> +			rs->ti->error = "Target length not divisible by number of data devices";
> +			return -EINVAL;
> +		}

I'm not entirely sure what you are trying to do here, but I don't think it
works.

At the very least you would need to convert the "sectors_per_dev" number to a
'chunks_per_dev' before multiplying by raid_disks and dividing by copies.

But even that isn't necessary.  If you have a 3-device near=2 array with an
odd number of chunks per device, that will still work.  The last chunk won't
be mirrored, so won't be used.
Until a couple of weeks ago a recovery of the last device would trigger an
error when we try to recover that last chunk, but that is fixed now.

So if you want to impose this limitation (and there is some merit in making
sure people don't confuse themselves), I suggest it get imposed in the
user-space tools which create the RAID10.


Otherwise it looks good.

Thanks,
NeilBrown

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