Re: [PATCH 2/2] multipath-tools/libmultipath: Add args min_avg_latency for path_latency.

[Martin Wilck <mwilck@suse.com>]

Dear Yang,

On Thu, 2017-07-20 at 11:36 +0800, Yang Feng wrote:
>   Add args min_avg_latency of logarithmic scale, for
> prioritizers/path_latency.c.
> Min average latency is not constant 1us, and is set by user.
> Certainly, max average
> latency value is still 100s. It make support better for more scenes,
> because it can
> deal better with the normal standard deviation of path latency. For
> example, when the
> standard deviation value is 200us and the average latency of the
> normal paths is 1ms,
> args base_num can be set to 5 and args min_avg_latency can be set to
> 2ms, so the paths
> will be grouped in priority groups with path latency <=2ms, (2ms,
> 10ms], (10ms, 50ms],
> etc.

Nack. You need this because you are using a wrong calculation for
standard deviation. If your scale is logarithmic, you need to calculate
the standard deviation on a log scale, too. The scientific term is
"geometric standard deviation".

https://en.wikipedia.org/wiki/Geometric_standard_deviation

Suppose you really have 3 classes of devices with us, ms, and seconds
average latency, respectively. The latency of the devices in the
"seconds" class will also vary on the order of seconds (e.g. 3.5-5s).
That's obviously impossible for the us and ms class devices. Rather,
it's reasonable to assume that the us devices will have us (or sub-us)
standard deviation and the ms devices have ms (or sub-ms) standard
deviation, or in other words, the uncertainty is roughly in the same
order of magnitude as the average.

This is exactly how the geometric standard deviation behaves.

I think I actually remarked this on your previous patch, but the patch
has been  applied without that having been fixed. It would be good if
you could fix it now.

> 
> Signed-off-by: Yang Feng <philip.yang@huawei.com>
> Reviewed-by: Martin Wilck <mwilck@suse.com>

Please don't add my Reviewed-by: tag for patches I haven't reviewed, or
have negatively reviewed. Reviewed-by: is supposed to indicate
approval.

Wrt your prio_args parser, could you perhaps support a syntax that is
similar to other prioritizers, e.g. "base_num=5 io_num=10"?

Regards
Martin


