All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/3] libmultipath: assign variable to make gcc happy
@ 2020-03-26  4:22 Benjamin Marzinski
  2020-03-26  4:22 ` [PATCH v2 2/3] libmutipath: don't close fd on dm_lib_release Benjamin Marzinski
  2020-03-26  4:22 ` [PATCH v2 3/3] libmultipath: allow force reload with no active paths Benjamin Marzinski
  0 siblings, 2 replies; 4+ messages in thread
From: Benjamin Marzinski @ 2020-03-26  4:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

There is nothing wrong with is_queueing not being set at the start
of __set_no_path_retry(), it will always get set before it is accessed,
but gcc 8.2.1 is failing with

structs_vec.c: In function ‘__set_no_path_retry’:
structs_vec.c:339:7: error: ‘is_queueing’ may be used uninitialized in
this function [-Werror=maybe-uninitialized]
  bool is_queueing;
       ^~~~~~~~~~~

so, assign a value to make it happy.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 3dbbaa0f..077f2e42 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -336,7 +336,7 @@ static void leave_recovery_mode(struct multipath *mpp)
 
 void __set_no_path_retry(struct multipath *mpp, bool check_features)
 {
-	bool is_queueing;
+	bool is_queueing = false; /* assign a value to make gcc happy */
 
 	check_features = check_features && mpp->features != NULL;
 	if (check_features)
-- 
2.17.2

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 2/3] libmutipath: don't close fd on dm_lib_release
  2020-03-26  4:22 [PATCH v2 1/3] libmultipath: assign variable to make gcc happy Benjamin Marzinski
@ 2020-03-26  4:22 ` Benjamin Marzinski
  2020-03-26  8:41   ` Martin Wilck
  2020-03-26  4:22 ` [PATCH v2 3/3] libmultipath: allow force reload with no active paths Benjamin Marzinski
  1 sibling, 1 reply; 4+ messages in thread
From: Benjamin Marzinski @ 2020-03-26  4:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

If dm_hold_control_open() isn't set, when dm_lib_release() is called, it
will close the control fd. The control fd will get re-opened on the next
dm_task_run() call, but if there is a dm_task_run() call already
in progress in another thread, it can fail. Since many of the
device-mapper callouts happen with the vecs lock held, this wasn't too
noticeable, but there is code that calls dm_task_run() without the
vecs lock held, notably the dmevent waiter code.

Since, as Martin pointed out, dm_hold_control_open() hasn't always
existed in libdevmapper, check if it's supported on compilation,
and update the version requirements if so.

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

diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index e5651e49..ad690a49 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -36,6 +36,10 @@ ifneq ($(call check_func,dm_task_deferred_remove,/usr/include/libdevmapper.h),0)
 	CFLAGS += -DLIBDM_API_DEFERRED
 endif
 
+ifneq ($(call check_func,dm_hold_control_dev,/usr/include/libdevmapper.h),0)
+	CFLAGS += -DLIBDM_API_HOLD_CONTROL
+endif
+
 OBJS = memory.o parser.o vector.o devmapper.o callout.o \
 	hwtable.o blacklist.o util.o dmparser.o config.o \
 	structs.o discovery.o propsel.o dict.o \
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index bed8ddc6..13a1cf53 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -108,7 +108,9 @@ dm_lib_prereq (void)
 {
 	char version[64];
 	int v[3];
-#if defined(LIBDM_API_DEFERRED)
+#if defined(LIBDM_API_HOLD_CONTROL)
+	int minv[3] = {1, 2, 111};
+#elif defined(LIBDM_API_DEFERRED)
 	int minv[3] = {1, 2, 89};
 #elif defined(DM_SUBSYSTEM_UDEV_FLAG0)
 	int minv[3] = {1, 2, 82};
@@ -254,6 +256,9 @@ void libmp_dm_init(void)
 	memcpy(conf->version, version, sizeof(version));
 	put_multipath_config(conf);
 	dm_init(verbosity);
+#ifdef LIBDM_API_HOLD_CONTROL
+	dm_hold_control_dev(1);
+#endif
 	dm_udev_set_sync_support(libmp_dm_udev_sync);
 }
 
-- 
2.17.2

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v2 3/3] libmultipath: allow force reload with no active paths
  2020-03-26  4:22 [PATCH v2 1/3] libmultipath: assign variable to make gcc happy Benjamin Marzinski
  2020-03-26  4:22 ` [PATCH v2 2/3] libmutipath: don't close fd on dm_lib_release Benjamin Marzinski
