[PATCH 0/6] multipath cleanups

[PATCH 0/6] multipath cleanups

[Benjamin Marzinski]

Patches 0003 & 0004 fix an issue that I've seen with paths whose checker
takes too long when multipathd is starting up and creating devices.

The others are minor build fixes or small cleanups to my previous
patchset.

Benjamin Marzinski (6):
  Makefile.inc: trim extra information from systemd version
  kpartx: fix -Wsign-compare error
  libmultipath: remove code duplication in path counting
  libmultipath: count pending paths as active on loads
  libmultipath: deal with flushing no maps
  multipath: deal with delegation failures correctly

 Makefile.inc                    |  2 +-
 kpartx/kpartx.c                 |  2 +-
 libmpathpersist/mpath_persist.c |  4 +--
 libmultipath/devmapper.c        |  5 ++--
 libmultipath/structs.c          | 52 ++++++++++++++++-----------------
 libmultipath/structs.h          |  2 +-
 multipath/main.c                |  9 ++++--
 7 files changed, 39 insertions(+), 37 deletions(-)

-- 
2.17.2

[PATCH 1/6] Makefile.inc: trim extra information from systemd version

[Benjamin Marzinski]

Some systemd versions print extra information in the
"pkg-config --modversion" output, which confuses make. Trim this
off.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 Makefile.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.inc b/Makefile.inc
index e7256e3a..8ea3352d 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -37,7 +37,7 @@ endif
 
 ifndef SYSTEMD
 	ifeq ($(shell pkg-config --modversion libsystemd >/dev/null 2>&1 && echo 1), 1)
