[PATCH 0/9] misc cleanups and bugfixes

[PATCH 0/9] misc cleanups and bugfixes

[Benjamin Marzinski]

These patches are all just small cleanup and bugfixes.

Benjamin Marzinski (9):
  libdmmp: minor Makefile cleanup
  multipath-tools: remove incdir from Makefiles
  libdmmp: don't disconnect from multipathd twice
  multipathd: don't call strlen on NULL variables
  libdmmp: move libdmmp.pc install location
  multipathd: drop lock before calling uev_add_path
  multipathd: allow devices to switch from RW to RO
  libmultipath: don't set max_sectors_kb on reloads
  multipath: fix segfault with disable_changed_wwids

 Makefile.inc             |  3 +--
 libdmmp/Makefile         | 18 +++++++--------
 libdmmp/libdmmp.c        |  1 -
 libdmmp/test/Makefile    |  2 +-
 libmpathcmd/Makefile     |  6 ++---
 libmpathpersist/Makefile |  6 ++---
 libmultipath/configure.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--
 libmultipath/devmapper.c |  9 ++++----
 libmultipath/discovery.c | 23 -------------------
 libmultipath/discovery.h |  1 -
 libmultipath/structs.h   |  1 +
 multipathd/main.c        | 21 ++++++++++++-----
 12 files changed, 96 insertions(+), 55 deletions(-)

-- 
1.8.3.1

[PATCH 1/9] libdmmp: minor Makefile cleanup

[Benjamin Marzinski]

This patch deletes the "/" after $(DESTDIR) and removes
libdmmp_speed_test on 'make clean'

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libdmmp/Makefile      | 18 +++++++++---------
 libdmmp/test/Makefile |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/libdmmp/Makefile b/libdmmp/Makefile
