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).
>
>> @@ -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.
>> @@ -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.
brassow
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
