Re: [dm-devel] [PATCH v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Martin Wilck]

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 
> > > ---
> > >  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 , 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] [git pull] device mapper fixes for 4.12-rc7

[Mike Snitzer]

Hi Linus,

The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452:

  Linux 4.12-rc5 (2017-06-11 16:48:20 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git 
tags/for-4.12/dm-fixes-4

for you to fetch changes up to feb7695fe9fb83084aa29de0094774f4c9d4c9fc:

  dm io: fix duplicate bio completion due to missing ref count (2017-06-21 
12:04:50 -0400)

Please pull, thanks.
Mike


- a revert of a DM mirror commit that has proven to make the code prone
  to crash

- a DM io reference count fix that resolves a NULL pointer seen when
  issuing discards to a DM mirror target's device whose mirror legs do
  not all support discards

- a couple DM integrity fixes


Mike Snitzer (3):
  Revert "dm mirror: use all available legs on multiple failures"
  dm integrity: fix to not disable/enable interrupts from interrupt context
  dm io: fix duplicate bio completion due to missing ref count

Ondrej Mosnáček (1):
  dm integrity: reject mappings too large for device

 drivers/md/dm-integrity.c | 12 ++--
 drivers/md/dm-io.c|  4 ++--
 drivers/md/dm-raid1.c | 21 +++--
 3 files changed, 31 insertions(+), 6 deletions(-)

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

[Benjamin Marzinski]

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 
> > ---
> >  libmultipath/configure.c | 15 ---
> >  1 file changed, 15 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 55dbb261..39912f05 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -1097,21 +1097,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();
> > 
> 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,
and then call setup_multipath() afterwards, which would call
set_no_path_retry() to set it to the actual correct value. 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.

-Ben

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke  Teamlead Storage & Networking
> h...@suse.de +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 10/11] libmultipath: don't try to set hwhandler if it is retained

[Benjamin Marzinski]

On Thu, Jun 22, 2017 at 04:59:12PM +0200, 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.
> 
> Signed-off-by: Martin Wilck 

ACK

-Ben

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

Re: [dm-devel] [PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+

[Benjamin Marzinski]

On Thu, Jun 22, 2017 at 04:59:11PM +0200, 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.

ACK

-Ben

> 
> Signed-off-by: Martin Wilck 
> Reviewed-by: Hannes Reinecke 
> ---
>  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 
> + * 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), 

[dm-devel] [PATCH] multipath-tools: beautify path_latency.c code

[Xose Vazquez Perez]

Mainly running scripts/Lindent, from kernel dir, to replace indent spaces
by tabs.

Cc: Yang Feng 
Cc: Christophe Varoqui 
Cc: device-mapper development 
Signed-off-by: Xose Vazquez Perez 
---
 libmultipath/prioritizers/path_latency.c | 354 +++
 1 file changed, 177 insertions(+), 177 deletions(-)

diff --git a/libmultipath/prioritizers/path_latency.c 
b/libmultipath/prioritizers/path_latency.c
index 34b734b..9fc2dfc 100644
--- a/libmultipath/prioritizers/path_latency.c
+++ b/libmultipath/prioritizers/path_latency.c
@@ -17,8 +17,8 @@
  * Author(s): Yang Feng 
  *
  * This file is released under the GPL version 2, or any later version.
- *
  */
+
 #include 
 #include 
 #include 
@@ -31,27 +31,28 @@
 
 #define pp_pl_log(prio, fmt, args...) condlog(prio, "path_latency prio: " fmt, 
##args)
 
-#define MAX_IO_NUM  200
-#define MIN_IO_NUM  2
+#define MAX_IO_NUM 200
+#define MIN_IO_NUM 2
 
-#define MAX_BASE_NUM10
-#define MIN_BASE_NUM2
+#define MAX_BASE_NUM   10
+#define MIN_BASE_NUM   2
 
-#define MAX_AVG_LATENCY 1.  /*Unit: us*/
-#define MIN_AVG_LATENCY 1.  /*Unit: us*/
+#define MAX_AVG_LATENCY1.  /* Unit: us */
+#define MIN_AVG_LATENCY1.  /* Unit: us */
 
