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
