On Mon, 23 Jul 2012 20:18:03 -0500 Brassow Jonathan <jbrassow@xxxxxxxxxx>
wrote:
>
> On Jul 23, 2012, at 5:26 PM, NeilBrown wrote:
>
> > On Mon, 23 Jul 2012 14:49:22 -0500 Jonathan Brassow <jbrassow@xxxxxxxxxx>
>
> </snip>
>
> >
> >>
> >> br@@ -436,6 +475,7 @@ static int parse_raid_params(struct raid
> >> if (rebuild_cnt > rs->raid_type->parity_devs)
> >> rs->ti->error = "Too many rebuild devices specified for given RAID type";
> >> break;
> >> + case 10:
> >> default:
> >> DMERR("The rebuild parameter is not supported for %s", rs->raid_type->name);
> >> rs->ti->error = "Rebuild not supported for this RAID type";
> >
> > This hunk doesn't apply for me, or against 3.5. Is there some patch I'm
> > missing.
> > I do vaguely recall you changing this to a switch statement I think, but I
> > still have an if statement here.
> > If I'm missing a patch - could you resend it please?
>
> My fault. I sent these two patches to dm-devel and failed to CC linux-raid or you.
>
> https://www.redhat.com/archives/dm-devel/2012-July/msg00041.html
> https://www.redhat.com/archives/dm-devel/2012-July/msg00042.html
Maybe that is where I saw it before - I thought I had.
>
> Agk hasn't pushed these two yet and we should probably wait for that to happen. The above two patches are necessary for this patch, but are dependent upon other patches that agk has staged at the moment. The timing is not working out well, and we'll have to wait.
I can do "waiting" (as long as you can help with the waking up when the time
comes).
>
> >>> @@ -536,8 +585,30 @@ 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) {
> >> + if (raid10_copies > rs->md.raid_disks) {
> >> + rs->ti->error = "Not enough devices to satisfy specification";
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* (Len * #mirrors) / #devices */
> >> + sectors_per_dev = rs->ti->len * raid10_copies;
> >> + if (sector_div(sectors_per_dev, rs->md.raid_disks)) {
> >> + rs->ti->error = "Target length not evenly divisible by number of stripes";
> >> + return -EINVAL;
> >> + }
> >
> > This test is still completely pointless, and putting an extra test for chunk
> > alignment after it doesn't make it any less pointless.
> > And putting an important division inside the condition of an if(), hides it a
> > bit more than I like.
> > But it probably isn't worth arguing about it any more so once I can get a
> > patch to apply I'll take it.
>
> I'll pull the division out of the conditional so that it's a little more visible. Once agk has pushed the aforementioned patches, I'll repost this patch with that change as 'v5'.
>
> I don't want to belabor the issue - especially since you are kind enough to be accommodating, but I don't know how I can get by without calculating and setting 'mddev->dev_sectors'. MD can't get that information from anywhere else (when setting up RAID through DM). The main point is to compute sectors_per_dev, but secondarily I am checking for other conditions - like not aligning on chunk boundaries or not being evenly divisible. Failure to set sectors_per_dev - or set it correctly - results in an ill-sized array (mddev->array_sectors). The per device value is not passed in via DM table either - it must be computed. So, I don't understand why it is pointless.
The division is certainly needed. Getting the correct "sectors_per_dev" is
obviously required.
It is the testing of the remainder that is pointless. A non-zero remainder
is not necessarily bad, a zero remainder is not a guarantee that everything
is good.
The test that ti->len == rs->md.array_sectors (which you included, thanks) is
the important test and it makes all the other tests on sizes redundant.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
