| Message ID | CABr-Gndh4LLgd=T00GdtwNVrFgL7Te91BwjZ0Dh4uYyDi3c9bA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On 07/31/2016 09:26 PM, Christophe Varoqui wrote: > Ben, Hannes, > > Can you review this patch, adding a new 'serial' keyword to the > weightedpath prioritizer. > > I compile-tested it only, as I have no testing environment at hand at > the moment. > > I commited it in a separate 'weightedpath-serial' branch for now. > > http://git.opensvc.com/?p=multipath-tools/.git;a=commitdiff;h=4dd16d99281104fc3504ad73626894a5c3702fb3 > > Thanks, > Christophe Varoqui > OpenSVC > Well. In general, sure, fine, I don't have any issues with that. If the customer wants to diddle with his array that way... The more general problem I'm seeing is that our current two-layered priority setup (path groups with distinct priorities and paths within them) might not be leading to issues with larger and more complex scenarios. ATM we already have the problem that clustered scenarios like this: Storage node 1(active): Path 1 (optimal): LUN 1, LUN2 Path 2 (non-optimal): LUN 1, LUN2 Storage node 2(passive): Path 1(optimal): LUN 1, LUN2 Path 2(non-optimal): LUN 1, LUN2 can not be represented properly with multipath tools. We are forced to either a) set 'storage node 2' to 'failed', which would kill any cluster instance accessing only 'storage node 2' or b) map all priorities from 'storage node 2' to '0', thereby losing all priority information Things become even more convoluted if both storage nodes are in fact accessible, or if someone would be using different transports. Cheers, Hannes
On Mon, Aug 1, 2016 at 9:49 AM, Hannes Reinecke <hare@suse.de> wrote: > On 07/31/2016 09:26 PM, Christophe Varoqui wrote: > >> Ben, Hannes, >> >> Can you review this patch, adding a new 'serial' keyword to the >> weightedpath prioritizer. >> >> I compile-tested it only, as I have no testing environment at hand at >> the moment. >> >> I commited it in a separate 'weightedpath-serial' branch for now. >> >> >> http://git.opensvc.com/?p=multipath-tools/.git;a=commitdiff;h=4dd16d99281104fc3504ad73626894a5c3702fb3 >> >> Thanks, >> Christophe Varoqui >> OpenSVC >> >> Well. > In general, sure, fine, I don't have any issues with that. > If the customer wants to diddle with his array that way... > > The more general problem I'm seeing is that our current two-layered > priority setup (path groups with distinct priorities and paths within them) > might not be leading to issues with larger and more complex scenarios. > > ATM we already have the problem that clustered scenarios like this: > > Storage node 1(active): > Path 1 (optimal): > LUN 1, LUN2 > Path 2 (non-optimal): > LUN 1, LUN2 > > Storage node 2(passive): > Path 1(optimal): > LUN 1, LUN2 > Path 2(non-optimal): > LUN 1, LUN2 > > can not be represented properly with multipath tools. > We are forced to either > a) set 'storage node 2' to 'failed', which would kill > any cluster instance accessing only 'storage node 2' > or > b) map all priorities from 'storage node 2' to '0', > thereby losing all priority information > > Things become even more convoluted if both storage nodes are in fact > accessible, or if someone would be using different transports. > > Would something like "prio alua+weightedpath" produce correct priorities for the path grouping ? where priorities reported by alua would be added to those reported by weighted path. That syntax extension would reduce the need to develop more complex prioritizers. The prio_args to prio mapping, wouldn't be nice though. Concerning transports, we can also extend the weightedpath prioritizer, and if the multi-prioritizer setup is implemented we could even weight on serial+transport with a "prio weighedpath+weightedpath" setup. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 08/01/2016 10:42 AM, Christophe Varoqui wrote: > > > On Mon, Aug 1, 2016 at 9:49 AM, Hannes Reinecke <hare@suse.de > <mailto:hare@suse.de>> wrote: > > On 07/31/2016 09:26 PM, Christophe Varoqui wrote: > > Ben, Hannes, > > Can you review this patch, adding a new 'serial' keyword to the > weightedpath prioritizer. > > I compile-tested it only, as I have no testing environment at > hand at > the moment. > > I commited it in a separate 'weightedpath-serial' branch for now. > > http://git.opensvc.com/?p=multipath-tools/.git;a=commitdiff;h=4dd16d99281104fc3504ad73626894a5c3702fb3 > > Thanks, > Christophe Varoqui > OpenSVC > > Well. > In general, sure, fine, I don't have any issues with that. > If the customer wants to diddle with his array that way... > > The more general problem I'm seeing is that our current two-layered > priority setup (path groups with distinct priorities and paths > within them) might not be leading to issues with larger and more > complex scenarios. > > ATM we already have the problem that clustered scenarios like this: > > Storage node 1(active): > Path 1 (optimal): > LUN 1, LUN2 > Path 2 (non-optimal): > LUN 1, LUN2 > > Storage node 2(passive): > Path 1(optimal): > LUN 1, LUN2 > Path 2(non-optimal): > LUN 1, LUN2 > > can not be represented properly with multipath tools. > We are forced to either > a) set 'storage node 2' to 'failed', which would kill > any cluster instance accessing only 'storage node 2' > or > b) map all priorities from 'storage node 2' to '0', > thereby losing all priority information > > Things become even more convoluted if both storage nodes are in fact > accessible, or if someone would be using different transports. > > Would something like "prio alua+weightedpath" produce correct priorities > for the path grouping ? where priorities reported by alua would be added > to those reported by weighted path. That syntax extension would reduce > the need to develop more complex prioritizers. > Hmm. Allowing stacked prioritizers is a nice idea. But then we need to impose some preference here; if we do not set any restrictions on the value of the prioritizers we end up with a jumble of (essentially unreadable) priorities. EG if your weightedpath returns values of '5' or '0' they'll be readily obscured by alua information, which uses '5' for the non-optimized path. So if we were to got that route we need to restrict the values of the prioritizers to eg 256, and shift the stacked prioritizer values ontop of each other. EG with a stacked 'prio_alua+weightedpath' we should end up with a priority of 0xAAWW. With that we can allow up to 4 levels of stacking (or 8 if we extend that to 64 bits), and still keep source-level compability with the original code. We could even reduce the permissive values for the prioritzers even more; 16 is enough even for ALUA, and that would leave us with enough room of 1024 possible stacking levels :-) But in general I like the idea. Cheers, Hannes
Or we could honor arythmetic expressions like "5*alua+weightedpath", giving users more control about preferences (and more opportunities to step on their toes, sure). Another idea, less invasive if possible, but less versatile : - Merge the alua prioritizer into the weightedpath prioritizer, given the optimized/non-optimized and other ALUA states are available in the path struct and have their snprint_path_* function and %wildcard. - Then weightedpath prio_args can be extended to support additive priorities, like "alua <optimized state>:<... state> 10 serial foo 20" On Mon, Aug 1, 2016 at 1:40 PM, Hannes Reinecke <hare@suse.de> wrote: > On 08/01/2016 10:42 AM, Christophe Varoqui wrote: > >> >> >> On Mon, Aug 1, 2016 at 9:49 AM, Hannes Reinecke <hare@suse.de >> <mailto:hare@suse.de>> wrote: >> >> On 07/31/2016 09:26 PM, Christophe Varoqui wrote: >> >> Ben, Hannes, >> >> Can you review this patch, adding a new 'serial' keyword to the >> weightedpath prioritizer. >> >> I compile-tested it only, as I have no testing environment at >> hand at >> the moment. >> >> I commited it in a separate 'weightedpath-serial' branch for now. >> >> >> http://git.opensvc.com/?p=multipath-tools/.git;a=commitdiff;h=4dd16d99281104fc3504ad73626894a5c3702fb3 >> >> Thanks, >> Christophe Varoqui >> OpenSVC >> >> Well. >> In general, sure, fine, I don't have any issues with that. >> If the customer wants to diddle with his array that way... >> >> The more general problem I'm seeing is that our current two-layered >> priority setup (path groups with distinct priorities and paths >> within them) might not be leading to issues with larger and more >> complex scenarios. >> >> ATM we already have the problem that clustered scenarios like this: >> >> Storage node 1(active): >> Path 1 (optimal): >> LUN 1, LUN2 >> Path 2 (non-optimal): >> LUN 1, LUN2 >> >> Storage node 2(passive): >> Path 1(optimal): >> LUN 1, LUN2 >> Path 2(non-optimal): >> LUN 1, LUN2 >> >> can not be represented properly with multipath tools. >> We are forced to either >> a) set 'storage node 2' to 'failed', which would kill >> any cluster instance accessing only 'storage node 2' >> or >> b) map all priorities from 'storage node 2' to '0', >> thereby losing all priority information >> >> Things become even more convoluted if both storage nodes are in fact >> accessible, or if someone would be using different transports. >> >> Would something like "prio alua+weightedpath" produce correct priorities >> for the path grouping ? where priorities reported by alua would be added >> to those reported by weighted path. That syntax extension would reduce >> the need to develop more complex prioritizers. >> >> Hmm. > Allowing stacked prioritizers is a nice idea. > But then we need to impose some preference here; if we do not set any > restrictions on the value of the prioritizers we end up with a jumble of > (essentially unreadable) priorities. > EG if your weightedpath returns values of '5' or '0' they'll be readily > obscured by alua information, which uses '5' for the non-optimized path. > > So if we were to got that route we need to restrict the values of the > prioritizers to eg 256, and shift the stacked prioritizer values ontop of > each other. > EG with a stacked 'prio_alua+weightedpath' we should end up with a > priority of 0xAAWW. > With that we can allow up to 4 levels of stacking (or 8 if we extend that > to 64 bits), and still keep source-level compability with the original code. > We could even reduce the permissive values for the prioritzers even more; > 16 is enough even for ALUA, and that would leave us with enough room of > 1024 possible stacking levels :-) > > But in general I like the idea. > > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > hare@suse.de +49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton > HRB 21284 (AG Nürnberg) > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 08/01/2016 02:25 PM, Christophe Varoqui wrote: > Or we could honor arythmetic expressions like "5*alua+weightedpath", > giving users more control about preferences (and more opportunities to > step on their toes, sure). > Ugh. No. You still would need to know which values the 'alua' (or any prioritizer) will be returning. ATM there is no guarantee nor even advise which values a prioritizer should return. Which in itself is a good idea IMHO. > Another idea, less invasive if possible, but less versatile : > > - Merge the alua prioritizer into the weightedpath prioritizer, given > the optimized/non-optimized and other ALUA states are available in the > path struct and have their snprint_path_* function and %wildcard. > Hmm. I still prefer the 'stacked prioritizer' approach, what with it being far more flexible. > - Then weightedpath prio_args can be extended to support additive > priorities, like "alua <optimized state>:<... state> 10 serial foo 20" > Which again would require us to define the possible return values from the alua (or any other handler). Cheers, Hannes
On 07/31/2016 12:26 PM, Christophe Varoqui wrote: > Can you review this patch, adding a new 'serial' keyword to the > weightedpath prioritizer. Is the following part of the patch intended? } else if (!strcmp(regex, WWN)) { - if (build_wwn_path(pp, path, FILE_NAME_SIZE) != 0) { + if (build_serial_path(pp, path, FILE_NAME_SIZE) != 0) { Additionally, have you considered to apply the mulitpathd -> multipathd spelling fix in this patch as a separate patch? Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Oops no ! Thanks for noting. An incremental fix is now merged. You are right about the spelling fix that accidentaly was wrapped into this patch, but the patch has already been pushed online as-is so I don't see how I can improve that now. Regards, Christophe On Mon, Aug 1, 2016 at 6:02 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote: > On 07/31/2016 12:26 PM, Christophe Varoqui wrote: > >> Can you review this patch, adding a new 'serial' keyword to the >> weightedpath prioritizer. >> > > Is the following part of the patch intended? > > } else if (!strcmp(regex, WWN)) { > - if (build_wwn_path(pp, path, FILE_NAME_SIZE) != 0) { > + if (build_serial_path(pp, path, FILE_NAME_SIZE) != 0) { > > Additionally, have you considered to apply the mulitpathd -> multipathd > spelling fix in this patch as a separate patch? > > Thanks, > > Bart. > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/print.c b/libmultipath/print.c index 7c556b3..94d6384 100644 --- a/libmultipath/print.c +++ b/libmultipath/print.c @@ -503,7 +503,7 @@ snprint_path_size (char * buff, size_t len, struct path * pp) return snprint_size(buff, len, pp->size); } -static int +int snprint_path_serial (char * buff, size_t len, struct path * pp) { return snprint_str(buff, len, pp->serial); diff --git a/libmultipath/print.h b/libmultipath/print.h index 9306e50..6839fc7 100644 --- a/libmultipath/print.h +++ b/libmultipath/print.h @@ -112,6 +112,7 @@ int snprint_devices (struct config *, char *, int, struct vectors *); int snprint_hwtable (struct config *, char *, int, vector); int snprint_mptable (struct config *, char *, int, vector); int snprint_overrides (struct config *, char *, int, struct hwentry *); +int snprint_path_serial (char *, size_t, struct path *); int snprint_host_wwnn (char *, size_t, struct path *); int snprint_host_wwpn (char *, size_t, struct path *); int snprint_tgt_wwnn (char *, size_t, struct path *); diff --git a/libmultipath/prioritizers/weightedpath.c b/libmultipath/prioritizers/weightedpath.c index e8168fe..e53ab48 100644 --- a/libmultipath/prioritizers/weightedpath.c +++ b/libmultipath/prioritizers/weightedpath.c @@ -53,6 +53,16 @@ do { \ } while(0) static int +build_serial_path(struct path *pp, char *str, int len) +{ + char *p = str; + + p += snprint_path_serial(p, str + len - p, pp); + CHECK_LEN; + return 0; +} + +static int build_wwn_path(struct path *pp, char *str, int len) { char *p = str; @@ -103,8 +113,13 @@ int prio_path_weight(struct path *pp, char *prio_args) pp->sg_id.channel, pp->sg_id.scsi_id, pp->sg_id.lun); } else if (!strcmp(regex, DEV_NAME)) { strcpy(path, pp->dev); + } else if (!strcmp(regex, SERIAL)) { + if (build_serial_path(pp, path, FILE_NAME_SIZE) != 0) { + FREE(arg); + return priority; + } } else if (!strcmp(regex, WWN)) { - if (build_wwn_path(pp, path, FILE_NAME_SIZE) != 0) { + if (build_serial_path(pp, path, FILE_NAME_SIZE) != 0) { FREE(arg); return priority; } diff --git a/libmultipath/prioritizers/weightedpath.h b/libmultipath/prioritizers/weightedpath.h index 93d8c43..a1b268f 100644 --- a/libmultipath/prioritizers/weightedpath.h +++ b/libmultipath/prioritizers/weightedpath.h @@ -4,6 +4,7 @@ #define PRIO_WEIGHTED_PATH "weightedpath" #define HBTL "hbtl" #define DEV_NAME "devname" +#define SERIAL "serial" #define WWN "wwn" #define DEFAULT_PRIORITY 0 diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index ecaef1f..e73e917 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -243,17 +243,25 @@ prioritizers: .TP 12 .I weighted Needs a value of the form -.I "<hbtl|devname|wwn> <regex1> <prio1> <regex2> <prio2> ..." +.I "<hbtl|devname|serial|wwn> <regex1> <prio1> <regex2> <prio2> ..." +.RS +.TP 8 .I hbtl regex can be of SCSI H:B:T:L format Ex: 1:0:.:. , *:0:0:. +.TP .I devname regex can be of device name format Ex: sda , sd.e +.TP +.I serial +regex can be of serial number format Ex: .*J1FR.*324 The serial can be looked up through sysfs or by running multipathd show paths format "%z" Ex: 0395J1FR904324 +.TP .I wwn regex can be of the form .I "host_wwnn:host_wwpn:target_wwnn:target_wwpn" these values can be looked up through sysfs or by running -.I mulitpathd show paths format "%N:%R:%n:%r" Ex: 0x200100e08ba0aea0:0x210100e08ba0aea0:.*:.* , .*:.*:iqn.2009-10.com.redhat.msp.lab.ask-06:.* -.TP +.I multipathd show paths format "%N:%R:%n:%r" Ex: 0x200100e08ba0aea0:0x210100e08ba0aea0:.*:.* ,
Ben, Hannes, Can you review this patch, adding a new 'serial' keyword to the weightedpath prioritizer. I compile-tested it only, as I have no testing environment at hand at the moment. I commited it in a separate 'weightedpath-serial' branch for now. http://git.opensvc.com/?p=multipath-tools/.git;a=commitdiff;h=4dd16d99281104fc3504ad73626894a5c3702fb3 Thanks, Christophe Varoqui OpenSVC --- commit 4dd16d99281104fc3504ad73626894a5c3702fb3 Author: Christophe Varoqui <christophe.varoqui@opensvc.com> Date: Sun Jul 31 21:08:14 2016 +0200 multipath: add serial keyword to the weightedpath prioritizer Allow the weightedpath prioritizer to set priority to paths to specific serial numbers, expressed as regular expressions. Example: prio weightedpath prio_args "serial .*101 30 .*102 10" This feature is most synthetic for cross-site + all-paths-active topologies, where servers on a site want to prefer paths to the local storage heads. .*:.*:iqn.2009-10.com.redhat.msp.lab.ask-06:.* +.RE +.TP 12 .I alua If .I exclusive_pref_bit -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel