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

[Martin Wilck <mwilck@suse.com>]

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?


> val
>     |---------------|---------------|---------------|	 |----
> -----------|
>     |priority rank1 |priority rank2 |priority rank3 |... |priority
> rank4 |
>     |---------------|---------------|---------------|    |-----------
> ----|
>                        Priority Rank Partitioning
> ---
>  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.

> diff --git a/libmultipath/prioritizers/delayedpath.c
> b/libmultipath/prioritizers/delayedpath.c
> new file mode 100644
> index 0000000..4c1cfea
> --- /dev/null
> +++ b/libmultipath/prioritizers/delayedpath.c
> @@ -0,0 +1,246 @@
> +/*
> + * (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@huawei.com>
> + *            Zou Ming <zouming.zouming@huawei.com>
> + *
> + * This file is released under the GPL.
> + */
> +#include <stdio.h>
> +#include <ctype.h>
> +#include <sys/time.h>
> +
> +#include "debug.h"
> +#include "prio.h"
> +#include "structs.h"
> +#include "../libmultipath/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                "SEC"
> +#define CHAR_MSEC               "MSEC"
> +#define CHAR_USEC               "USEC"

I suggest to use "s", "ms", and "us" here instead.

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.

> +
> +enum interval_type {
> +    INTERVAL_SEC,
> +    INTERVAL_MSEC,
> +    INTERVAL_USEC,
> +    INTERVAL_INVALID
> +};
> +
> +static 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)
> +{  
> +    /*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] == '_')

> +    {
> +        memcpy(type, CHAR_USEC, strlen(CHAR_USEC)+1);        
> +        return INTERVAL_USEC;
> +    }
> +
> +    /*is MSEC*/
> +    if ((strstr(source, CHAR_MSEC) != NULL) 
> +        && (strstr(source, CHAR_MSEC)[4] == '_'))
> +    {
> +        memcpy(type, CHAR_MSEC, strlen(CHAR_MSEC)+1);
> +        return INTERVAL_MSEC;
> +    }
> +
> +    /*is SEC*/
> +	if ((strstr(source, CHAR_SEC) != NULL)
> +        && (strstr(source, CHAR_SEC)[4] == '_'))
> +    {
> +        memcpy(type, CHAR_SEC, strlen(CHAR_SEC)+1);
> +        return INTERVAL_SEC;
> +    }
> +
> +    return INTERVAL_INVALID;
> +}
> +
> +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.

> +{
> +    char source[MAX_CHAR_SIZE];
> +    char char_type[MAX_CHAR_SIZE];
> +    char under[] = "_";
> +    char *token  = NULL;
> +    char *tmp = NULL;
> +    char *saveptr = NULL;
> +    unsigned int size = strlen(args);
> +
> +    if ((args == NULL) || (beforestring == NULL) 
> +        || (afterstring == 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, under) == NULL)
> +        return 0;
> +
> +    *type = get_interval_type(source, char_type);
> +    if (*type == INTERVAL_INVALID)
> +        return 0;
> +
> +    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.

> +    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".

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?

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

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

> +
> +    *delay_interval = atoi(delayintervalstr);
> +    *cons_num = atoi(consnumstr);
> +
> +    if (checkargvalid(*delay_interval, *cons_num, *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 timeval tv;
> +
> +    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)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.

> +        after = timeval_to_us(&tv);
> +
> +        delay = after - before;
> +        if (delay < 0)
> +        {
> +            condlog(0, "%s: delay calc error", pp->dev);
> +            return 1;
> +        }
> +    	
> +        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;
> +}

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

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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