index 082078a..1c5329a 100644
--- a/libdmmp/Makefile
+++ b/libdmmp/Makefile
@@ -29,27 +29,27 @@ $(LIBS): $(OBJS)
 install:
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(INSTALL_PROGRAM) -m 644 -D \
-		$(HEADERS) $(DESTDIR)/$(includedir)/$(HEADERS)
-	$(LN) $(LIBS) $(DESTDIR)/$(syslibdir)/$(DEVLIB)
+		$(HEADERS) $(DESTDIR)$(includedir)/$(HEADERS)
+	$(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
 	$(INSTALL_PROGRAM) -m 644 -D \
-		$(PKGFILE).in $(DESTDIR)/$(pkgconfdir)/$(PKGFILE)
+		$(PKGFILE).in $(DESTDIR)$(pkgconfdir)/$(PKGFILE)
 	perl -i -pe 's|__VERSION__|$(LIBDMMP_VERSION)|g' \
-		$(DESTDIR)/$(pkgconfdir)/$(PKGFILE)
+		$(DESTDIR)$(pkgconfdir)/$(PKGFILE)
 	perl -i -pe 's|__LIBDIR__|$(syslibdir)|g' \
-		$(DESTDIR)/$(pkgconfdir)/$(PKGFILE)
+		$(DESTDIR)$(pkgconfdir)/$(PKGFILE)
 	perl -i -pe 's|__INCLUDEDIR__|$(includedir)|g' \
-		$(DESTDIR)/$(pkgconfdir)/$(PKGFILE)
+		$(DESTDIR)$(pkgconfdir)/$(PKGFILE)
 	@for file in docs/man/*.3.gz; do \
 		$(INSTALL_PROGRAM) -m 644 -D \
 			$$file \
-			$(DESTDIR)/$(man3dir)/ || exit $?; \
+			$(DESTDIR)$(man3dir)/ || exit $?; \
 	done
 
 uninstall:
 	$(RM) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(RM) $(DESTDIR)$(includedir)/$(HEADERS)
-	$(RM) $(DESTDIR)/$(syslibdir)/$(DEVLIB)
-	@for file in $(DESTDIR)/$(man3dir)/dmmp_*; do \
+	$(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB)
+	@for file in $(DESTDIR)$(man3dir)/dmmp_*; do \
 		$(RM) $$file; \
 	done
 	$(RM) $(DESTDIR)$(man3dir)/libdmmp.h*
diff --git a/libdmmp/test/Makefile b/libdmmp/test/Makefile
index 68f1af3..acfb3bf 100644
--- a/libdmmp/test/Makefile
+++ b/libdmmp/test/Makefile
@@ -27,4 +27,4 @@ speed_test: $(SPD_TEST_EXEC)
 		time -p ./$(SPD_TEST_EXEC)
 
 clean:
-	rm -f $(TEST_EXEC)
+	rm -f $(TEST_EXEC) $(SPD_TEST_EXEC)
-- 
1.8.3.1

[PATCH 2/9] multipath-tools: remove incdir from Makefiles

[Benjamin Marzinski]

Makefile.in defines both incdir and includedir, and they are both the
same. This patch removes incdir, and makes all the Makefiles use
includedir.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 Makefile.inc             | 1 -
 libmpathcmd/Makefile     | 6 +++---
 libmpathpersist/Makefile | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Makefile.inc b/Makefile.inc
index 740081e..c8b1142 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -55,7 +55,6 @@ man8dir		= $(prefix)/usr/share/man/man8
 man5dir		= $(prefix)/usr/share/man/man5
 man3dir		= $(prefix)/usr/share/man/man3
 syslibdir	= $(prefix)/$(LIB)
-incdir		= $(prefix)/usr/include
 libdir		= $(prefix)/$(LIB)/multipath
 unitdir		= $(prefix)/$(SYSTEMDPATH)/systemd/system
 mpathpersistdir	= $(TOPDIR)/libmpathpersist
diff --git a/libmpathcmd/Makefile b/libmpathcmd/Makefile
index b8e29cb..9cda94c 100644
--- a/libmpathcmd/Makefile
+++ b/libmpathcmd/Makefile
@@ -16,13 +16,13 @@ install: $(LIBS)
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(syslibdir)
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
-	$(INSTALL_PROGRAM) -d $(DESTDIR)$(incdir)
-	$(INSTALL_PROGRAM) -m 644 mpath_cmd.h $(DESTDIR)$(incdir)
+	$(INSTALL_PROGRAM) -d $(DESTDIR)$(includedir)
+	$(INSTALL_PROGRAM) -m 644 mpath_cmd.h $(DESTDIR)$(includedir)
 
 uninstall:
 	$(RM) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB)
-	$(RM) $(DESTDIR)$(incdir)/mpath_cmd.h
+	$(RM) $(DESTDIR)$(includedir)/mpath_cmd.h
 
 clean:
 	$(RM) core *.a *.o *.so *.so.* *.gz
diff --git a/libmpathpersist/Makefile b/libmpathpersist/Makefile
index 96da639..857c8d8 100644
--- a/libmpathpersist/Makefile
+++ b/libmpathpersist/Makefile
@@ -24,17 +24,17 @@ install: $(LIBS)
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(syslibdir)
 	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(man3dir)
-	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(incdir)
+	$(INSTALL_PROGRAM) -m 755 -d $(DESTDIR)$(includedir)
 	$(LN) $(LIBS) $(DESTDIR)$(syslibdir)/$(DEVLIB)
 	$(INSTALL_PROGRAM) -m 644 mpath_persistent_reserve_in.3.gz $(DESTDIR)$(man3dir)
 	$(INSTALL_PROGRAM) -m 644 mpath_persistent_reserve_out.3.gz $(DESTDIR)$(man3dir)
-	$(INSTALL_PROGRAM) -m 644 mpath_persist.h $(DESTDIR)$(incdir)
+	$(INSTALL_PROGRAM) -m 644 mpath_persist.h $(DESTDIR)$(includedir)
 
 uninstall:
 	$(RM) $(DESTDIR)$(syslibdir)/$(LIBS)
 	$(RM) $(DESTDIR)$(man3dir)/mpath_persistent_reserve_in.3.gz
 	$(RM) $(DESTDIR)$(man3dir)/mpath_persistent_reserve_out.3.gz
-	$(RM) $(DESTDIR)$(incdir)/mpath_persist.h
+	$(RM) $(DESTDIR)$(includedir)/mpath_persist.h
 	$(RM) $(DESTDIR)$(syslibdir)/$(DEVLIB)
 
 clean:
-- 
1.8.3.1

Re: [PATCH 2/9] multipath-tools: remove incdir from Makefiles

[Martin Wilck]

On Fri, 2017-04-07 at 01:16 -0500, Benjamin Marzinski wrote:
> Makefile.in defines both incdir and includedir, and they are both the
> same. This patch removes incdir, and makes all the Makefiles use
> includedir.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

To be fair, incdir was there before includedir ... but as you added
incdir yourself in c146b58, you were certainly authorized to remove it
again :-)

Martin

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

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

[PATCH 3/9] libdmmp: don't disconnect from multipathd twice

[Benjamin Marzinski]

libdmmp already disconnects from multipathd at the end of the
dmmp_mpath_array_get, so it doesn't need to do it earlier in the
function as well.

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

diff --git a/libdmmp/libdmmp.c b/libdmmp/libdmmp.c
index e29a639..3906335 100644
--- a/libdmmp/libdmmp.c
+++ b/libdmmp/libdmmp.c
@@ -174,7 +174,6 @@ int dmmp_mpath_array_get(struct dmmp_context *ctx,
 		errno_save = errno;
 		memset(errno_str_buff, 0, _ERRNO_STR_BUFF_SIZE);
 		strerror_r(errno_save, errno_str_buff, _ERRNO_STR_BUFF_SIZE);
-		mpath_disconnect(socket_fd);
 		if (errno_save == ETIMEDOUT) {
 			rc = DMMP_ERR_IPC_TIMEOUT;
 			_error(ctx, "IPC communication timeout, try to "
-- 
1.8.3.1

[PATCH 4/9] multipathd: don't call strlen on NULL variables

[Benjamin Marzinski]

strlen has undefined results when passed a NULL variable, so don't do
it.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 283d81d..f671d58 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1076,7 +1076,8 @@ uxsock_trigger (char * str, char ** reply, int * len, bool is_root,
 	    (strncmp(str, "list", strlen("list")) != 0) &&
 	    (strncmp(str, "show", strlen("show")) != 0)) {
 		*reply = STRDUP("permission deny: need to be root");
-		*len = strlen(*reply) + 1;
+		if (*reply)
+			*len = strlen(*reply) + 1;
 		return 1;
 	}
 
@@ -1087,12 +1088,14 @@ uxsock_trigger (char * str, char ** reply, int * len, bool is_root,
 			*reply = STRDUP("timeout\n");
 		else
 			*reply = STRDUP("fail\n");
-		*len = strlen(*reply) + 1;
+		if (*reply)
+			*len = strlen(*reply) + 1;
 		r = 1;
 	}
 	else if (!r && *len == 0) {
 		*reply = STRDUP("ok\n");
-		*len = strlen(*reply) + 1;
+		if (*reply)
+			*len = strlen(*reply) + 1;
 		r = 0;
 	}
 	/* else if (r < 0) leave *reply alone */
-- 
1.8.3.1

Re: [PATCH 4/9] multipathd: don't call strlen on NULL variables

[Martin Wilck]

On Fri, 2017-04-07 at 01:16 -0500, Benjamin Marzinski wrote:
> strlen has undefined results when passed a NULL variable, so don't do
> it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

This is certainly correct. Yet I have two remarks: 

1) There are many more calls to strlen() in the multipath-tools code
which would need to be likewise protected.