-#define DEFAULT_PRIORITY0
+#define DEFAULT_PRIORITY   0
 
-#define MAX_CHAR_SIZE   30
+#define MAX_CHAR_SIZE  30
 
-#define USEC_PER_SEC100LL
-#define NSEC_PER_USEC   1000LL
+#define USEC_PER_SEC   100LL
+#define NSEC_PER_USEC  1000LL
 
 static long long path_latency[MAX_IO_NUM];
 
 static inline long long timeval_to_us(const struct timespec *tv)
 {
-   return ((long long) tv->tv_sec * USEC_PER_SEC) + (tv->tv_nsec / 
NSEC_PER_USEC);
+   return ((long long)tv->tv_sec * USEC_PER_SEC) +
+   (tv->tv_nsec / NSEC_PER_USEC);
 }
 
 static int do_readsector0(int fd, unsigned int timeout)
@@ -60,198 +61,197 @@ static int do_readsector0(int fd, unsigned int timeout)
unsigned char sbuf[SENSE_BUFF_LEN];
int ret;
 
-   ret = sg_read(fd, [0], 4096, [0],
- SENSE_BUFF_LEN, timeout);
+   ret = sg_read(fd, [0], 4096, [0], SENSE_BUFF_LEN, timeout);
 
return ret;
 }
 
 int check_args_valid(int io_num, int base_num)
 {
-if ((io_num < MIN_IO_NUM) || (io_num > MAX_IO_NUM))
-{
-pp_pl_log(0, "args io_num is outside the valid range");
-return 0;
-}
-
-if ((base_num < MIN_BASE_NUM) || (base_num > MAX_BASE_NUM))
-{
-pp_pl_log(0, "args base_num is outside the valid range");
-return 0;
-}
-
-return 1;
+   if ((io_num < MIN_IO_NUM) || (io_num > MAX_IO_NUM)) {
+   pp_pl_log(0, "args io_num is outside the valid range");
+   return 0;
+   }
+
+   if ((base_num < MIN_BASE_NUM) || (base_num > MAX_BASE_NUM)) {
+   pp_pl_log(0, "args base_num is outside the valid range");
+   return 0;
+   }
+
+   return 1;
 }
 
-/* In multipath.conf, args form: io_num|base_num. For example,
-*  args is "20|10", this function can get io_num value 20, and
-   base_num value 10.
-*/
-static int get_ionum_and_basenum(char *args,
- int *ionum,
- int *basenum)
+/*
+ * In multipath.conf, args form: io_num|base_num. For example,
+ * args is "20|10", this function can get io_num value 20, and
+ * base_num value 10.
+ */
+static int get_ionum_and_basenum(char *args, int *ionum, int *basenum)
 {
-char source[MAX_CHAR_SIZE];
-char vertica = '|';
-char *endstrbefore = NULL;
-char *endstrafter = NULL;
-unsigned int size = strlen(args);
-
-if ((args == NULL) || (ionum == NULL) || (basenum == NULL))
-{
-pp_pl_log(0, "args string is NULL");
-return 0;
-}
-
-if ((size < 1) || (size > MAX_CHAR_SIZE-1))
-{
-pp_pl_log(0, "args string's size is too long");
-return 0;
-}
-
-memcpy(source, args, size+1);
-
-if (!isdigit(source[0]))
-{
-pp_pl_log(0, "invalid prio_args format: %s", source);
-return 0;
-}
-
-*ionum = (int)strtoul(source, , 10);
-if (endstrbefore[0] != vertica)
-{
-pp_pl_log(0, "invalid prio_args format: %s", source);
-return 0;
-}
-
-if (!isdigit(endstrbefore[1]))
-{
-pp_pl_log(0, "invalid prio_args format: %s", source);
-return 0;
-}
-
-*basenum = (long long)strtol([1], , 10);
-if (check_args_valid(*ionum, *basenum) == 0)
-{
-return 0;
-}
-
-return 1;
+   char source[MAX_CHAR_SIZE];
+   char vertica = '|';
+   char *endstrbefore = NULL;
+   char *endstrafter = 

[dm-devel] [PATCH v4 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Martin Wilck]

We set the queue_if_no_path feature in assemble_map() already,
no need to set it here again.

Signed-off-by: Martin Wilck 
---
 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

[dm-devel] [PATCH v4 04/11] libmultipath: merge_hwe: fix queue_if_no_path logic

[Martin Wilck]

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 
Acked-by: Benjamin Marzinski 
---
 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

[Martin Wilck]

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 
Acked-by: Benjamin Marzinski 
Reviewed-by: Hannes Reinecke 
---
 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

[Martin Wilck]

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 
Acked-by: Benjamin Marzinski 
Reviewed-by: Hannes Reinecke 
---
 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 06/11] multipath.conf.5: document no_path_retry vs. queue_if_no_path

