Re: [dm-devel] [PATCH] multipath-tools:Prioritizer based on a time-delay algorithm
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, _interval, _num, )
> == 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(, NULL);
> +before = timeval_to_us();
> +
> +if (do_readsector0(pp->fd, timeout) == 2)
> +{
> +condlog(0, "%s: path down", pp->dev);
> +return 1;
> +}
> +
> +(void)gettimeofday(, 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();
> +
> +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 <mwi...@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
Re: [dm-devel] [PATCH 6/6] mpath_persist: Don't join threads that don't exist
On Tue, 2017-05-09 at 11:57 -0500, Benjamin Marzinski wrote: > There are a couple of places in the mpath_persist code that were > calling > pthread join, even if the pthread_create call failed, and the thread > id > was undefined. This can cause crashes, so don't do it. > > Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com> Reviewed-by: Martin Wilck <mwi...@suse.com> -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 1/3] dm: a basic support for using the select or poll function
On Thu, 2017-05-11 at 21:21 +0100, Alasdair G Kergon wrote: > On Thu, May 11, 2017 at 09:49:14PM +0200, Martin Wilck wrote: > > Here is an idea: the best way to avoid the race mentioned by Mike > > might > > be to set priv->global_event_nr to the highest event nr reported by > > the > > DM_LIST_DEVICES ioctl when this ioctl returns. DM_LIST_DEVICES > > would > > then represent your "separate action", which would fit quite well, > > and > > the DM_DEV_ARM_POLL ioctl wouldn't be needed. It would be > > guaranteed > > that if any event had occured after the previous DM_LIST_DEVICES, > > whether or not that had been before or after the poll() call, > > userspace > > would notice. > > > It's not really good practice to overload operations by adding and > relying upon side-effects. Yes, you could add a flag to control the > side-effect, but a separate ioctl seems to be a perfectly reasonable > approach in the present case, allowing for flexible use and not > forcing > you to generate the list when you don't need it. Except that it can cause userspace to miss events, as I tried to point out in my reply to Mike. My point is: the system call that resets the base counter should be the same system call that retrieves the events since the last reset operation. Unless I'm missing something essential, that's the only way to avoid userspace receiving events with a potentially large delay. If that system call can't be ioctl(DM_DEVICE_INFO) as you argue, it should be something else, but it should be _one_. Regards Martin -- Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH v2 0/2] Small multipath fixes
Two minor fixes that I found necessary recently. Both touch potentially bigger issues - the first one print.c, which might need some general overhaul at some time in the future, and the second one the build process of multipath tools, which is currently lacking an "autobuild" mechanism. My intention at was to keep the patches small and non-controversial, therefore I didn't delve into the big issues just yet. Changes wrt v1 (suggestions by Bart Van Assche): - patch 1/2: replaced ENDLINE by a function - patch 2/2: Removed nonstandard <<< construct, and made sure STACKPROT is only set once per makefile. Martin Wilck (2): libmultipath: print.c: make sure lines are 0-terminated multipath-tools: fix compilation with gcc < 4.9 Makefile.inc | 15 ++- libmultipath/print.c | 25 + 2 files changed, 31 insertions(+), 9 deletions(-) -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 1/2] libmultipath: print.c: make sure lines are 0-terminated
Under certain conditions, the output of "multipath -ll" or
"multipathd show topology", or the multipathd logs, may
contain garbage characters. Fix that by making sure that
the strings are always properly zero-terminated.
While touching this code, substitute a function for the
complex ENDLINE macro.
Note 1: The way this is coded, the previously written
character is overwritten by the newline. That behavior is
unchanged by this patch. I didn't want to fuzz with the
carefully crafted print.c code more than necessary at
this point.
Note 2: The condition (c <= line + len - 1) is equivalent to
(TAIL >= 0). It should always hold the way ENDLINE is called
after PRINT and PAD in print.c, and is only added as an
additional safeguard, e.g. against future code changes.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/print.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 7c2a1588..95dff90b 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -39,9 +39,18 @@ do { \
s = c; \
} while (0)
-#define ENDLINE \
- if (c > line) \
- line[c - line - 1] = '\n'
+static char *
+__endline(char *line, size_t len, char *c)
+{
+ if (c > line) {
+ if (c >= line + len)
+ c = line + len - 1;
+ *(c - 1) = '\n';
+ *c = '\0';
+ }
+ return c;
+}
+
#define PRINT(var, size, format, args...) \
do { \
fwd = snprintf(var, size, format, ##args); \
@@ -802,7 +811,7 @@ snprint_multipath_header (char * line, int len, char *
format)
PAD(data->width);
} while (*f++);
- ENDLINE;
+ __endline(line, len, c);
return (c - line);
}
@@ -838,7 +847,7 @@ snprint_multipath (char * line, int len, char * format,
buff[0] = '\0';
} while (*f++);
- ENDLINE;
+ __endline(line, len, c);
return (c - line);
}
@@ -869,7 +878,7 @@ snprint_path_header (char * line, int len, char * format)
PAD(data->width);
} while (*f++);
- ENDLINE;
+ __endline(line, len, c);
return (c - line);
}
@@ -904,7 +913,7 @@ snprint_path (char * line, int len, char * format,
PAD(data->width);
} while (*f++);
- ENDLINE;
+ __endline(line, len, c);
return (c - line);
}
@@ -938,7 +947,7 @@ snprint_pathgroup (char * line, int len, char * format,
PAD(data->width);
} while (*f++);
- ENDLINE;
+ __endline(line, len, c);
return (c - line);
}
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipath-tools: enable libdmmp installation in alternative directory
Introduce a new Makefile variable, usr_prefix, to be used for libdmmp and the associated pkgconfig file. Some distributions install those libraries which are necessary for booting (and mounting /usr) in a different location (/lib or /lib64) than other libraries (/usr/lib or /usr/lib64). On such distributions, installation to the different paths can be achieved by setting "usr_prefix=/usr". This will affect only libdmmp at this time, as all other libaries in the multipath-tools package may be relevant for booting. For distributions on which /lib and /lib64 are just symlinks to their /usr counterparts, nothing changes. Comments are welcome. Signed-off-by: Martin Wilck <mwi...@suse.com> --- Makefile.inc | 4 +++- libdmmp/Makefile | 11 ++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Makefile.inc b/Makefile.inc index ad55aa10..ad08f307 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -47,6 +47,7 @@ endif prefix = exec_prefix= $(prefix) +usr_prefix = $(prefix) bindir = $(exec_prefix)/sbin libudevdir = $(prefix)/$(SYSTEMDPATH)/udev udevrulesdir = $(libudevdir)/rules.d @@ -55,6 +56,7 @@ man8dir = $(prefix)/usr/share/man/man8 man5dir= $(prefix)/usr/share/man/man5 man3dir= $(prefix)/usr/share/man/man3 syslibdir = $(prefix)/$(LIB) +usrlibdir = $(usr_prefix)/$(LIB) libdir = $(prefix)/$(LIB)/multipath unitdir= $(prefix)/$(SYSTEMDPATH)/systemd/system mpathpersistdir= $(TOPDIR)/libmpathpersist @@ -62,7 +64,7 @@ mpathcmddir = $(TOPDIR)/libmpathcmd thirdpartydir = $(TOPDIR)/third-party libdmmpdir = $(TOPDIR)/libdmmp includedir = $(prefix)/usr/include -pkgconfdir = $(prefix)/$(LIB)/pkgconfig +pkgconfdir = $(usrlibdir)/pkgconfig GZIP = gzip -9 -c RM = rm -f diff --git a/libdmmp/Makefile b/libdmmp/Makefile index 1c5329aa..908e294b 100644 --- a/libdmmp/Makefile +++ b/libdmmp/Makefile @@ -27,15 +27,16 @@ $(LIBS): $(OBJS) $(LN) $@ $(DEVLIB) install: - $(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS) + mkdir -p $(DESTDIR)$(usrlibdir) + $(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(usrlibdir)/$(LIBS) $(INSTALL_PROGRAM) -m 644 -D \ $(HEADERS) $(DESTDIR)$(includedir)/$(HEADERS) - $(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB) + $(LN) $(LIBS) $(DESTDIR)$(usrlibdir)/$(DEVLIB) $(INSTALL_PROGRAM) -m 644 -D \ $(PKGFILE).in $(DESTDIR)$(pkgconfdir)/$(PKGFILE) perl -i -pe 's|__VERSION__|$(LIBDMMP_VERSION)|g' \ $(DESTDIR)$(pkgconfdir)/$(PKGFILE) - perl -i -pe 's|__LIBDIR__|$(syslibdir)|g' \ + perl -i -pe 's|__LIBDIR__|$(usrlibdir)|g' \ $(DESTDIR)$(pkgconfdir)/$(PKGFILE) perl -i -pe 's|__INCLUDEDIR__|$(includedir)|g' \ $(DESTDIR)$(pkgconfdir)/$(PKGFILE) @@ -46,9 +47,9 @@ install: done uninstall: - $(RM) $(DESTDIR)$(syslibdir)/$(LIBS) + $(RM) $(DESTDIR)$(usrlibdir)/$(LIBS) $(RM) $(DESTDIR)$(includedir)/$(HEADERS) - $(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB) + $(RM) $(DESTDIR)$(usrlibdir)/$(DEVLIB) @for file in $(DESTDIR)$(man3dir)/dmmp_*; do \ $(RM) $$file; \ done -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 4/6] libmultipath: fix suspended devs from failed reloads
On Tue, 2017-05-16 at 12:33 -0500, Benjamin Marzinski wrote: > On Thu, May 11, 2017 at 10:26:52PM +0200, Martin Wilck wrote: > > On Tue, 2017-05-09 at 11:57 -0500, Benjamin Marzinski wrote: > > > When multipath reloads a device, it can either fail while loading > > > the > > > new table or while resuming the device. If it fails while > > > resuming > > > the > > > device, the device can get stuck in the suspended state. To fix > > > this, > > > multipath needs to resume the device again so that it can > > > continue > > > using > > > the old table. > > > > > > Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com> > > > --- > > > libmultipath/devmapper.c | 19 ++- > > > libmultipath/devmapper.h | 1 + > > > 2 files changed, 19 insertions(+), 1 deletion(-) > > > > > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > > > index 2c4a13a..69b634b 100644 > > > --- a/libmultipath/devmapper.c > > > +++ b/libmultipath/devmapper.c > > > @@ -396,7 +396,13 @@ int dm_addmap_reload(struct multipath *mpp, > > > char > > > *params, int flush) > > > if (r) > > > r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, > > > !flush, > > > 1, udev_flags, 0); > > > - return r; > > > + if (r) > > > + return r; > > > + > > > + if (dm_is_suspended(mpp->alias)) > > > + dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, > > > !flush, > > > 1, > > > + udev_flags, 0); > > > + return 0; > > > } > > > > Why would the second DM_DEVICE_RESUME call succeed if the first one > > failed? > > Because if the first resume fails, device-mapper rolls back to the > original table. Ah. I didn't know that, and I'm likely to forget it again. Would you mind adding a short comment at that point in the code? Otherwise, ACK. Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH] multipath-tools:Prioritizer based on a time-delay algorithm
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;
> +}
see above.
> +
> +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;
> +}
> +
see above.
> +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, _interval, _num, )
> == 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, );
> +before = timeval_to_us();
> +
> +if (do_readsector0(pp->fd, timeout) == 2)
> +{
> +condlog(0, "%s: path down", pp->dev);
> +return -1;
> +}
> +
> +(void)clock_gettime(CLOCK_MONOTONIC, );
> +after = timeval_to_us();
> +
> +delay = after - before;
> +
> +min = (min <= delay) ? min : delay;
> +max = (max >= delay) ? max : delay;
> +
> +toldelay += delay;
> + }
> +
> +toldelay -= min + max;
Why are you doing this? If you want to discard "extreme" values, this
is probably not sufficient. If cons == 3, this will have the effect to
use a single measurement rather than an average, is that intended?
Btw, as you are doing statistics here anyway, you may want to calculate
the estimate of the standard deviation and warn the user if the
delay_interval is smaller than, say, 2 * standard deviation.
Please consider printing a message with the measured value at debug
level 3 or higher.
> +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 000..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 100LL
> +#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
should be "delayedpath" here?
> +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.
It might be good to give an example here, like this:
"If delay_interval=10ms, the paths will be grouped in priority groups
with path latency 0-10ms, 10-20ms, 20-30ms, etc."
> +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.
--
Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH 1/2] libmultipath: print.c: make sure lines are 0-terminated
Under certain conditions, the output of "multipath -ll" or
"multipathd show topology", or the multipathd logs, may
contain garbage characters. Fix that by making sure that
the strings are always properly zero-terminated.
Note 1: The way this is coded, the previously written
character is overwritten by the newline. That behavior is
unchanged by this patch. I didn't want to fuzz with the
carefully crafted print.c code more than necessary at
this point.
Note 2: The condition (c <= line + len - 1) is equivalent to
(TAIL >= 0). It should always hold the way ENDLINE is called
after PRINT and PAD in print.c, and is only added as an
additional safeguard, e.g. against future code changes.
---
libmultipath/print.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 7c2a1588..3a07fcf5 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -40,8 +40,14 @@ do { \
} while (0)
#define ENDLINE \
- if (c > line) \
- line[c - line - 1] = '\n'
+ if (c > line) { \
+ if (c <= line + len - 1) { \
+ *(c - 1) = '\n';\
+ *c = '\0'; \
+ } else \
+ line[len - 1] = '\0'; \
+ }
+
#define PRINT(var, size, format, args...) \
do { \
fwd = snprintf(var, size, format, ##args); \
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 0/2] Small multipath fixes
Two minor fixes that I found necessary recently. Both touch potentially bigger issues - the first one print.c, which might need some general overhaul at some time in the future, and the second one the build process of multipath tools, which is currently lacking an "autobuild" mechanism. My intention at was to keep the patches small and non-controversial, therefore I didn't delve into the big issues just yet. Martin Wilck (2): libmultipath: print.c: make sure lines are 0-terminated multipath-tools: fix compilation with gcc < 4.9 Makefile.inc | 15 ++- libmultipath/print.c | 10 -- 2 files changed, 22 insertions(+), 3 deletions(-) -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/2] multipath-tools: fix compilation with gcc < 4.9
gcc supports -fstack-protector-strong since v4.9 only. Add a "poor man's
autoconf" test to check whether the option is supported on the given
build system.
Fixes: 596f51f3
"multipath-tools: Replace -fstack-protector with -fstack-protector-strong"
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
Makefile.inc | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/Makefile.inc b/Makefile.inc
index 8361e6c4..3ef134f6 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -69,10 +69,23 @@ RM = rm -f
LN = ln -sf
INSTALL_PROGRAM= install
+# $(call TEST_CC_OPTION,option,fallback)
+# Test if the C compiler supports the option.
+# Evaluates to "option" if yes, and "fallback" otherwise.
+TEST_CC_OPTION = $(shell \
+ if $(CC) -o /dev/null -c "$(1)" -xc - <<<'int main(void){return 0;}'
&>/dev/null; \
+ then \
+ echo "$(1)"; \
+ else \
+ echo "$(2)"; \
+ fi)
+
+STACKPROT = $(call TEST_CC_OPTION,-fstack-protector-strong,-fstack-protector)
+
OPTFLAGS = -O2 -g -pipe -Wall -Wextra -Wformat=2 -Werror=implicit-int \
-Werror=implicit-function-declaration -Werror=format-security
\
-Wno-sign-compare -Wno-unused-parameter -Wno-clobbered \
- -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector-strong \
+ -Wp,-D_FORTIFY_SOURCE=2 $(STACKPROT) \
--param=ssp-buffer-size=4
CFLAGS = $(OPTFLAGS) -fPIC -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\"
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 09/12] kpartx: use partition UUID for non-DM devices
For dm devices, kpartx uses an UUID check at partition removal
to make sure it only deletes partitions it previously created.
Non-DM parent devices such as loop devices don't generally have
a UUID.
Introduce a "fake" UUID for these devices to make sure kpartx
deletes only devices it had created previously. Otherwise kpartx
might e.g. delete LVM LVs that are inside a device it is trying
to delete partitions for. It seems to be wiser to make sure the
user delete these manually before running "kpartx -d".
With the fake UUID in place, we can re-introduce the UUID check
for non-DM device that was removed in the earlier patch
"kpartx: dm_remove_partmaps: support non-dm devices".
This disables also deletion of partition mappings created
by earlier versions of kpartx. If kpartx has been updated after
partition mappings were created, the "-f" flag can be used
to force delting these partitions, too.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/devmapper.c | 2 +-
kpartx/kpartx.c| 34 ++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index b714ba4e..4ab58ce9 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -511,7 +511,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
/*
* skip if uuids don't match
*/
- if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
+ if (uuid && dm_compare_uuid(uuid, names->name)) {
if (rd->verbose)
printf("%s: is not a kpartx partition. Not
removing\n",
names->name);
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index ea728b42..587c3dfe 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -60,6 +60,31 @@ struct pt {
int ptct = 0;
int udev_sync = 0;
+/*
+ * UUID format for partitions created on non-DM devices
+ * ${UUID_PREFIX}devnode_${MAJOR}:${MINOR}_${NONDM_UUID_SUFFIX}"
+ * where ${UUID_PREFIX} is "part${PARTNO}-" (see devmapper.c).
+ *
+ * The suffix should be sufficiently unique to avoid incidental conflicts;
+ * the value below is a base64-encoded random number.
+ * The UUID format shouldn't be changed between kpartx releases.
+ */
+#define NONDM_UUID_PREFIX "devnode"
+#define NONDM_UUID_SUFFIX "Wh5pYvM"
+
+static char *
+nondm_create_uuid(dev_t devt)
+{
+#define NONDM_UUID_BUFLEN (34 + sizeof(NONDM_UUID_PREFIX) + \
+ sizeof(NONDM_UUID_SUFFIX))
+ static char uuid_buf[NONDM_UUID_BUFLEN];
+ snprintf(uuid_buf, sizeof(uuid_buf), "%s_%u:%u_%s",
+NONDM_UUID_PREFIX, major(devt), minor(devt),
+NONDM_UUID_SUFFIX);
+ uuid_buf[NONDM_UUID_BUFLEN-1] = '\0';
+ return uuid_buf;
+}
+
static void
addpts(char *t, ptreader f)
{
@@ -360,6 +385,15 @@ main(int argc, char **argv){
uuid = dm_mapuuid(mapname);
}
+ /*
+* We are called for a non-DM device.
+* Make up a fake UUID for the device, unless "-d -f" is given.
+* This allows deletion of partitions created with older kpartx
+* versions which didn't use the fake UUID during creation.
+*/
+ if (!uuid && !(what == DELETE && force_devmap))
+ uuid = nondm_create_uuid(buf.st_rdev);
+
if (!mapname)
mapname = device + off;
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 05/12] kpartx: dm_remove_partmaps: support non-dm devices
Commit 3d709241 improved partition removal, but broke support
for handling partitions of non-dm devices such as loop devices
or RAM disks.
This requires passing the dev_t of the device down to
do_foreach_partmap(). Doing so, there's little use in trying
to derive major/minor numbers from the "mapname" any more
(which actually is the device name for non-DM devices).
But we can use this to find out whether the device in question
is a dm device.
The test for UUID match doesn't work for non-DM devices (this
shall be improved in a later patch in this series).
The test for equal name of parent dev and partition is only valid
for dm devices. For non-dm devices such as loop, "/dev/mapper/loop0"
could, theoretically, be a partition of "/dev/loop0"
(and we don't want to rely on map names).
Fixes: 3d709241 "kpartx: sanitize delete partitions"
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/devmapper.c | 22 +++---
kpartx/devmapper.h | 2 +-
kpartx/kpartx.c| 3 ++-
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 8b125be5..b7a7390c 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include "devmapper.h"
#define _UUID_PREFIX "part"
@@ -438,6 +439,7 @@ struct remove_data {
static int
do_foreach_partmaps (const char * mapname, const char *uuid,
+dev_t devt,
int (*partmap_func)(const char *, void *),
void *data)
{
@@ -449,6 +451,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
int major, minor;
char dev_t[32];
int r = 1;
+ int is_dmdev = 1;
if (!(dmt = dm_task_create(DM_DEVICE_LIST)))
return 1;
@@ -466,15 +469,20 @@ do_foreach_partmaps (const char * mapname, const char
*uuid,
goto out;
}
- if (dm_devn(mapname, , ))
- goto out;
+ if (dm_devn(mapname, , ) ||
+ (major != major(devt) || minor != minor(devt)))
+ /*
+* The latter could happen if a dm device "/dev/mapper/loop0"
+* exits while kpartx is called on "/dev/loop0".
+*/
+ is_dmdev = 0;
- sprintf(dev_t, "%d:%d", major, minor);
+ sprintf(dev_t, "%d:%d", major(devt), minor(devt));
do {
/*
* skip our devmap
*/
- if (!strcmp(names->name, mapname))
+ if (is_dmdev && !strcmp(names->name, mapname))
goto next;
/*
@@ -502,7 +510,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
/*
* skip if uuids don't match
*/
- if (dm_compare_uuid(uuid, names->name)) {
+ if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
if (rd->verbose)
printf("%s: is not a kpartx partition. Not
removing\n",
names->name);
@@ -543,8 +551,8 @@ remove_partmap(const char *name, void *data)
}
int
-dm_remove_partmaps (char * mapname, char *uuid, int verbose)
+dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose)
{
struct remove_data rd = { verbose };
- return do_foreach_partmaps(mapname, uuid, remove_partmap, );
+ return do_foreach_partmaps(mapname, uuid, devt, remove_partmap, );
}
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index 9988ec0f..2e28c780 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -18,7 +18,7 @@ char * dm_mapname(int major, int minor);
dev_t dm_get_first_dep(char *devname);
char * dm_mapuuid(const char *mapname);
int dm_devn (const char * mapname, int *major, int *minor);
-int dm_remove_partmaps (char * mapname, char *uuid, int verbose);
+int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
int dm_no_partitions(char * mapname);
#endif /* _KPARTX_DEVMAPPER_H */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index e0c105f4..ea728b42 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -446,7 +446,8 @@ main(int argc, char **argv){
break;
case DELETE:
- r = dm_remove_partmaps(mapname, uuid, verbose);
+ r = dm_remove_partmaps(mapname, uuid, buf.st_rdev,
+ verbose);
if (loopdev) {
if (del_loop(loopdev)) {
if (verbose)
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 01/12] kpartx: test-kpartx: new unit test program
This is a unit test program for kpartx, in particular for deleting
partitions.
NOTE: This test program fails with current kpartx; full patch series
needed to make it work.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/test-kpartx | 254 +
1 file changed, 254 insertions(+)
create mode 100755 kpartx/test-kpartx
diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
new file mode 100755
index ..7c45cd14
--- /dev/null
+++ b/kpartx/test-kpartx
@@ -0,0 +1,254 @@
+#! /bin/bash
+
+# This is a unit test program for kpartx, in particular for deleting
partitions.
+#
+# The rationale is the following:
+#
+# 1) kpartx should delete all mappings it created beforehand.
+# 2) kpartx should handle partitions on dm devices and other devices
+# (e.g. loop devices) equally well.
+# 3) kpartx should only delete "partitions", which are single-target
+# linear mappings into a block device. Other maps should not be touched.
+# 4) kpartx should only delete mappings it created itself beforehand.
+# In particular, it shouldn't delete LVM LVs, even if they are fully
+# contained in the block device at hand and thus look like partitions
+# in the first place. (For historical compatibility reasons, we allow
+# such mappings to be deleted with the -f/--force flag).
+# 5) DM map names may be changed, thus kpartx shouldn't rely on them to
+# check whether a mapping is a partition of a particular device. It is
+# legal for a partition of /dev/loop0 to be named "loop0".
+
+# Note: This program tries hard to clean up, but if tests fail,
+# stale DM or loop devices may keep lurking around.
+
+# Set WORKDIR in environment to existing dir to for persistence
+# WARNING: exisiting files will be truncated.
+# If empty, test will be done in temporary dir
+: ${WORKDIR:=}
+# Set this environment variable to test an alternative kpartx executable
+: ${KPARTX:=}
+# Options to pass to kpartx always
+: ${KPARTX_OPTS:=-s}
+# Time to wait for device nodes to appear (microseconds)
+# Waiting is only needed if "s" is not in $KPARTX_OPTS
+: ${WAIT_US:=0}
+
+# IMPORTANT: The ERR trap is essential for this program to work correctly!
+trap 'LINE=$LINENO; trap - ERR; echo "== error in $BASH_COMMAND on line $LINE
==" >&2; exit 1' ERR
+trap 'cleanup' 0
+
+CLEANUP=:
+cleanup() {
+trap - ERR
+trap - 0
+if [[ $OK ]]; then
+ echo == all tests completed successfully == >&2
+else
+ echo == step $STEP failed == >&2
+fi
+eval "$CLEANUP" &>/dev/null
+}
+
+push_cleanup() {
+CLEANUP="$@;$CLEANUP"
+}
+
+pop_cleanup() {
+# CAUTION: simplistic
+CLEANUP=${CLEANUP#*;}
+}
+
+step() {
+STEP="$@"
+echo == Test step: $STEP == >&2
+}
+
+mk_partitions() {
+parted -s $1 mklabel msdos
+parted -s -- $1 mkpart prim ext2 1MiB -1s
+}
+
+step preparation
+
+[[ $UID -eq 0 ]]
+[[ $KPARTX ]] || {
+if [[ -x $PWD/kpartx/kpartx ]]; then
+ KPARTX=$PWD/kpartx/kpartx
+else
+ KPARTX=$(which kpartx)
+fi
+}
+[[ $KPARTX ]]
+
+FILE1=kpartx1
+FILE2=kpartx2
+FILE3=kpartx3
+SIZE=$((1024*1024*1024)) # use bytes as units here
+SECTSIZ=512
+OFFS=32# offset of linear mapping into dev, sectors
+VG=kpvg # volume group name
+LV=kplv # logical vol name
+LVMCONF='devices { filter = [ "a|/dev/loop.*|", r".*" ] }'
+
+OK=
+
+[[ $WORKDIR ]] || {
+WORKDIR=$(mktemp -d /tmp/kpartx-XX)
+push_cleanup 'rm -rf $WORKDIR'
+}
+
+push_cleanup "cd $PWD"
+cd "$WORKDIR"
+
+step "create loop devices"
+truncate -s $SIZE $FILE1
+truncate -s $SIZE $FILE2
+truncate -s $SIZE $FILE3
+
+LO1=$(losetup -f $FILE1 --show)
+push_cleanup 'losetup -d $LO1'
+LO2=$(losetup -f $FILE2 --show)
+push_cleanup 'losetup -d $LO2'
+LO3=$(losetup -f $FILE3 --show)
+push_cleanup 'losetup -d $LO3'
+
+[[ $LO1 && $LO2 && $LO3 && -b $LO1 && -b $LO2 && -b $LO3 ]]
+DEV1=$(stat -c "%t:%T" $LO1)
+DEV2=$(stat -c "%t:%T" $LO2)
+DEV3=$(stat -c "%t:%T" $LO3)
+
+usleep $WAIT_US
+
+step "create DM devices (spans)"
+# Create two linear mappings spanning two loopdevs.
+# One of them gets a pathological name colliding with
+# the loop device name.
+# These mappings must not be removed by kpartx.
+# They also serve as DM devices to test partition removal on those.
+
+TABLE="\
+0 $((SIZE/SECTSIZ-OFFS)) linear $DEV1 $OFFS
+$((SIZE/SECTSIZ-OFFS)) $((SIZE/SECTSIZ-OFFS)) linear $DEV2 $OFFS"
+
+SPAN1=kpt
+SPAN2=$(basename $LO2)
+dmsetup create $SPAN1 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN1'
+
+dmsetup create $SPAN2 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN2'
+
+usleep $WAIT_US
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+
+step "creat
[dm-devel] [PATCH v2 08/12] libmultipath: don't treat multi-linear mappings as partitions
dm_type is used in libmultipath only to check whether a mapping
is "linear", with the intention to test if it represents a
"partition". This test returns TRUE also for mappings with
multiple targets, the first of which happens to be a linear
mapping into the target device. This is questionable, it's
hard to assign a "type" to such maps anyway.
Fix this by returning an error for multi-target maps.
This is analogous to the patch
"kpartx: don't treat multi-linear mappings as partitions".
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/devmapper.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 5fb9d9ac..c19dcb62 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -572,7 +572,7 @@ out:
* returns:
*1 : match
*0 : no match
- * -1 : empty map
+ * -1 : empty map, or more than 1 target
*/
int dm_type(const char *name, char *type)
{
@@ -594,10 +594,11 @@ int dm_type(const char *name, char *type)
goto out;
/* Fetch 1st target */
- dm_get_next_target(dmt, NULL, , ,
- _type, );
-
- if (!target_type)
+ if (dm_get_next_target(dmt, NULL, , ,
+ _type, ) != NULL)
+ /* multiple targets */
+ r = -1;
+ else if (!target_type)
r = -1;
else if (!strcmp(target_type, type))
r = 1;
@@ -1185,9 +1186,9 @@ do_foreach_partmaps (const char * mapname,
do {
if (
/*
-* if devmap target is "linear"
+* if there is only a single "linear" target
*/
- (dm_type(names->name, TGT_PART) > 0) &&
+ (dm_type(names->name, TGT_PART) == 1) &&
/*
* and both uuid end with same suffix starting
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 02/12] kpartx: remove "no_partitions" support
The kernel does not support the "no_partitions" feature - remove it.
Distributions who want to keep support for this feature should
re-enable it with a distro-specific patch.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/devmapper.c | 29 -
kpartx/kpartx.c| 5 -
2 files changed, 34 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index cf6650c6..8aca9592 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -542,32 +542,3 @@ dm_remove_partmaps (char * mapname, char *uuid, int
verbose)
struct remove_data rd = { verbose };
return do_foreach_partmaps(mapname, uuid, remove_partmap, );
}
-
-#define FEATURE_NO_PART "no_partitions"
-
-int
-dm_no_partitions(char *mapname)
-{
- char params[PARAMS_SIZE], *ptr;
- int i, num_features;
-
- if (dm_get_map(mapname, params))
- return 0;
-
- ptr = params;
- num_features = strtoul(params, , 10);
- if ((ptr == params) || num_features == 0) {
- /* No features found, return success */
- return 0;
- }
- for (i = 0; (i < num_features); i++) {
- if (!ptr || ptr > params + strlen(params))
- break;
- /* Skip whitespaces */
- while(ptr && *ptr == ' ') ptr++;
- if (!strncmp(ptr, FEATURE_NO_PART, strlen(FEATURE_NO_PART)))
- return 1;
- ptr = strchr(ptr, ' ');
- }
- return 0;
-}
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 58e60ffe..e0c105f4 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -362,11 +362,6 @@ main(int argc, char **argv){
if (!mapname)
mapname = device + off;
- if (!force_devmap &&
-dm_no_partitions(mapname)) {
- /* Feature 'no_partitions' is set, return */
- return 0;
- }
if (delim == NULL) {
delim = malloc(DELIM_SIZE);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 06/12] kpartx: dm_devn: return error for non-existent device
For non-existent maps (ENXIO from ioctl()), dm_task_run and dm_task_get_info return success. We need to check info.exists. Signed-off-by: Martin Wilck <mwi...@suse.com> --- kpartx/devmapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c index b7a7390c..6cee120d 100644 --- a/kpartx/devmapper.c +++ b/kpartx/devmapper.c @@ -295,7 +295,7 @@ dm_devn (const char * mapname, int *major, int *minor) if (!dm_task_run(dmt)) goto out; - if (!dm_task_get_info(dmt, )) + if (!dm_task_get_info(dmt, ) || info.exists == 0) goto out; *major = info.major; -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 03/12] kpartx: remove is_loop_device
This function is not used any more.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/lopart.c | 31 ---
kpartx/lopart.h | 1 -
2 files changed, 32 deletions(-)
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 2eb3f631..44f0c277 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -62,37 +62,6 @@ xstrdup (const char *s)
return t;
}
-int is_loop_device(const char *device)
-{
- struct stat statbuf;
- int loopmajor;
-#if 1
- loopmajor = 7;
-#else
- FILE *procdev;
- char line[100], *cp;
-
- loopmajor = 0;
-
- if ((procdev = fopen(PROC_DEVICES, "r")) != NULL) {
-
- while (fgets (line, sizeof(line), procdev)) {
-
- if ((cp = strstr (line, " loop\n")) != NULL) {
- *cp='\0';
- loopmajor=atoi(line);
- break;
- }
- }
-
- fclose(procdev);
- }
-#endif
- return (loopmajor && stat(device, ) == 0 &&
- S_ISBLK(statbuf.st_mode) &&
- major(statbuf.st_rdev) == loopmajor);
-}
-
#define SIZE(a) (sizeof(a)/sizeof(a[0]))
char *find_loop_by_file(const char *filename)
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index a512353b..d3bad10a 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,6 +1,5 @@
extern int verbose;
extern int set_loop (const char *, const char *, int, int *);
extern int del_loop (const char *);
-extern int is_loop_device (const char *);
extern char * find_unused_loop_device (void);
extern char * find_loop_by_file (const char *);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 07/12] kpartx: don't treat multi-linear mappings as partitions
kpartx -d treats any map that has a "linear" mapping into a
device as first target as a partition of this device.
This is wrong, because linear mappings may consist of
several pieces, combining multiple devices into one
(LVM logical volumes are an example of such a
mapping). Partitions have to be single-target mappings.
Fix this by returning an error in dm_type() if a map with
multiple targets is encountered. The "type" of a map with
two or more targets is a difficult concept, anyway.
test case:
truncate -s 1G test0 test1
losetup /dev/loop0 test0
losetup /dev/loop1 test1
dmsetup create <
---
kpartx/devmapper.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 6cee120d..b714ba4e 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -392,10 +392,11 @@ dm_type(const char * name, char * type)
goto out;
/* Fetch 1st target */
- dm_get_next_target(dmt, NULL, , ,
- _type, );
-
- if (!target_type)
+ if (dm_get_next_target(dmt, NULL, , ,
+ _type, ) != NULL)
+ /* more than one target */
+ r = -1;
+ else if (!target_type)
r = -1;
else if (!strcmp(target_type, type))
r = 1;
@@ -500,7 +501,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
/*
* skip if devmap target is not "linear"
*/
- if (!dm_type(names->name, "linear")) {
+ if (dm_type(names->name, "linear") != 1) {
if (rd->verbose)
printf("%s: is not a linear target. Not
removing\n",
names->name);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 04/12] kpartx: relax and improve UUID check in dm_compare_uuid
It is wrong to assume that UUIDs of parent devices always contain
"mpath". Fix that, and make the check for the kpartx-specific prefix
"part%d-" stricter and more explicit.
Moreover, avoid duplication of string constants and properly express
the dependencies of the various constants.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/devmapper.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 8aca9592..8b125be5 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -10,8 +10,10 @@
#include
#include "devmapper.h"
-#define UUID_PREFIX "part%d-"
-#define MAX_PREFIX_LEN 8
+#define _UUID_PREFIX "part"
+#define UUID_PREFIX _UUID_PREFIX "%d-"
+#define _UUID_PREFIX_LEN (sizeof(_UUID_PREFIX) - 1)
+#define MAX_PREFIX_LEN (_UUID_PREFIX_LEN + 4)
#define PARAMS_SIZE 1024
int dm_prereq(char * str, int x, int y, int z)
@@ -417,9 +419,13 @@ dm_compare_uuid(const char *mapuuid, const char *partname)
if (!partuuid)
return 1;
- if (!strncmp(partuuid, "part", 4)) {
- char *p = strstr(partuuid, "mpath-");
- if (p && !strcmp(mapuuid, p))
+ if (!strncmp(partuuid, _UUID_PREFIX, _UUID_PREFIX_LEN)) {
+ char *p = partuuid + _UUID_PREFIX_LEN;
+ /* skip partition number */
+ while (isdigit(*p))
+ p++;
+ if (p != partuuid + _UUID_PREFIX_LEN && *p == '-' &&
+ !strcmp(mapuuid, p + 1))
r = 0;
}
free(partuuid);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 00/12] fixes for kpartx -d
Working on a bug report about kpartx not properly removing partitions for loop devices, I realized a number of glitches and improperly handled corner cases in the kpartx code for deleting partitions. Some mappings are not deleted although they should be, and others are deleted although that is clearly wrong. This patch series fixes the issues I found. The series starts with a test program demonstrating the problems. The program succeeds only after all patches of this series are applied. Here is my summary of what I think how kpartx should behave: 1) kpartx should delete all mappings it created beforehand. 2) kpartx should handle partitions on dm devices and other devices (e.g. loop devices) equally well. 3) kpartx should only delete "partitions", which are single-target linear mappings into a block device. Other maps should not be touched. 4) kpartx should only delete mappings it created itself beforehand. In particular, it shouldn't delete LVM LVs, even if they are fully contained in the block device at hand and thus look like partitions in the first place. (For historical compatibility reasons, allow such mappings to be deleted with the -f/--force flag). 5) DM map names may be changed, thus kpartx shouldn't rely on them to check whether a mapping is a partition of a particular device. It is legal for a partition of /dev/loop0 to be named "loop0". One patch has an obvious libdevmapper equivalent and is therefore included (08/12) although this series is otherwise focused only on kpartx. Patch 04/12 would also have a libdevmapper equivalent, but I haven't included it because it would conflict with Ben's previously posted patch "libmultipath: fix partition detection". The patches are based on Christophe's tree; Christophe, if you prefer, I can rebase them on top of Ben's late submissions. Feedback is welcome. Changes wrt v1: - Test program (01/12): improved cleanup, and used "kpartx -s" rather than waiting. - At Ben's suggestion, removed "no_partitions" support rather than fixing it. - Previous 04/12 split into two patches (04+05/12), improving and separating out the part that would similarly apply to libmultipath (see above). - New UUID format in patch 09/12 since the previous one wasn't well-received; the "-kpartx-" part was superfluous, as partition UUIDs start with "part%s-" anyway. - Added the trivial fix 12/12. Martin Wilck (12): kpartx: test-kpartx: new unit test program kpartx: remove "no_partitions" support kpartx: remove is_loop_device kpartx: relax and improve UUID check in dm_compare_uuid kpartx: dm_remove_partmaps: support non-dm devices kpartx: dm_devn: return error for non-existent device kpartx: don't treat multi-linear mappings as partitions libmultipath: don't treat multi-linear mappings as partitions kpartx: use partition UUID for non-DM devices kpartx: use absolute path for regular files kpartx: find_loop_by_file: use sysfs kpartx: include sys/sysmacros.h kpartx/devmapper.c | 80 ++- kpartx/devmapper.h | 2 +- kpartx/kpartx.c | 50 -- kpartx/lopart.c | 75 ++ kpartx/lopart.h | 1 - kpartx/sysmacros.h | 9 -- kpartx/test-kpartx | 254 +++ libmultipath/devmapper.c | 15 +-- 8 files changed, 371 insertions(+), 115 deletions(-) delete mode 100644 kpartx/sysmacros.h create mode 100755 kpartx/test-kpartx -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 11/12] kpartx: find_loop_by_file: use sysfs
Rather then searching through all of /dev, look up loop devices
in /sys/devices/virtual/block. This is cleaner and more robust
(/dev/loop$Xp$Y symlinks may confuse kpartx).
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/lopart.c | 42 +++---
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 44f0c277..0521d7dc 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -68,25 +68,46 @@ char *find_loop_by_file(const char *filename)
{
DIR *dir;
struct dirent *dent;
- char dev[64], *found = NULL;
+ char dev[64], *found = NULL, *p;
int fd;
struct stat statbuf;
struct loop_info loopinfo;
+ const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
+ char path[PATH_MAX];
- dir = opendir("/dev");
+ dir = opendir(VIRT_BLOCK);
if (!dir)
return NULL;
while ((dent = readdir(dir)) != NULL) {
if (strncmp(dent->d_name,"loop",4))
continue;
- if (!strcmp(dent->d_name, "loop-control"))
- continue;
- sprintf(dev, "/dev/%s", dent->d_name);
- fd = open (dev, O_RDONLY);
+ if (snprintf(path, PATH_MAX, "%s/%s/dev", VIRT_BLOCK,
+dent->d_name) >= PATH_MAX)
+ continue;
+
+ fd = open(path, O_RDONLY);
if (fd < 0)
- break;
+ continue;
+
+ if (read(fd, dev, sizeof(dev)) <= 0) {
+ close(fd);
+ continue;
+ }
+
+ close(fd);
+
+ dev[sizeof(dev)-1] = '\0';
+ p = strchr(dev, '\n');
+ if (p != NULL)
+ *p = '\0';
+ if (snprintf(path, PATH_MAX, "/dev/block/%s", dev) >= PATH_MAX)
+ continue;
+
+ fd = open (path, O_RDONLY);
+ if (fd < 0)
+ continue;
if (fstat (fd, ) != 0 ||
!S_ISBLK(statbuf.st_mode)) {
@@ -99,13 +120,12 @@ char *find_loop_by_file(const char *filename)
continue;
}
+ close (fd);
+
if (0 == strcmp(filename, loopinfo.lo_name)) {
- close (fd);
- found = xstrdup(dev);
+ found = realpath(path, NULL);
break;
}
-
- close (fd);
}
closedir(dir);
return found;
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 10/12] kpartx: use absolute path for regular files
kpartx supports being called for a regular file, in which
case it assumes that it should set up a loop device or use
an existing one. Because the loopinfo.lo_name field contains
an absolute path, matching with existing loop devices fails
for relative paths. Matching by basename only would be unreliable.
Therefore, convert relative to absolute path for regular files.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/kpartx.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 587c3dfe..c76b5b46 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -355,7 +355,13 @@ main(int argc, char **argv){
if (S_ISREG (buf.st_mode)) {
/* already looped file ? */
- loopdev = find_loop_by_file(device);
+ char rpath[PATH_MAX];
+ if (realpath(device, rpath) == NULL) {
+ fprintf(stderr, "Error: %s: %s\n", device,
+ strerror(errno));
+ exit (1);
+ }
+ loopdev = find_loop_by_file(rpath);
if (!loopdev && what == DELETE)
exit (0);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 12/12] kpartx: include sys/sysmacros.h
Finish the work started in ea436715. Fixes a compilation warning. Signed-off-by: Martin Wilck <mwi...@suse.com> --- kpartx/lopart.c| 2 +- kpartx/sysmacros.h | 9 - 2 files changed, 1 insertion(+), 10 deletions(-) delete mode 100644 kpartx/sysmacros.h diff --git a/kpartx/lopart.c b/kpartx/lopart.c index 0521d7dc..70054459 100644 --- a/kpartx/lopart.c +++ b/kpartx/lopart.c @@ -27,7 +27,7 @@ #include #include #include -#include "sysmacros.h" +#include #include #include "lopart.h" diff --git a/kpartx/sysmacros.h b/kpartx/sysmacros.h deleted file mode 100644 index 171b33d4.. --- a/kpartx/sysmacros.h +++ /dev/null @@ -1,9 +0,0 @@ -/* versions to be used with > 16-bit dev_t - leave unused for now */ - -#ifndef major -#define major(dev) ((dev) >> 8) -#endif - -#ifndef minor -#define minor(dev) ((dev) & 0xff) -#endif -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 1/3] dm: a basic support for using the select or poll function
On Thu, 2017-05-11 at 11:10 -0700, Andy Grover wrote: > > I think getting one readiness notification for multiple events should > be ok. > > But, the semantics of poll(2) are level-triggered, meaning if a fd > is > ready then it will stay ready through multiple invocations of poll() > until something is done to satisfy it (e.g. for a TCP socket, > read()ing > all the available data). Therefore we can't make it "edge-triggered" > by > auto-clearing - it has to be a separate action. Hm, perhaps. Here is an idea: the best way to avoid the race mentioned by Mike might be to set priv->global_event_nr to the highest event nr reported by the DM_LIST_DEVICES ioctl when this ioctl returns. DM_LIST_DEVICES would then represent your "separate action", which would fit quite well, and the DM_DEV_ARM_POLL ioctl wouldn't be needed. It would be guaranteed that if any event had occured after the previous DM_LIST_DEVICES, whether or not that had been before or after the poll() call, userspace would notice. Cheers, Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 4/5] kpartx: default to running in sync mode
On Thu, 2017-05-11 at 12:46 -0500, Benjamin Marzinski wrote: > On Thu, May 11, 2017 at 02:42:08PM +0200, Martin Wilck wrote: > > Hi Ben, > > > > On Mon, 2017-04-24 at 17:39 -0500, Benjamin Marzinski wrote: > > > > > When users run kpartx, they would naturally assume that when it > > > completes, the devices have been created. However, kpartx runs in > > > async > > > mode by default. This seems like it is likely to trip up users. > > > > Is this just likely, or do you have evidence that it really did > > irritate users? You introduced "sync mode", together with the "-s" > > flag, yourself in 2010, and unless I'm mistaken, kpartx operated in > > "async" mode before that, too, because there was no "sync" mode. > > Yes. I made this patch after the second time a customer complained > that > kpartx wasn't working correctly, when the real issue was that their > scripts were assuming that after the kpartx command completed, those > partition device nodes would be present. Obviously, 2 isn't a huge > number, but it certainly has caused confusion in some cases. 2 > "likely", that makes a difference. > > I'm not too fond of the idea to change a default which has been > > established for such a long time just because user mistakes are > > "likely". For one thing, such changes are difficult to document in > > a > > way that doesn't cause confusion. Also, if someone writes a script > > in > > which she wants kpartx to run asynchronously, it will be difficult > > to > > do so in a manner which is portable between distributions if some > > distributions include this patch and some don't. > > I see your point, but assuming that I am correct that most users do > assume that kpartx runs synchronously, I feel like we shouldn't keep > a > bad default forever, simply because it's always been that way. lvm > and > dmsetup wait for udev by default (well, on lvm IIRC it is a compile- > time > setting, but here is some SUSE documentation showing that it uses > udev > synchronization by default > https://www.suse.com/documentation/sles-12/stor_admin/data/sec_lvm_cl > i.html) > kpartx does seem to be the odd one out. > > > As an alternative, we might simply warn about the default > > asynchronous > > behavior in a prominent place in the man page. > > We could, and if you are opposed to this patch, that would probably > help > cut down any confusion (assuming that when customers see things go > wrong > they read the documentation before calling support). I still don't like it too much. The scripting issue is my biggest concern - users need at least a reliable way to test whether or not "-n" is supported by kpartx on a given system. But I, alone, am not in a position to veto this. Does anyone else have an opinion on this change? Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 3/6] libmultipath: refactor calls to get dm device info
On Tue, 2017-05-09 at 11:57 -0500, Benjamin Marzinski wrote:
> Multipath has a number of functions that all do basically the same
> thing to get members of the dm_info structure. This just refactors
> them
> to use common code to do it.
This is nice, but it might be even better to simply store all relevant
information from DM_DEVICE_INFO in the mpp structure in the first
place, avoiding the need to do this ioctl multiple times to get various
pieces of information.
That can be done in a separate patch, of course.
Reviewed-by: Martin Wilck <mwi...@suse.com>
Regards
Martin
>
> Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com>
> ---
> libmultipath/devmapper.c | 171 +---
> --
> libmultipath/devmapper.h | 3 +-
> multipathd/cli_handlers.c | 24 ++-
> 3 files changed, 42 insertions(+), 156 deletions(-)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 1c6b87e..2c4a13a 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -399,16 +399,16 @@ int dm_addmap_reload(struct multipath *mpp,
> char *params, int flush)
> return r;
> }
>
> -int dm_map_present(const char * str)
> +static int
> +do_get_info(const char *name, struct dm_info *info)
> {
> - int r = 0;
> + int r = -1;
> struct dm_task *dmt;
> - struct dm_info info;
>
> if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> - return 0;
> + return r;
>
> - if (!dm_task_set_name(dmt, str))
> + if (!dm_task_set_name(dmt, name))
> goto out;
>
> dm_task_no_open_count(dmt);
> @@ -416,16 +416,25 @@ int dm_map_present(const char * str)
> if (!dm_task_run(dmt))
> goto out;
>
> - if (!dm_task_get_info(dmt, ))
> + if (!dm_task_get_info(dmt, info))
> goto out;
>
> - if (info.exists)
> - r = 1;
> + if (!info->exists)
> + goto out;
> +
> + r = 0;
> out:
> dm_task_destroy(dmt);
> return r;
> }
>
> +int dm_map_present(const char * str)
> +{
> + struct dm_info info;
> +
> + return (do_get_info(str, ) == 0);
> +}
> +
> int dm_get_map(const char *name, unsigned long long *size, char
> *outparams)
> {
> int r = 1;
> @@ -646,29 +655,15 @@ out:
> static int
> dm_dev_t (const char * mapname, char * dev_t, int len)
> {
> - int r = 1;
> - struct dm_task *dmt;
> struct dm_info info;
>
> - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> - return 0;
> -
> - if (!dm_task_set_name(dmt, mapname))
> - goto out;
> -
> - if (!dm_task_run(dmt))
> - goto out;
> -
> - if (!dm_task_get_info(dmt, ) || !info.exists)
> - goto out;
> + if (do_get_info(mapname, ) != 0)
> + return 1;
>
> if (snprintf(dev_t, len, "%i:%i", info.major, info.minor) >
> len)
> - goto out;
> + return 1;
>
> - r = 0;
> -out:
> - dm_task_destroy(dmt);
> - return r;
> + return 0;
> }
>
> int
> @@ -700,59 +695,16 @@ out:
> }
>
> int
> -dm_get_major (char * mapname)
> +dm_get_major_minor(const char *name, int *major, int *minor)
> {
> - int r = -1;
> - struct dm_task *dmt;
> struct dm_info info;
>
> - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> - return 0;
> -
> - if (!dm_task_set_name(dmt, mapname))
> - goto out;
> -
> - if (!dm_task_run(dmt))
> - goto out;
> -
> - if (!dm_task_get_info(dmt, ))
> - goto out;
> -
> - if (!info.exists)
> - goto out;
> -
> - r = info.major;
> -out:
> - dm_task_destroy(dmt);
> - return r;
> -}
> -
> -int
> -dm_get_minor (char * mapname)
> -{
> - int r = -1;
> - struct dm_task *dmt;
> - struct dm_info info;
> -
> - if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> - return 0;
> -
> - if (!dm_task_set_name(dmt, mapname))
> - goto out;
> -
> - if (!dm_task_run(dmt))
> - goto out;
> -
> - if (!dm_task_get_info(dmt, ))
> - goto out;
> -
> - if (!info.exists)
> - goto out;
> + if (do_get_info(name, ) != 0)
> + return -1;
>
> - r = info.minor;
> -out:
> - dm_task_destroy(dmt);
> - return r;
> + *major = info.major;
> + *minor = info.minor;
> + return 0;
> }
>
> static int
>
Re: [dm-devel] [PATCH 5/6] kpartx: fix device checks
On Tue, 2017-05-09 at 11:57 -0500, Benjamin Marzinski wrote: > There are a number of issues in the the kpartx device checking code. > First, it accepts files that are neither regular files or a block > device > nodes (you can run kpartx on character devices or directories, and it > will treat them as block devices). When trying to figure out the > basename of a device, the code returns garbage if the path doesn't > include a '/'. Finally, the set_delimiter code can access memory > outside > of the string if an empty string is passed in. > > Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com> Reviewed-by: Martin Wilck <mwi...@suse.com> -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 1/3] dm: a basic support for using the select or poll function
On Thu, 2017-05-11 at 09:21 -0400, Mike Snitzer wrote: > On Thu, May 11 2017 at 5:43am -0400, > Martin Wilck <mwi...@suse.com> wrote: > > > On Thu, 2017-05-11 at 11:39 +0200, Martin Wilck wrote: > > > On Tue, 2017-05-09 at 12:10 -0700, Andy Grover wrote: > > > > From: Mikulas Patocka <mpato...@redhat.com> > > > > > > > > This is the very simple patch for polling on the > > > > /dev/mapper/control > > > > device. The select or poll function waits until any event > > > > happens > > > > on > > > > any > > > > dm device since opening the /dev/mapper/control device. When > > > > select > > > > or > > > > poll returns the device as readable, we must close and reopen > > > > the > > > > device > > > > to wait for new dm events. > > > > > > Why have you done it that way? Couldn't you just save the > > > dm_global_event_nr at the time poll() is called? > > > > I should have read your patch 2/3 before posting ... but I'm still > > missing why the counter can't simply be set at poll() time. > > If you did that then you would have a race where: > > 1) userspace has recorded events prior to poll() > 2) an event triggers an increment of dm_global_event_nr before > userspace > calls poll() > 3) then userspace calls poll() -- only to find that after poll() > returns > multiple events have occurred. > > Which implies missed handling of events. I see - but I don't see yet how the ioctl approach (or the close()/open() based one, for that matter) would avoid this race. 1) application processes event N 2) event N+1 arrives in the kernel 3) user space issues ioctl or close()/open() sequence, N+1 is recorded in priv->global_event_nr 4) user space runs poll() 5) event N+2 arrives 4 weeks later and poll returns ... meaning that event N+1 has been left unprocessed for 4 weeks. Or what I am missing? AFAICS, the only way for user space to make sure it misses no events would be passing the number of the last processed event down the kernel in the ioctl call (and the kernel would use that value, rather than its internal counter, for initializing priv->global_event_nr). Regards Martin > But if I'm wrong I'm sure Andy or Mikulas will correct me. > > Thanks, > Mike > -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH v4 0/1] multipath-tools: Prioritizer based on a latency algorithm
Hello Yang, > > Actually, you're not alone here; several other storage array setups > > suffer from the same problem. > > > > Eg if you have a site-failover setup with two storage arrays at > > different locations the problem is more-or-less the same; > > both arrays potentially will be displaying identical priority > > information, despite one array being remote. > > > > It's up to the value set of the argument "latency_interval".For > example, > If latency_interval=10ms, the paths will be grouped in priority > groups > with path latency 0-10ms, 10-20ms, 20-30ms, etc. If the argument > "latency_interval" is set to appropriate value and the distance > between > two arrays is not enough far, two priorities may be the same, But > it's > OK, because between two arrays, the gap of average path latency is > very > small and tolerable. I wonder if it would make sense to use "logarithmically" scaled latency intervals here. It wouldn't make a large difference whether the latency is 1ms or 2ms, but if we have paths where the latencies differ by order of magnitude, it would be very important to make a distinction. With the current linear intervals, it would be hard to get this right (us interval size would result in too many intervals, and sec interval size wouldn't allow a distinction between us and ms latencies). By using logarithmic scale, you could setup the latency intevals e.g like this: < 10us, 10us-100us, 100us-1ms, 1ms-10ms, 10ms-100ms, > 100ms IMO that would be better, in particular if the latencies differ strongly between paths. Regards Martin -- Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH v2 6/8] multipath.conf.5: document no_path_retry vs. queue_if_no_path
Clarify the documentation about option precedence. Signed-off-by: Martin Wilck <mwi...@suse.com> --- multipath/multipath.conf.5 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index f04ff194..6959ba5c 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -365,7 +365,9 @@ Possible values for the feature list are: .\" XXX .I queue_if_no_path (Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. -Identical to the \fIno_path_retry\fR with \fIqueue\fR value. See KNOWN ISSUES. +Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this +feature and \fIno_path_retry\fR are set, the latter value takes +precedence. See KNOWN ISSUES. .TP .I no_partitions Disable automatic partitions generation via kpartx. -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 7/8] multipath.conf.5: Remove ??? and other minor fixes
Remove the FIXME markers by filling in missing content, and make some other minor fixes. Signed-off-by: Martin Wilck <mwi...@suse.com> --- multipath/multipath.conf.5 | 48 +- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 6959ba5c..d8856577 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -258,7 +258,7 @@ Return a constant priority of \fI1\fR. .I sysfs Use the sysfs attributes \fIaccess_state\fR and \fIpreferred_path\fR to generate the path priority. This prioritizer accepts the optional prio_arg -\fIexclusive_pref_bit\fR +\fIexclusive_pref_bit\fR. .TP .I emc (Hardware-dependent) @@ -296,14 +296,19 @@ Generate the path priority based on the regular expression and the priority provided as argument. Requires prio_args keyword. .TP .I datacore -.\" XXX -???. Requires prio_args keyword. +(Hardware-dependent) +Generate the path priority for some Datacore storage arrays. Requires prio_args +keyword. .TP .I iet -.\" XXX -???. Requires prio_args keyword. -.TP -The default is: \fBconst\fR +(iSCSI only) +Generate path priority for iSCSI targets based on IP address. Requires +prio_args keyword. +.PP +The default depends on the \fBdetect_prio\fR setting: If \fBdetect_prio\fR is +\fByes\fR (default), the default priority algorithm is \fBsysfs\fR (except for +NetAPP E-Series, where it is \fBalua\fR). If \fBdetect_prio\fR is +\fBno\fR, the default priority algorithm is \fBconst\fR. .RE . . @@ -344,12 +349,12 @@ If \fIexclusive_pref_bit\fR is set, paths with the \fIpreferred path\fR bit set will always be in their own path group. .TP .I datacore -.\" XXX -\fIpreferredsds\fR ???. +\fIpreferredsds\fR (required) denotes the preferred "SDS name" for datacore +arrays. \fItimeout\fR (optional) is the timeout for the INQUIRY, in ms. .TP .I iet -.\" XXX -\fIpreferredip\fR ???. +\fIpreferredip=...\fR (required) denotes the preferred IP address (in dotted decimal +notation) for iSCSI targets. .TP The default is: \fB\fR .RE @@ -364,29 +369,28 @@ Possible values for the feature list are: .TP 12 .\" XXX .I queue_if_no_path -(Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. +(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active. Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this feature and \fIno_path_retry\fR are set, the latter value takes precedence. See KNOWN ISSUES. .TP -.I no_partitions -Disable automatic partitions generation via kpartx. -.TP .\" XXX .I pg_init_retries -(Since ??? kernel) Number of times to retry pg_init, it must be between 1 and 50. +(Since kernel 2.6.24) Number of times to retry pg_init, it must be between 1 and 50. .TP .\" XXX .I pg_init_delay_msecs -(Since ??? kernel) Number of msecs before pg_init retry, it must be between 0 and 6. +(Since kernel 2.6.38) Number of msecs before pg_init retry, it must be between 0 and 6. .TP .\" XXX .I queue_mode -(Since ??? kernel) Select the the queue_mode per multipath device. -Where can be \fIbio\fR, \fIrq\fR or \fImq\fR. Which corresponds to -bio-based, request_fn rq-based, and blk-mq rq-based respectively. -.TP -The default is: \fB0\fR +(Since kernel 4.8) Select the the queueing mode per multipath device. + can be \fIbio\fR, \fIrq\fR or \fImq\f, which corresponds to +bio-based, request-based, and block-multiqueue (blk-mq) request-based, +respectively. + +The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR. It is +\fImq\fR if the latter is set, and \fIrq\fR otherwise. .RE . . -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 4/8] libmultipath: merge_hwe: fix queue_if_no_path logic
The logic applied here should match the logic in select_features().
This is achieved by calling reconcile_features_with_options().
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/config.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 61bbba91..4f1ecee3 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -25,6 +25,7 @@
#include "prio.h"
#include "devmapper.h"
#include "mpath_cmd.h"
+#include "propsel.h"
static int
hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
@@ -318,6 +319,8 @@ set_param_str(char * str)
static int
merge_hwe (struct hwentry * dst, struct hwentry * src)
{
+ int id_len;
+ char *id;
merge_str(vendor);
merge_str(product);
merge_str(revision);
@@ -353,15 +356,14 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
merge_num(san_path_err_forget_rate);
merge_num(san_path_err_recovery_time);
- /*
-* Make sure features is consistent with
-* no_path_retry
-*/
- if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
- remove_feature(>features, "queue_if_no_path");
- else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
- add_feature(>features, "queue_if_no_path");
-
+ id_len = strlen(dst->vendor) + strlen(dst->product) + 2;
+ id = MALLOC(id_len);
+ if (id != NULL)
+ snprintf(id, id_len, "%s/%s", dst->vendor, dst->product);
+ reconcile_features_with_options(id, >features,
+ >no_path_retry,
+ >retain_hwhandler);
+ FREE(id);
return 0;
}
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 5/8] libmultipath: assemble_map: fix queue_if_no_path logic
It is wrong to remove the queue_if_no_path feature if no_path_retry
is unset. Rather, in this case the feature should neither be added
nor removed.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/dmparser.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 469e60d2..8d0c7af1 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -74,13 +74,12 @@ assemble_map (struct multipath * mp, char * params, int len)
* We have to set 'queue_if_no_path' here even
* to avoid path failures during map reload.
*/
- if (mp->no_path_retry == NO_PATH_RETRY_UNDEF ||
- mp->no_path_retry == NO_PATH_RETRY_FAIL) {
+ if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
/* remove queue_if_no_path settings */
condlog(3, "%s: remove queue_if_no_path from '%s'",
mp->alias, mp->features);
remove_feature(, no_path_retry);
- } else {
+ } else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
add_feature(, no_path_retry);
}
if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 2/8] libmultipath: add/remove_feature: use const char* for feature
Change the argument type for the feature to add or remove to
const char*, making it possible to pass const strings without
warnings.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/structs.c | 30 --
libmultipath/structs.h | 4 ++--
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e225f8b4..28704676 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -513,10 +513,11 @@ void setup_feature(struct multipath *mpp, char *feature)
}
}
-int add_feature(char **f, char *n)
+int add_feature(char **f, const char *n)
{
int c = 0, d, l = 0;
char *e, *p, *t;
+ const char *q;
if (!f)
return 1;
@@ -554,14 +555,14 @@ int add_feature(char **f, char *n)
if ((c % 10) == 9)
l++;
c++;
- p = n;
- while (*p != '\0') {
- if (*p == ' ' && p[1] != '\0' && p[1] != ' ') {
+ q = n;
+ while (*q != '\0') {
+ if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
if ((c % 10) == 9)
l++;
c++;
}
- p++;
+ q++;
}
t = MALLOC(l + 1);
@@ -601,10 +602,11 @@ int add_feature(char **f, char *n)
return 0;
}
-int remove_feature(char **f, char *o)
+int remove_feature(char **f, const char *o)
{
int c = 0, d, l;
char *e, *p, *n;
+ const char *q;
if (!f || !*f)
return 1;
@@ -630,18 +632,18 @@ int remove_feature(char **f, char *o)
/* Just spaces, return */
if (*o == '\0')
return 0;
- e = o + strlen(o);
- while (*e == ' ')
- e--;
- d = (int)(e - o);
+ q = o + strlen(o);
+ while (*q == ' ')
+ q--;
+ d = (int)(q - o);
/* Update feature count */
c--;
- p = o;
- while (p[0] != '\0') {
- if (p[0] == ' ' && p[1] != ' ' && p[1] != '\0')
+ q = o;
+ while (q[0] != '\0') {
+ if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0')
c--;
- p++;
+ q++;
}
/* Quick exit if all features have been removed */
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 01e031ad..8ea984d9 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -369,8 +369,8 @@ int pathcountgr (struct pathgroup *, int);
int pathcount (struct multipath *, int);
int pathcmp (struct pathgroup *, struct pathgroup *);
void setup_feature(struct multipath *, char *);
-int add_feature (char **, char *);
-int remove_feature (char **, char *);
+int add_feature (char **, const char *);
+int remove_feature (char **, const char *);
extern char sysfs_path[PATH_SIZE];
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic
This patch set attempts to sanitize the logic used for consistently handling options that can be set both via the "features" string and explicit multipath.conf options. This is most prominently "no_path_retry" vs. "queue_if_no_path", but also "retain_attached_hw_handler" vs. the feature of the same name. The logic this patch set follows is: - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored (this is the case nowadays for almost all "real" storage arrays, thanks to hwtable). - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry queue". ... likewise for "retain_attached_hw_handler". In the long run, we should get rid of the "features" settings duplicating configuration options altogether; the patch set prepares this by printing warning messages. The logic is implemented in the new function reconcile_features_with_options, which is called from both select_features() and merge_hwe(). In setup_map(), we need to call select_features() after select_no_path_retry() to make this work. The actual feature setting for device-mapper is made in assemble_map(), the patch set also fixes the logic there. The patch set documents the behavior in the man page, and adds some more man page fixes. Finally, by skipping superfluous default initializations in load_config(), the log messages for the respective config settings become more appropriate. Review and comments are highly welcome. Changes wrt v1: - Suggestions from Ben Marzinski: * Made sure "multipathd show config" still works (1/8). * Fixed logging for default setting of "max_sectors" (1/8) * Consistent internal treatment of mp->features (3/8, 4/8) * Fixed whitespace error (6/8) * Added deprecated warnings (8/8) - Made sure the same logic is used in propsel.c and config.c by calling the same function (3/8, 4/8) - Added deprecated warnings (8/8) Martin Wilck (8): libmultipath: load_config: skip setting unnecessary defaults libmultipath: add/remove_feature: use const char* for feature libmultipath: clarify option conflicts for "features" libmultipath: merge_hwe: fix queue_if_no_path logic libmultipath: assemble_map: fix queue_if_no_path logic multipath.conf.5: document no_path_retry vs. queue_if_no_path multipath.conf.5: Remove ??? and other minor fixes libmultipath: add deprecated warning for some features settings libmultipath/config.c | 36 +++-- libmultipath/configure.c | 4 +-- libmultipath/dict.c| 11 --- libmultipath/dmparser.c| 5 ++- libmultipath/propsel.c | 79 +- libmultipath/propsel.h | 3 ++ libmultipath/structs.c | 30 ++ libmultipath/structs.h | 4 +-- multipath/multipath.conf.5 | 52 -- 9 files changed, 136 insertions(+), 88 deletions(-) -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 8/8] libmultipath: add deprecated warning for some features settings
The device-mapper features "queue_if_no_path" and
"retain_attached_hw_handler" should be set via the configuration
keywords "no_path_retry" and "retain_attached_hw_handler",
respectively, not via "features".
Print a warning if these "features" settings are encountered.
So far these "features" settings will only be ignored if the
respective other keyword is set, so in particular
'features "1 queue_if_no_path"' will still work as expected, but
cause the warning to be printed.
We should consider ignoring these completely in a future version.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/propsel.c | 6 ++
multipath/multipath.conf.5 | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index ccd067f5..f11052f2 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -290,6 +290,9 @@ void reconcile_features_with_options(const char *id, char
**features, int* no_pa
* it's translated into "no_path_retry queue" here.
*/
if (strstr(*features, q_i_n_p)) {
+ condlog(0, "%s: option 'features \"1 %s\"' is deprecated, "
+ "please use 'no_path_retry queue' instead",
+ id, q_i_n_p);
if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
*no_path_retry = NO_PATH_RETRY_QUEUE;
print_no_path_retry(buff, sizeof(buff),
@@ -307,6 +310,9 @@ void reconcile_features_with_options(const char *id, char
**features, int* no_pa
remove_feature(features, q_i_n_p);
}
if (strstr(*features, r_a_h_h)) {
+ condlog(0, "%s: option 'features \"1 %s\"' is deprecated, "
+ "please use '%s yes' instead",
+ id, r_a_h_h, r_a_h_h);
if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
condlog(3, "%s: %s = on (inherited setting from feature
'%s')",
id, r_a_h_h, r_a_h_h);
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index d8856577..bacd5e30 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -369,7 +369,7 @@ Possible values for the feature list are:
.TP 12
.\" XXX
.I queue_if_no_path
-(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
+(Deprecated, superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this
feature and \fIno_path_retry\fR are set, the latter value takes
precedence. See KNOWN ISSUES.
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic
Hello Ben, On Tue, 2017-06-20 at 13:44 -0500, Benjamin Marzinski wrote: > > > Review and comments are highly welcome. > > ACK for the set Thanks a lot. I'd be grateful if you could also take a look at my previous submissions which have seen no review yet: "libmpathpersist: use extern struct udev from main program" "libmultipath: lazy device-mapper initialization" "mpathpersist.8: add missing documentation for -K, -C, -l" Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic
On Tue, 2017-06-20 at 13:44 -0500, Benjamin Marzinski wrote: > Review and comments are highly welcome. > > ACK for the set I found another issue with the patch set, will send v3 shortly. Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap
On Thu, 2017-06-22 at 08:23 +0200, Hannes Reinecke wrote: > On 06/21/2017 05:06 PM, Martin Wilck wrote: > > We set the queue_if_no_path feature in assemble_map() already, > > no need to set it here again. > > > > Signed-off-by: Martin Wilck <mwi...@suse.com> > > --- > > libmultipath/configure.c | 15 --- > > 1 file changed, 15 deletions(-) > > > > [...] > Watch out. > 'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is > active, > and removed afterwards. > So there might be valid reasons why it's set here. > Have you checked? Yes, I'm pretty certain that this is correct. We're in coalesce_paths() here, while we are setting up or reconfiguring maps. The call sequence is setup_map() assemble_map() domap() ... and then comes the code I'm removing. We set the feature string in assemble_map() correctly. Thus the removed code just repeated the same setting that had already been applied in domap(). This code has been in that place for a very long time, AFAICS it originates from times where the features string was not correctly set up before creating or reloading the map. The logic for disabling queue_if_no_path if the retry count is reached is implemented elsewhere, mainly in set_no_path_retry() (called e.g. from ev_remove_path()) and retry_count_tick() (called from checker loop). Do you see a case that I have overlooked? Best, Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH v3 10/11] libmultipath: don't try to set hwhandler if it is retained
On Thu, 2017-06-22 at 08:21 +0200, Hannes Reinecke wrote: > On 06/21/2017 05:06 PM, Martin Wilck wrote: > > Setting a device handler only works if retain_attached_hw_handler > > is 'no', or if the kernel didn't auto-assign a handler. If this > > is not the case, don't even attempt to set a different handler. > > > > This requires reading the sysfs "dh_state" path attribute. > > For internal consistency, this attribute must be updated after > > domap(). > > > > Signed-off-by: Martin Wilck <mwi...@suse.com> > > --- > > libmultipath/configure.c | 52 > > +++- > > libmultipath/discovery.c | 4 > > libmultipath/propsel.c | 15 ++ > > libmultipath/structs.h | 2 ++ > > 4 files changed, 72 insertions(+), 1 deletion(-) > > > > [...] > > Hmm. > > Not sure if I fully agree with this. > I do see the need to read 'dh_state' from pathinfo(), just to figure > out > if an hardware handler is already loaded. > > But once select_hwhandler is done it's quite pointless to update the > dh_state; what exactly _would_ be the error action in this case? > Plus the code detects the failure, but then doesn't do anything with > it... My concern was that multipathd might carry along wrong state and possibly print it in log messages, irritating users. It's true, there is no reasonable error action, and it's quite a lot of code just for this minor purpose. > So, please, if you insist on checking dh_state please implement > correct > error action here, like updating the 'hwhandler' value to that found > in > dh_state or disabling the hardware handler if it's found to be > detached. If it's fine with you and other reviewers, I'll happily remove that part of the patch, and just keep the part in select_hwhandler(). If we want proper error handling, we'd need to check that the handler of the loaded map as well as the handlers of all paths are set to the handler configured in multipathd. Unless I'm mistaken, it isn't guaranteed that all paths will be using the same handler after a map is set up. Besides re-reading the dh_state of all paths, this check would also require re-reading and disassembling the map, and no reasonable error action is to be seen other then updating the internal state, lots of code for almost nothing. Cheers, Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH v3 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic
On Wed, 2017-06-21 at 17:06 +0200, Martin Wilck wrote: > > Changes wrt v2: > - Added Acked-by:/Reviewed-by: tags > - 1/11: clarify comment in select_max_sectors_kb (Hannes Reinecke) > - 3/11: call select_retain_hwhandler before select_features > - 8/11: don't suggest using retain_attached_hw_handler in log msg > - 9/11, 10/11, 11/11: new Forgot to mention: rebased to Christophe's latest code (847cc43c). Martin -- Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH v3 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+
Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
let dm detach device handlers") imply "retain_attached_hw_handler yes".
Clarify this in the propsel code, log messages, and documentation.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/configure.c | 3 ++-
libmultipath/dmparser.c| 3 ++-
libmultipath/propsel.c | 7 ++-
libmultipath/util.c| 36
libmultipath/util.h| 2 ++
multipath/multipath.conf.5 | 15 +++
6 files changed, 59 insertions(+), 7 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index a7f2b443..74b6f52a 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -572,7 +572,8 @@ select_action (struct multipath * mpp, vector curmp, int
force_reload)
}
if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
- mpp->retain_hwhandler != cmpp->retain_hwhandler) {
+ mpp->retain_hwhandler != cmpp->retain_hwhandler &&
+ get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
mpp->action = ACT_RELOAD;
condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
mpp->alias);
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 1121c715..b647c256 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -97,7 +97,8 @@ assemble_map (struct multipath * mp, char * params, int len)
} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
add_feature(, no_path_retry);
}
- if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
+ if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
+ get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
add_feature(, retain_hwhandler);
APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index e885a845..d609394e 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config *conf, struct
multipath *mp)
if (!VERSION_GE(conf->version, minv_dm_retain)) {
mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
- origin = "(setting: WARNING, requires kernel version >= 1.5.0)";
+ origin = "(setting: WARNING, requires kernel dm-mpath version
>= 1.5.0)";
+ goto out;
+ }
+ if (get_linux_version_code() >= KERNEL_VERSION(4, 3, 0)) {
+ mp->retain_hwhandler = RETAIN_HWHANDLER_ON;
+ origin = "(setting: implied in kernel >= 4.3.0)";
goto out;
}
mp_set_ovr(retain_hwhandler);
diff --git a/libmultipath/util.c b/libmultipath/util.c
index b90cd8b0..dff2ed3c 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -380,3 +381,38 @@ int systemd_service_enabled(const char *dev)
found = systemd_service_enabled_in(dev, "/run");
return found;
}
+
+static int _linux_version_code;
+static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
+
+/* Returns current kernel version encoded as major*65536 + minor*256 + patch,
+ * so, for example, to check if the kernel is greater than 2.2.11:
+ *
+ * if (get_linux_version_code() > KERNEL_VERSION(2,2,11)) { }
+ *
+ * Copyright (C) 1999-2004 by Erik Andersen <ander...@codepoet.org>
+ * Code copied from busybox (GPLv2 or later)
+ */
+static void
+_set_linux_version_code(void)
+{
+ struct utsname name;
+ char *t;
+ int i, r;
+
+ uname(); /* never fails */
+ t = name.release;
+ r = 0;
+ for (i = 0; i < 3; i++) {
+ t = strtok(t, ".");
+ r = r * 256 + (t ? atoi(t) : 0);
+ t = NULL;
+ }
+ _linux_version_code = r;
+}
+
+int get_linux_version_code(void)
+{
+ pthread_once(&_lvc_initialized, _set_linux_version_code);
+ return _linux_version_code;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index b087e32e..45291be8 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -15,6 +15,8 @@ char *convert_dev(char *dev, int is_path_device);
char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev);
void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
int systemd_service_enabled(const char *dev);
+int get_linux_version_code(void);
+#define KERNEL_VERSION(maj, min, ptc) maj) * 256) + (min)) * 256 + (ptc))
#define safe_sprintf(var, format, args...) \
snprintf(var, sizeof(var), format, ##args) >= sizeof(var)
diff --git a/multipath/multipath.conf.5 b/multipath/multipa
[dm-devel] [PATCH v3 10/11] libmultipath: don't try to set hwhandler if it is retained
Setting a device handler only works if retain_attached_hw_handler
is 'no', or if the kernel didn't auto-assign a handler. If this
is not the case, don't even attempt to set a different handler.
This requires reading the sysfs "dh_state" path attribute.
For internal consistency, this attribute must be updated after domap().
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/configure.c | 52 +++-
libmultipath/discovery.c | 4
libmultipath/propsel.c | 15 ++
libmultipath/structs.h | 2 ++
4 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 74b6f52a..55dbb261 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -275,15 +275,21 @@ int setup_map(struct multipath *mpp, char *params, int
params_size)
/*
* properties selectors
+*
+* Ordering matters for some properties:
+* - features after no_path_retry and retain_hwhandler
+* - hwhandler after retain_hwhandler
+* No guarantee that this list is complete, check code in
+* propsel.c if in doubt.
*/
conf = get_multipath_config();
select_pgfailback(conf, mpp);
select_pgpolicy(conf, mpp);
select_selector(conf, mpp);
- select_hwhandler(conf, mpp);
select_no_path_retry(conf, mpp);
select_retain_hwhandler(conf, mpp);
select_features(conf, mpp);
+ select_hwhandler(conf, mpp);
select_rr_weight(conf, mpp);
select_minio(conf, mpp);
select_mode(conf, mpp);
@@ -706,6 +712,48 @@ fail:
return 1;
}
+static void
+check_dh_state_changed(struct multipath *mp)
+{
+ struct config *conf;
+ struct path newp, *pp;
+ struct pathgroup *pg;
+ int i, j;
+
+ conf = get_multipath_config();
+
+ vector_foreach_slot (mp->pg, pg, j) {
+ vector_foreach_slot (pg->paths, pp, i) {
+ if (!pp->udev || !strlen(pp->dh_state) ||
+ (conf->retain_hwhandler == RETAIN_HWHANDLER_ON &&
+strcmp(pp->dh_state, "detached")))
+ continue;
+
+ memset(, 0, sizeof(newp));
+ memcpy(newp.dev, pp->dev, sizeof(newp.dev));
+ newp.udev = udev_device_ref(pp->udev);
+
+ if (pathinfo(, conf, DI_SYSFS) == PATHINFO_OK) {
+ if (strncmp(newp.dh_state, pp->dh_state,
+ SCSI_DH_SIZE)) {
+ condlog(3, "%s: dh_state changed from
%s to %s",
+ pp->dev,
+ pp->dh_state,
+ newp.dh_state);
+ memcpy(pp->dh_state, newp.dh_state,
+ SCSI_DH_SIZE);
+ }
+ } else
+ condlog(1, "%s: failed to update dh_state",
+ pp->dev);
+
+ udev_device_unref(newp.udev);
+ }
+ }
+
+ put_multipath_config(conf);
+}
+
/*
* Return value:
*/
@@ -828,6 +876,8 @@ int domap(struct multipath *mpp, char *params, int
is_daemon)
}
}
dm_setgeometry(mpp);
+
+ check_dh_state_changed(mpp);
return DOMAP_OK;
}
return DOMAP_FAIL;
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 663c8eaa..8c0c6a9f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1188,6 +1188,10 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
condlog(3, "%s: tgt_node_name = %s",
pp->dev, pp->tgt_node_name);
+ if (sysfs_attr_get_value(parent, "dh_state",
+pp->dh_state, sizeof(pp->dh_state)) >= 0)
+ condlog(3, "%s: dh_state = %s", pp->dev, pp->dh_state);
+
return 0;
}
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d609394e..d1b3d416 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -345,7 +345,22 @@ out:
int select_hwhandler(struct config *conf, struct multipath *mp)
{
char *origin;
+ struct path *pp;
+ char handler[SCSI_DH_SIZE+2];
+ int i;
+ if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
+ vector_foreach_slot(mp->paths, pp, i) {
+ if (strlen(pp->dh_state) &&
+ strcmp(pp->dh_state, "detached&q
[dm-devel] [PATCH v3 08/11] libmultipath: add deprecated warning for some features settings
The device-mapper features "queue_if_no_path" and
"retain_attached_hw_handler" should be set via the configuration
keywords "no_path_retry" and "retain_attached_hw_handler",
respectively, not via "features".
Print a warning if these "features" settings are encountered.
So far these "features" settings will only be ignored if the
respective other keyword is set, so in particular
'features "1 queue_if_no_path"' will still work as expected, but
cause the warning to be printed.
We should consider ignoring these completely in a future version.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/propsel.c | 5 +
multipath/multipath.conf.5 | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index bc9e130b..e885a845 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -290,6 +290,9 @@ void reconcile_features_with_options(const char *id, char
**features, int* no_pa
* it's translated into "no_path_retry queue" here.
*/
if (strstr(*features, q_i_n_p)) {
+ condlog(0, "%s: option 'features \"1 %s\"' is deprecated, "
+ "please use 'no_path_retry queue' instead",
+ id, q_i_n_p);
if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
*no_path_retry = NO_PATH_RETRY_QUEUE;
print_no_path_retry(buff, sizeof(buff),
@@ -307,6 +310,8 @@ void reconcile_features_with_options(const char *id, char
**features, int* no_pa
remove_feature(features, q_i_n_p);
}
if (strstr(*features, r_a_h_h)) {
+ condlog(0, "%s: option 'features \"1 %s\"' is deprecated",
+ id, r_a_h_h);
if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
condlog(3, "%s: %s = on (inherited setting from feature
'%s')",
id, r_a_h_h, r_a_h_h);
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 000a42ec..b32d0383 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -389,7 +389,7 @@ Possible values for the feature list are:
.TP 12
.\" XXX
.I queue_if_no_path
-(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
+(Deprecated, superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this
feature and \fIno_path_retry\fR are set, the latter value takes
precedence. See KNOWN ISSUES.
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 02/11] libmultipath: add/remove_feature: use const char* for feature
Change the argument type for the feature to add or remove to
const char*, making it possible to pass const strings without
warnings.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
libmultipath/structs.c | 30 --
libmultipath/structs.h | 4 ++--
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e225f8b4..28704676 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -513,10 +513,11 @@ void setup_feature(struct multipath *mpp, char *feature)
}
}
-int add_feature(char **f, char *n)
+int add_feature(char **f, const char *n)
{
int c = 0, d, l = 0;
char *e, *p, *t;
+ const char *q;
if (!f)
return 1;
@@ -554,14 +555,14 @@ int add_feature(char **f, char *n)
if ((c % 10) == 9)
l++;
c++;
- p = n;
- while (*p != '\0') {
- if (*p == ' ' && p[1] != '\0' && p[1] != ' ') {
+ q = n;
+ while (*q != '\0') {
+ if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
if ((c % 10) == 9)
l++;
c++;
}
- p++;
+ q++;
}
t = MALLOC(l + 1);
@@ -601,10 +602,11 @@ int add_feature(char **f, char *n)
return 0;
}
-int remove_feature(char **f, char *o)
+int remove_feature(char **f, const char *o)
{
int c = 0, d, l;
char *e, *p, *n;
+ const char *q;
if (!f || !*f)
return 1;
@@ -630,18 +632,18 @@ int remove_feature(char **f, char *o)
/* Just spaces, return */
if (*o == '\0')
return 0;
- e = o + strlen(o);
- while (*e == ' ')
- e--;
- d = (int)(e - o);
+ q = o + strlen(o);
+ while (*q == ' ')
+ q--;
+ d = (int)(q - o);
/* Update feature count */
c--;
- p = o;
- while (p[0] != '\0') {
- if (p[0] == ' ' && p[1] != ' ' && p[1] != '\0')
+ q = o;
+ while (q[0] != '\0') {
+ if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0')
c--;
- p++;
+ q++;
}
/* Quick exit if all features have been removed */
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 01e031ad..8ea984d9 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -369,8 +369,8 @@ int pathcountgr (struct pathgroup *, int);
int pathcount (struct multipath *, int);
int pathcmp (struct pathgroup *, struct pathgroup *);
void setup_feature(struct multipath *, char *);
-int add_feature (char **, char *);
-int remove_feature (char **, char *);
+int add_feature (char **, const char *);
+int remove_feature (char **, const char *);
extern char sysfs_path[PATH_SIZE];
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 04/11] libmultipath: merge_hwe: fix queue_if_no_path logic
The logic applied here should match the logic in select_features().
This is achieved by calling reconcile_features_with_options().
Signed-off-by: Martin Wilck <mwi...@suse.com>
Acked-by: Benjamin Marzinski <bmarz...@redhat.com>
---
libmultipath/config.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 60e345b3..a9b3eda2 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -25,6 +25,7 @@
#include "prio.h"
#include "devmapper.h"
#include "mpath_cmd.h"
+#include "propsel.h"
static int
hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
@@ -318,6 +319,8 @@ set_param_str(char * str)
static int
merge_hwe (struct hwentry * dst, struct hwentry * src)
{
+ int id_len;
+ char *id;
merge_str(vendor);
merge_str(product);
merge_str(revision);
@@ -353,15 +356,14 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
merge_num(san_path_err_forget_rate);
merge_num(san_path_err_recovery_time);
- /*
-* Make sure features is consistent with
-* no_path_retry
-*/
- if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
- remove_feature(>features, "queue_if_no_path");
- else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
- add_feature(>features, "queue_if_no_path");
-
+ id_len = strlen(dst->vendor) + strlen(dst->product) + 2;
+ id = MALLOC(id_len);
+ if (id != NULL)
+ snprintf(id, id_len, "%s/%s", dst->vendor, dst->product);
+ reconcile_features_with_options(id, >features,
+ >no_path_retry,
+ >retain_hwhandler);
+ FREE(id);
return 0;
}
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic
This patch set attempts to sanitize the logic used for consistently handling options that can be set both via the "features" string and explicit multipath.conf options. This is most prominently "no_path_retry" vs. "queue_if_no_path", but also "retain_attached_hw_handler" vs. the feature of the same name. The logic this patch set follows is: - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored (this is the case nowadays for almost all "real" storage arrays, thanks to hwtable). - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry queue". ... likewise for "retain_attached_hw_handler". In the long run, we should get rid of the "features" settings duplicating configuration options altogether; the patch set prepares this by printing warning messages. The logic is implemented in the new function reconcile_features_with_options, which is called from both select_features() and merge_hwe(). In setup_map(), we need to call select_features() after select_no_path_retry() to make this work. The actual feature setting for device-mapper is made in assemble_map(), the patch set also fixes the logic there. The patch set documents the behavior in the man page, and adds some more man page fixes. By skipping superfluous default initializations in load_config(), the log messages for the respective config settings become more appropriate. The logic for setting hardware handler is also improved. Since kernel 4.3, "retain_attached_hw_handler yes" is implicitly set by the kernel, and setting the hardware handler from user space is only possible in special situations. Acknowledge that in multipathd, and don't try to set or unset either this feature or the hwhandler attribute itself if it's doomed to fail. Review and comments are highly welcome. Changes wrt v1: - Suggestions from Ben Marzinski: * Made sure "multipathd show config" still works (1/8). * Fixed logging for default setting of "max_sectors" (1/8) * Consistent internal treatment of mp->features (3/8, 4/8) * Fixed whitespace error (6/8) * Added deprecated warnings (8/8) - Made sure the same logic is used in propsel.c and config.c by calling the same function (3/8, 4/8) - Added deprecated warnings (8/8) Changes wrt v2: - Added Acked-by:/Reviewed-by: tags - 1/11: clarify comment in select_max_sectors_kb (Hannes Reinecke) - 3/11: call select_retain_hwhandler before select_features - 8/11: don't suggest using retain_attached_hw_handler in log msg - 9/11, 10/11, 11/11: new Martin Wilck (11): libmultipath: load_config: skip setting unnecessary defaults libmultipath: add/remove_feature: use const char* for feature libmultipath: clarify option conflicts for "features" libmultipath: merge_hwe: fix queue_if_no_path logic libmultipath: assemble_map: fix queue_if_no_path logic multipath.conf.5: document no_path_retry vs. queue_if_no_path multipath.conf.5: Remove ??? and other minor fixes libmultipath: add deprecated warning for some features settings libmultipath: retain_attached_hw_handler obsolete with 4.3+ libmultipath: don't try to set hwhandler if it is retained libmultipath: don't [un]set queue_if_no_path after domap libmultipath/config.c | 36 +--- libmultipath/configure.c | 72 libmultipath/dict.c| 11 +++-- libmultipath/discovery.c | 4 ++ libmultipath/dmparser.c| 8 ++-- libmultipath/propsel.c | 101 ++--- libmultipath/propsel.h | 3 ++ libmultipath/structs.c | 30 +++--- libmultipath/structs.h | 6 ++- libmultipath/util.c| 36 libmultipath/util.h| 2 + multipath/multipath.conf.5 | 67 ++ 12 files changed, 266 insertions(+), 110 deletions(-) -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 05/11] libmultipath: assemble_map: fix queue_if_no_path logic
It is wrong to remove the queue_if_no_path feature if no_path_retry
is unset. Rather, in this case the feature should neither be added
nor removed.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Acked-by: Benjamin Marzinski <bmarz...@redhat.com>
---
libmultipath/dmparser.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index ba09dc72..1121c715 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -89,13 +89,12 @@ assemble_map (struct multipath * mp, char * params, int len)
* We have to set 'queue_if_no_path' here even
* to avoid path failures during map reload.
*/
- if (mp->no_path_retry == NO_PATH_RETRY_UNDEF ||
- mp->no_path_retry == NO_PATH_RETRY_FAIL) {
+ if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
/* remove queue_if_no_path settings */
condlog(3, "%s: remove queue_if_no_path from '%s'",
mp->alias, mp->features);
remove_feature(, no_path_retry);
- } else {
+ } else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
add_feature(, no_path_retry);
}
if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 03/11] libmultipath: clarify option conflicts for "features"
The "features" option in multipath.conf can possibly conflict
with "no_path_retry" and "retain_attached_hw_handler".
Currently, "no_path_retry" takes precedence, unless it is set to
"fail", in which case it's overridden by "queue_if_no_path".
This is odd, either "features" or "no_path_retry" should take
precedence.
No precedence rules are defined for "retain_attached_hw_handler".
Make this behavior more consistent by always giving precedence
to the explicit config file options, and improve logging.
Put this logic into a separate function, which can be used
from other places in the code.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/configure.c | 6 ++---
libmultipath/propsel.c | 68 +---
libmultipath/propsel.h | 3 +++
3 files changed, 59 insertions(+), 18 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index bd090d9a..a7f2b443 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -280,18 +280,18 @@ int setup_map(struct multipath *mpp, char *params, int
params_size)
select_pgfailback(conf, mpp);
select_pgpolicy(conf, mpp);
select_selector(conf, mpp);
- select_features(conf, mpp);
select_hwhandler(conf, mpp);
+ select_no_path_retry(conf, mpp);
+ select_retain_hwhandler(conf, mpp);
+ select_features(conf, mpp);
select_rr_weight(conf, mpp);
select_minio(conf, mpp);
- select_no_path_retry(conf, mpp);
select_mode(conf, mpp);
select_uid(conf, mpp);
select_gid(conf, mpp);
select_fast_io_fail(conf, mpp);
select_dev_loss(conf, mpp);
select_reservation_key(conf, mpp);
- select_retain_hwhandler(conf, mpp);
select_deferred_remove(conf, mpp);
select_delay_watch_checks(conf, mpp);
select_delay_wait_checks(conf, mpp);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index c8ccede5..bc9e130b 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -269,6 +269,55 @@ out:
return mp->alias ? 0 : 1;
}
+void reconcile_features_with_options(const char *id, char **features, int*
no_path_retry,
+ int *retain_hwhandler)
+{
+ static const char q_i_n_p[] = "queue_if_no_path";
+ static const char r_a_h_h[] = "retain_attached_hw_handler";
+ char buff[12];
+
+ if (*features == NULL)
+ return;
+ if (id == NULL)
+ id = "UNKNOWN";
+
+ /*
+* We only use no_path_retry internally. The "queue_if_no_path"
+* device-mapper feature is derived from it when the map is loaded.
+* For consistency, "queue_if_no_path" is removed from the
+* internal libmultipath features string.
+* For backward compatibility we allow 'features "1 queue_if_no_path"';
+* it's translated into "no_path_retry queue" here.
+*/
+ if (strstr(*features, q_i_n_p)) {
+ if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
+ *no_path_retry = NO_PATH_RETRY_QUEUE;
+ print_no_path_retry(buff, sizeof(buff),
+ no_path_retry);
+ condlog(3, "%s: no_path_retry = %s (inherited setting
from feature '%s')",
+ id, buff, q_i_n_p);
+ };
+ /* Warn only if features string is overridden */
+ if (*no_path_retry != NO_PATH_RETRY_QUEUE) {
+ print_no_path_retry(buff, sizeof(buff),
+ no_path_retry);
+ condlog(2, "%s: ignoring feature '%s' because
no_path_retry is set to '%s'",
+ id, q_i_n_p, buff);
+ }
+ remove_feature(features, q_i_n_p);
+ }
+ if (strstr(*features, r_a_h_h)) {
+ if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
+ condlog(3, "%s: %s = on (inherited setting from feature
'%s')",
+ id, r_a_h_h, r_a_h_h);
+ *retain_hwhandler = RETAIN_HWHANDLER_ON;
+ } else if (*retain_hwhandler == RETAIN_HWHANDLER_OFF)
+ condlog(2, "%s: ignoring feature '%s' because %s is set
to 'off'",
+ id, r_a_h_h, r_a_h_h);
+ remove_feature(features, r_a_h_h);
+ }
+}
+
int select_features(struct config *conf, struct multipath *mp)
{
char *origin;
@@ -280,19 +329,11 @@ int select_features(struct config *conf, struct multipath
*mp)
mp_set_default(features, DEFAULT_FEATURES);
out:
mp->features = STRDUP(mp
[dm-devel] [PATCH v3 07/11] multipath.conf.5: Remove ??? and other minor fixes
Remove the FIXME markers by filling in missing content, and make some other minor fixes. Signed-off-by: Martin Wilck <mwi...@suse.com> Acked-by: Benjamin Marzinski <bmarz...@redhat.com> --- multipath/multipath.conf.5 | 48 +- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index d5d9438a..000a42ec 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -258,7 +258,7 @@ Return a constant priority of \fI1\fR. .I sysfs Use the sysfs attributes \fIaccess_state\fR and \fIpreferred_path\fR to generate the path priority. This prioritizer accepts the optional prio_arg -\fIexclusive_pref_bit\fR +\fIexclusive_pref_bit\fR. .TP .I emc (Hardware-dependent) @@ -300,14 +300,19 @@ Generate the path priority based on a latency algorithm. Requires prio_args keyword. .TP .I datacore -.\" XXX -???. Requires prio_args keyword. +(Hardware-dependent) +Generate the path priority for some Datacore storage arrays. Requires prio_args +keyword. .TP .I iet -.\" XXX -???. Requires prio_args keyword. -.TP -The default is: \fBconst\fR +(iSCSI only) +Generate path priority for iSCSI targets based on IP address. Requires +prio_args keyword. +.PP +The default depends on the \fBdetect_prio\fR setting: If \fBdetect_prio\fR is +\fByes\fR (default), the default priority algorithm is \fBsysfs\fR (except for +NetAPP E-Series, where it is \fBalua\fR). If \fBdetect_prio\fR is +\fBno\fR, the default priority algorithm is \fBconst\fR. .RE . . @@ -364,12 +369,12 @@ If \fIexclusive_pref_bit\fR is set, paths with the \fIpreferred path\fR bit set will always be in their own path group. .TP .I datacore -.\" XXX -\fIpreferredsds\fR ???. +\fIpreferredsds\fR (required) denotes the preferred "SDS name" for datacore +arrays. \fItimeout\fR (optional) is the timeout for the INQUIRY, in ms. .TP .I iet -.\" XXX -\fIpreferredip\fR ???. +\fIpreferredip=...\fR (required) denotes the preferred IP address (in dotted decimal +notation) for iSCSI targets. .TP The default is: \fB\fR .RE @@ -384,29 +389,28 @@ Possible values for the feature list are: .TP 12 .\" XXX .I queue_if_no_path -(Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. +(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active. Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this feature and \fIno_path_retry\fR are set, the latter value takes precedence. See KNOWN ISSUES. .TP -.I no_partitions -Disable automatic partitions generation via kpartx. -.TP .\" XXX .I pg_init_retries -(Since ??? kernel) Number of times to retry pg_init, it must be between 1 and 50. +(Since kernel 2.6.24) Number of times to retry pg_init, it must be between 1 and 50. .TP .\" XXX .I pg_init_delay_msecs -(Since ??? kernel) Number of msecs before pg_init retry, it must be between 0 and 6. +(Since kernel 2.6.38) Number of msecs before pg_init retry, it must be between 0 and 6. .TP .\" XXX .I queue_mode -(Since ??? kernel) Select the the queue_mode per multipath device. -Where can be \fIbio\fR, \fIrq\fR or \fImq\fR. Which corresponds to -bio-based, request_fn rq-based, and blk-mq rq-based respectively. -.TP -The default is: \fB0\fR +(Since kernel 4.8) Select the the queueing mode per multipath device. + can be \fIbio\fR, \fIrq\fR or \fImq\f, which corresponds to +bio-based, request-based, and block-multiqueue (blk-mq) request-based, +respectively. + +The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR. It is +\fImq\fR if the latter is set, and \fIrq\fR otherwise. .RE . . -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v3 01/11] libmultipath: load_config: skip setting unnecessary defaults
We have the logic for setting defaults for paths and maps in propsel.c. By pre-setting conf values with defaults in load_config(), we generate irritating log messages like 'features = "0" (setting: multipath.conf defaults/devices section)' if multipath.conf doesn't contain a features setting at all. For some config settings, we need to use declare_def_snprint_defint() now to make sure "multipathd show config" prints all defaults correctly. To avoid confusion, we don't do this for "max_sectors", because multipathd leaves this untouched by default. Signed-off-by: Martin Wilck <mwi...@suse.com> Acked-by: Benjamin Marzinski <bmarz...@redhat.com> --- libmultipath/config.c | 16 libmultipath/dict.c| 11 +++ libmultipath/propsel.c | 6 ++ 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index 6b236019..60e345b3 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -606,40 +606,24 @@ load_config (char * file) if (!conf->verbosity) conf->verbosity = DEFAULT_VERBOSITY; - conf->minio = DEFAULT_MINIO; - conf->minio_rq = DEFAULT_MINIO_RQ; get_sys_max_fds(>max_fds); conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE); conf->wwids_file = set_default(DEFAULT_WWIDS_FILE); conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR); - conf->features = set_default(DEFAULT_FEATURES); - conf->flush_on_last_del = DEFAULT_FLUSH; conf->attribute_flags = 0; conf->reassign_maps = DEFAULT_REASSIGN_MAPS; conf->checkint = DEFAULT_CHECKINT; conf->max_checkint = 0; - conf->pgfailback = DEFAULT_FAILBACK; - conf->fast_io_fail = DEFAULT_FAST_IO_FAIL; - conf->retain_hwhandler = DEFAULT_RETAIN_HWHANDLER; - conf->detect_prio = DEFAULT_DETECT_PRIO; - conf->detect_checker = DEFAULT_DETECT_CHECKER; conf->force_sync = DEFAULT_FORCE_SYNC; conf->partition_delim = DEFAULT_PARTITION_DELIM; conf->processed_main_config = 0; conf->find_multipaths = DEFAULT_FIND_MULTIPATHS; conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT; - conf->uid_attribute = set_default(DEFAULT_UID_ATTRIBUTE); conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES; conf->retrigger_delay = DEFAULT_RETRIGGER_DELAY; conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT; - conf->deferred_remove = DEFAULT_DEFERRED_REMOVE; - conf->skip_kpartx = DEFAULT_SKIP_KPARTX; conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS; conf->remove_retries = 0; - conf->max_sectors_kb = DEFAULT_MAX_SECTORS_KB; - conf->san_path_err_threshold = DEFAULT_ERR_CHECKS; - conf->san_path_err_forget_rate = DEFAULT_ERR_CHECKS; - conf->san_path_err_recovery_time = DEFAULT_ERR_CHECKS; /* * preload default hwtable diff --git a/libmultipath/dict.c b/libmultipath/dict.c index 82066f67..9dc10904 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -630,7 +630,7 @@ print_fast_io_fail(char * buff, int len, void *ptr) } declare_def_handler(fast_io_fail, set_fast_io_fail) -declare_def_snprint(fast_io_fail, print_fast_io_fail) +declare_def_snprint_defint(fast_io_fail, print_fast_io_fail, DEFAULT_FAST_IO_FAIL) declare_ovr_handler(fast_io_fail, set_fast_io_fail) declare_ovr_snprint(fast_io_fail, print_fast_io_fail) declare_hw_handler(fast_io_fail, set_fast_io_fail) @@ -1082,7 +1082,8 @@ declare_hw_snprint(delay_wait_checks, print_off_int_undef) declare_mp_handler(delay_wait_checks, set_off_int_undef) declare_mp_snprint(delay_wait_checks, print_off_int_undef) declare_def_handler(san_path_err_threshold, set_off_int_undef) -declare_def_snprint(san_path_err_threshold, print_off_int_undef) +declare_def_snprint_defint(san_path_err_threshold, print_off_int_undef, + DEFAULT_ERR_CHECKS) declare_ovr_handler(san_path_err_threshold, set_off_int_undef) declare_ovr_snprint(san_path_err_threshold, print_off_int_undef) declare_hw_handler(san_path_err_threshold, set_off_int_undef) @@ -1090,7 +1091,8 @@ declare_hw_snprint(san_path_err_threshold, print_off_int_undef) declare_mp_handler(san_path_err_threshold, set_off_int_undef) declare_mp_snprint(san_path_err_threshold, print_off_int_undef) declare_def_handler(san_path_err_forget_rate, set_off_int_undef) -declare_def_snprint(san_path_err_forget_rate, print_off_int_undef) +declare_def_snprint_defint(san_path_err_forget_rate, print_off_int_undef, + DEFAULT_ERR_CHECKS) declare_ovr_handler(san_path_err_forget_rate, set_off_int_undef) declare_ovr_snprint(san_path_err_forget_rate, print_off_int_undef) declare_hw_handler(san_path_err_forget_rate, set_off_int_undef) @@ -1098,7 +1100,8 @@ d
[dm-devel] [PATCH v4 10/11] libmultipath: don't try to set hwhandler if it is retained
Setting a device handler only works if retain_attached_hw_handler
is 'no', or if the kernel didn't auto-assign a handler. If this
is not the case, don't even attempt to set a different handler.
This requires reading the sysfs "dh_state" path attribute.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/configure.c | 8 +++-
libmultipath/propsel.c | 35 +++
2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 74b6f52a..03874f47 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -275,15 +275,21 @@ int setup_map(struct multipath *mpp, char *params, int
params_size)
/*
* properties selectors
+*
+* Ordering matters for some properties:
+* - features after no_path_retry and retain_hwhandler
+* - hwhandler after retain_hwhandler
+* No guarantee that this list is complete, check code in
+* propsel.c if in doubt.
*/
conf = get_multipath_config();
select_pgfailback(conf, mpp);
select_pgpolicy(conf, mpp);
select_selector(conf, mpp);
- select_hwhandler(conf, mpp);
select_no_path_retry(conf, mpp);
select_retain_hwhandler(conf, mpp);
select_features(conf, mpp);
+ select_hwhandler(conf, mpp);
select_rr_weight(conf, mpp);
select_minio(conf, mpp);
select_mode(conf, mpp);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d609394e..a86207a0 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -19,8 +19,10 @@
#include "discovery.h"
#include "dict.h"
#include "util.h"
+#include "sysfs.h"
#include "prioritizers/alua_rtpg.h"
#include
+#include
pgpolicyfn *pgpolicies[] = {
NULL,
@@ -342,9 +344,42 @@ out:
return 0;
}
+static int get_dh_state(struct path *pp, char *value, size_t value_len)
+{
+ struct udev_device *ud;
+
+ if (pp->udev == NULL)
+ return -1;
+
+ ud = udev_device_get_parent_with_subsystem_devtype(
+ pp->udev, "scsi", "scsi_device");
+ if (ud == NULL)
+ return -1;
+
+ return sysfs_attr_get_value(ud, "dh_state", value, value_len);
+}
+
int select_hwhandler(struct config *conf, struct multipath *mp)
{
char *origin;
+ struct path *pp;
+ /* dh_state is no longer than "detached" */
+ char handler[12];
+ char *dh_state;
+ int i;
+
+ dh_state = [2];
+ if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
+ vector_foreach_slot(mp->paths, pp, i) {
+ if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
+ && strcmp(dh_state, "detached")) {
+ memcpy(handler, "1 ", 2);
+ mp->hwhandler = handler;
+ origin = "(setting: retained by kernel driver)";
+ goto out;
+ }
+ }
+ }
mp_set_hwe(hwhandler);
mp_set_conf(hwhandler);
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 07/11] multipath.conf.5: Remove ??? and other minor fixes
Remove the FIXME markers by filling in missing content, and make some other minor fixes. Signed-off-by: Martin Wilck <mwi...@suse.com> Acked-by: Benjamin Marzinski <bmarz...@redhat.com> Reviewed-by: Hannes Reinecke <h...@suse.com> --- multipath/multipath.conf.5 | 48 +- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index d5d9438a..000a42ec 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -258,7 +258,7 @@ Return a constant priority of \fI1\fR. .I sysfs Use the sysfs attributes \fIaccess_state\fR and \fIpreferred_path\fR to generate the path priority. This prioritizer accepts the optional prio_arg -\fIexclusive_pref_bit\fR +\fIexclusive_pref_bit\fR. .TP .I emc (Hardware-dependent) @@ -300,14 +300,19 @@ Generate the path priority based on a latency algorithm. Requires prio_args keyword. .TP .I datacore -.\" XXX -???. Requires prio_args keyword. +(Hardware-dependent) +Generate the path priority for some Datacore storage arrays. Requires prio_args +keyword. .TP .I iet -.\" XXX -???. Requires prio_args keyword. -.TP -The default is: \fBconst\fR +(iSCSI only) +Generate path priority for iSCSI targets based on IP address. Requires +prio_args keyword. +.PP +The default depends on the \fBdetect_prio\fR setting: If \fBdetect_prio\fR is +\fByes\fR (default), the default priority algorithm is \fBsysfs\fR (except for +NetAPP E-Series, where it is \fBalua\fR). If \fBdetect_prio\fR is +\fBno\fR, the default priority algorithm is \fBconst\fR. .RE . . @@ -364,12 +369,12 @@ If \fIexclusive_pref_bit\fR is set, paths with the \fIpreferred path\fR bit set will always be in their own path group. .TP .I datacore -.\" XXX -\fIpreferredsds\fR ???. +\fIpreferredsds\fR (required) denotes the preferred "SDS name" for datacore +arrays. \fItimeout\fR (optional) is the timeout for the INQUIRY, in ms. .TP .I iet -.\" XXX -\fIpreferredip\fR ???. +\fIpreferredip=...\fR (required) denotes the preferred IP address (in dotted decimal +notation) for iSCSI targets. .TP The default is: \fB\fR .RE @@ -384,29 +389,28 @@ Possible values for the feature list are: .TP 12 .\" XXX .I queue_if_no_path -(Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. +(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active. Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this feature and \fIno_path_retry\fR are set, the latter value takes precedence. See KNOWN ISSUES. .TP -.I no_partitions -Disable automatic partitions generation via kpartx. -.TP .\" XXX .I pg_init_retries -(Since ??? kernel) Number of times to retry pg_init, it must be between 1 and 50. +(Since kernel 2.6.24) Number of times to retry pg_init, it must be between 1 and 50. .TP .\" XXX .I pg_init_delay_msecs -(Since ??? kernel) Number of msecs before pg_init retry, it must be between 0 and 6. +(Since kernel 2.6.38) Number of msecs before pg_init retry, it must be between 0 and 6. .TP .\" XXX .I queue_mode -(Since ??? kernel) Select the the queue_mode per multipath device. -Where can be \fIbio\fR, \fIrq\fR or \fImq\fR. Which corresponds to -bio-based, request_fn rq-based, and blk-mq rq-based respectively. -.TP -The default is: \fB0\fR +(Since kernel 4.8) Select the the queueing mode per multipath device. + can be \fIbio\fR, \fIrq\fR or \fImq\f, which corresponds to +bio-based, request-based, and block-multiqueue (blk-mq) request-based, +respectively. + +The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR. It is +\fImq\fR if the latter is set, and \fIrq\fR otherwise. .RE . . -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+
Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
let dm detach device handlers") imply "retain_attached_hw_handler yes".
Clarify this in the propsel code, log messages, and documentation.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
libmultipath/configure.c | 3 ++-
libmultipath/dmparser.c| 3 ++-
libmultipath/propsel.c | 7 ++-
libmultipath/util.c| 36
libmultipath/util.h| 2 ++
multipath/multipath.conf.5 | 15 +++
6 files changed, 59 insertions(+), 7 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index a7f2b443..74b6f52a 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -572,7 +572,8 @@ select_action (struct multipath * mpp, vector curmp, int
force_reload)
}
if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
- mpp->retain_hwhandler != cmpp->retain_hwhandler) {
+ mpp->retain_hwhandler != cmpp->retain_hwhandler &&
+ get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
mpp->action = ACT_RELOAD;
condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
mpp->alias);
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 1121c715..b647c256 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -97,7 +97,8 @@ assemble_map (struct multipath * mp, char * params, int len)
} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
add_feature(, no_path_retry);
}
- if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
+ if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
+ get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
add_feature(, retain_hwhandler);
APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index e885a845..d609394e 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config *conf, struct
multipath *mp)
if (!VERSION_GE(conf->version, minv_dm_retain)) {
mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
- origin = "(setting: WARNING, requires kernel version >= 1.5.0)";
+ origin = "(setting: WARNING, requires kernel dm-mpath version
>= 1.5.0)";
+ goto out;
+ }
+ if (get_linux_version_code() >= KERNEL_VERSION(4, 3, 0)) {
+ mp->retain_hwhandler = RETAIN_HWHANDLER_ON;
+ origin = "(setting: implied in kernel >= 4.3.0)";
goto out;
}
mp_set_ovr(retain_hwhandler);
diff --git a/libmultipath/util.c b/libmultipath/util.c
index b90cd8b0..dff2ed3c 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -380,3 +381,38 @@ int systemd_service_enabled(const char *dev)
found = systemd_service_enabled_in(dev, "/run");
return found;
}
+
+static int _linux_version_code;
+static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
+
+/* Returns current kernel version encoded as major*65536 + minor*256 + patch,
+ * so, for example, to check if the kernel is greater than 2.2.11:
+ *
+ * if (get_linux_version_code() > KERNEL_VERSION(2,2,11)) { }
+ *
+ * Copyright (C) 1999-2004 by Erik Andersen <ander...@codepoet.org>
+ * Code copied from busybox (GPLv2 or later)
+ */
+static void
+_set_linux_version_code(void)
+{
+ struct utsname name;
+ char *t;
+ int i, r;
+
+ uname(); /* never fails */
+ t = name.release;
+ r = 0;
+ for (i = 0; i < 3; i++) {
+ t = strtok(t, ".");
+ r = r * 256 + (t ? atoi(t) : 0);
+ t = NULL;
+ }
+ _linux_version_code = r;
+}
+
+int get_linux_version_code(void)
+{
+ pthread_once(&_lvc_initialized, _set_linux_version_code);
+ return _linux_version_code;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index b087e32e..45291be8 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -15,6 +15,8 @@ char *convert_dev(char *dev, int is_path_device);
char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev);
void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
int systemd_service_enabled(const char *dev);
+int get_linux_version_code(void);
+#define KERNEL_VERSION(maj, min, ptc) maj) * 256) + (min)) * 256 + (ptc))
#define safe_sprintf(var, format, args...) \
snprintf(var, sizeof(var), format, ##args) >= sizeof(var)
diff -
[dm-devel] [PATCH v4 08/11] libmultipath: add deprecated warning for some features settings
The device-mapper features "queue_if_no_path" and
"retain_attached_hw_handler" should be set via the configuration
keywords "no_path_retry" and "retain_attached_hw_handler",
respectively, not via "features".
Print a warning if these "features" settings are encountered.
So far these "features" settings will only be ignored if the
respective other keyword is set, so in particular
'features "1 queue_if_no_path"' will still work as expected, but
cause the warning to be printed.
We should consider ignoring these completely in a future version.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
libmultipath/propsel.c | 5 +
multipath/multipath.conf.5 | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index bc9e130b..e885a845 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -290,6 +290,9 @@ void reconcile_features_with_options(const char *id, char
**features, int* no_pa
* it's translated into "no_path_retry queue" here.
*/
if (strstr(*features, q_i_n_p)) {
+ condlog(0, "%s: option 'features \"1 %s\"' is deprecated, "
+ "please use 'no_path_retry queue' instead",
+ id, q_i_n_p);
if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
*no_path_retry = NO_PATH_RETRY_QUEUE;
print_no_path_retry(buff, sizeof(buff),
@@ -307,6 +310,8 @@ void reconcile_features_with_options(const char *id, char
**features, int* no_pa
remove_feature(features, q_i_n_p);
}
if (strstr(*features, r_a_h_h)) {
+ condlog(0, "%s: option 'features \"1 %s\"' is deprecated",
+ id, r_a_h_h);
if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
condlog(3, "%s: %s = on (inherited setting from feature
'%s')",
id, r_a_h_h, r_a_h_h);
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 000a42ec..b32d0383 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -389,7 +389,7 @@ Possible values for the feature list are:
.TP 12
.\" XXX
.I queue_if_no_path
-(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
+(Deprecated, superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this
feature and \fIno_path_retry\fR are set, the latter value takes
precedence. See KNOWN ISSUES.
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic
This patch set attempts to sanitize the logic used for consistently handling options that can be set both via the "features" string and explicit multipath.conf options. This is most prominently "no_path_retry" vs. "queue_if_no_path", but also "retain_attached_hw_handler" vs. the feature of the same name. The logic this patch set follows is: - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored (this is the case nowadays for almost all "real" storage arrays, thanks to hwtable). - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry queue". ... likewise for "retain_attached_hw_handler". In the long run, we should get rid of the "features" settings duplicating configuration options altogether; the patch set prepares this by printing warning messages. The logic is implemented in the new function reconcile_features_with_options, which is called from both select_features() and merge_hwe(). In setup_map(), we need to call select_features() after select_no_path_retry() to make this work. The actual feature setting for device-mapper is made in assemble_map(), the patch set also fixes the logic there. The patch set documents the behavior in the man page, and adds some more man page fixes. By skipping superfluous default initializations in load_config(), the log messages for the respective config settings become more appropriate. The logic for setting hardware handler is also improved. Since kernel 4.3, "retain_attached_hw_handler yes" is implicitly set by the kernel, and setting the hardware handler from user space is only possible in special situations. Acknowledge that in multipathd, and don't try to set or unset either this feature or the hwhandler attribute itself if it's doomed to fail. Review and comments are highly welcome. Changes wrt v1: - Suggestions from Ben Marzinski: * Made sure "multipathd show config" still works (1/8). * Fixed logging for default setting of "max_sectors" (1/8) * Consistent internal treatment of mp->features (3/8, 4/8) * Fixed whitespace error (6/8) * Added deprecated warnings (8/8) - Made sure the same logic is used in propsel.c and config.c by calling the same function (3/8, 4/8) - Added deprecated warnings (8/8) Changes wrt v2: - Added Acked-by:/Reviewed-by: tags - 1/11: clarify comment in select_max_sectors_kb (Hannes Reinecke) - 3/11: call select_retain_hwhandler before select_features - 8/11: don't suggest using retain_attached_hw_handler in log msg - 9/11, 10/11, 11/11: new Changes wrt v3: - Added Reviewed-by: tags (kept Ben's Ack in 4/11 although patch slightly changed) - 4/11: use a buffer on stack rather than malloc (Hannes Reinecke) - 10/11: Simplify by checking dh_state only in select_handler (Hannes Reinecke) Martin Wilck (11): libmultipath: load_config: skip setting unnecessary defaults libmultipath: add/remove_feature: use const char* for feature libmultipath: clarify option conflicts for "features" libmultipath: merge_hwe: fix queue_if_no_path logic libmultipath: assemble_map: fix queue_if_no_path logic multipath.conf.5: document no_path_retry vs. queue_if_no_path multipath.conf.5: Remove ??? and other minor fixes libmultipath: add deprecated warning for some features settings libmultipath: retain_attached_hw_handler obsolete with 4.3+ libmultipath: don't try to set hwhandler if it is retained libmultipath: don't [un]set queue_if_no_path after domap libmultipath/config.c | 31 +++- libmultipath/configure.c | 28 --- libmultipath/dict.c| 11 +++-- libmultipath/dmparser.c| 8 +-- libmultipath/propsel.c | 121 +++-- libmultipath/propsel.h | 3 ++ libmultipath/structs.c | 30 +-- libmultipath/structs.h | 4 +- libmultipath/util.c| 36 ++ libmultipath/util.h| 2 + multipath/multipath.conf.5 | 67 +++-- 11 files changed, 231 insertions(+), 110 deletions(-) -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 06/11] multipath.conf.5: document no_path_retry vs. queue_if_no_path
Clarify the documentation about option precedence. Signed-off-by: Martin Wilck <mwi...@suse.com> Acked-by: Benjamin Marzinski <bmarz...@redhat.com> Reviewed-by: Hannes Reinecke <h...@suse.com> --- multipath/multipath.conf.5 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 0049cba7..d5d9438a 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -385,7 +385,9 @@ Possible values for the feature list are: .\" XXX .I queue_if_no_path (Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. -Identical to the \fIno_path_retry\fR with \fIqueue\fR value. See KNOWN ISSUES. +Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this +feature and \fIno_path_retry\fR are set, the latter value takes +precedence. See KNOWN ISSUES. .TP .I no_partitions Disable automatic partitions generation via kpartx. -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 03/11] libmultipath: clarify option conflicts for "features"
The "features" option in multipath.conf can possibly conflict
with "no_path_retry" and "retain_attached_hw_handler".
Currently, "no_path_retry" takes precedence, unless it is set to
"fail", in which case it's overridden by "queue_if_no_path".
This is odd, either "features" or "no_path_retry" should take
precedence.
No precedence rules are defined for "retain_attached_hw_handler".
Make this behavior more consistent by always giving precedence
to the explicit config file options, and improve logging.
Put this logic into a separate function, which can be used
from other places in the code.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
libmultipath/configure.c | 6 ++---
libmultipath/propsel.c | 68 +---
libmultipath/propsel.h | 3 +++
3 files changed, 59 insertions(+), 18 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index bd090d9a..a7f2b443 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -280,18 +280,18 @@ int setup_map(struct multipath *mpp, char *params, int
params_size)
select_pgfailback(conf, mpp);
select_pgpolicy(conf, mpp);
select_selector(conf, mpp);
- select_features(conf, mpp);
select_hwhandler(conf, mpp);
+ select_no_path_retry(conf, mpp);
+ select_retain_hwhandler(conf, mpp);
+ select_features(conf, mpp);
select_rr_weight(conf, mpp);
select_minio(conf, mpp);
- select_no_path_retry(conf, mpp);
select_mode(conf, mpp);
select_uid(conf, mpp);
select_gid(conf, mpp);
select_fast_io_fail(conf, mpp);
select_dev_loss(conf, mpp);
select_reservation_key(conf, mpp);
- select_retain_hwhandler(conf, mpp);
select_deferred_remove(conf, mpp);
select_delay_watch_checks(conf, mpp);
select_delay_wait_checks(conf, mpp);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index c8ccede5..bc9e130b 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -269,6 +269,55 @@ out:
return mp->alias ? 0 : 1;
}
+void reconcile_features_with_options(const char *id, char **features, int*
no_path_retry,
+ int *retain_hwhandler)
+{
+ static const char q_i_n_p[] = "queue_if_no_path";
+ static const char r_a_h_h[] = "retain_attached_hw_handler";
+ char buff[12];
+
+ if (*features == NULL)
+ return;
+ if (id == NULL)
+ id = "UNKNOWN";
+
+ /*
+* We only use no_path_retry internally. The "queue_if_no_path"
+* device-mapper feature is derived from it when the map is loaded.
+* For consistency, "queue_if_no_path" is removed from the
+* internal libmultipath features string.
+* For backward compatibility we allow 'features "1 queue_if_no_path"';
+* it's translated into "no_path_retry queue" here.
+*/
+ if (strstr(*features, q_i_n_p)) {
+ if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
+ *no_path_retry = NO_PATH_RETRY_QUEUE;
+ print_no_path_retry(buff, sizeof(buff),
+ no_path_retry);
+ condlog(3, "%s: no_path_retry = %s (inherited setting
from feature '%s')",
+ id, buff, q_i_n_p);
+ };
+ /* Warn only if features string is overridden */
+ if (*no_path_retry != NO_PATH_RETRY_QUEUE) {
+ print_no_path_retry(buff, sizeof(buff),
+ no_path_retry);
+ condlog(2, "%s: ignoring feature '%s' because
no_path_retry is set to '%s'",
+ id, q_i_n_p, buff);
+ }
+ remove_feature(features, q_i_n_p);
+ }
+ if (strstr(*features, r_a_h_h)) {
+ if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
+ condlog(3, "%s: %s = on (inherited setting from feature
'%s')",
+ id, r_a_h_h, r_a_h_h);
+ *retain_hwhandler = RETAIN_HWHANDLER_ON;
+ } else if (*retain_hwhandler == RETAIN_HWHANDLER_OFF)
+ condlog(2, "%s: ignoring feature '%s' because %s is set
to 'off'",
+ id, r_a_h_h, r_a_h_h);
+ remove_feature(features, r_a_h_h);
+ }
+}
+
int select_features(struct config *conf, struct multipath *mp)
{
char *origin;
@@ -280,19 +329,11 @@ int select_features(struct config *conf, struct multipath
*mp)
mp_set_default(features, DEFAULT_FEATURES);
out:
[dm-devel] [PATCH v4 01/11] libmultipath: load_config: skip setting unnecessary defaults
We have the logic for setting defaults for paths and maps in propsel.c. By pre-setting conf values with defaults in load_config(), we generate irritating log messages like 'features = "0" (setting: multipath.conf defaults/devices section)' if multipath.conf doesn't contain a features setting at all. For some config settings, we need to use declare_def_snprint_defint() now to make sure "multipathd show config" prints all defaults correctly. To avoid confusion, we don't do this for "max_sectors", because multipathd leaves this untouched by default. Signed-off-by: Martin Wilck <mwi...@suse.com> Acked-by: Benjamin Marzinski <bmarz...@redhat.com> Reviewed-by: Hannes Reinecke <h...@suse.com> --- libmultipath/config.c | 16 libmultipath/dict.c| 11 +++ libmultipath/propsel.c | 6 ++ 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index 6b236019..60e345b3 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -606,40 +606,24 @@ load_config (char * file) if (!conf->verbosity) conf->verbosity = DEFAULT_VERBOSITY; - conf->minio = DEFAULT_MINIO; - conf->minio_rq = DEFAULT_MINIO_RQ; get_sys_max_fds(>max_fds); conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE); conf->wwids_file = set_default(DEFAULT_WWIDS_FILE); conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR); - conf->features = set_default(DEFAULT_FEATURES); - conf->flush_on_last_del = DEFAULT_FLUSH; conf->attribute_flags = 0; conf->reassign_maps = DEFAULT_REASSIGN_MAPS; conf->checkint = DEFAULT_CHECKINT; conf->max_checkint = 0; - conf->pgfailback = DEFAULT_FAILBACK; - conf->fast_io_fail = DEFAULT_FAST_IO_FAIL; - conf->retain_hwhandler = DEFAULT_RETAIN_HWHANDLER; - conf->detect_prio = DEFAULT_DETECT_PRIO; - conf->detect_checker = DEFAULT_DETECT_CHECKER; conf->force_sync = DEFAULT_FORCE_SYNC; conf->partition_delim = DEFAULT_PARTITION_DELIM; conf->processed_main_config = 0; conf->find_multipaths = DEFAULT_FIND_MULTIPATHS; conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT; - conf->uid_attribute = set_default(DEFAULT_UID_ATTRIBUTE); conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES; conf->retrigger_delay = DEFAULT_RETRIGGER_DELAY; conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT; - conf->deferred_remove = DEFAULT_DEFERRED_REMOVE; - conf->skip_kpartx = DEFAULT_SKIP_KPARTX; conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS; conf->remove_retries = 0; - conf->max_sectors_kb = DEFAULT_MAX_SECTORS_KB; - conf->san_path_err_threshold = DEFAULT_ERR_CHECKS; - conf->san_path_err_forget_rate = DEFAULT_ERR_CHECKS; - conf->san_path_err_recovery_time = DEFAULT_ERR_CHECKS; /* * preload default hwtable diff --git a/libmultipath/dict.c b/libmultipath/dict.c index 82066f67..9dc10904 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -630,7 +630,7 @@ print_fast_io_fail(char * buff, int len, void *ptr) } declare_def_handler(fast_io_fail, set_fast_io_fail) -declare_def_snprint(fast_io_fail, print_fast_io_fail) +declare_def_snprint_defint(fast_io_fail, print_fast_io_fail, DEFAULT_FAST_IO_FAIL) declare_ovr_handler(fast_io_fail, set_fast_io_fail) declare_ovr_snprint(fast_io_fail, print_fast_io_fail) declare_hw_handler(fast_io_fail, set_fast_io_fail) @@ -1082,7 +1082,8 @@ declare_hw_snprint(delay_wait_checks, print_off_int_undef) declare_mp_handler(delay_wait_checks, set_off_int_undef) declare_mp_snprint(delay_wait_checks, print_off_int_undef) declare_def_handler(san_path_err_threshold, set_off_int_undef) -declare_def_snprint(san_path_err_threshold, print_off_int_undef) +declare_def_snprint_defint(san_path_err_threshold, print_off_int_undef, + DEFAULT_ERR_CHECKS) declare_ovr_handler(san_path_err_threshold, set_off_int_undef) declare_ovr_snprint(san_path_err_threshold, print_off_int_undef) declare_hw_handler(san_path_err_threshold, set_off_int_undef) @@ -1090,7 +1091,8 @@ declare_hw_snprint(san_path_err_threshold, print_off_int_undef) declare_mp_handler(san_path_err_threshold, set_off_int_undef) declare_mp_snprint(san_path_err_threshold, print_off_int_undef) declare_def_handler(san_path_err_forget_rate, set_off_int_undef) -declare_def_snprint(san_path_err_forget_rate, print_off_int_undef) +declare_def_snprint_defint(san_path_err_forget_rate, print_off_int_undef, + DEFAULT_ERR_CHECKS) declare_ovr_handler(san_path_err_forget_rate, set_off_int_undef) declare_ovr_snprint(san_path_err_forget_rate, print_off_int_undef) declare_hw_handler(san_path_err_forget_ra
[dm-devel] [PATCH v4 04/11] libmultipath: merge_hwe: fix queue_if_no_path logic
The logic applied here should match the logic in select_features().
This is achieved by calling reconcile_features_with_options().
Signed-off-by: Martin Wilck <mwi...@suse.com>
Acked-by: Benjamin Marzinski <bmarz...@redhat.com>
---
libmultipath/config.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 60e345b3..b21a3aa1 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -25,6 +25,7 @@
#include "prio.h"
#include "devmapper.h"
#include "mpath_cmd.h"
+#include "propsel.h"
static int
hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
@@ -318,6 +319,7 @@ set_param_str(char * str)
static int
merge_hwe (struct hwentry * dst, struct hwentry * src)
{
+ char id[SCSI_VENDOR_SIZE+SCSI_PRODUCT_SIZE];
merge_str(vendor);
merge_str(product);
merge_str(revision);
@@ -353,15 +355,10 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
merge_num(san_path_err_forget_rate);
merge_num(san_path_err_recovery_time);
- /*
-* Make sure features is consistent with
-* no_path_retry
-*/
- if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
- remove_feature(>features, "queue_if_no_path");
- else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
- add_feature(>features, "queue_if_no_path");
-
+ snprintf(id, sizeof(id), "%s/%s", dst->vendor, dst->product);
+ reconcile_features_with_options(id, >features,
+ >no_path_retry,
+ >retain_hwhandler);
return 0;
}
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 02/11] libmultipath: add/remove_feature: use const char* for feature
Change the argument type for the feature to add or remove to
const char*, making it possible to pass const strings without
warnings.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Acked-by: Benjamin Marzinski <bmarz...@redhat.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
libmultipath/structs.c | 30 --
libmultipath/structs.h | 4 ++--
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e225f8b4..28704676 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -513,10 +513,11 @@ void setup_feature(struct multipath *mpp, char *feature)
}
}
-int add_feature(char **f, char *n)
+int add_feature(char **f, const char *n)
{
int c = 0, d, l = 0;
char *e, *p, *t;
+ const char *q;
if (!f)
return 1;
@@ -554,14 +555,14 @@ int add_feature(char **f, char *n)
if ((c % 10) == 9)
l++;
c++;
- p = n;
- while (*p != '\0') {
- if (*p == ' ' && p[1] != '\0' && p[1] != ' ') {
+ q = n;
+ while (*q != '\0') {
+ if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
if ((c % 10) == 9)
l++;
c++;
}
- p++;
+ q++;
}
t = MALLOC(l + 1);
@@ -601,10 +602,11 @@ int add_feature(char **f, char *n)
return 0;
}
-int remove_feature(char **f, char *o)
+int remove_feature(char **f, const char *o)
{
int c = 0, d, l;
char *e, *p, *n;
+ const char *q;
if (!f || !*f)
return 1;
@@ -630,18 +632,18 @@ int remove_feature(char **f, char *o)
/* Just spaces, return */
if (*o == '\0')
return 0;
- e = o + strlen(o);
- while (*e == ' ')
- e--;
- d = (int)(e - o);
+ q = o + strlen(o);
+ while (*q == ' ')
+ q--;
+ d = (int)(q - o);
/* Update feature count */
c--;
- p = o;
- while (p[0] != '\0') {
- if (p[0] == ' ' && p[1] != ' ' && p[1] != '\0')
+ q = o;
+ while (q[0] != '\0') {
+ if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0')
c--;
- p++;
+ q++;
}
/* Quick exit if all features have been removed */
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 01e031ad..8ea984d9 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -369,8 +369,8 @@ int pathcountgr (struct pathgroup *, int);
int pathcount (struct multipath *, int);
int pathcmp (struct pathgroup *, struct pathgroup *);
void setup_feature(struct multipath *, char *);
-int add_feature (char **, char *);
-int remove_feature (char **, char *);
+int add_feature (char **, const char *);
+int remove_feature (char **, const char *);
extern char sysfs_path[PATH_SIZE];
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 05/11] libmultipath: assemble_map: fix queue_if_no_path logic
It is wrong to remove the queue_if_no_path feature if no_path_retry
is unset. Rather, in this case the feature should neither be added
nor removed.
Signed-off-by: Martin Wilck <mwi...@suse.com>
Acked-by: Benjamin Marzinski <bmarz...@redhat.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
libmultipath/dmparser.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index ba09dc72..1121c715 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -89,13 +89,12 @@ assemble_map (struct multipath * mp, char * params, int len)
* We have to set 'queue_if_no_path' here even
* to avoid path failures during map reload.
*/
- if (mp->no_path_retry == NO_PATH_RETRY_UNDEF ||
- mp->no_path_retry == NO_PATH_RETRY_FAIL) {
+ if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
/* remove queue_if_no_path settings */
condlog(3, "%s: remove queue_if_no_path from '%s'",
mp->alias, mp->features);
remove_feature(, no_path_retry);
- } else {
+ } else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
add_feature(, no_path_retry);
}
if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v4 11/11] libmultipath: don't [un]set queue_if_no_path after domap
We set the queue_if_no_path feature in assemble_map() already,
no need to set it here again.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/configure.c | 15 ---
1 file changed, 15 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 03874f47..98589150 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1053,21 +1053,6 @@ int coalesce_paths (struct vectors * vecs, vector newmp,
char * refwwid,
remove_feature(>features,
"queue_if_no_path");
}
- else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF) {
- if (mpp->no_path_retry == NO_PATH_RETRY_FAIL) {
- condlog(3, "%s: unset queue_if_no_path feature",
- mpp->alias);
- if (!dm_queue_if_no_path(mpp->alias, 0))
- remove_feature(>features,
- "queue_if_no_path");
- } else {
- condlog(3, "%s: set queue_if_no_path feature",
- mpp->alias);
- if (!dm_queue_if_no_path(mpp->alias, 1))
- add_feature(>features,
- "queue_if_no_path");
- }
- }
if (!is_daemon && mpp->action != ACT_NOTHING) {
conf = get_multipath_config();
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap
On Thu, 2017-06-22 at 14:21 -0500, Benjamin Marzinski wrote: > On Thu, Jun 22, 2017 at 08:23:44AM +0200, Hannes Reinecke wrote: > > On 06/21/2017 05:06 PM, Martin Wilck wrote: > > > We set the queue_if_no_path feature in assemble_map() already, > > > no need to set it here again. > > > > > > Signed-off-by: Martin Wilck <mwi...@suse.com> > > > --- > > > libmultipath/configure.c | 15 --- > > > 1 file changed, 15 deletions(-) > > > > > > > > > > Watch out. > > 'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is > > active, > > and removed afterwards. > > So there might be valid reasons why it's set here. > > Have you checked? > > IIRC, we used to always set queue_if_no_path before reloading the > map, Why? I've searched for comments explaining that but found nothing. Anyway, we've ceased to do so at least since 7331cf5 "Correctly update feature strings", merged in May 2011. > and then call setup_multipath() afterwards, which would call > set_no_path_retry() to set it to the actual correct value. setup_multipath() is called too, a bit later in the code flow, from configure() after coalesce_paths() and coalesce_maps(). So we're currently setting queue_if_no_path 3 times when creating maps: in domap(), after domap(), and in setup_multipath->set_no_path_retry(). With my patch, we do it only twice :-) The call to dm_queue_if_no_path directly after domap(), which my patch removes, is ancient, it came with 0e6e3113 "[multipath] Extension of the "no_path_retry" scope to the multipath" in 2005. > If we are > correctly setting the feature line when we reload the map, that's a > better solution. Obviously, we can't strip set_no_path_retry() out of > __setup_multipath() since we rely on that to deal with deal with > going > into recovery mode. However, without some more thought and code > reading, > I'm not sure if we do still need the calls to dm_queue_if_no_path() > there for some other reason anymore. At any rate, they won't hurt > anything, except for causing a redundant call to device-mapper. > > Also, if we're yanking these dm_queue_if_no_path() calls from > coalesce_paths, I don't see why we need them in reload_map(), where > they > also seem redundant if we just correctly set the features argument > when > we reloaded the table. > > ACK, but I wouldn't mind seeing the calls pulled from reload_map as > well. Agreed, but I propose to do that in a separate patch. No need to repost the whole series for that. Martin -- Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH] kpartx.rules: Fix syntax error in skip_kpartx code
Fixes: 22736419 "kpartx.rules: respect skip_kpartx flag"
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/kpartx.rules | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index a9587917..64d550de 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -37,7 +37,7 @@ ENV{ID_FS_USAGE}=="filesystem|other",
ENV{ID_FS_LABEL_ENC}=="?*", \
# Create dm tables for partitions
ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end"
-ENV{ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
+ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end"
ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \
RUN+="/sbin/kpartx -un -p -part /dev/$name"
--
2.13.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+
On Fri, 2017-06-23 at 19:25 +0200, Xose Vazquez Perez wrote:
> On 06/22/2017 04:59 PM, Martin Wilck wrote:
>
> > Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
> > let dm detach device handlers") imply "retain_attached_hw_handler
> > yes".
> >
> > Clarify this in the propsel code, log messages, and documentation.
> >
> > Signed-off-by: Martin Wilck <mwi...@suse.com>
> > Reviewed-by: Hannes Reinecke <h...@suse.com>
> > ---
> > libmultipath/configure.c | 3 ++-
> > libmultipath/dmparser.c| 3 ++-
> > libmultipath/propsel.c | 7 ++-
> > libmultipath/util.c| 36
> >
> > libmultipath/util.h| 2 ++
> > multipath/multipath.conf.5 | 15 +++
> > 6 files changed, 59 insertions(+), 7 deletions(-)
>
> [...]
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config
> > *conf, struct multipath *mp)
> >
> > if (!VERSION_GE(conf->version, minv_dm_retain)) {
> > mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
> > - origin = "(setting: WARNING, requires kernel
> > version >= 1.5.0)";
> > + origin = "(setting: WARNING, requires kernel dm-
> > mpath version >= 1.5.0)";
>
> It would be more informative replace the dm-mpath version with the
> kernel version. No one cares about subsystems versions.
I disagree. This code should also work for vendor kernels which may
e.g. contain patches to update dm-mpath without updating the main
kernel (utsname) version.
The reason I used get_linux_version_code() for the new check my patch
introduced was that unfortunately, the dm-mpath version has not been
changed when the "retain_attached_hwhandler" feature was removed in
4.3. The next dm-mpath version change (to 1.10) happened in 4.4.
Thus I couldn't use the dm-mpath version and had to fallback to
utsname.
Thinking about it, the new check should probably be (dm_mpath version
>= 1.10 OR kernel verson >= 4.3). IMO that can be handled in an
incremental patch.
Regards,
Martin
--
Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 3/7] libmultipath: clarify option conflicts for "features"
Hi Ben,
On Thu, 2017-06-15 at 14:54 -0500, Benjamin Marzinski wrote:
> On Wed, Jun 14, 2017 at 12:55:50AM +0200, Martin Wilck wrote:
> > The "features" option in multipath.conf can possibly conflict
> > with the "no_path_retry" and "retain_attached_hw_handler" options.
> >
> > Currently, "no_path_retry" takes precedence, unless it is set to
> > "fail", in which case it's overridden. No precedence rules are
> > defined for "retain_attached_hw_handler".
> >
> > Make this behavior more consistent by always giving precedence
> > to the explicit config file options, and improve logging.
> >
> > Signed-off-by: Martin Wilck <mwi...@suse.com>
> > ---
> > libmultipath/configure.c | 4 ++--
> > libmultipath/propsel.c | 37 --
> > ---
> > 2 files changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index bd090d9a..fd4721dd 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -280,11 +280,11 @@ int setup_map(struct multipath *mpp, char
> > *params, int params_size)
> > select_pgfailback(conf, mpp);
> > select_pgpolicy(conf, mpp);
> > select_selector(conf, mpp);
> > - select_features(conf, mpp);
> > select_hwhandler(conf, mpp);
> > + select_no_path_retry(conf, mpp);
> > + select_features(conf, mpp);
> > select_rr_weight(conf, mpp);
> > select_minio(conf, mpp);
> > - select_no_path_retry(conf, mpp);
> > select_mode(conf, mpp);
> > select_uid(conf, mpp);
> > select_gid(conf, mpp);
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 99d17e65..4267aa04 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -272,6 +272,9 @@ out:
> > int select_features(struct config *conf, struct multipath *mp)
> > {
> > char *origin;
> > + char buff[12];
> > + static const char q_i_n_p[] = "queue_if_no_path";
> > + static const char r_a_h_h[] =
> > "retain_attached_hw_handler";
> >
> > mp_set_mpe(features);
> > mp_set_ovr(features);
> > @@ -280,19 +283,30 @@ int select_features(struct config *conf,
> > struct multipath *mp)
> > mp_set_default(features, DEFAULT_FEATURES);
> > out:
> > mp->features = STRDUP(mp->features);
> > - condlog(3, "%s: features = \"%s\" %s", mp->alias, mp-
> > >features, origin);
> >
> > - if (strstr(mp->features, "queue_if_no_path")) {
> > - if (mp->no_path_retry == NO_PATH_RETRY_UNDEF)
> > + if (strstr(mp->features, q_i_n_p)) {
> > + if (mp->no_path_retry == NO_PATH_RETRY_UNDEF) {
> > mp->no_path_retry = NO_PATH_RETRY_QUEUE;
> > - else if (mp->no_path_retry == NO_PATH_RETRY_FAIL)
> > {
> > - condlog(1, "%s: config error, overriding
> > 'no_path_retry' value",
> > - mp->alias);
> > - mp->no_path_retry = NO_PATH_RETRY_QUEUE;
> > - } else if (mp->no_path_retry !=
> > NO_PATH_RETRY_QUEUE)
> > - condlog(1, "%s: config error, ignoring
> > 'queue_if_no_path' because no_path_retry=%d",
> > - mp->alias, mp->no_path_retry);
> > + print_no_path_retry(buff, sizeof(buff),
> > + >no_path_retry);
> > + condlog(3, "%s: no_path_retry = %s
> > (inherited setting from feature '%s')",
> > + mp->alias, buff, q_i_n_p);
> > + } else {
> > + print_no_path_retry(buff, sizeof(buff),
> > + >no_path_retry);
> > + condlog(2, "%s: ignoring feature '%s'
> > because no_path_retry is set to '%s'",
> > + mp->alias, q_i_n_p, buff);
> > + remove_feature(>features, q_i_n_p);
> > + };
>
> This is just a nit, and it won't hurt anything by remaining unfixed,
> but
> it is odd that if you go into select_features() with no_path_retry
> set
> to queue (for any length of time) and features includes
> "queue_if_no_path", you will exit with no_path_retry still set to
> queue
> and "queue_if_no_path" removed from fea
[dm-devel] [PATCH] Clarify commit message in select_max_sectors_kb()
Do you like it better this way? --- libmultipath/propsel.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index f11052f2..6d628052 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -798,8 +798,9 @@ int select_max_sectors_kb(struct config *conf, struct multipath * mp) mp_set_conf(max_sectors_kb); mp_set_default(max_sectors_kb, DEFAULT_MAX_SECTORS_KB); /* -* In the default case, we will not modify max_sectors_kb. -* Don't print a log message to avoid user confusion. +* In the default case, we will not modify max_sectors_kb in sysfs +* (see sysfs_set_max_sectors_kb()). +* Don't print a log message here to avoid user confusion. */ return 0; out: -- 2.13.1 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 08/15] dm mpath: merge do_end_io_bio into multipath_end_io_bio
On Thu, 2017-05-18 at 15:18 +0200, Christoph Hellwig wrote:
> This simplifies the code and especially the error passing a bit and
> will help with the next patch.
>
> Signed-off-by: Christoph Hellwig <h...@lst.de>
> ---
> drivers/md/dm-mpath.c | 42 -
> -
> 1 file changed, 16 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 3df056b73b66..b1cb0273b081 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -1510,24 +1510,26 @@ static int multipath_end_io(struct dm_target
> *ti, struct request *clone,
> return r;
> }
>
> -static int do_end_io_bio(struct multipath *m, struct bio *clone,
> - int error, struct dm_mpath_io *mpio)
> +static int multipath_end_io_bio(struct dm_target *ti, struct bio
> *clone, int error)
> {
> + struct multipath *m = ti->private;
> + struct dm_mpath_io *mpio = get_mpio_from_bio(clone);
> + struct pgpath *pgpath = mpio->pgpath;
> unsigned long flags;
>
> - if (!error)
> - return 0; /* I/O complete */
> + BUG_ON(!mpio);
You dereferenced mpio already above.
Regards,
Martin
>
> - if (noretry_error(error))
> - return error;
> + if (!error || noretry_error(error))
> + goto done;
>
> - if (mpio->pgpath)
> - fail_path(mpio->pgpath);
> + if (pgpath)
> + fail_path(pgpath);
>
> if (atomic_read(>nr_valid_paths) == 0 &&
> !test_bit(MPATHF_QUEUE_IF_NO_PATH, >flags)) {
> dm_report_EIO(m);
> - return -EIO;
> + error = -EIO;
> + goto done;
> }
>
> /* Queue for the daemon to resubmit */
> @@ -1539,28 +1541,16 @@ static int do_end_io_bio(struct multipath *m,
> struct bio *clone,
> if (!test_bit(MPATHF_QUEUE_IO, >flags))
> queue_work(kmultipathd, >process_queued_bios);
>
> - return DM_ENDIO_INCOMPLETE;
> -}
> -
> -static int multipath_end_io_bio(struct dm_target *ti, struct bio
> *clone, int error)
> -{
> - struct multipath *m = ti->private;
> - struct dm_mpath_io *mpio = get_mpio_from_bio(clone);
> - struct pgpath *pgpath;
> - struct path_selector *ps;
> - int r;
> -
> - BUG_ON(!mpio);
> -
> - r = do_end_io_bio(m, clone, error, mpio);
> - pgpath = mpio->pgpath;
> + error = DM_ENDIO_INCOMPLETE;
> +done:
> if (pgpath) {
> - ps = >pg->ps;
> + struct path_selector *ps = >pg->ps;
> +
> if (ps->type->end_io)
> ps->type->end_io(ps, >path, mpio-
> >nr_bytes);
> }
>
> - return r;
> + return error;
> }
>
> /*
--
Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH] mpathpersist.8: add missing documentation for -K, -C, -l
Furthermore, add a reference to the sg_persist man page. --- mpathpersist/mpathpersist.8 | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/mpathpersist/mpathpersist.8 b/mpathpersist/mpathpersist.8 index 4b15666f..a8982e65 100644 --- a/mpathpersist/mpathpersist.8 +++ b/mpathpersist/mpathpersist.8 @@ -34,6 +34,12 @@ attribute must be defined in the \fI/etc/multipath.conf\fR file. Otherwise the \fBmultipathd\fR daemon will not check for persistent reservation for newly discovered paths or reinstated paths. . +.LP +\fBmpathpersist\fR supports the same command-line options as the +\fBsg_persist\fR utility. +. +Consult the \fBsg_persist (8)\fR manual page for an in-depth discussion of the +various options. . .\" .SH OPTIONS @@ -89,6 +95,10 @@ PR Out parameter 'APTPL'. PR In: Read Keys. . .TP +.BI \--param-rk=\fIRK\fB|\-K " RK" +PR Out parameter reservation key (RK is in hex, up to 8 bytes). +. +.TP .BI \--param-sark=\fISARK\fB|\-S " SARK" PR Out parameter service action reservation key (SARK is in hex). . @@ -97,6 +107,10 @@ PR Out parameter service action reservation key (SARK is in hex). PR Out: Preempt. . .TP +.B \--clear|\-C +PR Out: Clear registrations. +. +.TP .B \--preempt-abort|\-A PR Out: Preempt and Abort. . @@ -140,6 +154,10 @@ PR Out: Reserve. .BI \--transport-id=\fITIDS\fB|\-X " TIDS" TransportIDs can be mentioned in several forms. . +.TP +.BI \--alloc-length=\fILEN\fB|\-l " LEN" +PR In: maximum allocation length. LEN is a decimal number between 0 and 8192. +. . .\" .SH EXAMPLE @@ -164,7 +182,8 @@ Read the reservation status of the /dev/mapper/mpath9 device: .\" . .BR multipath (8), -.BR multipathd (8). +.BR multipathd (8), +.BR sg_persist (8). . . .\" -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: add comment about resuming
On Thu, 2017-05-18 at 16:37 -0500, Benjamin Marzinski wrote: > The reason for the second resume in my commit "libmultipath: fix > suspended devs from failed reloads" is not obvious from the multipath > code, so add a comment. Nice, thank you. Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 4/4] multipath: Fix a potential buffer overflow
Hi Bart, On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote: > Avoid that gcc 7 reports the following warning: > > cli_handlers.c:1340:18: warning: '%d' directive writing between 1 and > 3 bytes into a region of size 2 [-Wformat-overflow=] > sprintf(*reply,"%d",mpp->prflag); > > Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com> > --- > multipathd/cli_handlers.c | 14 ++ > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > index 04c73866..460fea1f 100644 > --- a/multipathd/cli_handlers.c > +++ b/multipathd/cli_handlers.c > @@ -1,6 +1,9 @@ > /* > * Copyright (c) 2005 Christophe Varoqui > */ > + > +#define _GNU_SOURCE > + > #include "checkers.h" > #include "memory.h" > #include "vector.h" > @@ -1332,14 +1335,9 @@ cli_getprstatus (void * v, char ** reply, int > * len, void * data) > > condlog(3, "%s: prflag = %u", param, (unsigned int)mpp- > >prflag); > > - *reply =(char *)malloc(2); > - *len = 2; > - memset(*reply,0,2); > - > - > - sprintf(*reply,"%d",mpp->prflag); > - (*reply)[1]='\0'; > - > + *len = asprintf(reply, "%d", mpp->prflag); > + if (*len < 0) > + return 1; > > condlog(3, "%s: reply = %s", param, *reply); > how about this simpler patch, as prflag is actually a boolean? diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c index 04c73866..c31ebd34 100644 --- a/multipathd/cli_handlers.c +++ b/multipathd/cli_handlers.c @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data) memset(*reply,0,2); - sprintf(*reply,"%d",mpp->prflag); + sprintf(*reply, "%d", !!mpp->prflag); (*reply)[1]='\0'; -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 3/4] libmultipath/datacore: Remove dead code
On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote:
> Remove those variables a value is assigned to but that are never
> used. This patch avoids that gcc 7 reports the following warning:
>
> datacore.c:98:22: warning: '
>' directive output may be truncated writing 1 byte into a region
> of size between 0 and 8 [-Wformat-truncation=]
> snprintf(vendor, 8, "%.8s\n", inqBuffp + 8);
>
> Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
> ---
> libmultipath/prioritizers/datacore.c | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/libmultipath/prioritizers/datacore.c
> b/libmultipath/prioritizers/datacore.c
> index 36465ac4..59c98164 100644
> --- a/libmultipath/prioritizers/datacore.c
> +++ b/libmultipath/prioritizers/datacore.c
> @@ -35,10 +35,6 @@
> int datacore_prio (const char *dev, int sg_fd, char * args)
> {
> int k;
> - char vendor[8];
> - char product[32];
> - char luname[32];
> - char wwpn[32];
> char sdsname[32];
> unsigned char inqCmdBlk[INQ_CMD_LEN] = { INQ_CMD_CODE, 0, 0,
> 0, INQ_REPLY_LEN, 0 };
> unsigned char inqBuff[INQ_REPLY_LEN];
> @@ -95,11 +91,7 @@ int datacore_prio (const char *dev, int sg_fd,
> char * args)
> if ((io_hdr.info & SG_INFO_OK_MASK) != SG_INFO_OK)
> return 0;
>
> - snprintf(vendor, 8, "%.8s\n", inqBuffp + 8);
> - snprintf(product, 17, "%.16s", inqBuffp + 16);
> - snprintf(luname, 21, "%.19s", inqBuffp + 36);
> - snprintf(wwpn, 17, "%.16s", inqBuffp + 96);
> - snprintf(sdsname, 17, "%.16s", inqBuffp + 112);
> + snprintf(sdsname, sizeof(sdsname), "%.16s", inqBuffp + 112);
>
> if (strstr(sdsname , preferredsds))
> return 1;
I was wondering whether passing sizeof(sdsname) rather than 17 might
make a difference, and verified that that isn't the case.
Reviewed-by: Martin Wilck <mwi...@suse.com>
--
Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 4/4] multipath: Fix a potential buffer overflow
On Tue, 2017-06-13 at 12:53 -0700, Bart Van Assche wrote: > On 06/13/17 12:29, Martin Wilck wrote: > > how about this simpler patch, as prflag is actually a boolean? > > > > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > > index 04c73866..c31ebd34 100644 > > --- a/multipathd/cli_handlers.c > > +++ b/multipathd/cli_handlers.c > > @@ -1337,7 +1337,7 @@ cli_getprstatus (void * v, char ** reply, int > > * len, void * data) > > memset(*reply,0,2); > > > > > > - sprintf(*reply,"%d",mpp->prflag); > > + sprintf(*reply, "%d", !!mpp->prflag); > > (*reply)[1]='\0'; > > Hello Martin, > > Every sprintf() call requires careful analysis to see whether or not > it > triggers a buffer overflow. I really would like to get rid of that > sprintf() call. Then we could write snprintf(*reply, 2, "%d", !!mpp->prflag); without needing _GNU_SOURCE. Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 2/4] libmultipath: Simplify assemble_map()
On Tue, 2017-06-13 at 09:33 -0700, Bart Van Assche wrote: > Introduce a macro for appending formatted text to the output buffer. > Eliminate the local variables 'shift' and 'freechar'. Move the > code for freeing a temporary buffer to the end of the function. > Handle snprintf() conversion errors. This patch avoids that gcc 7 > reports the following warning: > > dmparser.c:137:20: warning: '__builtin_snprintf' output truncated > before the last format character [-Wformat-truncation=] > snprintf(p, 1, "\n"); > ^ > > Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com> > --- > libmultipath/dmparser.c | 67 +++-- > > 1 file changed, 31 insertions(+), 36 deletions(-) > Reviewed-by: Martin Wilck <mwi...@suse.com> -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] libmultipath issue
Hi James, On Tue, 2017-06-13 at 11:19 -0500, James Shoemaker wrote: >I have a piece of hardware, a 16 bay storagetek optically > connected SAN to > be specific, that returns empty string for Device id. > >I did the following patch to resolve the issue, it basically > allows empty > string for vendor and product, I couldn't find anywhere that had an > issue with > it.. > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 663c8ea..92d0e49 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -1149,12 +1149,12 @@ scsi_sysfs_pathinfo (struct path * pp, vector > hwtable) > if (!attr_path || pp->sg_id.host_no == -1) > return 1; > > - if (sysfs_get_vendor(parent, pp->vendor_id, SCSI_VENDOR_SIZE) > <= 0) > + if (sysfs_get_vendor(parent, pp->vendor_id, SCSI_VENDOR_SIZE) > < 0) > return 1; > > condlog(3, "%s: vendor = %s", pp->dev, pp->vendor_id); > > - if (sysfs_get_model(parent, pp->product_id, > SCSI_PRODUCT_SIZE) <= 0) > + if (sysfs_get_model(parent, pp->product_id, > SCSI_PRODUCT_SIZE) < 0) > return 1; > > condlog(3, "%s: product = %s", pp->dev, pp->product_id); In general, yes, I think this would work. vendor_id and product_id are primarily used to lookup parameters in the hwtable, and there we have no entries with empty vendor or product. One side issue that needs to be considered is printout, though: snprint_multipath_(vpr|prod|vend) will just print nothing if either vendor or product has zero length. It would be good if they'd print e.g. "" and "" instead. Regards Martin PS: Patch in plain text format would be appreciated. -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 4/4] multipath: Fix a potential buffer overflow
Hello Bart, On Tue, 2017-06-13 at 13:21 -0700, Bart Van Assche wrote: > > > > Hello Martin, > > > > > > Every sprintf() call requires careful analysis to see whether or > > > not > > > it > > > triggers a buffer overflow. I really would like to get rid of > > > that > > > sprintf() call. > > > > Then we could write > > > > snprintf(*reply, 2, "%d", !!mpp->prflag); > > > > without needing _GNU_SOURCE. > > Hello Martin, > > There are already three other multipath-tools source files that > #define > _GNU_SOURCE so I don't see what's wrong with using _GNU_SOURCE. Yes, I saw that. I haven't reviewed the reason why _GNU_SOURCE is used in the other places. In general it's a thing I'd rather avoid for portability reasons. In this particular case, I think the problem at hand be easily solved without resorting to _GNU_SOURCE. But well, it's not a thing worth fighting about. May Christophe decide. Martin -- Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH 0/7] no_path_retry/queue_if_no_path logic & logging/man page fixes
This patch set attempts to sanitize the logic used for consistently handling options that can be set both via the "features" string and explicit multipath.conf options. This is most prominently "no_path_retry" vs. "queue_if_no_path", but also "retain_attached_hw_handler" vs. the feature of the same name. The logic this patch set follows is: - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored (this is the case nowadays for almost all "real" storage arrays, thanks to hwtable). - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry queue". ... likewise for "retain_attached_hw_handler". The logic is implemented in select_features() (which needs to be called after select_no_path_retry()) for multipath groups, and similarly in merge_hwe() for hwtable entries. The actual feature setting is made in assemble_map(), I also fix a problem there. The patch set documents the behavior in the man page, and adds some more man page fixes. Finally, by skipping superfluous default initializations in load_config(), the log messages for the respective config settings become more appropriate. Review and comments are highly welcome. Regards, Martin Martin Wilck (7): libmultipath: load_config: skip setting unnecessary defaults libmultipath: add/remove_feature: use const char* for feature libmultipath: clarify option conflicts for "features" libmultipath: merge_hwe: fix queue_if_no_path logic libmultipath: assemble_map: fix queue_if_no_path logic multipath.conf.5: document no_path_retry vs. queue_if_no_path multipath.conf.5: Remove ??? and other minor fixes libmultipath/config.c | 36 ++-- libmultipath/configure.c | 4 ++-- libmultipath/dmparser.c| 5 ++--- libmultipath/propsel.c | 39 +- libmultipath/structs.c | 30 +- libmultipath/structs.h | 4 ++-- multipath/multipath.conf.5 | 52 ++ 7 files changed, 92 insertions(+), 78 deletions(-) -- 2.13.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 4/7] libmultipath: merge_hwe: fix queue_if_no_path logic
The logic applied here should match the logic in select_features().
If no_path_retry is anything but "undef", queue_if_no_path can be
removed from the feature string, assemble_map() will infer it
correctly.
The case where no_path_retry is undefined and "queue_if_no_path"
is set is treated as if "no_path_retry queue" had been set.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/config.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 61bbba91..b928fbe7 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -355,12 +355,24 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
/*
* Make sure features is consistent with
-* no_path_retry
+* no_path_retry.
+* The logic should be consistent with select_features().
+* The actual queue_if_no_path feature is set in assemble_map().
*/
- if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
+ if (dst->no_path_retry == NO_PATH_RETRY_UNDEF &&
+ strstr(dst->features, "queue_if_no_path")) {
+ condlog(3, "%s/%s: 'queue_if_no_path' is set, assuming
no_path_retry='queue'",
+ dst->vendor, dst->product);
+ dst->no_path_retry = NO_PATH_RETRY_QUEUE;
+ }
+ else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF) {
+ condlog(3, "%s/%s: 'no_path_retry' is set, ignoring
'queue_if_no_path'",
+ dst->vendor, dst->product);
remove_feature(>features, "queue_if_no_path");
- else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
- add_feature(>features, "queue_if_no_path");
+ }
+
+ if (dst->retain_hwhandler != RETAIN_HWHANDLER_UNDEF)
+ remove_feature(>features, "retain_attached_hw_handler");
return 0;
}
--
2.13.0
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 5/7] libmultipath: assemble_map: fix queue_if_no_path logic
It is wrong to remove the queue_if_no_path feature if no_path_retry
is unset. Rather, in this case the feature should neither be added
nor removed.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/dmparser.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 469e60d2..8d0c7af1 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -74,13 +74,12 @@ assemble_map (struct multipath * mp, char * params, int len)
* We have to set 'queue_if_no_path' here even
* to avoid path failures during map reload.
*/
- if (mp->no_path_retry == NO_PATH_RETRY_UNDEF ||
- mp->no_path_retry == NO_PATH_RETRY_FAIL) {
+ if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
/* remove queue_if_no_path settings */
condlog(3, "%s: remove queue_if_no_path from '%s'",
mp->alias, mp->features);
remove_feature(, no_path_retry);
- } else {
+ } else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
add_feature(, no_path_retry);
}
if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
--
2.13.0
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/7] libmultipath: add/remove_feature: use const char* for feature
Change the argument type for the feature to add or remove to
const char*, making it possible to pass const strings without
warnings.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/structs.c | 30 --
libmultipath/structs.h | 4 ++--
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e225f8b4..28704676 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -513,10 +513,11 @@ void setup_feature(struct multipath *mpp, char *feature)
}
}
-int add_feature(char **f, char *n)
+int add_feature(char **f, const char *n)
{
int c = 0, d, l = 0;
char *e, *p, *t;
+ const char *q;
if (!f)
return 1;
@@ -554,14 +555,14 @@ int add_feature(char **f, char *n)
if ((c % 10) == 9)
l++;
c++;
- p = n;
- while (*p != '\0') {
- if (*p == ' ' && p[1] != '\0' && p[1] != ' ') {
+ q = n;
+ while (*q != '\0') {
+ if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
if ((c % 10) == 9)
l++;
c++;
}
- p++;
+ q++;
}
t = MALLOC(l + 1);
@@ -601,10 +602,11 @@ int add_feature(char **f, char *n)
return 0;
}
-int remove_feature(char **f, char *o)
+int remove_feature(char **f, const char *o)
{
int c = 0, d, l;
char *e, *p, *n;
+ const char *q;
if (!f || !*f)
return 1;
@@ -630,18 +632,18 @@ int remove_feature(char **f, char *o)
/* Just spaces, return */
if (*o == '\0')
return 0;
- e = o + strlen(o);
- while (*e == ' ')
- e--;
- d = (int)(e - o);
+ q = o + strlen(o);
+ while (*q == ' ')
+ q--;
+ d = (int)(q - o);
/* Update feature count */
c--;
- p = o;
- while (p[0] != '\0') {
- if (p[0] == ' ' && p[1] != ' ' && p[1] != '\0')
+ q = o;
+ while (q[0] != '\0') {
+ if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0')
c--;
- p++;
+ q++;
}
/* Quick exit if all features have been removed */
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 01e031ad..8ea984d9 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -369,8 +369,8 @@ int pathcountgr (struct pathgroup *, int);
int pathcount (struct multipath *, int);
int pathcmp (struct pathgroup *, struct pathgroup *);
void setup_feature(struct multipath *, char *);
-int add_feature (char **, char *);
-int remove_feature (char **, char *);
+int add_feature (char **, const char *);
+int remove_feature (char **, const char *);
extern char sysfs_path[PATH_SIZE];
--
2.13.0
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 1/7] libmultipath: load_config: skip setting unnecessary defaults
We have the logic for setting defaults for paths and maps in propsel.c. By pre-setting conf values with defaults in load_config(), we generate irritating log messages like 'features = "0" (setting: multipath.conf defaults/devices section)' even if multipath.conf doesn't contain a 'features' setting at all. With this patch, these defaults are correctly logged as "multipath internal". Signed-off-by: Martin Wilck <mwi...@suse.com> --- libmultipath/config.c | 16 libmultipath/propsel.c | 2 +- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index bb6619b3..61bbba91 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -599,40 +599,24 @@ load_config (char * file) if (!conf->verbosity) conf->verbosity = DEFAULT_VERBOSITY; - conf->minio = DEFAULT_MINIO; - conf->minio_rq = DEFAULT_MINIO_RQ; get_sys_max_fds(>max_fds); conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE); conf->wwids_file = set_default(DEFAULT_WWIDS_FILE); conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR); - conf->features = set_default(DEFAULT_FEATURES); - conf->flush_on_last_del = DEFAULT_FLUSH; conf->attribute_flags = 0; conf->reassign_maps = DEFAULT_REASSIGN_MAPS; conf->checkint = DEFAULT_CHECKINT; conf->max_checkint = 0; - conf->pgfailback = DEFAULT_FAILBACK; - conf->fast_io_fail = DEFAULT_FAST_IO_FAIL; - conf->retain_hwhandler = DEFAULT_RETAIN_HWHANDLER; - conf->detect_prio = DEFAULT_DETECT_PRIO; - conf->detect_checker = DEFAULT_DETECT_CHECKER; conf->force_sync = DEFAULT_FORCE_SYNC; conf->partition_delim = DEFAULT_PARTITION_DELIM; conf->processed_main_config = 0; conf->find_multipaths = DEFAULT_FIND_MULTIPATHS; conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT; - conf->uid_attribute = set_default(DEFAULT_UID_ATTRIBUTE); conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES; conf->retrigger_delay = DEFAULT_RETRIGGER_DELAY; conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT; - conf->deferred_remove = DEFAULT_DEFERRED_REMOVE; - conf->skip_kpartx = DEFAULT_SKIP_KPARTX; conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS; conf->remove_retries = 0; - conf->max_sectors_kb = DEFAULT_MAX_SECTORS_KB; - conf->san_path_err_threshold = DEFAULT_ERR_CHECKS; - conf->san_path_err_forget_rate = DEFAULT_ERR_CHECKS; - conf->san_path_err_recovery_time = DEFAULT_ERR_CHECKS; /* * preload default hwtable diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index 385063aa..99d17e65 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -752,7 +752,7 @@ int select_max_sectors_kb(struct config *conf, struct multipath * mp) mp_set_ovr(max_sectors_kb); mp_set_hwe(max_sectors_kb); mp_set_conf(max_sectors_kb); - return 0; + mp_set_default(max_sectors_kb, DEFAULT_MAX_SECTORS_KB); out: condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb, origin); -- 2.13.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 3/7] libmultipath: clarify option conflicts for "features"
The "features" option in multipath.conf can possibly conflict
with the "no_path_retry" and "retain_attached_hw_handler" options.
Currently, "no_path_retry" takes precedence, unless it is set to
"fail", in which case it's overridden. No precedence rules are
defined for "retain_attached_hw_handler".
Make this behavior more consistent by always giving precedence
to the explicit config file options, and improve logging.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/configure.c | 4 ++--
libmultipath/propsel.c | 37 -
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index bd090d9a..fd4721dd 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -280,11 +280,11 @@ int setup_map(struct multipath *mpp, char *params, int
params_size)
select_pgfailback(conf, mpp);
select_pgpolicy(conf, mpp);
select_selector(conf, mpp);
- select_features(conf, mpp);
select_hwhandler(conf, mpp);
+ select_no_path_retry(conf, mpp);
+ select_features(conf, mpp);
select_rr_weight(conf, mpp);
select_minio(conf, mpp);
- select_no_path_retry(conf, mpp);
select_mode(conf, mpp);
select_uid(conf, mpp);
select_gid(conf, mpp);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 99d17e65..4267aa04 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -272,6 +272,9 @@ out:
int select_features(struct config *conf, struct multipath *mp)
{
char *origin;
+ char buff[12];
+ static const char q_i_n_p[] = "queue_if_no_path";
+ static const char r_a_h_h[] = "retain_attached_hw_handler";
mp_set_mpe(features);
mp_set_ovr(features);
@@ -280,19 +283,30 @@ int select_features(struct config *conf, struct multipath
*mp)
mp_set_default(features, DEFAULT_FEATURES);
out:
mp->features = STRDUP(mp->features);
- condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
- if (strstr(mp->features, "queue_if_no_path")) {
- if (mp->no_path_retry == NO_PATH_RETRY_UNDEF)
+ if (strstr(mp->features, q_i_n_p)) {
+ if (mp->no_path_retry == NO_PATH_RETRY_UNDEF) {
mp->no_path_retry = NO_PATH_RETRY_QUEUE;
- else if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
- condlog(1, "%s: config error, overriding
'no_path_retry' value",
- mp->alias);
- mp->no_path_retry = NO_PATH_RETRY_QUEUE;
- } else if (mp->no_path_retry != NO_PATH_RETRY_QUEUE)
- condlog(1, "%s: config error, ignoring
'queue_if_no_path' because no_path_retry=%d",
- mp->alias, mp->no_path_retry);
+ print_no_path_retry(buff, sizeof(buff),
+ >no_path_retry);
+ condlog(3, "%s: no_path_retry = %s (inherited setting
from feature '%s')",
+ mp->alias, buff, q_i_n_p);
+ } else {
+ print_no_path_retry(buff, sizeof(buff),
+ >no_path_retry);
+ condlog(2, "%s: ignoring feature '%s' because
no_path_retry is set to '%s'",
+ mp->alias, q_i_n_p, buff);
+ remove_feature(>features, q_i_n_p);
+ };
}
+ if (strstr(mp->features, r_a_h_h) &&
+ mp->retain_hwhandler == RETAIN_HWHANDLER_OFF) {
+ condlog(2, "%s: ignoring feature '%s' because %s is set to
'off'",
+ mp->alias, r_a_h_h, r_a_h_h);
+ remove_feature(>features, r_a_h_h);
+ }
+
+ condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
return 0;
}
@@ -469,9 +483,6 @@ out:
if (origin)
condlog(3, "%s: no_path_retry = %s %s", mp->alias, buff,
origin);
- else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF)
- condlog(3, "%s: no_path_retry = %s (inherited setting)",
- mp->alias, buff);
else
condlog(3, "%s: no_path_retry = undef (setting: multipath
internal)",
mp->alias);
--
2.13.0
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 7/7] multipath.conf.5: Remove ??? and other minor fixes
Remove the FIXME markers by filling in missing content, and make some other minor fixes. Signed-off-by: Martin Wilck <mwi...@suse.com> --- multipath/multipath.conf.5 | 48 +- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index e6944faf..033e2052 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -258,7 +258,7 @@ Return a constant priority of \fI1\fR. .I sysfs Use the sysfs attributes \fIaccess_state\fR and \fIpreferred_path\fR to generate the path priority. This prioritizer accepts the optional prio_arg -\fIexclusive_pref_bit\fR +\fIexclusive_pref_bit\fR. .TP .I emc (Hardware-dependent) @@ -296,14 +296,19 @@ Generate the path priority based on the regular expression and the priority provided as argument. Requires prio_args keyword. .TP .I datacore -.\" XXX -???. Requires prio_args keyword. +(Hardware-dependent) +Generate the path priority for some Datacore storage arrays. Requires prio_args +keyword. .TP .I iet -.\" XXX -???. Requires prio_args keyword. -.TP -The default is: \fBconst\fR +(iSCSI only) +Generate path priority for iSCSI targets based on IP address. Requires +prio_args keyword. +.PP +The default depends on the \fBdetect_prio\fR setting: If \fBdetect_prio\fR is +\fByes\fR (default), the default priority algorithm is \fBsysfs\fR (except for +NetAPP E-Series, where it is \fBalua\fR). If \fBdetect_prio\fR is +\fBno\fR, the default priority algorithm is \fBconst\fR. .RE . . @@ -344,12 +349,12 @@ If \fIexclusive_pref_bit\fR is set, paths with the \fIpreferred path\fR bit set will always be in their own path group. .TP .I datacore -.\" XXX -\fIpreferredsds\fR ???. +\fIpreferredsds\fR (required) denotes the preferred "SDS name" for datacore +arrays. \fItimeout\fR (optional) is the timeout for the INQUIRY, in ms. .TP .I iet -.\" XXX -\fIpreferredip\fR ???. +\fIpreferredip=...\fR (required) denotes the preferred IP address (in dotted decimal +notation) for iSCSI targets. .TP The default is: \fB\fR .RE @@ -364,29 +369,28 @@ Possible values for the feature list are: .TP 12 .\" XXX .I queue_if_no_path -(Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. +(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active. Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this feature and \fIno_path_retry\fR are set, the latter value takes precedence. See KNOWN ISSUES. .TP -.I no_partitions -Disable automatic partitions generation via kpartx. -.TP .\" XXX .I pg_init_retries -(Since ??? kernel) Number of times to retry pg_init, it must be between 1 and 50. +(Since kernel 2.6.24) Number of times to retry pg_init, it must be between 1 and 50. .TP .\" XXX .I pg_init_delay_msecs -(Since ??? kernel) Number of msecs before pg_init retry, it must be between 0 and 6. +(Since kernel 2.6.38) Number of msecs before pg_init retry, it must be between 0 and 6. .TP .\" XXX .I queue_mode -(Since ??? kernel) Select the the queue_mode per multipath device. -Where can be \fIbio\fR, \fIrq\fR or \fImq\fR. Which corresponds to -bio-based, request_fn rq-based, and blk-mq rq-based respectively. -.TP -The default is: \fB0\fR +(Since kernel 4.8) Select the the queueing mode per multipath device. + can be \fIbio\fR, \fIrq\fR or \fImq\f, which corresponds to +bio-based, request-based, and block-multiqueue (blk-mq) request-based, +respectively. + +The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR. It is +\fImq\fR if the latter is set, and \fIrq\fR otherwise. .RE . . -- 2.13.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 6/7] multipath.conf.5: document no_path_retry vs. queue_if_no_path
Clarify the documentation about option precedence. Signed-off-by: Martin Wilck <mwi...@suse.com> --- multipath/multipath.conf.5 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index f04ff194..e6944faf 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -365,7 +365,9 @@ Possible values for the feature list are: .\" XXX .I queue_if_no_path (Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active. -Identical to the \fIno_path_retry\fR with \fIqueue\fR value. See KNOWN ISSUES. +Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this +feature and \fIno_path_retry\fR are set, the latter value takes +precedence. See KNOWN ISSUES. .TP .I no_partitions Disable automatic partitions generation via kpartx. -- 2.13.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v5 1/1] multipath-tools: Prioritizer based on a latency algorithm
Hello Yang,
thanks for your work, and apologies for making you wait so long.
I still have some suggestions. Please see below.
On Fri, 2017-06-09 at 17:58 +0800, Yang Feng wrote
> libmultipath/prioritizers: Prioritizer for device mapper multipath,
> where the corresponding priority values of specific paths are
> provided
> by a latency algorithm. And the latency algorithm is dependent on the
> following arguments(latency_interval and io_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 latency can be calculated.
> 2. According to the average latency of each path and the weight value
> "latency_interval", the priority "rc" of each path can be provided.
>
>latency_interval latency_interval latency_interval laten
> cy_interval
> |--|--|--|...|
> --|
> | priority rank 1 | priority rank 2 | priority rank 3
> |...| priority rank x |
> |--|--|--|...|
> --|
> Priority Rank Partitioning
>
> Signed-off-by: Yang Feng <philip.y...@huawei.com>
> Reviewed-by: Benjamin Marzinski <bmarz...@redhat.com>
> Reviewed-by: Martin Wilck <mwi...@suse.com>
> Reviewed-by: Xose Vazquez Perez <xose.vazq...@gmail.com>
> Reviewed-by: Hannes Reinecke <h...@suse.de>
>
> +#define MAX_LATENCY_INTERVAL10/*unit: s*/
> +#define MIN_LATENCY_INTERVAL1 /*unit: us, or ms, or s*/
Why not use the same unit or both values?
> +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.
> +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".
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.
> +
> +/* 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, 1], us:[1, 1000] */
> +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".
> +/* 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".
> +
> +*ionum = (int)strtoul(source, , 10);
> +if
Re: [dm-devel] [PATCH 00/10] fixes for kpartx -d
On Mon, 2017-05-08 at 09:47 +0200, Martin Wilck wrote:
> The full partition UUID for partitions used by my code looks like
> this
> (see patch 08/10):
>
> {UUID_PREFIX}${MAJOR}:${MINOR}-${NONDM_UUID_SUFFIX}"
... where UUID_PREFIX is what kpartx uses anyway, i.e. "part$N".
Martin
--
Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 08/10] kpartx: use partition UUID for non-DM devices
On Fri, 2017-05-05 at 23:30 +0100, Alasdair G Kergon wrote:
> On Sat, May 06, 2017 at 12:05:57AM +0200, Martin Wilck wrote:
> > Introduce a "fake" UUID for these devices to make sure kpartx
> > deletes only devices it had created previously. Otherwise kpartx
> > might e.g. delete LVM LVs that are inside a device it is trying
> > to delete partitions for. It seems to be wiser to make sure the
> > user delete these manually before running "kpartx -d".
> >
> > With the fake UUID in place, we can re-introduce the UUID check
> > for non-DM device that was removed in the earlier patch
> > "kpartx: dm_remove_partmaps: support non-dm devices".
> >
> > This disables also deletion of partition mappings created
> > by earlier versions of kpartx. If kpartx has been updated after
> > partition mappings were created, the "-f" flag can be used
> > to force delting these partitions, too.
>
>
> Indeed - always supply a uuid for every dm device created.
> Add a standard prefix to the beginning of it, ideally "KPARTX-" to be
> consistent with other dm tools ("LVM-", "CRYPT-" etc.) if you're
> able to break compatibility, but "part" is still unique and
> adequate.
> (Stacked devices can end up with a stack of prefixes - only the
> leftmost one
> counts.)
As noted in my reply to Hannes' mail, I'm using the prefix "part%u-",
the same prefix that kpartx currently used for partitions of DM
devices.
The full format e.g. for /dev/mapper/loop0p1 is
"part1-7:0-kpartx-Wh5pYvM7uc60hh74"
and correspondingly, the "fake UUID" used for /dev/loop0 would be
"7:0-kpartx-Wh5pYvM7uc60hh74"
If you don't like the fact that this starts with "7:0", changing this
to e.g. "KPARTX-7.0-$something" is no problem as long as patch isn't
applied.
The last part ("Wh...") is a hard-coded string, just to avoid
accidental conflicts. I was wondering whether it would make sense to
some sort of encoded "host ID" to make this UUID truly "universal". My
personal opinion is that this would be over-engineered, because these
UUID will be visible only only on the host that called "kpartx -a" in
the first place.
Cheers,
Martin
--
Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 00/10] fixes for kpartx -d
On Fri, 2017-05-05 at 23:18 +0100, Alasdair G Kergon wrote: > On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote: > > 3) kpartx should only delete "partitions", which are single- > > target > > linear mappings into a block device. Other maps should not be > > touched. > > The prefix on the dm device's uuid should guarantee this: all devices > kpartx creates should have the same initial characters (a > not-quite-standard form "part" IIRC instead of "KPARTX-") and any > devices without those initial characters must be ignored. This works only for partitions on DM devices, not e.g. for loop devices. These devices obviously have no DM UUID; and thus kpartx also doesn't set an UUID for the partition devices it creates. That's the main point of this series. Martin -- Dr. Martin Wilck <mwi...@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
Re: [dm-devel] [PATCH 00/10] fixes for kpartx -d
On Sat, 2017-05-06 at 00:30 +0200, Martin Wilck wrote: > On Fri, 2017-05-05 at 23:18 +0100, Alasdair G Kergon wrote: > > On Sat, May 06, 2017 at 12:05:49AM +0200, Martin Wilck wrote: > > > 3) kpartx should only delete "partitions", which are single- > > > target > > > linear mappings into a block device. Other maps should not > > > be > > > touched. > > > > The prefix on the dm device's uuid should guarantee this: all > > devices > > kpartx creates should have the same initial characters (a > > not-quite-standard form "part" IIRC instead of "KPARTX-") and any > > devices without those initial characters must be ignored. > > This works only for partitions on DM devices, not e.g. for loop > devices. These devices obviously have no DM UUID; and thus kpartx > also > doesn't set an UUID for the partition devices it creates. > That's the main point of this series. Moreover: before even looking at the UUID, kpartx discards mappings that are not "linear" and don't map into the device in question. I added the additional case that it should also disregard mappings with two or more targets which can't be regarded as simple "linear" type (but in the current code, they are). With the UUID test in place, this may seem kind of redundant, but it follows the general logic that obvious non-partition devices should be discarded before checking the UUID. Martin -- Dr. Martin Wilck <mwi...@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
[dm-devel] [PATCH 08/10] kpartx: use partition UUID for non-DM devices
For dm devices, kpartx uses an UUID check at partition removal
to make sure it only deletes partitions it previously created.
Non-DM parent devices such as loop devices don't generally have
a UUID.
Introduce a "fake" UUID for these devices to make sure kpartx
deletes only devices it had created previously. Otherwise kpartx
might e.g. delete LVM LVs that are inside a device it is trying
to delete partitions for. It seems to be wiser to make sure the
user delete these manually before running "kpartx -d".
With the fake UUID in place, we can re-introduce the UUID check
for non-DM device that was removed in the earlier patch
"kpartx: dm_remove_partmaps: support non-dm devices".
This disables also deletion of partition mappings created
by earlier versions of kpartx. If kpartx has been updated after
partition mappings were created, the "-f" flag can be used
to force delting these partitions, too.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/devmapper.c | 2 +-
kpartx/kpartx.c| 29 +
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 5380b2e9..40ec26cf 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -505,7 +505,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
/*
* skip if uuids don't match
*/
- if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
+ if (uuid && dm_compare_uuid(uuid, names->name)) {
if (rd->verbose)
printf("%s: is not a kpartx partition. Not
removing\n",
names->name);
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index e2056b7f..d9057fba 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -60,6 +60,26 @@ struct pt {
int ptct = 0;
int udev_sync = 0;
+/*
+ * UUID format for partitions created on non-DM devices
+ * ${UUID_PREFIX}${MAJOR}:${MINOR}-${NONDM_UUID_SUFFIX}"
+ * The suffix should be sufficiently random to avoid unintended conflicts,
+ * and shouldn't be changed.
+ * The value below is a bas64-encoded 96-bit random number.
+ */
+#define NONDM_UUID_SUFFIX "-kpartx-Wh5pYvM7uc60hh74"
+
+static char *
+nondm_create_uuid(dev_t devt)
+{
+#define NONDM_UUID_BUFLEN (32 + sizeof(NONDM_UUID_SUFFIX))
+ static char uuid_buf[NONDM_UUID_BUFLEN];
+ snprintf(uuid_buf, sizeof(uuid_buf), "%u:%u%s",
+major(devt), minor(devt), NONDM_UUID_SUFFIX);
+ uuid_buf[NONDM_UUID_BUFLEN-1] = '\0';
+ return uuid_buf;
+}
+
static void
addpts(char *t, ptreader f)
{
@@ -360,6 +380,15 @@ main(int argc, char **argv){
uuid = dm_mapuuid(mapname);
}
+ /*
+* We are called for a non-DM device.
+* Make up a fake UUID for the device, unless "-d -f" is given.
+* This allows deletion of partitions created with older kpartx
+* versions which didn't use the fake UUID during creation.
+*/
+ if (!uuid && !(what == DELETE && force_devmap))
+ uuid = nondm_create_uuid(buf.st_rdev);
+
if (!mapname)
mapname = device + off;
else if (!force_devmap &&
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 02/10] kpartx: avoid ioctl error for loop devices
Commit 3d709241 causes kpartx to attempt a dm ioctl on a loop
device. This causes an error message
"device-mapper: table ioctl on loop4 failed: No such device or address".
Fixes: 3d709241 "kpartx: sanitize delete partitions"
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/kpartx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 58e60ffe..e9b09492 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -362,7 +362,7 @@ main(int argc, char **argv){
if (!mapname)
mapname = device + off;
- if (!force_devmap &&
+ else if (!force_devmap &&
dm_no_partitions(mapname)) {
/* Feature 'no_partitions' is set, return */
return 0;
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 01/10] kpartx: test-kpartx: new unit test program
This is a unit test program for kpartx, in particular for deleting
partitions.
NOTE: This test program fails with current kpartx; full patch series
needed to make it work.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/test-kpartx | 253 +
1 file changed, 253 insertions(+)
create mode 100755 kpartx/test-kpartx
diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
new file mode 100755
index ..03e8030e
--- /dev/null
+++ b/kpartx/test-kpartx
@@ -0,0 +1,253 @@
+#! /bin/bash
+
+# This is a unit test program for kpartx, in particular for deleting
partitions.
+#
+# The rationale is the following:
+#
+# 1) kpartx should delete all mappings it created beforehand.
+# 2) kpartx should handle partitions on dm devices and other devices
+# (e.g. loop devices) equally well.
+# 3) kpartx should only delete "partitions", which are single-target
+# linear mappings into a block device. Other maps should not be touched.
+# 4) kpartx should only delete mappings it created itself beforehand.
+# In particular, it shouldn't delete LVM LVs, even if they are fully
+# contained in the block device at hand and thus look like partitions
+# in the first place. (For historical compatibility reasons, we allow
+# such mappings to be deleted with the -f/--force flag).
+# 5) DM map names may be changed, thus kpartx shouldn't rely on them to
+# check whether a mapping is a partition of a particular device. It is
+# legal for a partition of /dev/loop0 to be named "loop0".
+
+# Note: This program tries hard to clean up, but if tests fail,
+# stale DM or loop devices may keep lurking around.
+
+# Set WORKDIR in environment to existing dir to for persistence
+# WARNING: exisiting files will be truncated.
+# If empty, test will be done in temporary dir
+: ${WORKDIR:=}
+# Set this environment variable to test an alternative kpartx executable
+: ${KPARTX:=}
+# Time to wait for device nodes to appear (microseconds)
+: ${WAIT_US:=10}
+
+# IMPORTANT: The ERR trap is essential for this program to work correctly!
+trap 'LINE=$LINENO; trap - ERR; echo "== error in $BASH_COMMAND on line $LINE
==" >&2; exit 1' ERR
+trap 'cleanup' 0
+
+CLEANUP=:
+cleanup() {
+trap - ERR
+trap - 0
+if [[ $OK ]]; then
+ echo == all tests completed successfully == >&2
+else
+ echo == step $STEP failed == >&2
+fi
+eval "$CLEANUP" &>/dev/null
+}
+
+push_cleanup() {
+CLEANUP="$@;$CLEANUP"
+}
+
+pop_cleanup() {
+# CAUTION: simplistic
+CLEANUP=${CLEANUP#*;}
+}
+
+step() {
+STEP="$@"
+echo == Test step: $STEP == >&2
+}
+
+mk_partitions() {
+parted -s $1 mklabel msdos
+parted -s -- $1 mkpart prim ext2 1MiB -1s
+}
+
+step preparation
+
+[[ $UID -eq 0 ]]
+[[ $KPARTX ]] || {
+if [[ -x $PWD/kpartx/kpartx ]]; then
+ KPARTX=$PWD/kpartx/kpartx
+else
+ KPARTX=$(which kpartx)
+fi
+}
+[[ $KPARTX ]]
+
+FILE1=kpartx1
+FILE2=kpartx2
+FILE3=kpartx3
+SIZE=$((1024*1024*1024)) # use bytes as units here
+SECTSIZ=512
+OFFS=32# offset of linear mapping into dev, sectors
+VG=kpvg # volume group name
+LV=kplv # logical vol name
+LVMCONF='devices { filter = [ "a|/dev/loop.*|", r".*" ] }'
+
+OK=
+
+[[ $WORKDIR ]] || {
+WORKDIR=$(mktemp -d /tmp/kpartx-XX)
+push_cleanup 'rm -rf $WORKDIR'
+}
+
+push_cleanup "cd $PWD"
+cd "$WORKDIR"
+
+step "create loop devices"
+truncate -s $SIZE $FILE1
+truncate -s $SIZE $FILE2
+truncate -s $SIZE $FILE3
+
+LO1=$(losetup -f $FILE1 --show)
+push_cleanup 'losetup -d $LO1'
+LO2=$(losetup -f $FILE2 --show)
+push_cleanup 'losetup -d $LO2'
+LO3=$(losetup -f $FILE3 --show)
+push_cleanup 'losetup -d $LO3'
+
+[[ $LO1 && $LO2 && $LO3 && -b $LO1 && -b $LO2 && -b $LO3 ]]
+DEV1=$(stat -c "%t:%T" $LO1)
+DEV2=$(stat -c "%t:%T" $LO2)
+DEV3=$(stat -c "%t:%T" $LO3)
+
+usleep $WAIT_US
+
+step "create DM devices (spans)"
+# Create two linear mappings spanning two loopdevs.
+# One of them gets a pathological name colliding with
+# the loop device name.
+# These mappings must not be removed by kpartx.
+# They also serve as DM devices to test partition removal on those.
+
+TABLE="\
+0 $((SIZE/SECTSIZ-OFFS)) linear $DEV1 $OFFS
+$((SIZE/SECTSIZ-OFFS)) $((SIZE/SECTSIZ-OFFS)) linear $DEV2 $OFFS"
+
+SPAN1=kpt
+SPAN2=$(basename $LO2)
+dmsetup create $SPAN1 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN1'
+
+dmsetup create $SPAN2 <<<"$TABLE"
+push_cleanup 'dmsetup remove -f $SPAN2'
+
+usleep $WAIT_US
+[[ -b /dev/mapper/$SPAN1 ]]
+[[ -b /dev/mapper/$SPAN2 ]]
+
+step "create vg on $LO3"
+# On the 3rd loop device, we create a VG and an LV
+# The LV should not be removed by kpartx
[dm-devel] [PATCH 06/10] kpartx: don't treat multi-linear mappings as partitions
kpartx -d treats any map that has a "linear" mapping into a
device as first target as a partition of this device.
This is wrong, because linear mappings may consist of
several pieces, combining multiple devices into one
(LVM logical volumes are an example of such a
mapping). Partitions have to be single-target mappings.
Fix this by returning an error in dm_type() if a map with
multiple targets is encountered. The "type" of a map with
two or more targets is a difficult concept, anyway.
test case:
truncate -s 1G test0 test1
losetup /dev/loop0 test0
losetup /dev/loop1 test1
dmsetup create <
---
kpartx/devmapper.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index d6ccd000..5380b2e9 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -390,10 +390,11 @@ dm_type(const char * name, char * type)
goto out;
/* Fetch 1st target */
- dm_get_next_target(dmt, NULL, , ,
- _type, );
-
- if (!target_type)
+ if (dm_get_next_target(dmt, NULL, , ,
+ _type, ) != NULL)
+ /* more than one target */
+ r = -1;
+ else if (!target_type)
r = -1;
else if (!strcmp(target_type, type))
r = 1;
@@ -494,7 +495,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
/*
* skip if devmap target is not "linear"
*/
- if (!dm_type(names->name, "linear")) {
+ if (dm_type(names->name, "linear") != 1) {
if (rd->verbose)
printf("%s: is not a linear target. Not
removing\n",
names->name);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 09/10] kpartx: use absolute path for regular files
kpartx supports being called for a regular file, in which
case it assumes that it should set up a loop device or use
an existing one. Because the loopinfo.lo_name field contains
an absolute path, matching with existing loop devices fails
for relative paths. Matching by basename only would be unreliable.
Therefore, convert relative to absolute path for regular files.
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/kpartx.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index d9057fba..3bd16452 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -350,7 +350,13 @@ main(int argc, char **argv){
if (S_ISREG (buf.st_mode)) {
/* already looped file ? */
- loopdev = find_loop_by_file(device);
+ char rpath[PATH_MAX];
+ if (realpath(device, rpath) == NULL) {
+ fprintf(stderr, "Error: %s: %s\n", device,
+ strerror(errno));
+ exit (1);
+ }
+ loopdev = find_loop_by_file(rpath);
if (!loopdev && what == DELETE)
exit (0);
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 04/10] kpartx: dm_remove_partmaps: support non-dm devices
Commit 3d709241 improved partition removal, but broke support
for handling partitions of non-dm devices such as loop devices
or RAM disks.
This requires passing the dev_t of the device down to
do_foreach_partmap(). Doing so, there's little use in trying
to derive major/minor numbers from the "mapname" any more
(which actually is the device name for non-DM devices).
But we can use this to find out whether the device in question
is a dm device.
The test for UUID match doesn't work for non-DM devices (this
shall be improved in a later patch in this series).
The test for equal name of parent dev and partition is only valid
for dm devices. For non-dm devices such as loop, "/dev/mapper/loop0"
could, theoretically, be a partition of "/dev/loop0"
(and we don't want to rely on map names).
Fixes: 3d709241 "kpartx: sanitize delete partitions"
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/devmapper.c | 26 +-
kpartx/devmapper.h | 2 +-
kpartx/kpartx.c| 3 ++-
3 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index cf6650c6..8f48a705 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include "devmapper.h"
#define UUID_PREFIX "part%d-"
@@ -418,8 +419,8 @@ dm_compare_uuid(const char *mapuuid, const char *partname)
return 1;
if (!strncmp(partuuid, "part", 4)) {
- char *p = strstr(partuuid, "mpath-");
- if (p && !strcmp(mapuuid, p))
+ char *p = strchr(partuuid, '-');
+ if (p && !strcmp(mapuuid, p + 1))
r = 0;
}
free(partuuid);
@@ -432,6 +433,7 @@ struct remove_data {
static int
do_foreach_partmaps (const char * mapname, const char *uuid,
+dev_t devt,
int (*partmap_func)(const char *, void *),
void *data)
{
@@ -443,6 +445,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
int major, minor;
char dev_t[32];
int r = 1;
+ int is_dmdev = 1;
if (!(dmt = dm_task_create(DM_DEVICE_LIST)))
return 1;
@@ -460,15 +463,20 @@ do_foreach_partmaps (const char * mapname, const char
*uuid,
goto out;
}
- if (dm_devn(mapname, , ))
- goto out;
+ if (dm_devn(mapname, , ) ||
+ (major != major(devt) || minor != minor(devt)))
+ /*
+* The latter could happen if a dm device "/dev/mapper/loop0"
+* exits while kpartx is called on "/dev/loop0".
+*/
+ is_dmdev = 0;
- sprintf(dev_t, "%d:%d", major, minor);
+ sprintf(dev_t, "%d:%d", major(devt), minor(devt));
do {
/*
* skip our devmap
*/
- if (!strcmp(names->name, mapname))
+ if (is_dmdev && !strcmp(names->name, mapname))
goto next;
/*
@@ -496,7 +504,7 @@ do_foreach_partmaps (const char * mapname, const char *uuid,
/*
* skip if uuids don't match
*/
- if (dm_compare_uuid(uuid, names->name)) {
+ if (is_dmdev && uuid && dm_compare_uuid(uuid, names->name)) {
if (rd->verbose)
printf("%s: is not a kpartx partition. Not
removing\n",
names->name);
@@ -537,10 +545,10 @@ remove_partmap(const char *name, void *data)
}
int
-dm_remove_partmaps (char * mapname, char *uuid, int verbose)
+dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose)
{
struct remove_data rd = { verbose };
- return do_foreach_partmaps(mapname, uuid, remove_partmap, );
+ return do_foreach_partmaps(mapname, uuid, devt, remove_partmap, );
}
#define FEATURE_NO_PART "no_partitions"
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index 9988ec0f..2e28c780 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -18,7 +18,7 @@ char * dm_mapname(int major, int minor);
dev_t dm_get_first_dep(char *devname);
char * dm_mapuuid(const char *mapname);
int dm_devn (const char * mapname, int *major, int *minor);
-int dm_remove_partmaps (char * mapname, char *uuid, int verbose);
+int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
int dm_no_partitions(char * mapname);
#endif /* _KPARTX_DEVMAPPER_H */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index e9b09492..e2056b7f 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -451,7 +451,8 @@ main(int argc, char **argv){
[dm-devel] [PATCH 10/10] kpartx: find_loop_by_file: use sysfs
Rather then searching through all of /dev, look up loop devices
in /sys/devices/virtual/block. This is cleaner and more robust
(/dev/loop$Xp$Y symlinks may confuse kpartx).
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
kpartx/lopart.c | 42 +++---
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 44f0c277..0521d7dc 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -68,25 +68,46 @@ char *find_loop_by_file(const char *filename)
{
DIR *dir;
struct dirent *dent;
- char dev[64], *found = NULL;
+ char dev[64], *found = NULL, *p;
int fd;
struct stat statbuf;
struct loop_info loopinfo;
+ const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
+ char path[PATH_MAX];
- dir = opendir("/dev");
+ dir = opendir(VIRT_BLOCK);
if (!dir)
return NULL;
while ((dent = readdir(dir)) != NULL) {
if (strncmp(dent->d_name,"loop",4))
continue;
- if (!strcmp(dent->d_name, "loop-control"))
- continue;
- sprintf(dev, "/dev/%s", dent->d_name);
- fd = open (dev, O_RDONLY);
+ if (snprintf(path, PATH_MAX, "%s/%s/dev", VIRT_BLOCK,
+dent->d_name) >= PATH_MAX)
+ continue;
+
+ fd = open(path, O_RDONLY);
if (fd < 0)
- break;
+ continue;
+
+ if (read(fd, dev, sizeof(dev)) <= 0) {
+ close(fd);
+ continue;
+ }
+
+ close(fd);
+
+ dev[sizeof(dev)-1] = '\0';
+ p = strchr(dev, '\n');
+ if (p != NULL)
+ *p = '\0';
+ if (snprintf(path, PATH_MAX, "/dev/block/%s", dev) >= PATH_MAX)
+ continue;
+
+ fd = open (path, O_RDONLY);
+ if (fd < 0)
+ continue;
if (fstat (fd, ) != 0 ||
!S_ISBLK(statbuf.st_mode)) {
@@ -99,13 +120,12 @@ char *find_loop_by_file(const char *filename)
continue;
}
+ close (fd);
+
if (0 == strcmp(filename, loopinfo.lo_name)) {
- close (fd);
- found = xstrdup(dev);
+ found = realpath(path, NULL);
break;
}
-
- close (fd);
}
closedir(dir);
return found;
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 07/10] libmultipath: don't treat multi-linear mappings as partitions
dm_type is used in libmultipath only to check whether a mapping
is "linear", with the intention to test if it represents a
"partition". This test returns TRUE also for mappings with
multiple targets, the first of which happens to be a linear
mapping into the target device. This is questionable, it's
hard to assign a "type" to such maps anyway.
Fix this by returning an error for multi-target maps.
This is analogous to the patch
"kpartx: don't treat multi-linear mappings as partitions".
Signed-off-by: Martin Wilck <mwi...@suse.com>
---
libmultipath/devmapper.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 5fb9d9ac..c19dcb62 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -572,7 +572,7 @@ out:
* returns:
*1 : match
*0 : no match
- * -1 : empty map
+ * -1 : empty map, or more than 1 target
*/
int dm_type(const char *name, char *type)
{
@@ -594,10 +594,11 @@ int dm_type(const char *name, char *type)
goto out;
/* Fetch 1st target */
- dm_get_next_target(dmt, NULL, , ,
- _type, );
-
- if (!target_type)
+ if (dm_get_next_target(dmt, NULL, , ,
+ _type, ) != NULL)
+ /* multiple targets */
+ r = -1;
+ else if (!target_type)
r = -1;
else if (!strcmp(target_type, type))
r = 1;
@@ -1185,9 +1186,9 @@ do_foreach_partmaps (const char * mapname,
do {
if (
/*
-* if devmap target is "linear"
+* if there is only a single "linear" target
*/
- (dm_type(names->name, TGT_PART) > 0) &&
+ (dm_type(names->name, TGT_PART) == 1) &&
/*
* and both uuid end with same suffix starting
--
2.12.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 00/10] fixes for kpartx -d
Working on a bug report about kpartx not properly removing partitions for loop devices, I realized a number of glitches and improperly handled corner cases in the kpartx code for deleting partitions. Some mappings are not deleted although they should be, and others are deleted although that is clearly wrong. This patch series fixes the issues I found. The series starts with a test program demonstrating the problems. The program succeeds only after all patches of this series are applied. Here is my summary of what I think how kpartx should behave: 1) kpartx should delete all mappings it created beforehand. 2) kpartx should handle partitions on dm devices and other devices (e.g. loop devices) equally well. 3) kpartx should only delete "partitions", which are single-target linear mappings into a block device. Other maps should not be touched. 4) kpartx should only delete mappings it created itself beforehand. In particular, it shouldn't delete LVM LVs, even if they are fully contained in the block device at hand and thus look like partitions in the first place. (For historical compatibility reasons, allow such mappings to be deleted with the -f/--force flag). 5) DM map names may be changed, thus kpartx shouldn't rely on them to check whether a mapping is a partition of a particular device. It is legal for a partition of /dev/loop0 to be named "loop0". One patch has an obvious libdevmapper equivalent and is therefore included although this series is otherwise focused only on kpartx. Feedback is welcome. Martin Wilck (10): kpartx: test-kpartx: new unit test program kpartx: avoid ioctl error for loop devices kpartx: remove is_loop_device kpartx: dm_remove_partmaps: support non-dm devices kpartx: dm_devn: return error for non-existent device kpartx: don't treat multi-linear mappings as partitions libmultipath: don't treat multi-linear mappings as partitions kpartx: use partition UUID for non-DM devices kpartx: use absolute path for regular files kpartx: find_loop_by_file: use sysfs kpartx/devmapper.c | 39 +--- kpartx/devmapper.h | 2 +- kpartx/kpartx.c | 42 +++- kpartx/lopart.c | 73 ++ kpartx/lopart.h | 1 - kpartx/test-kpartx | 253 +++ libmultipath/devmapper.c | 15 +-- 7 files changed, 356 insertions(+), 69 deletions(-) create mode 100755 kpartx/test-kpartx -- 2.12.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
