Re: [PATCH] multipath-tools:Prioritizer based on a time-delay algorithm

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

 



Hello Martin,

How about this patch?
Thanks a lot,
Best regards.


On 2017/5/15 18:44, Yang Feng wrote:
> Hello Martin,
> 
> Thank you very much for your remarks. I am sorry for late reply, Please find my answer and the updated patch.
> 
>> Hello Yang,
>>
>> thank you for your work. Please find my remarks below.
>>
>> On Mon, 2017-05-08 at 11:58 +0800, Yang Feng wrote:
>>> Prioritizer for device mapper multipath, where the corresponding
>>> priority
>>> values of specific paths are provided by a time-delay algorithm. And
>>> the
>>> time-delay algorithm is dependent on the following
>>> arguments(delay_interval,
>>> cons_num).
>>> The principle of the algorithm is illustrated as follows:
>>> 1. By sending a certain number "cons_num" of read IOs to the current
>>> path
>>>    continuously, the IOs' average delay can be calculated.
>>> 2. According to the average delay of each path and the weight value
>>>    "delay_interval", the priority "rc" of each path can be provided.
>>>
>>>      delay_interval  delay_interval  delay_interval       delay_inter
>>
>> How does this algorithm behave under load? Can we be sure that
>> priorities don't start to fluctuate wildly because busy paths will
>> usually have longer latencies than idle ones?
> I have a lot of test under load. When the appropriate value of argument "delay_interval" is set,
> this algorithm behave well and can separate the paths who's average delay is more than others.
> When add a new path or the path's state change from down to up, getprio() of the prioritizer is triggered, and
> the current path is not under IOs.
> 
>>>  libmultipath/Makefile                   |   2 +-
>>>  libmultipath/checkers/Makefile          |   7 +-
>>>  libmultipath/checkers/emc_clariion.c    |   2 +-
>>>  libmultipath/checkers/libsg.c           |  94 ------------
>>>  libmultipath/checkers/libsg.h           |   9 --
>>>  libmultipath/checkers/readsector0.c     |   2 +-
>>>  libmultipath/libsg.c                    |  94 ++++++++++++
>>>  libmultipath/libsg.h                    |   9 ++
>>>  libmultipath/prioritizers/Makefile      |   6 +-
>>>  libmultipath/prioritizers/delayedpath.c | 246 
>>
>> Why do you have to move libsg for this? It's already used by various
>> checkers, why can't your checker do the same? If you really need to do
>> it, you should at least separate that part of the patch from the added
>> code.
> OK, this time, libsg will not be moved.
> 
>>> +
>>> +#define CHAR_SEC                "SEC"
>>> +#define CHAR_MSEC               "MSEC"
>>> +#define CHAR_USEC               "USEC"
>>
>> I suggest to use "s", "ms", and "us" here instead.
> OK, as the following patch.
> 
>> If you create an array of "const char*" instead like you did for
>> conversion_ratio below, you could implement get_interval_type() more
>> elegantly using a loop over that array.
> OK, as the following patch.
> 
>>> +static int get_interval_type(char *source, char *type)
>>> +{  
>>> +    /*is USEC*/
>>> +    if ((strstr(source, CHAR_USEC) != NULL)
>>> +        && (strstr(source, CHAR_USEC)[4] == '_'))
>>
>> Please avoid these double strstr() invocation. The compiler may
>> optimize it away, but it just looks strange. The following would 
>> look better to me, and I find it actually more readable:
>>
>>         if (((p = strstr(source, CHAR_USEC)) != NULL) && p[4] == '_')
> OK, as the following patch.
> 
>>> +static int get_string_from_under(char *args,
>>> +                                        char *beforestring,
>>> +                                        char *afterstring,
>>> +                                        int *type)
>>
>> Maybe you could figure out a more descriptive name for this function?
>>
>> A comment in the code showing how the string to be parsed typically
>> looks like would be helpful for the reader.
> OK, as the following patch.
> 
>>> +    token = strtok_r(source, under, &saveptr);
>>> +    token = strtok(token, char_type);
>>
>> I'm pretty sure this is is not what you intended to write. If char_type
>> is "usec", this would split the string at the possible delimiters 'u',
>> 's', 'e', and 'c' (the 2nd argument of strtok(3) is not a sequence, but
>> a 'set' of bytes). It might accidentally work with the input strings
>> you are using (in particular because you only look at the first token),
>> but nevertheless it's wrong.
> OK, as the following patch.
> 
>>> +    if ((token == NULL) || (saveptr == NULL))
>>> +        return 0;
>>> +
>>> +    tmp = token;
>>> +    while (*tmp != '\0')
>>> +        if (!isdigit(*tmp++))
>>> +            return 0;
>>> +
>>> +    tmp = saveptr;
>>> +    while (*tmp != '\0')
>>> +        if (!isdigit(*tmp++))
>>> +            return 0;
>>> +
>>> +    strncpy(beforestring, token, strlen(token) + 1);
>>> +    strncpy(afterstring, saveptr, strlen(saveptr) + 1);
>>> +    return 1;
>>> +}
>>
>> I don't think it's safe to use saveptr the way you do it. The strtok_r
>> man page says this parameter is for "internal use". While it makes
>> sense to assume that it points to the next token, I'm not sure if
>> that's guaranteed. You would be safe by calling 
>>
>>     somevar = strtok_r(NULL, under, &saveptr)
>>
>> and use "somevar".
> OK, as the following patch.
> 
>>
>> In general, this whole parsing code is odd. IIUC this parses input
>> looking like ([0-9]+)(SEC|MSEC|USEC)_([0-9]+) and sets beforestring,
>> type, and afterstring to the regex matches \1, \2, and \3,
>> respectively.
>>
>> Why don't you start parsing from the beginning of the input, e.g. with
>> strtoul(), and look at the rest later?
> OK, as the following patch.
> 
>>> +
>>> +int checkargvalid(int delay_interval, int cons_num, int type)
>>> +{
>>> +    if (type == INTERVAL_SEC)
>>> +    {
>>> +        if ((delay_interval < 1) || (delay_interval > 60))
>>> +            return 0;
>>> +    }
>>> +    else if (type != INTERVAL_INVALID)
>>> +    {
>>> +        if ((delay_interval < 1) || (delay_interval >= 1000))
>>> +            return 0;
>>> +    }
>>
>> You could be more forgiving here. 15000MSEC could be a legal value.
> Because this value is more than 1 second, you can use the unit second.
> 
>>
>>> +    
>>> +    if ((cons_num < 3) || (cons_num > 1000))
>>> +        return 0;
>>> +
>>> +    return 1;
>>> +}
>>> +
>>> +int get_delay_pref_arg(char *args, int *delay_interval, int
>>> *cons_num, int *type)
>>> +{
>>> +    char delayintervalstr[MAX_CHAR_SIZE];
>>> +    char consnumstr[MAX_CHAR_SIZE];
>>> +
>>> +    if (get_string_from_under(args, delayintervalstr, consnumstr,
>>> type) == 0)
>>> +        return 0;
>>
>> It might be good to write the parser so that the consnum part can be
>> left out by the user, and assume a reasonable default in that case.
> OK, as the following patch.
> 
>>> +    while (temp-- > 0)
>>> +    {
>>> +        (void)gettimeofday(&tv, NULL);
>>> +        before = timeval_to_us(&tv);		
>>> +
>>> +        if (do_readsector0(pp->fd, timeout) == 2)
>>> +        {
>>> +            condlog(0, "%s: path down", pp->dev);
>>> +            return 1;
>>> +        }
>>> +        
>>> +        (void)gettimeofday(&tv, NULL);
>>
>> It's better to use clock_gettime(CLOCK_MONOTONIC, ...) here. Then you
>> can throw away the delay < 0 check below.
> OK, as the following patch.
> 
>>> +    toldelay -= min + max;
>>> +    avgdelay = toldelay/(long long)(cons_num - 2);
>>> +    if (avgdelay > THRES_USEC_VALUE) 
>>> +    {           
>>> +        condlog(0, "%s: avgdelay is more than thresold", pp->dev);
>>> +        return 1;
>>> +    }
>>> +    
>>> +	ratio = get_conversion_ratio(type);
>>> +	rc = (int)(THRES_USEC_VALUE - (avgdelay/(((long
>>> long)delay_interval) * ratio)));
>>> +
>>> +    return rc;
>>> +}
>>
>> Is it reasonable to do these interval calculations synchronously in
>> getprio()? cons_num is limited to 1000, so this routine could issue
>> 1000 reads on the device before returning. In particular if the device
>> is under IO load and the delay is high, execution if this routine could
>> be really slow.
>>
>> It would make more sense to me to have a separate thread that
>> calculates some sort of "running average" for the delay of the
>> different paths, and have getprio() just fetch the current value of
>> that variable.
>>
>> Regards
>> Martin
>>
> When add a new path or the path's state change from down to up, getprio() of the prioritizer is triggered, and
> the current path is not under IOs.
> Usually, the 1000 reads will be finish fastly.
> And have getprio() need to get the up-to-date average delay of the current path, these interval calculations synchronously
> is essential because of unreasonable asynchronously process's executing time D-value, particulayly when add a new path or
> the path's state change down to up.
> 
> Please find the up-to-date patch below:
> 
> ---
>>From 1a9426dfbad00b5dbefc7020603e40e8896e4869 Mon Sep 17 00:00:00 2001
> From: Yang Feng <philip.yang@xxxxxxxxxx>
> Date: Mon, 15 May 2017 18:33:29 +0800
> Subject: [PATCH]  [PATCH] multipath-tools:Prioritizer based on a time-delay algorithm
>  Prioritizer for device mapper multipath, where the corresponding priority values of specific paths are provided by a time-delay
>  algorithm. And the time-delay algorithm is dependent on the following arguments(delay_interval, cons_num). The principle of the
>  algorithm is illustrated as follows:
>  1. By sending a certain number "cons_num" of read IOs to the current path    continuously, the IOs' average delay can be calculated.
>  2. According to the average delay of each path and the weight value "delay_interval", the priority "rc" of each path can be provided.
> 
>            delay_interval  delay_interval  delay_interval      delay_interval
> 	 |---------------|---------------|---------------|   |---------------|
> 	 |priority rank 1|priority rank 2|priority rank 3|...|priority rank x|
>          |---------------|---------------|---------------|   |---------------|
> 		               Priority Rank Partitioning
> ---
>  libmultipath/prioritizers/Makefile      |   6 +-
>  libmultipath/prioritizers/delayedpath.c | 261 ++++++++++++++++++++++++++++++++
>  libmultipath/prioritizers/delayedpath.h |  17 +++
>  multipath/multipath.conf.5              |  19 +++
>  4 files changed, 302 insertions(+), 1 deletion(-)
>  create mode 100644 libmultipath/prioritizers/delayedpath.c
>  create mode 100644 libmultipath/prioritizers/delayedpath.h
> 
> diff --git a/libmultipath/prioritizers/Makefile b/libmultipath/prioritizers/Makefile
> index 36b42e4..8df5234 100644
> --- a/libmultipath/prioritizers/Makefile
> +++ b/libmultipath/prioritizers/Makefile
> @@ -18,13 +18,17 @@ LIBS = \
>  	libpriorandom.so \
>  	libpriordac.so \
>  	libprioweightedpath.so \
> -	libpriosysfs.so
> +	libpriodelayedpath.so \
> +	libpriosysfs.so
> 
>  all: $(LIBS)
> 
>  libprioalua.so: alua.o alua_rtpg.o
>  	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> 
> +libpriodelayedpath.so: delayedpath.o  ../checkers/libsg.o
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> +
>  libprio%.so: %.o
>  	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> 
> diff --git a/libmultipath/prioritizers/delayedpath.c b/libmultipath/prioritizers/delayedpath.c
> new file mode 100644
> index 0000000..0490e8d
> --- /dev/null
> +++ b/libmultipath/prioritizers/delayedpath.c
> @@ -0,0 +1,261 @@
> +/*
> + * (C) Copyright HUAWEI Technology Corp. 2017, 2021   All Rights Reserved.
> + *
> + * main.c
> + *
> + * Prioritizer for device mapper multipath, where the corresponding priority
> + * values of specific paths are provided by a time-delay algorithm. And the
> + * time-delay algorithm is dependent on arguments.
> + *
> + * The principle of the algorithm as follows:
> + * 1. By sending a certain number "cons_num" of read IOs to the current path
> + *    continuously, the IOs' average delay can be calculated.
> + * 2. According to the average delay of each path and the weight value
> + *    "delay_interval", the priority "rc" of each path can be provided.
> + *
> + * Author(s): Yang Feng <philip.yang@xxxxxxxxxx>
> + *            Zou Ming <zouming.zouming@xxxxxxxxxx>
> + *
> + * This file is released under the GPL.
> + */
> +#include <stdio.h>
> +#include <ctype.h>
> +#include <time.h>
> +
> +#include "debug.h"
> +#include "prio.h"
> +#include "structs.h"
> +#include "../checkers/libsg.h"
> +
> +#include "delayedpath.h"
> +
> +#define THRES_USEC_VALUE        300000000LL    /*USEC, 300SEC*/
> +#define DEFAULT_DELAY_INTERVAL  10             /*MSEC*/
> +#define DEFAULT_CONS_NUM        20
> +
> +#define MAX_CHAR_SIZE           30
> +
> +#define CHAR_SEC                "s"
> +#define CHAR_MSEC               "ms"
> +#define CHAR_USEC               "us"
> +
> +enum interval_type {
> +    INTERVAL_SEC,
> +    INTERVAL_MSEC,
> +    INTERVAL_USEC,
> +    INTERVAL_INVALID
> +};
> +
> +/* interval_unit_str and interval_unit_type keep the same assignment sequence */
> +static const char interval_unit_str[][MAX_CHAR_SIZE] = {
> +    CHAR_USEC, CHAR_MSEC, CHAR_SEC
> +};
> +static const int interval_unit_type[] = {
> +    INTERVAL_USEC, INTERVAL_MSEC, INTERVAL_SEC
> +};
> +
> +static const int conversion_ratio[] = {
> +	[INTERVAL_SEC]		= USEC_PER_SEC,
> +	[INTERVAL_MSEC]	        = USEC_PER_MSEC,
> +	[INTERVAL_USEC]		= USEC_PER_USEC,
> +	[INTERVAL_INVALID]	= 0
> +};
> +
> +
> +static int do_readsector0(int fd, unsigned int timeout)
> +{
> +	unsigned char buf[4096];
> +	unsigned char sbuf[SENSE_BUFF_LEN];
> +	int ret;
> +
> +	ret = sg_read(fd, &buf[0], 4096, &sbuf[0],
> +		      SENSE_BUFF_LEN, timeout);
> +
> +	return ret;
> +}
> +
> +static int get_interval_type(char *source, char *type)
> +{
> +    char *p;
> +    int size;
> +    int i;
> +
> +    for (i = 0; i < sizeof(interval_unit_str)/MAX_CHAR_SIZE; i++)
> +    {
> +        size = strlen(interval_unit_str[i]);
> +        p = strstr(source, interval_unit_str[i]);
> +        if (p != NULL && p[size] == '|')
> +        {
> +            memcpy(type, interval_unit_str[i], size+1);
> +            return interval_unit_type[i];
> +        }
> +    }
> +
> +    return INTERVAL_INVALID;
> +}
> +
> +/* In multipath.conf, args form: delay_interval|cons_num. For example,
> +*  args is "10ms|20", this function can get 10, ms, and 20.
> +*/
> +static int get_digit_and_type(char *args,
> +                              int *interval,
> +                              int *consnum,
> +                              int *type)
> +{
> +    char typestr[MAX_CHAR_SIZE];
> +    char source[MAX_CHAR_SIZE];
> +    char vertica[] = "|";
> +    char *tokenbefore = NULL;
> +    char *tokenafter = NULL;
> +    char *tmp = NULL;
> +    unsigned int size = strlen(args);
> +
> +    if ((args == NULL) || (interval == NULL)
> +        || (consnum == NULL) || (type == NULL))
> +        return 0;
> +
> +    /* int type */
> +    if ((size < 1) || (size > MAX_CHAR_SIZE-1))
> +        return 0;
> +
> +    memcpy(source, args, size+1);
> +    if (strstr(source, vertica) == NULL)
> +        return 0;
> +
> +    *type = get_interval_type(source, typestr);
> +    if (*type == INTERVAL_INVALID)
> +    {
> +        condlog(0, "delay_interval type is invalid");
> +        return 0;
> +    }
> +
> +    tokenbefore = strtok(source, vertica);
> +    tokenafter = strtok(NULL, vertica);
> +    typestr[1] = '\0';
> +    tokenbefore = strtok(tokenbefore, typestr);
> +    if ((tokenbefore == NULL) || (tokenafter == NULL))
> +        return 0;
> +
> +    tmp = tokenbefore;
> +    while (*tmp != '\0')
> +        if (!isdigit(*tmp++))
> +        {
> +            condlog(0, "delay_interval string include invalid char");
> +            return 0;
> +        }
> +
> +    tmp = tokenafter;
> +    while (*tmp != '\0')
> +        if (!isdigit(*tmp++))
> +        {
> +            condlog(0, "cons_num string include invalid char");
> +            return 0;
> +        }
> +
> +    *interval = atoi(tokenbefore);
> +    *consnum = atoi(tokenafter);
> +
> +    return 1;
> +}
> +
> +int check_args_valid(int delay_interval, int cons_num, int type)
> +{
> +    if (type == INTERVAL_SEC)
> +    {
> +        if ((delay_interval < 1) || (delay_interval > 60))
> +        {
> +            condlog(0, "delay_interval values is invalid");
> +            return 0;
> +        }
> +    }
> +    else if (type != INTERVAL_INVALID)
> +    {
> +        if ((delay_interval < 1) || (delay_interval >= 1000))
> +        {
> +            condlog(0, "delay_interval values is invalid");
> +            return 0;
> +        }
> +    }
> +
> +    if ((cons_num < 3) || (cons_num > 1000))
> +    {
> +        condlog(0, "cons_num values is invalid");
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
> +int get_delay_pref_arg(char *args, int *delay_interval, int *cons_num, int *interval_type)
> +{
> +    if (get_digit_and_type(args, delay_interval, cons_num, interval_type) == 0)
> +        return 0;
> +
> +    if (check_args_valid(*delay_interval, *cons_num, *interval_type) == 0)
> +        return 0;
> +
> +    return 1;
> +}
> +
> +long long get_conversion_ratio(int type)
> +{
> +    return conversion_ratio[type];
> +}
> +
> +int getprio (struct path *pp, char *args, unsigned int timeout)
> +{
> +    int rc, delay_interval, cons_num, type, temp;
> +    long long delay, avgdelay, ratio;
> +    long long min = THRES_USEC_VALUE;
> +    long long max = 0;
> +    long long toldelay = 0;
> +    long long before, after;
> +    struct timespec tv;
> +
> +	if (pp->fd < 0)
> +	    return -PRIO_NO_INFORMATION;
> +
> +    if (get_delay_pref_arg(args, &delay_interval, &cons_num, &type) == 0)
> +    {
> +        condlog(3, "%s: get delay arg fail", pp->dev);
> +        delay_interval = DEFAULT_DELAY_INTERVAL;
> +        cons_num = DEFAULT_CONS_NUM;
> +        type = INTERVAL_MSEC;
> +    }
> +
> +    temp = cons_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);
> +
> +        delay = after - before;
> +    	
> +        min = (min <= delay) ? min : delay;
> +        max = (max >= delay) ? max : delay;
> +
> +        toldelay += delay;
> +    }
> +
> +    toldelay -= min + max;
> +    avgdelay = toldelay/(long long)(cons_num - 2);
> +    if (avgdelay > THRES_USEC_VALUE)
> +    {
> +        condlog(0, "%s: avgdelay is more than thresold", pp->dev);
> +        return 1;
> +    }
> +
> +	ratio = get_conversion_ratio(type);
> +	rc = (int)(THRES_USEC_VALUE - (avgdelay/(((long long)delay_interval) * ratio)));
> +
> +    return rc;
> +}
> diff --git a/libmultipath/prioritizers/delayedpath.h b/libmultipath/prioritizers/delayedpath.h
> new file mode 100644
> index 0000000..d8213e9
> --- /dev/null
> +++ b/libmultipath/prioritizers/delayedpath.h
> @@ -0,0 +1,17 @@
> +#ifndef _DELAYEDPATH_H
> +#define _DELAYEDPATH_H
> +
> +#define PRIO_DELAYED_PATH "delayedpath"
> +
> +#define PRIO_NO_INFORMATION 5
> +
> +#define USEC_PER_SEC      1000000LL
> +#define USEC_PER_MSEC     1000LL
> +#define USEC_PER_USEC     1LL
> +
> +static inline long long timeval_to_us(const struct timespec *tv)
> +{
> +	return ((long long) tv->tv_sec * USEC_PER_SEC) + (tv->tv_nsec >> 10);
> +}
> +
> +#endif
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 5939688..f1e126e 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -293,6 +293,10 @@ Generate a random priority between 1 and 10.
>  Generate the path priority based on the regular expression and the
>  priority provided as argument. Requires prio_args keyword.
>  .TP
> +.I delayedpath
> +Generate the path priority based on a time-delay algorithm.
> +Requires prio_args keyword.
> +.TP
>  .I datacore
>  .\" XXX
>  ???. Requires prio_args keyword.
> @@ -333,6 +337,21 @@ these values can be looked up through sysfs or by running \fImultipathd show pat
>  "%N:%R:%n:%r"\fR. For example: 0x200100e08ba0aea0:0x210100e08ba0aea0:.*:.* , .*:.*:iqn.2009-10.com.redhat.msp.lab.ask-06:.*
>  .RE
>  .TP 12
> +.I delayed
> +Needs a value of the form
> +\fI"<delay_interval|cons_num>"\fR
> +.RS
> +.TP 8
> +.I delay_interval
> +The interval values of average IO-time-delay between two different neighbour ranks of path priority, used to partition different priority ranks.
> +Form: XXs, or XXXus, or XXXms. Unit: Second, or Microsecond, or Millisecond. Valid Values: Integer, s [1, 60], ms [1, 1000), us [1, 1000),
> +For example: 10s, or 100us, or 100ms. The default is: 10ms.
> +.TP
> +.I cons_num
> +The number of read IOs sent to the current path continuously, used to calculate the average IO-time-delay. Valid Values: Integer, [3, 1000].
> +For example: 30. The default is: 20.
> +.RE
> +.TP 12
>  .I alua
>  If \fIexclusive_pref_bit\fR is set, paths with the \fIpreferred path\fR bit
>  set will always be in their own path group.
> 

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