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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Martin,

Thank you very much for your guidance and reviews.
Please find my replys as follows.
The up-to-date patch will be sent later.

Regards,
-Yang

On 2017/7/21 2:07, Martin Wilck wrote:
> 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.
>
Sorry, there is a mistake. I will fix it later.
>>
>> Signed-off-by: Yang Feng <philip.yang@xxxxxxxxxx>
>> Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
> 
> 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.
> 
Sorry, it will fixed.
> 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"?
> 
Sorry, I don't find this similar syntax. Could you give a example of the
other prioritizer.
> Regards
> Martin
> 
> 
>> Reviewed-by: Xose Vazquez Perez <xose.vazquez@xxxxxxxxx>
>> ---
>>  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
> 

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



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux