[dm-devel] [RFC] Change to the mirror table map

[Jonathan E Brassow]

On Oct 20, 2006, at 11:18 AM, Stefan Bader wrote:

> Hi Jonathan,
>
>
>> New features:
>>
>> I've added a format argument ("format2" in example).  It is always 
>> the first
>> argument to the mirror target.  If it isn't present, we can assume
>> the original
>> format.  If it is present, we can use the format specified.
>>
> Probably isn't needed with section keywords. Something not "core" or
> "disk" is format2.

or "clustered_disk" or "clustered_core" or any other logging 
implementation that we may develop in the future, like "multi_disk".  
The problem is, then we need to either encode all those in the 
mirror_ctr or wait for 'create_dirty_log' to return NULL - then pass on 
to the second format, which may also return an error.

And if we again decide to change the format in the future...

With the format argument, we don't need the extra logic.  We simply 
pass on the rest to the correct constructor function for that format.

>> The mapping table is broken up into sections.  The sections are 
>> allowed to be
>> in any order, and we should be able to add sections in the future if
>> necessary.
>>
> From the user side good since it allows more flexibility. The downside 
> is it adds complexity to the kernel parsing. And there might be 
> sections that depend on each other...

That's why the section identifier is there.  We can identify the 
sections and reorder them as we construct the mirror.  If we add new 
sections in the future that we may want decoded before some sections 
but after others, we don't need to worry about were they are placed in 
the table line.

Additionally, since we know the number of arguments, it is easy to 
identify sections and their arguments and pass them on to other 
functions that further break them down.

Attached, please find an example of how the new constructor might look.

>
>> The devices section starts with the keyword 'devices' and is followed 
>> by the
>> number of devices then the number of per device arguments.  This is 
>> not
>> consistent with the other sections.  It may be better to have the 
>> keyword
>> followed by the argument count, followed by the per device argument 
>> count.
>> Something like:
>>     devices 5 1 253:3 S 253:4 A
>> or
>>     devices 5 2 253:3 S 253:4 A
>>
> Or something like
>         devices 2 2 543:3 S 2 253:4 A
> so each device can have a different number of arguments?
>
>> depending on if you want to include the device in the per device 
>> argument
>> count.  Notice that I have done away with the 'offset' parameter 
>> found in
>> the original.  Multipath doesn't have that, and neither does mirroring
>> when specifying the log device.  For the above example, I did add an
>>
> Personally I think the offset is useful if doing mappings for complete
> partitions/disks and the log should rather have it as well.

For simplicity, I've kept the offset (pvmove uses it), removed the 
status arg, and made the section argument count consistent.  I now 
think it should look something like:
     devices 4 253:3 0 253:4 0
(The example patch expects this.)

>> The read balancing section starts with the keyword 'read-balance' and 
>> is
>> followed by the number of read balancing arguments.  After that, it is
>> just like multipathing.  If we were certain that the devices section 
>> did
>> not need any device specific arguments, we could combine the devices 
>> section
>> and the read-balance section.  (This could be the case if we decided 
>> to
>> add a section for initial device status and didn't need the offset 
>> argument.)
>> Mirror does need to keep it's own list of devices, however; and it 
>> might be
>> nice to be able to parse that section separate - rather than parse the
>> read-balancing section twice.
>>
> I don't know if this can be solved in a nice way without completely
> changing the way the layout is today. :/ I undestand why going this 
> way but it
> feels a bit ugly to have the same device specification twice. If there 
> would be
> something like
>         devices 2 2 253:3 rr_min=1000 3 253:4 mstate=m rr_min=1000
> every per device option would be in one place. But parsing would have 
> to
> be done more than once...

I agree that having to state a device multiple times is ugly.  However, 
I don't like parsing sections multiple times for different things.  I'd 
really like to have a 'section to function' mapping if possible.  
Additionally, we can't be sure what extra information (sections) we may 
add in the future.  (Think device status, or other).  That could be 
solved by your solution above and would simply change the per device 
argument count; but again, I'd like to avoid parsing the same section 
multiple times - or mangling the order of parameters to much just to 
pass them to say, the path selector constructor for read balancing.)

>
>> Multipath takes the approach of having positional arguments (e.g. the 
>> 'number
>> of features' argument is in a known place).  If we took this 
>> approach, we
>> could eliminate the keywords.  However, I think I prefer the ability 
>> to
>> identify sections.
>>
> It makes identification of section simpler to parse by the human reader
> which
> I like as well. ;-)
>
> Stefan
>

Thank-you for your comments Stefan.  Below is a quick patch of how I 
would see the code changes for the constructor.  (I didn't put in the 
per-section functions.  Nor did I put in the read balancing stuff yet.  
This is just an example of how to make the constructor more flexible... 
paving the way for read balancing.)

  brassow

-------------- next part --------------
A non-text attachment was scrubbed...
Name: mirror_table_change.patch
Type: application/octet-stream
Size: 4282 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20061020/778a1abb/attachment.obj>
-------------- next part --------------