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 firstargument 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 beFrom 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...in any order, and we should be able to add sections in the future if necessary.
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 AOr 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 inthe original. Multipath doesn't have that, and neither does mirroring when specifying the log device. For the above example, I did add anPersonally 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 isfollowed by the number of read balancing arguments. After that, it isjust 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 benice 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 completelychanging 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 besomething like devices 2 2 253:3 rr_min=1000 3 253:4 mstate=m rr_min=1000every per device option would be in one place. But parsing would have tobe 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 toidentify 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.)
Description: Binary data