diff mbox

[RFC] add serial keyword to the weightedpath prioritizer

Message ID CABr-Gndh4LLgd=T00GdtwNVrFgL7Te91BwjZ0Dh4uYyDi3c9bA@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show

Commit Message

Christophe Varoqui July 31, 2016, 7:26 p.m. UTC
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

Comments

Hannes Reinecke Aug. 1, 2016, 7:49 a.m. UTC | #1
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
Christophe Varoqui Aug. 1, 2016, 8:42 a.m. UTC | #2
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
Hannes Reinecke Aug. 1, 2016, 11:40 a.m. UTC | #3
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
Christophe Varoqui Aug. 1, 2016, 12:25 p.m. UTC | #4
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
Hannes Reinecke Aug. 1, 2016, 1:18 p.m. UTC | #5
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
Bart Van Assche Aug. 1, 2016, 4:02 p.m. UTC | #6
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
Christophe Varoqui Aug. 1, 2016, 4:11 p.m. UTC | #7
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 mbox

Patch

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:.*:.* ,