On Thu, Dec 08 2016, Artur Paszkiewicz wrote:
> On 12/07/2016 01:46 AM, NeilBrown wrote:
>> On Tue, Dec 06 2016, Artur Paszkiewicz wrote:
>>
>>> Add a source file for the new policy implementation and allow selecting
>>> the policy based on the policy_type parameter in r5l_init_log().
>>>
>>> Introduce a new flag for rdev state flags to allow enabling the new
>>> policy from userspace.
>>
>> This seems odd. Why is this a per-device flag?
>> It makes sense for "journal" to be a per-device flag, because only one
>> device is the journal device and it is obviously different from the
>> others.
>>
>> But with the ppl, all devices serve as journal devices. So we would
>> need to set journal_ppl on all devices? What happens if you set it on
>> some, but not others? I see you get an error.
>>
>> I think some sort of array-wide setting would make more sense, would it
>> not?
>
> Yes, it would. The problem exists only for external metadata, because
> for native there is a feature flag in the superblock and a corresponding
> flag in mddev->flags. Patch 12 adds a sysfs attribute to control the
> policy at runtime but it would have to be moved out of raid5 personality
> into the main md code. I didn't like the idea of adding something
> specific to raid5 to generic code and visible in sysfs for unrelated
> raid levels.
>
>> And what is an RWH??? A Really Weird Handle ??
>>
>> I guess it is probably a Raid5 Write Hole ? At the very least there
>> should be a comment explaining this when you define the enum. (remember,
>> you are trying to make it easier for reviewers).
>
> That's right, RWH stands for RAID Write Hole. I think I introduced it in
> the cover letter, but I'll explain it also in the code.
>
>> It might almost make sense for bitmap/metadata to be used here.
>> It can currently be "external" "internal" "clustered".
>> Allow also "journalled" or "partial-partiy-log" ???
>>
>> Maybe not ... but I'd definitely prefer a global setting, and one that
>> didn't use an obscure abbreviation.
>
> So do you think the sysfs attribute from patch 12 ("rwh_policy") could
> be made global? This would simplify things but it doesn't seem right.
> And about the abbreviation, should it be called "write_hole_policy" or
> "raid5_write_hole_policy"? Maybe using bitmap/metadata is not a bad
> idea...
How about we call it "resync_policy" which describes how to cope with
unexpected shutdown. Options include:
full regenerate all redundancy info after a crash
bitmap only regenerate redundancy info indicated by bitmap
(both these suseptible to write-hole on raid456)
journal raid456 only, though could theoretically be extended
to raid1, raid10 : log transactions and replay after crash
ppl raid456 only: log partial-parity before writes.
With external metadata, this must be set explicitly. With internal
metadata, it is set best on flags etc.
Thoughts? I'm not really happy with "full", but I cannot think of a
better name.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