[Martin Wilck]

Clarify the documentation about option precedence.

Signed-off-by: Martin Wilck 
Acked-by: Benjamin Marzinski 
Reviewed-by: Hannes Reinecke 
---
 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"

[Martin Wilck]

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 
Reviewed-by: Hannes Reinecke 
---
 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->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 

[dm-devel] [PATCH v4 01/11] libmultipath: load_config: skip setting unnecessary defaults

[Martin Wilck]

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 
Acked-by: Benjamin Marzinski 
Reviewed-by: Hannes Reinecke 
---
 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 @@ declare_hw_snprint(san_path_err_forget_rate, 
print_off_int_undef)
 declare_mp_handler(san_path_err_forget_rate, 

[dm-devel] [PATCH v4 08/11] libmultipath: add deprecated warning for some features settings

[Martin Wilck]

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 
Reviewed-by: Hannes Reinecke 
---
 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 07/11] multipath.conf.5: Remove ??? and other minor fixes

[Martin Wilck]

Remove the FIXME markers by filling in missing content,
and make some other minor fixes.

Signed-off-by: Martin Wilck 
Acked-by: Benjamin Marzinski 
Reviewed-by: Hannes Reinecke 
---
 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+

[Martin Wilck]

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 
Reviewed-by: Hannes Reinecke 
---
 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 
+ * 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/multipath.conf.5
index b32d0383..3b4e5187 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -698,15 +698,16 @@ The default is: \fB\fR
 .

[dm-devel] [PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic

[Martin Wilck]

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 10/11] libmultipath: don't try to set hwhandler if it is retained

[Martin Wilck]

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

Re: [dm-devel] [PATCH v3 10/11] libmultipath: don't try to set hwhandler if it is retained

[Martin Wilck]

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

[Martin Wilck]

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 
> > ---
> >  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 , 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 v6 1/2] crypto: Add IV generation algorithms

[Binoy Jayan]

Just for reference. Not for merging.

Currently, the iv generation algorithms are implemented in dm-crypt.c.
The goal is to move these algorithms from the dm layer to the kernel
crypto layer by implementing them as template ciphers so they can be
implemented in hardware for performance. As part of this patchset, the
iv-generation code is moved from the dm layer to the crypto layer and
adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
at a time. Each bio contains an in memory representation of physically
contiguous disk blocks. The dm layer sets up a chained scatterlist of
these blocks split into physically contiguous segments in memory so that
DMA can be performed. Also, the key management code is moved from dm layer
to the cryto layer since the key selection for encrypting neighboring
sectors depend on the keycount.

Synchronous crypto requests to encrypt/decrypt a sector are processed
sequentially. Asynchronous requests if processed in parallel, are freed
in the async callback. The storage space for the initialization vectors
are allocated in the iv generator implementations.

Interface to the crypto layer - include/crypto/geniv.h

Signed-off-by: Binoy Jayan 
---
 drivers/md/dm-crypt.c  | 1939 ++--
 include/crypto/geniv.h |   46 ++
 2 files changed, 1432 insertions(+), 553 deletions(-)
 create mode 100644 include/crypto/geniv.h

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 389a363..bef54f5 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -32,170 +32,120 @@
 #include 
 #include 
 #include 