2) If STRDUP("ok\n") returns NULL, we're likely to be so hosed that we
might as well call abort() anyway (which is a philosophy that I
personally quite like - multipathd is not such a vital part of the
system that it can't risk dying).

Regards
Martin

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

[PATCH 5/9] libdmmp: move libdmmp.pc install location

[Benjamin Marzinski]

/usr/share/pkgconfig is for non-architecture specific modules, and
/usr/lib/pkgconfig is for architecture specific modules. libdmmp.pc is
architecture specific, so it belongs in /usr/lib/pkgconfig

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 c8b1142..8361e6c 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -62,7 +62,7 @@ mpathcmddir	= $(TOPDIR)/libmpathcmd
 thirdpartydir	= $(TOPDIR)/third-party
 libdmmpdir	= $(TOPDIR)/libdmmp
 includedir	= $(prefix)/usr/include
-pkgconfdir	= $(prefix)/usr/share/pkgconfig
+pkgconfdir	= $(prefix)/$(LIB)/pkgconfig
 
 GZIP		= gzip -9 -c
 RM		= rm -f
-- 
1.8.3.1

[PATCH 6/9] multipathd: drop lock before calling uev_add_path

[Benjamin Marzinski]

commit c6a18f4541d0a161e2f5fed8c67d9732bf512b37 made uev_update_path
call uev_add_path while holding the vecs lock, which is deadlocks, since
uev_add_path grabs the vecs lock itself.

Cc: Alban Browaeys <prahal@yahoo.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index f671d58..f4ff13e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -974,6 +974,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 	struct path * pp;
 	struct config *conf;
 	int disable_changed_wwids;
+	int needs_reinit = 0;
 
 	conf = get_multipath_config();
 	disable_changed_wwids = conf->disable_changed_wwids;
@@ -1009,7 +1010,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		}
 
 		if (pp->initialized == INIT_REQUESTED_UDEV)
