[dm-devel] [PATCH 2/2] multipath-tools/libmultipath: Add args min_avg_latency for path_latency.
Yang Feng
philip.yang at huawei.com
Fri Jul 21 03:38:27 UTC 2017
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 at huawei.com>
>> Reviewed-by: Martin Wilck <mwilck at 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.
>
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 at 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
>
More information about the dm-devel
mailing list