@ 2020-03-26  4:22 ` Benjamin Marzinski
  1 sibling, 0 replies; 4+ messages in thread
From: Benjamin Marzinski @ 2020-03-26  4:22 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

If the partition information has changed on multipath devices (say,
because it was updated on another node that has access to the same
storage), users expect that running "multipathd reconfigure" will update
that.  However, if the checkers for the multipath device are pending for
too long when the the device is reconfigured, multipathd will give up
waiting for them, and refuse to reload the device, since there are no
active paths. This means that no kpartx update will be triggered.

Multipath is fully capable of reloading a multipath device that has no
active paths. This has been possible for years. If multipath is supposed
to reload the device, it should do so, even if there are no active paths.

Generally, when multipath is force reloaded, kpartx will be updated.
However when a device is reloaded with no paths, the udev rules won't
run kpartx.  But they also weren't running kpartx when the first valid
path appeared, even though the dm activation rules get run in this case.
This changes 11-dm-mpath.rules to run kpartx when a device goes from no
usable paths to having usable paths.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c    | 6 ------
 multipath/11-dm-mpath.rules | 2 +-
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index c95848a0..96c79610 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -710,12 +710,6 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 		return;
 	}
 
-	if (pathcount(mpp, PATH_UP) == 0) {
-		mpp->action = ACT_IMPOSSIBLE;
-		condlog(3, "%s: set ACT_IMPOSSIBLE (no usable path)",
-			mpp->alias);
-		return;
-	}
 	if (force_reload) {
 		mpp->force_udev_reload = 1;
 		mpp->action = ACT_RELOAD;
diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 07320a14..cd522e8c 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -75,7 +75,7 @@ ENV{MPATH_DEVICE_READY}=="0", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
 ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
 	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="",\
-	ENV{DM_ACTIVATION}="1"
+	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
 
 # The code to check multipath state ends here. We need to set
 # properties and symlinks regardless whether the map is usable or
-- 
2.17.2

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v2 2/3] libmutipath: don't close fd on dm_lib_release
  2020-03-26  4:22 ` [PATCH v2 2/3] libmutipath: don't close fd on dm_lib_release Benjamin Marzinski
@ 2020-03-26  8:41   ` Martin Wilck
  0 siblings, 0 replies; 4+ messages in thread
From: Martin Wilck @ 2020-03-26  8:41 UTC (permalink / raw)
  To: bmarzins, christophe.varoqui; +Cc: dm-devel

On Wed, 2020-03-25 at 23:22 -0500, Benjamin Marzinski wrote:
> If dm_hold_control_open() isn't set, when dm_lib_release() is called,
> it
> will close the control fd. The control fd will get re-opened on the
> next
> dm_task_run() call, but if there is a dm_task_run() call already
> in progress in another thread, it can fail. Since many of the
> device-mapper callouts happen with the vecs lock held, this wasn't
> too
> noticeable, but there is code that calls dm_task_run() without the
> vecs lock held, notably the dmevent waiter code.
> 
> Since, as Martin pointed out, dm_hold_control_open() hasn't always
> existed in libdevmapper, check if it's supported on compilation,
> and update the version requirements if so.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-03-26  8:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26  4:22 [PATCH v2 1/3] libmultipath: assign variable to make gcc happy Benjamin Marzinski
2020-03-26  4:22 ` [PATCH v2 2/3] libmutipath: don't close fd on dm_lib_release Benjamin Marzinski
2020-03-26  8:41   ` Martin Wilck
2020-03-26  4:22 ` [PATCH v2 3/3] libmultipath: allow force reload with no active paths Benjamin Marzinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.