-		SYSTEMD = $(shell pkg-config --modversion libsystemd)
+		SYSTEMD = $(shell pkg-config --modversion libsystemd | awk '{print $$1}')
 	else
 		ifeq ($(shell systemctl --version >/dev/null 2>&1 && echo 1), 1)
 			SYSTEMD = $(shell systemctl --version 2> /dev/null | \
-- 
2.17.2

[PATCH 2/6] kpartx: fix -Wsign-compare error

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/kpartx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index c24ad6d9..653ce0c8 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -738,7 +738,7 @@ struct block {
 /* blknr is always in 512 byte blocks */
 char *
 getblock (int fd, unsigned int blknr) {
-	unsigned int secsz = get_sector_size(fd);
+	int secsz = get_sector_size(fd);
 	unsigned int blks_per_sec = secsz / 512;
 	unsigned int secnr = blknr / blks_per_sec;
 	unsigned int blk_off = (blknr % blks_per_sec) * 512;
-- 
2.17.2

[PATCH 3/6] libmultipath: remove code duplication in path counting

[Benjamin Marzinski]

pathcountgr() is never used except by pathcount(), and neither is the
special case for PATH_WILD. Simplify this and make one function that is
used by both pathcount() and count_active_paths(). This will be used
again in a future patch.

Also use count_active_paths() in mpath_persist.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c |  4 +--
 libmultipath/structs.c          | 47 +++++++++++++--------------------
 libmultipath/structs.h          |  1 -
 3 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 3da7a6cf..a132f4e9 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -436,7 +436,7 @@ int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 
 	all_tg_pt = (mpp->all_tg_pt == ALL_TG_PT_ON ||
 		     paramp->sa_flags & MPATH_F_ALL_TG_PT_MASK);
-	active_pathcount = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
+	active_pathcount = count_active_paths(mpp);
 
 	if (active_pathcount == 0) {
 		condlog (0, "%s: no path available", mpp->wwid);
@@ -648,7 +648,7 @@ int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
 
-	active_pathcount = pathcount (mpp, PATH_UP) + pathcount (mpp, PATH_GHOST);
+	active_pathcount = count_active_paths(mpp);
 
 	struct threadinfo thread[active_pathcount];
 	memset(thread, 0, sizeof(thread));
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 2dd378c4..3eac3d61 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -455,49 +455,40 @@ find_path_by_devt (const struct _vector *pathvec, const char * dev_t)
 	return NULL;
 }
 
-int pathcountgr(const struct pathgroup *pgp, int state)
+static int do_pathcount(const struct multipath *mpp, const int *states,
+			unsigned int nr_states)
 {
+	struct pathgroup *pgp;
 	struct path *pp;
 	int count = 0;
-	int i;
+	unsigned int i, j, k;
 
-	vector_foreach_slot (pgp->paths, pp, i)
-		if ((pp->state == state) || (state == PATH_WILD))
-			count++;
+	if (!mpp->pg || !nr_states)
+		return count;
 
+	vector_foreach_slot (mpp->pg, pgp, i) {
+		vector_foreach_slot (pgp->paths, pp, j) {
+			for (k = 0; k < nr_states; k++) {
+				if (pp->state == states[k]) {
+					count++;
+					break;
+				}
+			}
+		}
+	}
 	return count;
 }
 
 int pathcount(const struct multipath *mpp, int state)
 {
-	struct pathgroup *pgp;
-	int count = 0;
-	int i;
-
-	if (mpp->pg) {
-		vector_foreach_slot (mpp->pg, pgp, i)
-			count += pathcountgr(pgp, state);
-	}
-	return count;
+	return do_pathcount(mpp, &state, 1);
 }
 
 int count_active_paths(const struct multipath *mpp)
 {
-	struct pathgroup *pgp;
-	struct path *pp;
-	int count = 0;
-	int i, j;
-
-	if (!mpp->pg)
-		return 0;
+	int states[] = {PATH_UP, PATH_GHOST};
 
-	vector_foreach_slot (mpp->pg, pgp, i) {
-		vector_foreach_slot (pgp->paths, pp, j) {
-			if (pp->state == PATH_UP || pp->state == PATH_GHOST)
-				count++;
-		}
-	}
-	return count;
+	return do_pathcount(mpp, states, 2);
 }
 
 int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d69bc2e9..0c03e711 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -446,7 +446,6 @@ struct path * find_path_by_devt (const struct _vector *pathvec, const char *devt
 struct path * find_path_by_dev (const struct _vector *pathvec, const char *dev);
 struct path * first_path (const struct multipath *mpp);
 
-int pathcountgr (const struct pathgroup *, int);
 int pathcount (const struct multipath *, int);
 int count_active_paths(const struct multipath *);
 int pathcmp (const struct pathgroup *, const struct pathgroup *);
-- 
2.17.2

[PATCH 4/6] libmultipath: count pending paths as active on loads

[Benjamin Marzinski]

When multipath loads a table, it signals to udev if there are no active
paths.  Multipath wasn't counting pending paths as active.  This meant
that if all the paths were pending, udev would treat the device as not
ready, and not run kpartx on it.  Even if the pending paths later
because active and were reinstated, the kernel would not send a new
uevent, because from its point of view, they were always up.

The alternative would be to continue to treat them as failed in the udev
rules, but then also tell the kernel that they were down, so that it
would trigger a uevent when they were reinstated. However, this could
lead to newly created multipath devices failing IO, simply because the
path checkers hadn't returned yet. Having udev assume that the the
device is up, like the kernel does, seems like the safer option.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 3 ++-
 libmultipath/structs.c   | 7 +++++++
 libmultipath/structs.h   | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index f597ff8b..126cd728 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -417,7 +417,8 @@ static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
 	/* DM_UDEV_DISABLE_LIBRARY_FALLBACK is added in dm_addmap */
 	return	(mpp->skip_kpartx == SKIP_KPARTX_ON ?
 		 MPATH_UDEV_NO_KPARTX_FLAG : 0) |
-		((count_active_paths(mpp) == 0 || mpp->ghost_delay_tick > 0) ?
+		((count_active_pending_paths(mpp) == 0 ||
+		  mpp->ghost_delay_tick > 0) ?
 		 MPATH_UDEV_NO_PATHS_FLAG : 0) |
 		(reload && !mpp->force_udev_reload ?
 		 MPATH_UDEV_RELOAD_FLAG : 0);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 3eac3d61..0d1f969d 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -491,6 +491,13 @@ int count_active_paths(const struct multipath *mpp)
 	return do_pathcount(mpp, states, 2);
 }
 
+int count_active_pending_paths(const struct multipath *mpp)
+{
+	int states[] = {PATH_UP, PATH_GHOST, PATH_PENDING};
+
+	return do_pathcount(mpp, states, 3);
+}
+
 int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
 {
 	int i, j;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 0c03e711..917e4083 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -448,6 +448,7 @@ struct path * first_path (const struct multipath *mpp);
 
 int pathcount (const struct multipath *, int);
 int count_active_paths(const struct multipath *);
+int count_active_pending_paths(const struct multipath *);
 int pathcmp (const struct pathgroup *, const struct pathgroup *);
 int add_feature (char **, const char *);
 int remove_feature (char **, const char *);
-- 
2.17.2

[PATCH 5/6] libmultipath: deal with flushing no maps

[Benjamin Marzinski]

dm_flush_maps() was failing if there were no device-mapper devices at
all, instead of returning success, since there is nothing to do.

Fixes: "libmultipath: make dm_flush_maps only return 0 on success"
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 126cd728..b8199cb5 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1024,10 +1024,10 @@ int dm_flush_maps (int need_suspend, int retries)
 	if (!(names = dm_task_get_names (dmt)))
 		goto out;
 
+	r = 0;
 	if (!names->dev)
 		goto out;
 
-	r = 0;
 	do {
 		if (need_suspend)
 			r |= dm_suspend_and_flush_map(names->name, retries);
-- 
2.17.2

[PATCH 6/6] multipath: deal with delegation failures correctly

[Benjamin Marzinski]

delegate_to_multipathd() was returning success, even if the multipathd
command failed. Also, if the command was set to fail with NOT_DELEGATED,
it shouldn't print any errors, since multipath will try to issue to
command itself.

Fixes: "multipath: delegate flushing maps to multipathd"
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 4c43314e..3da692dc 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -861,9 +861,12 @@ int delegate_to_multipathd(enum mpath_cmds cmd,
 		goto out;
 	}
 
-	if (reply != NULL && *reply != '\0' && strcmp(reply, "ok\n"))
-		printf("%s", reply);
-	r = DELEGATE_OK;
+	if (reply != NULL && *reply != '\0') {
+		if (strcmp(reply, "fail\n"))
+			r = DELEGATE_OK;
+		if (r != NOT_DELEGATED && strcmp(reply, "ok\n"))
+			printf("%s", reply);
+	}
 
 out:
 	FREE(reply);
-- 
2.17.2

Re: [PATCH 2/6] kpartx: fix -Wsign-compare error

[Martin Wilck]

On Mon, 2020-07-27 at 14:24 -0500, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  kpartx/kpartx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index c24ad6d9..653ce0c8 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -738,7 +738,7 @@ struct block {
>  /* blknr is always in 512 byte blocks */
>  char *
>  getblock (int fd, unsigned int blknr) {
> -	unsigned int secsz = get_sector_size(fd);
> +	int secsz = get_sector_size(fd);
>  	unsigned int blks_per_sec = secsz / 512;
>  	unsigned int secnr = blknr / blks_per_sec;
>  	unsigned int blk_off = (blknr % blks_per_sec) * 512;

I was wondering why I didn't see this one. Turns out that it occurs
only for 32bit compilations. It looks like sort of an overzealous
compiler warning to me, as it's only about a "!=" comparison that can
hardly produce a wrong or unexpected result.

Btw, not that it would matter much, but the value of he BLKSSZGET ioctl
is actually unsigned.

Anyway, ack.

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer

Re: [PATCH 3/6] libmultipath: remove code duplication in path counting

[Martin Wilck]

On Mon, 2020-07-27 at 14:24 -0500, Benjamin Marzinski wrote:
> pathcountgr() is never used except by pathcount(), and neither is the
> special case for PATH_WILD. Simplify this and make one function that
> is
> used by both pathcount() and count_active_paths(). This will be used
> again in a future patch.
> 
> Also use count_active_paths() in mpath_persist.

The patch looks correct. I had thought about something like it as well
when I first introduced count_active_paths(). But count_active_paths()
is used a *lot*, and often in critical situations. I wonder whether it
deserves an optimized version. The compiler can surely optimize better
with two constant expressions than with the additional for-loop over an
array with variable size. That's why back then I decided against
implementing it as a special case of a generic function. I actually
considered inlining count_active_paths().

It's hard to quantify the effect, and I haven't done any benchmarks. 
But still, can we perhaps keep the optimized version of
count_active_paths() itself?

Martin

Re: [PATCH 0/6] multipath cleanups

[Martin Wilck]

On Mon, 2020-07-27 at 14:24 -0500, Benjamin Marzinski wrote:
> Patches 0003 & 0004 fix an issue that I've seen with paths whose
> checker
> takes too long when multipathd is starting up and creating devices.
> 
> The others are minor build fixes or small cleanups to my previous
> patchset.
> 
> Benjamin Marzinski (6):
>   Makefile.inc: trim extra information from systemd version
>   kpartx: fix -Wsign-compare error
>   libmultipath: remove code duplication in path counting
>   libmultipath: count pending paths as active on loads
>   libmultipath: deal with flushing no maps
>   multipath: deal with delegation failures correctly
> 
>  Makefile.inc                    |  2 +-
>  kpartx/kpartx.c                 |  2 +-
>  libmpathpersist/mpath_persist.c |  4 +--
>  libmultipath/devmapper.c        |  5 ++--
>  libmultipath/structs.c          | 52 ++++++++++++++++---------------
> --
>  libmultipath/structs.h          |  2 +-
>  multipath/main.c                |  9 ++++--
>  7 files changed, 39 insertions(+), 37 deletions(-)
> 

For the series, except 3/6:

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer

Re: [PATCH 3/6] libmultipath: remove code duplication in path counting

[Benjamin Marzinski]

On Thu, Aug 06, 2020 at 10:48:12AM +0000, Martin Wilck wrote:
> On Mon, 2020-07-27 at 14:24 -0500, Benjamin Marzinski wrote:
> > pathcountgr() is never used except by pathcount(), and neither is the
> > special case for PATH_WILD. Simplify this and make one function that
> > is
> > used by both pathcount() and count_active_paths(). This will be used
> > again in a future patch.
> > 
> > Also use count_active_paths() in mpath_persist.
> 
> The patch looks correct. I had thought about something like it as well
> when I first introduced count_active_paths(). But count_active_paths()
> is used a *lot*, and often in critical situations. I wonder whether it
> deserves an optimized version. The compiler can surely optimize better
> with two constant expressions than with the additional for-loop over an
> array with variable size. That's why back then I decided against
> implementing it as a special case of a generic function. I actually
> considered inlining count_active_paths().
> 
> It's hard to quantify the effect, and I haven't done any benchmarks. 
> But still, can we perhaps keep the optimized version of
> count_active_paths() itself?

Sure.

-Ben

> 
> Martin
>