-
 #include 
-
-#define DM_MSG_PREFIX "crypt"
-
-/*
- * context holding the current state of a multi-part conversion
- */
-struct convert_context {
-   struct completion restart;
-   struct bio *bio_in;
-   struct bio *bio_out;
-   struct bvec_iter iter_in;
-   struct bvec_iter iter_out;
-   sector_t cc_sector;
-   atomic_t cc_pending;
-   struct skcipher_request *req;
+#include 
+#include 
+#include 
+#include 
+
+#define DM_MSG_PREFIX  "crypt"
+#define MAX_SG_LIST(BIO_MAX_PAGES * 8)
+#define MIN_IOS64
+#define LMK_SEED_SIZE  64 /* hash + 0 */
+#define TCW_WHITENING_SIZE 16
+
+struct geniv_ctx;
+struct geniv_req_ctx;
+
+/* Sub request for each of the skcipher_request's for a segment */
+struct geniv_subreq {
+   struct scatterlist src;
+   struct scatterlist dst;
+   struct geniv_req_ctx *rctx;
+   struct skcipher_request req CRYPTO_MINALIGN_ATTR;
 };
 
-/*
- * per bio private data
- */
-struct dm_crypt_io {
-   struct crypt_config *cc;
-   struct bio *base_bio;
-   struct work_struct work;
-
-   struct convert_context ctx;
-
-   atomic_t io_pending;
-   int error;
-   sector_t sector;
-
-   struct rb_node rb_node;
-} CRYPTO_MINALIGN_ATTR;
-
-struct dm_crypt_request {
-   struct convert_context *ctx;
-   struct scatterlist sg_in;
-   struct scatterlist sg_out;
+struct geniv_req_ctx {
+   struct geniv_subreq *subreq;
+   int is_write;
sector_t iv_sector;
+   unsigned int nents;
+   struct completion restart;
+   atomic_t req_pending;
+   struct skcipher_request *req;
 };
 
-struct crypt_config;
-
 struct crypt_iv_operations {
-   int (*ctr)(struct crypt_config *cc, struct dm_target *ti,
-  const char *opts);
-   void (*dtr)(struct crypt_config *cc);
-   int (*init)(struct crypt_config *cc);
-   int (*wipe)(struct crypt_config *cc);
-   int (*generator)(struct crypt_config *cc, u8 *iv,
-struct dm_crypt_request *dmreq);
-   int (*post)(struct crypt_config *cc, u8 *iv,
-   struct dm_crypt_request *dmreq);
+   int (*ctr)(struct geniv_ctx *ctx);
+   void (*dtr)(struct geniv_ctx *ctx);
+   int (*init)(struct geniv_ctx *ctx);
+   int (*wipe)(struct geniv_ctx *ctx);
+   int (*generator)(struct geniv_ctx *ctx,
+struct geniv_req_ctx *rctx,
+struct geniv_subreq *subreq, u8 *iv);
+   int (*post)(struct geniv_ctx *ctx,
+   struct geniv_req_ctx *rctx,
+   struct geniv_subreq *subreq, u8 *iv);
 };
 
-struct iv_essiv_private {
+struct geniv_essiv_private {
struct crypto_ahash *hash_tfm;
u8 *salt;
 };
 
-struct iv_benbi_private {
+struct geniv_benbi_private {
int shift;
 };
 
-#define LMK_SEED_SIZE 64 /* hash + 0 */
-struct iv_lmk_private {
+struct geniv_lmk_private {
struct crypto_shash *hash_tfm;
u8 *seed;
 };
 
-#define TCW_WHITENING_SIZE 16
-struct iv_tcw_private {
+struct geniv_tcw_private {
struct crypto_shash *crc32_tfm;
u8 *iv_seed;
u8 *whitening;
 };
 
-/*
- * Crypt: maps a linear range of a block device
- * and encrypts / decrypts at the same time.
- */
-enum flags { 

[dm-devel] [PATCH v6 0/2] IV Generation algorithms for dm-crypt

[Binoy Jayan]

===
dm-crypt optimization for larger block sizes
===

Currently, the iv generation algorithms are implemented in dm-crypt.c. The goal
is to move these algorithms from the dm layer to the kernel crypto layer by
implementing them as template ciphers so they can be used in relation with
algorithms like aes, and with multiple modes like cbc, ecb etc. As part of this
patchset, the iv-generation code is moved from the dm layer to the crypto layer
and adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
at a time. Each bio contains the in memory representation of physically
contiguous disk blocks. Since the bio itself may not be contiguous in main
memory, the dm layer sets up a chained scatterlist of these blocks split into
physically contiguous segments in memory so that DMA can be performed.

One challenge in doing so is that the IVs are generated based on a 512-byte
sector number. This infact limits the block sizes to 512 bytes. But this should
not be a problem if a hardware with iv generation support is used. The geniv
itself splits the segments into sectors so it could choose the IV based on
sector number. But it could be modelled in hardware effectively by not
splitting up the segments in the bio.

Another challenge faced is that dm-crypt has an option to use multiple keys.
The key selection is done based on the sector number. If the whole bio is
encrypted / decrypted with the same key, the encrypted volumes will not be
compatible with the original dm-crypt [without the changes]. So, the key
selection code is moved to crypto layer so the neighboring sectors are
encrypted with a different key.

The dm layer allocates space for iv. The hardware drivers can choose to make
use of this space to generate their IVs sequentially or allocate it on their
own. This can be moved to crypto layer too. Postponing this decision until
the requirement to integrate milan's changes are clear.

Interface to the crypto layer - include/crypto/geniv.h

More information on test procedure can be found in v1.
Results of performance tests with software crypto in v5.

The patch 'crypto: Multikey template for essiv' depends on
the following patches by Gilad:
 MAINTAINERS: add Gilad BY as maintainer for ccree
 staging: ccree: add devicetree bindings
 staging: ccree: add TODO list
 staging: add ccree crypto driver

Revisions:
--

v1: https://patchwork.kernel.org/patch/9439175
v2: https://patchwork.kernel.org/patch/9471923
v3: https://lkml.org/lkml/2017/1/18/170
v4: https://patchwork.kernel.org/patch/9559665
v5: https://patchwork.kernel.org/patch/9669237

v5 --> v6:
--

1. Moved allocation of initialization vectors to the iv-generator
2. Few consmetic changes as the consequence of the above
3. Few logical to boolean expressions for faster calculation
4. Included multikey template for splitting keys.
   This needs testing with real hardware (juno with ccree)
   and also modification. It is only for testing and not
   for inclusion upstream.

v4 --> v5
--

1. Fix for the multiple instance issue in /proc/crypto
2. Few cosmetic changes including struct alignment
3. Simplified 'struct geniv_req_info'

v3 --> v4
--
Fix for the bug reported by Gilad Ben-Yossef.
The element '__ctx' in 'struct skcipher_request req' overflowed into the
element 'struct scatterlist src' which immediately follows 'req' in
'struct geniv_subreq' and corrupted src.

v2 --> v3
--

1. Moved iv algorithms in dm-crypt.c for control
2. Key management code moved from dm layer to cryto layer
   so that cipher instance selection can be made depending on key_index
3. The revision v2 had scatterlist nodes created for every sector in the bio.
   It is modified to create only once scatterlist node to reduce memory
   foot print. Synchronous requests are processed sequentially. Asynchronous
   requests are processed in parallel and is freed in the async callback.
4. Changed allocation for sub-requests using mempool

v1 --> v2
--

1. dm-crypt changes to process larger block sizes (one segment in a bio)
2. Incorporated changes w.r.t. comments from Herbert.


Binoy Jayan (2):
  crypto: Add IV generation algorithms
  crypto: Multikey template for essiv

 drivers/md/dm-crypt.c| 1940 +++---
 drivers/staging/ccree/Makefile   |2 +-
 drivers/staging/ccree/essiv.c|  777 +++
 drivers/staging/ccree/essiv_sw.c | 1040 
 include/crypto/geniv.h   |   46 +
 5 files changed, 3251 insertions(+), 554 deletions(-)
 create mode 100644 drivers/staging/ccree/essiv.c
 create mode 100644 drivers/staging/ccree/essiv_sw.c
 create mode 100644 include/crypto/geniv.h

-- 
Binoy Jayan

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

Re: [dm-devel] [PATCH] multipath-tools: sync third-party headers with 3.13 upstream

[Bart Van Assche]

On Wed, 2017-06-21 at 14:26 +0200, Xose Vazquez Perez wrote:
> Cc: Bart Van Assche 
> Cc: Christophe Varoqui 
> Cc: device-mapper development 
> Signed-off-by: Xose Vazquez Perez 

Reviewed-by: Bart Van Assche 

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

[dm-devel] [PATCH v6 2/2] crypto: Multikey template for essiv

[Binoy Jayan]

Just for reference and to get the performance numbers.
Not for merging.

Depends on the following patches by Gilad:
 MAINTAINERS: add Gilad BY as maintainer for ccree
 staging: ccree: add devicetree bindings
 staging: ccree: add TODO list
 staging: add ccree crypto driver

A multi key template implementation which calls the underlying
iv generator 'essiv-aes-du512-dx' cum crypto algorithm. This
template sits on top of the underlying IV generator and accepts
a key length that is a multiple of the underlying key length.
This has not been tested on Juno with the CryptoCell accelerator
for which it was written for.

The underlying IV generator 'essiv-aes-du512-dx' generates IV for
every 512 byte blocks.

Signed-off-by: Binoy Jayan 
---
 drivers/md/dm-crypt.c|5 +-
 drivers/staging/ccree/Makefile   |2 +-
 drivers/staging/ccree/essiv.c|  777 
 drivers/staging/ccree/essiv_sw.c | 1040 ++
 4 files changed, 1821 insertions(+), 3 deletions(-)
 create mode 100644 drivers/staging/ccree/essiv.c
 create mode 100644 drivers/staging/ccree/essiv_sw.c

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index bef54f5..32f75dd 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1555,7 +1555,8 @@ static int __init geniv_register_algs(void)
if (err)
goto out_undo_plain;
 
-   err = crypto_register_template(_essiv_tmpl);
+   err = 0;
+   // err = crypto_register_template(_essiv_tmpl);
if (err)
goto out_undo_plain64;
 
@@ -1594,7 +1595,7 @@ static void __exit geniv_deregister_algs(void)
 {
crypto_unregister_template(_plain_tmpl);
crypto_unregister_template(_plain64_tmpl);
-   crypto_unregister_template(_essiv_tmpl);
+   // crypto_unregister_template(_essiv_tmpl);
crypto_unregister_template(_benbi_tmpl);
crypto_unregister_template(_null_tmpl);
crypto_unregister_template(_lmk_tmpl);
diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile
index 44f3e3e..524e930 100644
--- a/drivers/staging/ccree/Makefile
+++ b/drivers/staging/ccree/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o
-ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o 
ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o 
ssi_pm_ext.o
+ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o 
ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o 
ssi_pm_ext.o essiv.o
 ccree-$(CCREE_FIPS_SUPPORT) += ssi_fips.o ssi_fips_ll.o ssi_fips_ext.o 
ssi_fips_local.o
diff --git a/drivers/staging/ccree/essiv.c b/drivers/staging/ccree/essiv.c
new file mode 100644
index 000..719b8bf
--- /dev/null
+++ b/drivers/staging/ccree/essiv.c
@@ -0,0 +1,777 @@
+/*
+ * Copyright (C) 2003 Jana Saout 
+ * Copyright (C) 2004 Clemens Fruhwirth 
+ * Copyright (C) 2006-2015 Red Hat, Inc. All rights reserved.
+ * Copyright (C) 2013 Milan Broz 
+ *
+ * This file is released under the GPL.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DM_MSG_PREFIX  "crypt"
+#define MAX_SG_LIST(BIO_MAX_PAGES * 8)
+#define MIN_IOS64
+#define LMK_SEED_SIZE  64 /* hash + 0 */
+#define TCW_WHITENING_SIZE 16
+
+struct geniv_ctx;
+struct geniv_req_ctx;
+
+/* Sub request for each of the skcipher_request's for a segment */
+struct geniv_subreq {
+   struct scatterlist src;
+   struct scatterlist dst;
+   struct geniv_req_ctx *rctx;
+   struct skcipher_request req CRYPTO_MINALIGN_ATTR;
+};
+
+struct geniv_req_ctx {
+   struct geniv_subreq *subreq;
+   int is_write;
+   sector_t iv_sector;
+   unsigned int nents;
+   struct completion restart;
+   atomic_t req_pending;
+   struct skcipher_request *req;
+};
+
+struct crypt_iv_operations {
+   int (*ctr)(struct geniv_ctx *ctx);
+   void (*dtr)(struct geniv_ctx *ctx);
+   int (*init)(struct geniv_ctx *ctx);
+   int (*wipe)(struct geniv_ctx *ctx);
+   int (*generator)(struct geniv_ctx *ctx,
+struct geniv_req_ctx *rctx,
+struct geniv_subreq *subreq, u8 *iv);
+   int (*post)(struct geniv_ctx *ctx,
+   struct geniv_req_ctx *rctx,
+   struct geniv_subreq *subreq, u8 *iv);
+};
+
+struct geniv_ctx {
+   unsigned int tfms_count;
+   struct crypto_skcipher *child;
+   struct crypto_skcipher **tfms;
+   char *ivmode;
+   unsigned int iv_size;
+   unsigned int iv_start;

Re: [dm-devel] [PATCH v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Hannes Reinecke]

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 
> ---
>  libmultipath/configure.c | 15 ---
>  1 file changed, 15 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 55dbb261..39912f05 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1097,21 +1097,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();
> 
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?

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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

[Hannes Reinecke]

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 
> ---
>  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:
>   */
I would avoid using a temporary udev device here; either save the
dh_state value and call pathinfo() on the _actual_ device or read
dh_state directly from sysfs without an intermediate udev device.

> @@ -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 != 

Re: [dm-devel] [PATCH v3 08/11] libmultipath: add deprecated warning for some features settings

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 
> ---
>  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.
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 04/11] libmultipath: merge_hwe: fix queue_if_no_path logic

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 
> Acked-by: Benjamin Marzinski 
> ---
>  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;
>  }
>  
> 
Ugh.

Allocating a string just for having 'nice' debugging messages in
reconcile_features_with_options?
Please, don't.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 06/11] multipath.conf.5: document no_path_retry vs. queue_if_no_path

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> Clarify the documentation about option precedence.
> 
> Signed-off-by: Martin Wilck 
> Acked-by: Benjamin Marzinski 
> ---
>  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.
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 07/11] multipath.conf.5: Remove ??? and other minor fixes

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> Remove the FIXME markers by filling in missing content,
> and make some other minor fixes.
> 
> Signed-off-by: Martin Wilck 
> Acked-by: Benjamin Marzinski 
> ---
>  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
>  .
>  .
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 05/11] libmultipath: assemble_map: fix queue_if_no_path logic

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 
> Acked-by: Benjamin Marzinski 
> ---
>  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)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 03/11] libmultipath: clarify option conflicts for "features"

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 
> ---
>  libmultipath/configure.c |  6 ++---
>  libmultipath/propsel.c   | 68 
> +---
>  libmultipath/propsel.h   |  3 +++
>  3 files changed, 59 insertions(+), 18 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. 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 01/11] libmultipath: load_config: skip setting unnecessary defaults

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 
> Acked-by: Benjamin Marzinski 
> ---
>  libmultipath/config.c  | 16 
>  libmultipath/dict.c| 11 +++
>  libmultipath/propsel.c |  6 ++
>  3 files changed, 13 insertions(+), 20 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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