> Reviewed-by: Xose Vazquez Perez <xose.vazquez@gmail.com>
> ---
>  libmultipath/prioritizers/path_latency.c | 61
> ++++++++++++++++++++++----------
>  multipath/multipath.conf.5               | 14 +++++---
>  2 files changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/libmultipath/prioritizers/path_latency.c
> b/libmultipath/prioritizers/path_latency.c
> index 34b734b..a71faff 100644
> --- a/libmultipath/prioritizers/path_latency.c
> +++ b/libmultipath/prioritizers/path_latency.c
> @@ -66,7 +66,7 @@ static int do_readsector0(int fd, unsigned int
> timeout)
>  	return ret;
>  }
>  
> -int check_args_valid(int io_num, int base_num)
> +int check_args_valid(int io_num, int base_num, int min_avg_latency)
>  {
>      if ((io_num < MIN_IO_NUM) || (io_num > MAX_IO_NUM))
>      {
> @@ -80,24 +80,33 @@ int check_args_valid(int io_num, int base_num)
>          return 0;
>      }
>  
> +    if ((min_avg_latency < MIN_AVG_LATENCY) || (min_avg_latency >
> MAX_AVG_LATENCY))
> +    {
> +        pp_pl_log(0, "args min_avg_latency is outside the valid
> range");
> +        return 0;
> +    }
> +
>      return 1;
>  }
>  
> -/* In multipath.conf, args form: io_num|base_num. For example,
> -*  args is "20|10", this function can get io_num value 20, and
> -   base_num value 10.
> +/* In multipath.conf, args form: io_num|base_num|min_avg_latency.
> +*  For example, args is "20|10|1", this function can get io_num
> +*  value 20, base_num value 10, min_avg_latency value 1 (us).
>  */
> -static int get_ionum_and_basenum(char *args,
> -                                 int *ionum,
> -                                 int *basenum)
> +static int get_prio_args(char *args,
> +                    int *ionum,
> +                    int *basenum,
> +                    int *minavglatency)
>  {
>      char source[MAX_CHAR_SIZE];
>      char vertica = '|';
> -    char *endstrbefore = NULL;
> -    char *endstrafter = NULL;
> +    char *endstr1 = NULL;
> +    char *endstr2 = NULL;
> +    char *endstr3 = NULL;
>      unsigned int size = strlen(args);
>  
> -    if ((args == NULL) || (ionum == NULL) || (basenum == NULL))
> +    if ((args == NULL) || (ionum == NULL)
> +        || (basenum == NULL) || (minavglatency == NULL))
>      {
>          pp_pl_log(0, "args string is NULL");
>          return 0;
> @@ -117,21 +126,34 @@ static int get_ionum_and_basenum(char *args,
>          return 0;
>      }
>  
> -    *ionum = (int)strtoul(source, &endstrbefore, 10);
> -    if (endstrbefore[0] != vertica)
> +    *ionum = (int)strtoul(source, &endstr1, 10);
> +    if (endstr1[0] != vertica)
> +    {
> +        pp_pl_log(0, "invalid prio_args format: %s", source);
> +        return 0;
> +    }
> +
> +    if (!isdigit(endstr1[1]))
> +    {
> +        pp_pl_log(0, "invalid prio_args format: %s", source);
> +        return 0;
> +    }
> +
> +    *basenum = (long long)strtol(&endstr1[1], &endstr2, 10);
> +    if (endstr2[0] != vertica)
>      {
>          pp_pl_log(0, "invalid prio_args format: %s", source);
>          return 0;
>      }
>  
> -    if (!isdigit(endstrbefore[1]))
> +    if (!isdigit(endstr2[1]))
>      {
>          pp_pl_log(0, "invalid prio_args format: %s", source);
>          return 0;
>      }
>  
> -    *basenum = (long long)strtol(&endstrbefore[1], &endstrafter,
> 10);
> -    if (check_args_valid(*ionum, *basenum) == 0)
> +    *minavglatency = (long long)strtol(&endstr2[1], &endstr3, 10);
> +    if (check_args_valid(*ionum, *basenum, *minavglatency) == 0)
>      {
>          return 0;
>      }
> @@ -194,6 +216,7 @@ int getprio (struct path *pp, char *args,
> unsigned int timeout)
>      int index = 0;
>      int io_num;
>      int base_num;
> +    int min_avg_latency;
>      long long avglatency;
>      long long latency_interval;
>      long long standard_deviation;
> @@ -204,7 +227,7 @@ int getprio (struct path *pp, char *args,
> unsigned int timeout)
>      if (pp->fd < 0)
>          return -1;
>  
> -    if (get_ionum_and_basenum(args, &io_num, &base_num) == 0)
> +    if (get_prio_args(args, &io_num, &base_num, &min_avg_latency) ==
> 0)
>      {
>          pp_pl_log(0, "%s: get path_latency args fail", pp->dev);
>          return DEFAULT_PRIORITY;
> @@ -245,13 +268,13 @@ int getprio (struct path *pp, char *args,
> unsigned int timeout)
>      set can change latency_interval value corresponding to
> avglatency and is not constant.
>      Warn the user if latency_interval is smaller than (2 *
> standard_deviation), or equal */
>      standard_deviation = calc_standard_deviation(path_latency,
> index, avglatency);
> -    latency_interval = calc_latency_interval(avglatency,
> MAX_AVG_LATENCY, MIN_AVG_LATENCY, base_num);
> -    if ((latency_interval!= 0)
> +    latency_interval = calc_latency_interval(avglatency,
> MAX_AVG_LATENCY, min_avg_latency, base_num);
> +    if ((latency_interval != 0)
>          && (latency_interval <= (2 * standard_deviation)))
>          pp_pl_log(3, "%s: latency interval (%lld) according to
> average latency (%lld us) is smaller than "
>              "2 * standard deviation (%lld us), or equal, args
> base_num (%d) needs to be set bigger value",
>              pp->dev, latency_interval, avglatency,
> standard_deviation, base_num);
>  
> -    rc = calcPrio(avglatency, MAX_AVG_LATENCY, MIN_AVG_LATENCY,
> base_num);
> +    rc = calcPrio(avglatency, MAX_AVG_LATENCY, min_avg_latency,
> base_num);
>      return rc;
>  }
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 0049cba..e68e681 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -341,7 +341,7 @@ these values can be looked up through sysfs or by
> running \fImultipathd show pat
>  .TP 12
>  .I path_latency
>  Needs a value of the form
> -\fI"<io_num>|<base_num>"\fR
> +\fI"<io_num>|<base_num>|<min_avg_latency>"\fR
>  .RS
>  .TP 8
>  .I io_num
> @@ -350,9 +350,15 @@ Valid Values: Integer, [2, 200].
>  .TP
>  .I base_num
>  The base number value of logarithmic scale, used to partition
> different priority ranks. Valid Values: Integer,
> -[2, 10]. And Max average latency value is 100s, min average latency
> value is 1us.
> -For example: If base_num=10, the paths will be grouped in priority
> groups with path latency <=1us, (1us, 10us],
> -(10us, 100us], (100us, 1ms], (1ms, 10ms], (10ms, 100ms], (100ms,
> 1s], (1s, 10s], (10s, 100s], >100s.
> +[2, 10]. And Max average latency value is constant 100s.
> +For example: If base_num=10 and min_avg_latency=1, the paths will be
> grouped in priority groups with path latency <=1us,
> +(1us, 10us], (10us, 100us], (100us, 1ms], (1ms, 10ms], (10ms,
> 100ms], (100ms, 1s], (1s, 10s], (10s, 100s], >100s.
> +.TP
> +.I min_avg_latency
> +The min average latency value of logarithmic scale, used to
> partition different priority ranks. Valid Values:
> +Integer, [1, 10000000] (us). And  Max average latency value is
> constant 100s.
> +For example: If base_num=10 and min_avg_latency=1000, the paths will
> be grouped in priority groups with path latency <=1ms,
> +(1ms, 10ms], (10ms, 100ms], (100ms, 1s], (1s, 10s], (10s, 100s],
> >100s.
>  .RE
>  .TP 12
>  .I alua

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel