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

[NeilBrown <neilb@suse.de>]

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

On Mon, 16 Jul 2012 17:06:32 -0500 Brassow Jonathan <jbrassow@redhat.com>
wrote:

> 
> On Jul 12, 2012, at 1:32 AM, NeilBrown wrote:
> 
> >> 
> >> @@ -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.
> 
> I'm initializing to -1 because I know it is a value that will fail if not set later in the setup process.  I could just as easily choose the default values (near = 2).

I don't much care what value you use as long as it is of the same type as the
variable you assign it to.  So maybe UINT_MAX, or maybe '2'.


> 
> > 
> >> @@ -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?
> 
> Thanks.  For some reason I had thought that raid6 had level=5, like raid4...  Fixed.

I love consistency ... or at least I think I would if only I could find it
somewhere!

> 
> 
> 
> >> @@ -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.
> 
> I have seen what you do in calc_sectors(), I suppose I could bring that in.  However, I'm simply trying to find the value that will be preliminarily assigned to mddev->dev_sectors.  It is an enforced (perhaps unnecessary) constraint that the array length be divisible by the number of data devices.  I'm not accounting for chunk boundaries - I leave that to userspace to configure and MD to catch.

In that case I suggest you keep it out of dmraid.

It might make sense to check that the resulting array size matches what
user-space said to expect - or is at-least-as-big-as.  However I don't see
much point in other size changes there.

Thanks,
NeilBrown


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