Re: [PATCH v5 1/1] multipath-tools: Prioritizer based on a latency algorithm

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

 



Hi Martin,

Thanks for your reviews.
Please find my replys.
And the patch v6 will be sent later.

Regards,
Yang

---
> 
>> +#define MAX_LATENCY_INTERVAL    10            /*unit: s*/
>> +#define MIN_LATENCY_INTERVAL    1             /*unit: us, or ms, or s*/
> 
> Why not use the same unit or both values?
> 
It's easy to check the valid range of the "latency_interval" value.
For unit: us, or ms, or s, the min digital value is the same, is 1.
And for all unit, the max conversion value is the same, is 10 seconds.

>> +static inline long long timeval_to_us(const struct timespec *tv)
>> +{
>> +	return ((long long) tv->tv_sec * USEC_PER_SEC) + (tv-
>>> tv_nsec >> 10);
> 
> Please calculate correctly, divide by 1000.
> 
Thanks, it will be fixed.

>> +int check_args_valid(int io_num, long long latency_interval, int
>> type)
>> +{
>> +    if ((io_num < MIN_IO_NUM) || (io_num > MAX_IO_NUM))
>> +    {
>> +        condlog(0, "args io_num is more than the valid values
>> range");
>> +        return 0;
>> +    }
> 
> Please write "outside the valid range" rather than "more than the valid
> values range".
> 
Thanks, it will be fixed.

> In general, when you use condlog(), please prefix all messages with a
> string that clearly identifies the path_latency prioritizer, e.g.
> PRIO_PATH_LATENCY.
> I know we're not currently doing that consistently in multipath-tools,
> but that shouldn't be a reason for not doing when adding new code.
> 
Good, thanks, it will be fixed.

>> +
>> +    /* check value is set by using logarithmic scale, and base
>> number is 10 */
>> +    if ((latency_interval % BASE_NUMBER) != 0)
>> +    {
>> +        condlog(0, "args latency_interval is not set by using
>> logarithmic scale , where base number is 10");
>> +        return 0;
>> +    }
> 
> This is not what I meant with "logarithmic scale". See below.
> 
>> +
>> +    /* s:[1, 10], ms:[1, 10000], us:[1, 10000000] */
>> +    if ((latency_interval < MIN_LATENCY_INTERVAL)
>> +        || (latency_interval > (MAX_LATENCY_INTERVAL * USEC_PER_SEC
>> / conversion_ratio[type])))
>> +    {
>> +        condlog(0, "args latency_interval is more than the valid
>> values range");
>> +        return 0;
>> +    }
> 
> Again, please use "outside".
> 
Thanks, it will be fixed.

>> +/* In multipath.conf, args form: io_num|latency_interval. For
>> example,
>> +*  args is "20|10ms", this function can get 20, 10.
>> +*/
>> +static int get_interval_and_ionum(char *args,
>> +                                        int *ionum,
>> +                                        long long *interval)
>> +{
>> +    char source[MAX_CHAR_SIZE];
>> +    char vertica = '|';
>> +    char *endstrbefore = NULL;
>> +    char *endstrafter = NULL;
>> +    int type;
>> +    unsigned int size = strlen(args);
>> +    long long ratio;
>> +
>> +    if ((args == NULL) || (ionum == NULL) || (interval == NULL))
>> +    {
>> +        condlog(0, "args string is NULL");
>> +        return 0;
>> +    }
>> +
>> +    if ((size < 1) || (size > MAX_CHAR_SIZE-1))
>> +    {
>> +        condlog(0, "args string's size is too long");
>> +        return 0;
>> +    }
> 
> "too long" is not correct for the first (more likely) error condition.
> 
>> +
>> +    memcpy(source, args, size+1);
>> +
>> +    if (!isdigit(source[0]))
>> +    {
>> +        condlog(0, "args io_num string's first char is not digit");
>> +        return 0;
>> +    }
> 
> It would be ok to catch all these errors with a single line of code
> such as "path_latency: invalid prio_args format: %s".
> 
OK, it will be fixed.

>> +
>> +    *ionum = (int)strtoul(source, &endstrbefore, 10);
>> +    if (endstrbefore[0] != vertica)
>> +    {
>> +        condlog(0, "segmentation char is invalid");
>> +        return 0;
>> +    }
>> +
>> +    if (!isdigit(endstrbefore[1]))
>> +    {
>> +        condlog(0, "args latency_interval string's first char is not
>> digit");
>> +        return 0;
>> +    }
>> +
>> +    *interval = (long long)strtol(&endstrbefore[1], &endstrafter,
>> 10);
>> +    type = get_interval_type(endstrafter);
>> +    if (type == INTERVAL_INVALID)
>> +    {
>> +        condlog(0, "args latency_interval type is invalid");
>> +        return 0;
>> +    }
>> +
>> +    if (check_args_valid(*ionum, *interval, type) == 0)
>> +    {
>> +        return 0;
>> +    }
>> +
>> +	ratio = get_conversion_ratio(type);
>> +    *interval *= (long long)ratio;
>> +
>> +    return 1;
>> +}
>> +
>> +long long calc_standard_deviation(long long *path_latency, int size,
>> long long avglatency)
>> +{
>> +    int index;
>> +    long long total = 0;
>> +
>> +    for (index = 0; index < size; index++)
>> +    {
>> +        total += (path_latency[index] - avglatency) *
>> (path_latency[index] - avglatency);
>> +    }
>> +
>> +    total /= (size-1);
>> +
>> +    return (long long)sqrt((double)total);
>> +}
>> +
>> +int getprio (struct path *pp, char *args, unsigned int timeout)
>> +{
>> +    int rc, temp;
>> +    int index = 0;
>> +    int io_num;
>> +    long long latency_interval;
>> +    long long avglatency;
>> +    long long standard_deviation;
>> +    long long toldelay = 0;
>> +    long long before, after;
>> +    struct timespec tv;
>> +
>> +	if (pp->fd < 0)
>> +		return -1;
>> +
>> +    if (get_interval_and_ionum(args, &io_num, &latency_interval) ==
>> 0)
>> +    {
>> +        condlog(0, "%s: get path_latency args fail", pp->dev);
>> +        return DEFAULT_PRIORITY;
>> +    }
> 
> Could you just assume reasonable defaults for the path latency
> algorithm rather than returning a constant?
> 
En, I think returning a DEFAULT_PRIORITY "0" is no problem.
If return a assumed value, then the user can be confused, because
the defaults is not set.

>> +
>> +    memset(path_latency, 0, sizeof(path_latency));
>> +
>> +    temp = io_num;
>> +    while (temp-- > 0)
>> +    {
>> +        (void)clock_gettime(CLOCK_MONOTONIC, &tv);
>> +        before = timeval_to_us(&tv);
>> +
>> +        if (do_readsector0(pp->fd, timeout) == 2)
>> +        {
>> +            condlog(0, "%s: path down", pp->dev);
>> +            return -1;
>> +        }
>> +
>> +        (void)clock_gettime(CLOCK_MONOTONIC, &tv);
>> +        after = timeval_to_us(&tv);
>> +
>> +        path_latency[index] = after - before;
>> +        toldelay += path_latency[index++];
>> +    }
>> +
>> +    avglatency = toldelay/(long long)io_num;
>> +    condlog(4, "%s: average latency is (%lld)", pp->dev,
>> avglatency);
>> +
>> +    if (avglatency > THRES_USEC_VALUE)
>> +    {
>> +        condlog(0, "%s: average latency (%lld) is more than
>> thresold", pp->dev, avglatency);
>> +        return DEFAULT_PRIORITY;
>> +    }
> 
> Why don't you put simply put this in the highest latency bin? See
> below.
> 
Thanks, it will be fixed.

>> +
>> +    /* warn the user if the latency_interval set is smaller than (2
>> * standard deviation), or equal */
>> +    standard_deviation = calc_standard_deviation(path_latency,
>> index, avglatency);
>> +    if (latency_interval <= (2 * standard_deviation))
>> +        condlog(3, "%s: args latency_interval set is smaller than 2
>> * standard deviation (%lld us), or equal",
>> +            pp->dev, standard_deviation);
>> +
>> +	rc = (int)(THRES_USEC_VALUE - (avglatency/(long
>> long)latency_interval));
>> +    return rc;
>> +}
> 
> Hm, I think you misunderstood my "logarithmic scale" idea.
> By dividing the average by the interval size, you get ordinary linear
> scaling again. That doesn't help the problem.
> 
> Here is a sample program illustrating what I meant. It uses math.h
> functions, but that shouldn't be a problem; we're running in user
> space. The chosen interval size, corresponding to a factor of 1.995, is
> probably a good choice for many scenarios. Your could represent the
> range between 1us and 10s with 34 prio values, and still be able to
> differentiate paths with, say, 1.5 and 3 us latency.
> 
> If you use this, you should also use double type for calculating the
> average and stddev, and you must be careful to calculate the standard
> deviation for the logarithms, as interval size is not constant any more
> (only on logarithmic scale).
> 
> #include <stdio.h>
> #include <math.h>
> 
> /*
>  * For a given positive value x, return a "prio" between 0 and n
>  * using logarithmically scaled intervals between minval and maxval.
>  */
> 
> int log_prio(double x,
> 	     unsigned long minval, unsigned long maxval, unsigned n)
> {
> 	double lx, lmin, lmax;
> 	if (x <= minval)
> 		return n;
> 	if (x >= maxval)
> 		return 0;
> 	lx = log10(x);
> 	lmin = log10(minval);
> 	lmax = log10(maxval);
> 	return n - (n - 1) * (lx - lmin) / (lmax - lmin);
> }
> 
> int main(void)
> {
> 	int minval = 1;
> 	int maxval = 1000;
> 	int n = 11;
> 	double x;
> 	for (x = minval/2.; x < maxval*2.; x *= sqrt(2.))
> 		printf("%f: %d\n", x, log_prio(x, minval, maxval, n));
> 	return 0;
> }
> 
OK, the argument "latency_interval" will be delete, instead, the argument
"base_num" will be added for logarithmic scale.
And min value will be set 1 us, max value will be set 10000000 us (or 10 s).

> Regards,
> Martin
> 

--
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