* [RFC] Change to the mirror table map
@ 2006-10-19 20:56 Jonathan Brassow
2006-10-20 16:18 ` Stefan Bader
2006-10-24 9:30 ` Heinz Mauelshagen
0 siblings, 2 replies; 6+ messages in thread
From: Jonathan Brassow @ 2006-10-19 20:56 UTC (permalink / raw)
To: dm-devel, agk
The impetus for this was the desire to add read balancing to mirroring
and have it share code with multipath.
brassow
ORIGINAL:
<start> <length> mirror \
<log_type> <#log_params> <log_params> \
<#mirrors> <device1> <offset1> ... <deviceN> <offsetN>
Example:
0 1024000 mirror \
disk 3 253:2 1024 block_on_error \
2 253:3 0 253:4 0
MODIFIED:
[-------- logging section ---------] [- mirror devices section -] [--------------- read balancing section -------------]
0 71014400 mirror format2 log 4 disk 253:2 1024 block_on_error devices 2 1 253:3 S 253:4 A read-balance 8 round-robin 0 2 1 253:3 1000 253:4 1000
^ ^ ^ ^ ^ ^ ^ <^^^^^^^^^^^^^^^^^^^^^^^> ^ ^ ^ ^ ^ ^ ^ <^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^>
| | | | | | | | | | | | | | | |
| | | | | | | | | | | | | | | same as multipath
| | | | | | | | | | | | | | # read-balance args
| | | | | | | | | | | | | read-balance section indicator
| | | | | | | | | | | | Device status (slave/active/dead)
| | | | | | | | | | | Device
| | | | | | | | | | # of device args
| | | | | | | | | # of devices
| | | | | | | | device section indicator
| | | | | | | log type specific args
| | | | | | log type
| | | | | # of log args
| | | | log section indicator
| | | target table format
| | target name
| target length in 512-byte blocks
starting offset of target
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.
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.
The logging section starts with the keyword 'log' and is followed by the number
of log arguments. No changes to the dm_create_dirty_log function should be
necessary.
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
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
additional per device parameter to specify the status of a device. I was
imagining a scenario where IBM's slave context might be specified, or
a new mirror device is added to the set. With the indicator, you could
tell which devices were in-sync, and which ones needed recovery.
(Perhaps that is better left to an additional section, since it is not
yet known how this functionality would be added.)
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.
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Change to the mirror table map
2006-10-19 20:56 [RFC] Change to the mirror table map Jonathan Brassow
@ 2006-10-20 16:18 ` Stefan Bader
2006-10-20 19:36 ` Jonathan E Brassow
2006-10-24 9:30 ` Heinz Mauelshagen
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Bader @ 2006-10-20 16:18 UTC (permalink / raw)
To: device-mapper development; +Cc: agk
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" od
"disk"
is format2.
> 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
that
it adds complexity to the kernel parsing. And there might be sections that
depend on each other...
> 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.
> 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...
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Change to the mirror table map
2006-10-20 16:18 ` Stefan Bader
@ 2006-10-20 19:36 ` Jonathan E Brassow
2006-10-23 16:01 ` Stefan Bader
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan E Brassow @ 2006-10-20 19:36 UTC (permalink / raw)
To: device-mapper development; +Cc: agk
[-- Attachment #1: Type: text/plain, Size: 5546 bytes --]
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 #2: mirror_table_change.patch --]
[-- Type: application/octet-stream, Size: 4282 bytes --]
Index: linux-2.6.18/drivers/md/dm-raid1.c
===================================================================
--- linux-2.6.18.orig/drivers/md/dm-raid1.c 2006-10-20 10:39:24.000000000 -0500
+++ linux-2.6.18/drivers/md/dm-raid1.c 2006-10-20 13:54:55.000000000 -0500
@@ -1016,13 +1016,137 @@ static struct dirty_log *create_dirty_lo
}
/*
- * Construct a mirror mapping:
+ * mirror_format2_ctr
+ * @ti
+ * @argc
+ * @argv
*
- * log_type #log_params <log_params>
- * #mirrors [mirror_path offset]{2,}
+ * Format2:
+ * format2 \
+ * log <#log_params> <log_params> \
+ * devices <#dev_params> <device1> <offset1> ... <deviceN> <offsetN>
+ * Example:
+ * format2 log 4 disk 253:2 1024 block_on_error devices 2 253:3 0 253:4 0
*
- * log_type is "core" or "disk"
- * #log_params is between 1 and 3
+ * Format2 allows for new sections, as well as for the sections to
+ * be in any order.
+ *
+ * Returns: 0 on success, -EXXX on failure
+ */
+static int mirror_format2_ctr(struct dm_target *ti, unsigned int argc, char **argv)
+{
+ unsigned int log_argc, devices_argc, section_argc;
+ char **log_argv, **devices_argv, *section_id;
+ struct dirty_log *dl;
+ unsigned int nr_mirrors, m;
+
+ argv++; /* skip over "format2" arg */
+ argc--;
+
+ /* Identify the sections */
+ while (argc) {
+ section_id = argv[0];
+
+ /* FIXME: Break this down for better error reporting? */
+ if ((argc < 2) ||
+ (sscanf(argv[1], "%u", §ion_argc) != 1) ||
+ (section_argc > (argc - 2))) {
+ ti->error = "Invalid mirror mapping table";
+ return -EINVAL;
+ } else {
+ argc -= 2;
+ argv += 2;
+ }
+
+ if (!strcmp("log", section_id)) {
+ log_argv = argv;
+ log_argc = section_argc;
+ } else if (!strcmp("devices", section_id)) {
+ devices_argv = argv;
+ devices_argc = section_argc;
+ } else {
+ ti->error = "Unknown section in mirror mapping table";
+ return -EINVAL;
+ }
+ argc -= section_argc;
+ argv += section_argc;
+ }
+
+ /* Construct mirror - could make each section a separate function */
+ if (log_argc < 2) {
+ ti->error = "Insufficient mirror log arguments";
+ return -EINVAL;;
+ }
+ dl = dm_create_dirty_log(log_argv[0], ti, log_argc - 1, log_argv + 1);
+ if (!dl) {
+ ti->error = "Error creating mirror dirty log";
+ return -EINVAL;
+ }
+
+ if (!_check_region_size(ti, dl->type->get_region_size(dl))) {
+ ti->error = "Invalid region size";
+ dm_destroy_dirty_log(dl);
+ return -EINVAL;
+ }
+
+ /*
+ * Right now, devices_argc is equal to 1/2 the number of mirror
+ * devices specified. However, this could change in the
+ * future depending on feedback from other developers.
+ */
+ nr_mirrors = devices_argc/2;
+
+ if ((nr_mirrors < 2) || (nr_mirrors > KCOPYD_MAX_REGIONS + 1)) {
+ ti->error = "Invalid number of mirrors";
+ dm_destroy_dirty_log(dl);
+ return -EINVAL;
+ }
+
+ ms = alloc_context(nr_mirrors, dl->type->get_region_size(dl), ti, dl);
+ if (!ms) {
+ dm_destroy_dirty_log(dl);
+ return -ENOMEM;
+ }
+
+ /* Get the mirror parameter sets */
+ for (m = 0; m < nr_mirrors; m++) {
+ r = get_mirror(ms, ti, m, devices_argv + (2 * m));
+ if (r) {
+ free_context(ms, ti, m);
+ return r;
+ }
+ }
+
+ ti->private = ms;
+ ti->split_io = ms->rh.region_size;
+
+ r = kcopyd_client_create(DM_IO_PAGES, &ms->kcopyd_client);
+ if (r) {
+ free_context(ms, ti, ms->nr_mirrors);
+ return r;
+ }
+
+ add_mirror_set(ms);
+ return 0;
+}
+
+/*
+ * mirror_ctr
+ * @ti
+ * @argc
+ * @argv
+ *
+ * This function constructs the mirror map given the mapping
+ * table found in argv. If the first argument is 'format2',
+ * 'mirror_format2_ctr is used; otherwise, the following is
+ * the valid argument set:
+ * <log_type> <#log_params> <log_params> \
+ * <#mirrors> <device1> <offset1> ... <deviceN> <offsetN>
+ *
+ * See the log_type implementations for details on their
+ * argument list.
+ *
+ * Returns: 0 on success, -EXXX on failure
*/
#define DM_IO_PAGES 64
static int mirror_ctr(struct dm_target *ti, unsigned int argc, char **argv)
@@ -1032,6 +1156,9 @@ static int mirror_ctr(struct dm_target *
struct mirror_set *ms;
struct dirty_log *dl;
+ if (!strcmp("format2", argv[0]))
+ return mirror_format2_ctr(ti, argc, argv);
+
dl = create_dirty_log(ti, argc, argv, &args_used);
if (!dl)
return -EINVAL;
[-- Attachment #3: Type: text/plain, Size: 1 bytes --]
[-- Attachment #4: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Change to the mirror table map
2006-10-20 19:36 ` Jonathan E Brassow
@ 2006-10-23 16:01 ` Stefan Bader
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Bader @ 2006-10-23 16:01 UTC (permalink / raw)
To: device-mapper development; +Cc: agk
dm-devel-bounces@redhat.com wrote on 20.10.2006 21:36:54:
>
> > "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.
>
Well, I guess this is just a minor issue. It is a simple approach to
to it that way. My point was just that "core" and "disk" (if these
were not used as section label) would indicate the old format. The
new parser would get used for all formats, simply skipping sections
that it doesn't understand. But "formatX" is ok.
> 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.)
>
> > devices 2 2 253:3 rr_min=1000 3 253:4 mstate=m rr_min=1000
> > 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.)
>
Yes, it makes it simpler to break up parsing. The other approach, while
it might be simpler to read would mean every place interrested in device
options would have to parse them again. And it would require all parsers
to change. And that is a too big change.
What makes me a bit uneasy about having devices in several places is that
this allows configuration mistakes that could be fatal. For example give
different devices to device section and round-robin section...
>
> 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.)
>
Well, besides my "bad feelings" this looks good to me. But at the moment
I can't think of a better solution that wouldn't require changes to other
parsers. :(
Regards,
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Change to the mirror table map
2006-10-19 20:56 [RFC] Change to the mirror table map Jonathan Brassow
2006-10-20 16:18 ` Stefan Bader
@ 2006-10-24 9:30 ` Heinz Mauelshagen
2006-10-26 16:35 ` Jonathan E Brassow
1 sibling, 1 reply; 6+ messages in thread
From: Heinz Mauelshagen @ 2006-10-24 9:30 UTC (permalink / raw)
To: device-mapper development; +Cc: agk
On Thu, Oct 19, 2006 at 03:56:18PM -0500, Jonathan Brassow wrote:
> The impetus for this was the desire to add read balancing to mirroring
> and have it share code with multipath.
Makes sense.
>
> brassow
>
> ORIGINAL:
> <start> <length> mirror \
> <log_type> <#log_params> <log_params> \
> <#mirrors> <device1> <offset1> ... <deviceN> <offsetN>
>
> Example:
> 0 1024000 mirror \
> disk 3 253:2 1024 block_on_error \
> 2 253:3 0 253:4 0
>
>
> MODIFIED:
> [-------- logging section ---------] [- mirror devices section -] [--------------- read balancing section -------------]
> 0 71014400 mirror format2 log 4 disk 253:2 1024 block_on_error devices 2 1 253:3 S 253:4 A read-balance 8 round-robin 0 2 1 253:3 1000 253:4 1000
> ^ ^ ^ ^ ^ ^ ^ <^^^^^^^^^^^^^^^^^^^^^^^> ^ ^ ^ ^ ^ ^ ^ <^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^>
> | | | | | | | | | | | | | | | |
> | | | | | | | | | | | | | | | same as multipath
> | | | | | | | | | | | | | | # read-balance args
> | | | | | | | | | | | | | read-balance section indicator
> | | | | | | | | | | | | Device status (slave/active/dead)
> | | | | | | | | | | | Device
> | | | | | | | | | | # of device args
> | | | | | | | | | # of devices
> | | | | | | | | device section indicator
> | | | | | | | log type specific args
> | | | | | | log type
> | | | | | # of log args
> | | | | log section indicator
> | | | target table format
> | | target name
> | target length in 512-byte blocks
> starting offset of target
>
> 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.
Makes sense for backward compatibility and makes parsing easy.
>
> 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.
>
> The logging section starts with the keyword 'log' and is followed by the number
> of log arguments. No changes to the dm_create_dirty_log function should be
> necessary.
Again an advantage for parsing.
>
> 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
> 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.
Keep it: we need it to easier support things like lvm pvmove and for metadata
at the beginning of the mapped devices.
> Multipath doesn't have that, and neither does mirroring
> when specifying the log device. For the above example, I did add an
> additional per device parameter to specify the status of a device. I was
> imagining a scenario where IBM's slave context might be specified, or
> a new mirror device is added to the set. With the indicator, you could
> tell which devices were in-sync, and which ones needed recovery.
> (Perhaps that is better left to an additional section, since it is not
> yet known how this functionality would be added.)
Yes, we can have a device-properties or domething section after the devices
one once that hash been sorted.
>
> 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.
>
> 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.
I agree. Much more flexible for enhancements and for the ease of parsing.
Heinz
>
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Heinz Mauelshagen Red Hat GmbH
Consulting Development Engineer Am Sonnenhang 11
Storage Development 56242 Marienrachdorf
Germany
Mauelshagen@RedHat.com PHONE +49 171 7803392
FAX +49 2626 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] Change to the mirror table map
2006-10-24 9:30 ` Heinz Mauelshagen
@ 2006-10-26 16:35 ` Jonathan E Brassow
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan E Brassow @ 2006-10-26 16:35 UTC (permalink / raw)
To: mauelshagen, device-mapper development; +Cc: agk
I've been giving this more thought, and trying to code up a proof of
concept. It hasn't been going as well as I'd like, and I could use
some more input.
Coding up the constructor is pretty easy for virtually any scenario we
might imagine. The evolving ideas below make sense in that we can
simply pass on the sections to functions and don't have to rework the
interfaces for the log constructor or path selection constructor.
However, things start to break down when we look at the "status"
function. When we print the "STATUSTYPE_TABLE", it should produce
information in the same form as the mapping table. Currently, the log
and path selection modules print their own portions of that information
- into a string that is shared with the parent (mirror in this case).
That hurts our ability to specify the number of arguments for a
particular section, and destroys the ability to interleave sections
(e.g. 'devices 7 2 253:3 0 rr_min=1000 253:4 0 rr_min=1000')
We could remember the mapping table string and simply print that out -
bypassing the submodules. Or, we could alter the submodules in some
way. (Perhaps adding a function like "status_args" that would tell us
ahead of time how many arguments there are - allowing us to write "log
<#log_args>" before calling log_status.)
While we are thinking about this, I'd like to also include the
'features' idea into the mirror mapping table. For example: 'features
<#feature args> <features>', where <features> might be 'handle_errors'
and 'allow_io_delay 120'.
Right now, I'm leaning toward adding the additional function
"status_args" and adding the 'features' section as a new section.
Thoughts/advice are welcome,
brassow
On Oct 24, 2006, at 4:30 AM, Heinz Mauelshagen wrote:
> On Thu, Oct 19, 2006 at 03:56:18PM -0500, Jonathan Brassow wrote:
>> The impetus for this was the desire to add read balancing to mirroring
>> and have it share code with multipath.
>
> Makes sense.
>
>>
>> brassow
>>
>> ORIGINAL:
>> <start> <length> mirror \
>> <log_type> <#log_params> <log_params> \
>> <#mirrors> <device1> <offset1> ... <deviceN> <offsetN>
>>
>> Example:
>> 0 1024000 mirror \
>> disk 3 253:2 1024 block_on_error \
>> 2 253:3 0 253:4 0
>>
>>
>> MODIFIED:
>> [-------- logging section ---------] [-
>> mirror devices section -] [--------------- read balancing section
>> -------------]
>> 0 71014400 mirror format2 log 4 disk 253:2 1024 block_on_error
>> devices 2 1 253:3 S 253:4 A read-balance 8 round-robin 0 2 1 253:3
>> 1000 253:4 1000
>> ^ ^ ^ ^ ^ ^ ^ <^^^^^^^^^^^^^^^^^^^^^^^> ^
>> ^ ^ ^ ^ ^ ^
>> <^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^>
>> | | | | | | | | |
>> | | | | | | |
>> | | | | | | | | |
>> | | | | | | same as multipath
>> | | | | | | | | |
>> | | | | | # read-balance args
>> | | | | | | | | |
>> | | | | read-balance section indicator
>> | | | | | | | | |
>> | | | Device status (slave/active/dead)
>> | | | | | | | | |
>> | | Device
>> | | | | | | | | |
>> | # of device args
>> | | | | | | | | |
>> # of devices
>> | | | | | | | |
>> device section indicator
>> | | | | | | | log type specific args
>> | | | | | | log type
>> | | | | | # of log args
>> | | | | log section indicator
>> | | | target table format
>> | | target name
>> | target length in 512-byte blocks
>> starting offset of target
>>
>> 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.
>
> Makes sense for backward compatibility and makes parsing easy.
>
>>
>> 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.
>>
>> The logging section starts with the keyword 'log' and is followed by
>> the number
>> of log arguments. No changes to the dm_create_dirty_log function
>> should be
>> necessary.
>
> Again an advantage for parsing.
>
>>
>> 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
>> 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.
>
> Keep it: we need it to easier support things like lvm pvmove and for
> metadata
> at the beginning of the mapped devices.
>
>> Multipath doesn't have that, and neither does mirroring
>> when specifying the log device. For the above example, I did add an
>> additional per device parameter to specify the status of a device. I
>> was
>> imagining a scenario where IBM's slave context might be specified, or
>> a new mirror device is added to the set. With the indicator, you
>> could
>> tell which devices were in-sync, and which ones needed recovery.
>> (Perhaps that is better left to an additional section, since it is not
>> yet known how this functionality would be added.)
>
> Yes, we can have a device-properties or domething section after the
> devices
> one once that hash been sorted.
>
>>
>> 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.
>>
>> 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.
>
> I agree. Much more flexible for enhancements and for the ease of
> parsing.
>
> Heinz
>
>>
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
> =-=-=-=-
>
> Heinz Mauelshagen Red Hat GmbH
> Consulting Development Engineer Am Sonnenhang 11
> Storage Development 56242 Marienrachdorf
> Germany
> Mauelshagen@RedHat.com PHONE +49 171
> 7803392
> FAX +49 2626 924446
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
> =-=-=-=-
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-10-26 16:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-10-19 20:56 [RFC] Change to the mirror table map Jonathan Brassow
2006-10-20 16:18 ` Stefan Bader
2006-10-20 19:36 ` Jonathan E Brassow
2006-10-23 16:01 ` Stefan Bader
2006-10-24 9:30 ` Heinz Mauelshagen
2006-10-26 16:35 ` Jonathan E Brassow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.