-			retval = uev_add_path(uev, vecs, 1);
+			needs_reinit = 1;
 		else if (mpp && ro >= 0) {
 			condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
 
@@ -1041,7 +1042,8 @@ out:
 
 		condlog(0, "%s: spurious uevent, path not found", uev->kernel);
 	}
-
+	if (needs_reinit)
+		retval = uev_add_path(uev, vecs, 1);
 	return retval;
 }
 
-- 
1.8.3.1

[PATCH 7/9] multipathd: allow devices to switch from RW to RO

[Benjamin Marzinski]

Whenever multipathd tries to reload a device, even if it's because a
path switched from read/write to read-only, it tries to load the device
read/write first, and then falls back to read-only. When device-mapper
sees that multipath is using the same devices in the same state in its
new table, it simply reuses the devices from the old table, instead of
closing and re-opening them. This means that multipath can successfully
reload the multipath device read/write, even if a path device has
switched to read-only.  To deal with this, multipathd now doesn't try to
reload a device read/write when it sees that a path device has switched
to read-only.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 9 +++++----
 libmultipath/structs.h   | 1 +
 multipathd/main.c        | 3 +++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 044be2b..026418f 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -371,7 +371,7 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 
 int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 {
-	int r;
+	int r = 0;
 	uint16_t udev_flags = (flush ? 0 : MPATH_UDEV_RELOAD_FLAG) |
 			      ((mpp->skip_kpartx == SKIP_KPARTX_ON)?
 			       MPATH_UDEV_NO_KPARTX_FLAG : 0) |
@@ -383,10 +383,11 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 	 * DM_DEVICE_RESUME. So call DM_DEVICE_RESUME
 	 * after each successful call to DM_DEVICE_RELOAD.
 	 */
-	r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW,
-		      SKIP_KPARTX_OFF);
+	if (!mpp->force_readonly)
+		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
+			      ADDMAP_RW, SKIP_KPARTX_OFF);
 	if (!r) {
-		if (errno != EROFS)
+		if (!mpp->force_readonly && errno != EROFS)
 			return 0;
 		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp,
 			      params, ADDMAP_RO, SKIP_KPARTX_OFF);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index fdcfc85..98e13e4 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -271,6 +271,7 @@ struct multipath {
 	int san_path_err_recovery_time;
 	int skip_kpartx;
 	int max_sectors_kb;
+	int force_readonly;
 	unsigned int dev_loss;
 	uid_t uid;
 	gid_t gid;
diff --git a/multipathd/main.c b/multipathd/main.c
index f4ff13e..995e580 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1017,7 +1017,10 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			if (mpp->wait_for_udev)
 				mpp->wait_for_udev = 2;
 			else {
+				if (ro == 1)
+					pp->mpp->force_readonly = 1;
 				retval = reload_map(vecs, mpp, 0, 1);
+				pp->mpp->force_readonly = 0;
 				condlog(2, "%s: map %s reloaded (retval %d)",
 					uev->kernel, mpp->alias, retval);
 			}
-- 
1.8.3.1

Re: [PATCH 7/9] multipathd: allow devices to switch from RW to RO

[Martin Wilck]

On Fri, 2017-04-07 at 01:16 -0500, Benjamin Marzinski wrote:
> Whenever multipathd tries to reload a device, even if it's because a
> path switched from read/write to read-only, it tries to load the
> device
> read/write first, and then falls back to read-only. When device-
> mapper
> sees that multipath is using the same devices in the same state in
> its
> new table, it simply reuses the devices from the old table, instead
> of
> closing and re-opening them. This means that multipath can
> successfully
> reload the multipath device read/write, even if a path device has
> switched to read-only.  To deal with this, multipathd now doesn't try
> to
> reload a device read/write when it sees that a path device has
> switched
> to read-only.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/devmapper.c | 9 +++++----
>  libmultipath/structs.h   | 1 +
>  multipathd/main.c        | 3 +++
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index f4ff13e..995e580 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1017,7 +1017,10 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  			if (mpp->wait_for_udev)
>  				mpp->wait_for_udev = 2;
>  			else {
> +				if (ro == 1)
> +					pp->mpp->force_readonly = 1;
>  				retval = reload_map(vecs, mpp, 0,
> 1);
> +				pp->mpp->force_readonly = 0;

Why don't you leave this set to 1 until all paths have been switched to
rw mode? AFAICS, if any uevent arrives except a switch to ro (assume
several paths are ro and one switches back to rw), multipathd will
reload the map r/w. Or am I overlooking something?

Regards,
Martin

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

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

Re: [PATCH 7/9] multipathd: allow devices to switch from RW to RO

[Benjamin Marzinski]

On Thu, Apr 13, 2017 at 04:39:45PM +0200, Martin Wilck wrote:
> On Fri, 2017-04-07 at 01:16 -0500, Benjamin Marzinski wrote:
> > Whenever multipathd tries to reload a device, even if it's because a
> > path switched from read/write to read-only, it tries to load the
> > device
> > read/write first, and then falls back to read-only. When device-
> > mapper
> > sees that multipath is using the same devices in the same state in
> > its
> > new table, it simply reuses the devices from the old table, instead
> > of
> > closing and re-opening them. This means that multipath can
> > successfully
> > reload the multipath device read/write, even if a path device has
> > switched to read-only.  To deal with this, multipathd now doesn't try
> > to
> > reload a device read/write when it sees that a path device has
> > switched
> > to read-only.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/devmapper.c | 9 +++++----
> >  libmultipath/structs.h   | 1 +
> >  multipathd/main.c        | 3 +++
> >  3 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index f4ff13e..995e580 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1017,7 +1017,10 @@ uev_update_path (struct uevent *uev, struct
> > vectors * vecs)
> >  			if (mpp->wait_for_udev)
> >  				mpp->wait_for_udev = 2;
> >  			else {
> > +				if (ro == 1)
> > +					pp->mpp->force_readonly = 1;
> >  				retval = reload_map(vecs, mpp, 0,
> > 1);
> > +				pp->mpp->force_readonly = 0;
> 
> Why don't you leave this set to 1 until all paths have been switched to
> rw mode? AFAICS, if any uevent arrives except a switch to ro (assume
> several paths are ro and one switches back to rw), multipathd will
> reload the map r/w. Or am I overlooking something?

Once multipath has reloaded the map RO, the path devices that
device-mapper has opened for it will all be RO. So, when another uevent
comes along that causes multipathd to reload the map, the R/W reload
will fail. Since the path devices in the old table are in the wrong
state, the kernel is forced to re-open them in the correct state. This
will fail if any of them are set RO.

The problem that this is fixing is that multipath usually tries
reloading the table R/W first. This means that when a path switches from
R/W to RO, the path devices in the old table will be opened R/W. When
multipath tries to reload the table R/W, it can just reuse them, since
ther state matches.

Now, we could avoid attempting the useless R/W reload by tracking the RO
status of the paths, and only trying R/W when we don't know that a path
is RO, but then we would have to track the RO status of paths. But this
should be sufficient to make multipath devices turn RO (and stay that
way) when a path does. 

-Ben

> 
> Regards,
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 7/9] multipathd: allow devices to switch from RW to RO

[Martin Wilck]

On Fri, 2017-04-07 at 01:16 -0500, Benjamin Marzinski wrote:
> Whenever multipathd tries to reload a device, even if it's because a
> path switched from read/write to read-only, it tries to load the
> device
> read/write first, and then falls back to read-only. When device-
> mapper
> sees that multipath is using the same devices in the same state in
> its
> new table, it simply reuses the devices from the old table, instead
> of
> closing and re-opening them. This means that multipath can
> successfully
> reload the multipath device read/write, even if a path device has
> switched to read-only.  To deal with this, multipathd now doesn't try
> to
> reload a device read/write when it sees that a path device has
> switched
> to read-only.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/devmapper.c | 9 +++++----
>  libmultipath/structs.h   | 1 +
>  multipathd/main.c        | 3 +++
>  3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index f4ff13e..995e580 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1017,7 +1017,10 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  			if (mpp->wait_for_udev)
>  				mpp->wait_for_udev = 2;
>  			else {
> +				if (ro == 1)
> +					pp->mpp->force_readonly = 1;
>  				retval = reload_map(vecs, mpp, 0,
> 1);
> +				pp->mpp->force_readonly = 0;

Why don't you leave this set to 1 until all paths have been switched to
rw mode? AFAICS, if any uevent arrives except a switch to ro (assume
several paths are ro and one switches back to rw), multipathd will
reload the map r/w. Or am I overlooking something?

Regards,
Martin


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

[PATCH 8/9] libmultipath: don't set max_sectors_kb on reloads

[Benjamin Marzinski]

Multipath was setting max_sectors_kb on the multipath device and all its
path devices both when the device was created, and when it was reloaded.
The problem with this is that while this would set max_sectors_kb on all
the devices under multipath, it couldn't set this on devices on top of
multipath.  This meant that if a user lowered max_sectors_kb on an
already existing multipath device with a LV on top of it, the LV could
send down IO that was too large for the new max_sectors_kb value,
because the LV was still using the old value.  The solution to this is
to only set max_sectors_kb to the configured value when the device is
originally created, not when it is later reloaded.  Since not all paths
may be present when the device is original created, on reloads multipath
still needs to make sure that the max_sectors_kb value on all the path
devices is the same as the value of the multipath device. But if this
value doesn't match the configuration value, that's o.k.

This means that the max_sectors_kb value for a multipath device won't
change after it have been initially created. All of the devices created
on top of the multipath device will inherit that value, and all of the
devices will use it all the way down, so IOs will never be mis-sized.

I also moved sysfs_set_max_sectors_kb to configure.c, since it is only
called from there, and it it makes use of static functions from there.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++--
 libmultipath/discovery.c | 23 -------------------
 libmultipath/discovery.h |  1 -
 3 files changed, 58 insertions(+), 26 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index d955455..3eb1b7a 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -302,7 +302,6 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
 	select_max_sectors_kb(conf, mpp);
 
 	sysfs_set_scsi_tmo(mpp, conf->checkint);
-	sysfs_set_max_sectors_kb(mpp);
 	put_multipath_config(conf);
 	/*
 	 * assign paths to path groups -- start with no groups and all paths
@@ -432,6 +431,58 @@ is_mpp_known_to_udev(const struct multipath *mpp)
 	return ret;
 }
 
+static int
+sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
+{
+	struct pathgroup * pgp;
+	struct path *pp;
+	char buff[11];
+	int i, j, ret, err = 0;
+	struct udev_device *udd;
+	int max_sectors_kb;
+
+	if (mpp->max_sectors_kb == MAX_SECTORS_KB_UNDEF)
+		return 0;
+	max_sectors_kb = mpp->max_sectors_kb;
+	if (is_reload) {
+		if (!mpp->dmi && dm_get_info(mpp->alias, &mpp->dmi) != 0) {
+			condlog(1, "failed to get dm info for %s", mpp->alias);
+			return 1;
+		}
+		udd = get_udev_for_mpp(mpp);
+		if (!udd) {
+			condlog(1, "failed to get udev device to set max_sectors_kb for %s", mpp->alias);
+			return 1;
+		}
+		ret = sysfs_attr_get_value(udd, "queue/max_sectors_kb", buff,
+					   sizeof(buff));
+		udev_device_unref(udd);
+		if (ret <= 0) {
+			condlog(1, "failed to get current max_sectors_kb from %s", mpp->alias);
+			return 1;
+		}
+		if (sscanf(buff, "%u\n", &max_sectors_kb) != 1) {
+			condlog(1, "can't parse current max_sectors_kb from %s",
+				mpp->alias);
+			return 1;
+		}
+	}
+	snprintf(buff, 11, "%d", max_sectors_kb);
+
+	vector_foreach_slot (mpp->pg, pgp, i) {
+		vector_foreach_slot(pgp->paths, pp, j) {
+			ret = sysfs_attr_set_value(pp->udev,
+						   "queue/max_sectors_kb",
+						   buff, strlen(buff));
+			if (ret < 0) {
+				condlog(1, "failed setting max_sectors_kb on %s : %s", pp->dev, strerror(-ret));
+				err = 1;
+			}
+		}
+	}	
+	return err;
+}
+
 static void
 select_action (struct multipath * mpp, vector curmp, int force_reload)
 {
@@ -703,16 +754,19 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 			return DOMAP_RETRY;
 		}
 
+		sysfs_set_max_sectors_kb(mpp, 0);
 		r = dm_addmap_create(mpp, params);
 
 		lock_multipath(mpp, 0);
 		break;
 
 	case ACT_RELOAD:
+		sysfs_set_max_sectors_kb(mpp, 1);
 		r = dm_addmap_reload(mpp, params, 0);
 		break;
 
 	case ACT_RESIZE:
+		sysfs_set_max_sectors_kb(mpp, 1);
 		r = dm_addmap_reload(mpp, params, 1);
 		break;
 
@@ -728,8 +782,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		r = dm_rename(mpp->alias_old, mpp->alias,
 			      conf->partition_delim, mpp->skip_kpartx);
 		put_multipath_config(conf);
-		if (r)
+		if (r) {
+			sysfs_set_max_sectors_kb(mpp, 1);
 			r = dm_addmap_reload(mpp, params, 0);
+		}
 		break;
 
 	default:
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index bfb46b1..8c51254 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -214,29 +214,6 @@ declare_sysfs_get_str(vendor);
 declare_sysfs_get_str(model);
 declare_sysfs_get_str(rev);
 
-int
-sysfs_set_max_sectors_kb(struct multipath *mpp)
-{
-	struct path *pp;
-	char buff[11];
-	int i, ret, err = 0;
-
-	if (mpp->max_sectors_kb == MAX_SECTORS_KB_UNDEF)
-		return 0;
-	snprintf(buff, 11, "%d", mpp->max_sectors_kb);
-
-	vector_foreach_slot(mpp->paths, pp, i) {
-		ret = sysfs_attr_set_value(pp->udev, "queue/max_sectors_kb",
-					   buff, strlen(buff));
-		if (ret < 0) {
-			condlog(0, "failed setting max_sectors_kb on %s : %s",
-				pp->dev, strerror(-ret));
-			err = 1;
-		}
-	}
-	return err;
-}
-
 ssize_t
 sysfs_get_vpd (struct udev_device * udev, int pg,
 	       unsigned char * buff, size_t len)
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index d16a69a..0563bfd 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -50,7 +50,6 @@ ssize_t sysfs_get_vpd (struct udev_device * udev, int pg, unsigned char * buff,
 int sysfs_get_asymmetric_access_state(struct path *pp,
 				      char *buff, int buflen);
 int get_uid(struct path * pp, int path_state, struct udev_device *udev);
-int sysfs_set_max_sectors_kb(struct multipath *mpp);
 
 /*
  * discovery bitmask
-- 
1.8.3.1

[PATCH 9/9] multipath: fix segfault with disable_changed_wwids

[Benjamin Marzinski]

When a path wwid changes, uev_update_path was dereferencing pp->mpp
without checking if it was NULL.

Cc: Shichangkuo <shi.changkuo@h3c.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 995e580..0c61caa 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1002,7 +1002,8 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 				if (!pp->wwid_changed) {
 					pp->wwid_changed = 1;
 					pp->tick = 1;
-					dm_fail_path(pp->mpp->alias, pp->dev_t);
+					if (pp->mpp)
+						dm_fail_path(pp->mpp->alias, pp->dev_t);
 				}
 				goto out;
 			} else
-- 
1.8.3.1

Re: [PATCH 9/9] multipath: fix segfault with disable_changed_wwids

[Shichangkuo]

> -----Original Message-----
> From: Benjamin Marzinski [mailto:bmarzins@redhat.com]
> Sent: Friday, April 07, 2017 2:17 PM
> To: device-mapper development
> Cc: Christophe Varoqui; shichangkuo 09727 (Cloud)
> Subject: [PATCH 9/9] multipath: fix segfault with disable_changed_wwids
>
> When a path wwid changes, uev_update_path was dereferencing pp->mpp
> without checking if it was NULL.
>
> Cc: Shichangkuo <shi.changkuo@h3c.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c index 995e580..0c61caa
> 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1002,7 +1002,8 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>                               if (!pp->wwid_changed) {
>                                       pp->wwid_changed = 1;
>                                       pp->tick = 1;
> -                                     dm_fail_path(pp->mpp->alias, pp->dev_t);
> +                                     if (pp->mpp)
> +                                             dm_fail_path(pp->mpp->alias, pp->dev_t);
>                               }
>                               goto out;
>                       } else
> --
> 1.8.3.1

Thanks a lot for your work. It looks good to me, and I will have a test on this patch.

Thanks, again.
Changkuo
-------------------------------------------------------------------------------------------------------------------------------------
本邮件及其附件含有杭州华三通信技术有限公司的保密信息，仅限于发送给上面地址中列出
的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制、
或散发）本邮件中的信息。如果您错收了本邮件，请您立即电话或邮件通知发件人并删除本
邮件！
This e-mail and its attachments contain confidential information from H3C, which is
intended only for the person or entity whose address is listed above. Any use of the
information contained herein in any way (including, but not limited to, total or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
by phone or email immediately and delete it!

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

Re: [PATCH 0/9] misc cleanups and bugfixes

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 1250 bytes --]

Merged,
thanks.

On Fri, Apr 7, 2017 at 8:16 AM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> These patches are all just small cleanup and bugfixes.
>
> Benjamin Marzinski (9):
>   libdmmp: minor Makefile cleanup
>   multipath-tools: remove incdir from Makefiles
>   libdmmp: don't disconnect from multipathd twice
>   multipathd: don't call strlen on NULL variables
>   libdmmp: move libdmmp.pc install location
>   multipathd: drop lock before calling uev_add_path
>   multipathd: allow devices to switch from RW to RO
>   libmultipath: don't set max_sectors_kb on reloads
>   multipath: fix segfault with disable_changed_wwids
>
>  Makefile.inc             |  3 +--
>  libdmmp/Makefile         | 18 +++++++--------
>  libdmmp/libdmmp.c        |  1 -
>  libdmmp/test/Makefile    |  2 +-
>  libmpathcmd/Makefile     |  6 ++---
>  libmpathpersist/Makefile |  6 ++---
>  libmultipath/configure.c | 60 ++++++++++++++++++++++++++++++
> ++++++++++++++++--
>  libmultipath/devmapper.c |  9 ++++----
>  libmultipath/discovery.c | 23 -------------------
>  libmultipath/discovery.h |  1 -
>  libmultipath/structs.h   |  1 +
>  multipathd/main.c        | 21 ++++++++++++-----
>  12 files changed, 96 insertions(+), 55 deletions(-)
>
> --
> 1.8.3.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 1774 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/9] misc cleanups and bugfixes

[Martin Wilck]

On Fri, 2017-04-07 at 01:16 -0500, Benjamin Marzinski wrote:
> These patches are all just small cleanup and bugfixes.
> 
> Benjamin Marzinski (9):
>   libdmmp: minor Makefile cleanup
>   multipath-tools: remove incdir from Makefiles
>   libdmmp: don't disconnect from multipathd twice
>   multipathd: don't call strlen on NULL variables
>   libdmmp: move libdmmp.pc install location
>   multipathd: drop lock before calling uev_add_path
>   multipathd: allow devices to switch from RW to RO
>   libmultipath: don't set max_sectors_kb on reloads
>   multipath: fix segfault with disable_changed_wwids
> 
>  Makefile.inc             |  3 +--
>  libdmmp/Makefile         | 18 +++++++--------
>  libdmmp/libdmmp.c        |  1 -
>  libdmmp/test/Makefile    |  2 +-
>  libmpathcmd/Makefile     |  6 ++---
>  libmpathpersist/Makefile |  6 ++---
>  libmultipath/configure.c | 60
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  libmultipath/devmapper.c |  9 ++++----
>  libmultipath/discovery.c | 23 -------------------
>  libmultipath/discovery.h |  1 -
>  libmultipath/structs.h   |  1 +
>  multipathd/main.c        | 21 ++++++++++++-----
>  12 files changed, 96 insertions(+), 55 deletions(-)
> 

All of these are applied already, but still:

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

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

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