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@xxxxxxxxxx> > + * Zou Ming <zouming.zouming@xxxxxxxxxx> > + * > + * 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@xxxxxxxx>, 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel