[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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

Attachment: mirror_table_change.patch
Description: Binary data



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]