* [PATCH 01/19] multipathd: allow shutdown during configure()
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 02/19] multipathd: avoid sending "READY=1" to systemd on early shutdown mwilck
` (18 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski
Cc: Chongyun Wu, dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
reconfigure() can be a long-running operation; both initial path
discovery and initial map setup can take a long time. Allow
the main program to indicate that the process should be
interrupted if a shutdown signal was received.
We take advantage of the dynamic linker's symbol lookup ordering
here. The default implementation of should_exit never returns
true, but callers (like multipathd) can override it.
Cc: Chongyun Wu <wu.chongyun@h3c.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/configure.c | 6 ++++++
libmultipath/discovery.c | 3 +++
libmultipath/util.c | 5 +++++
libmultipath/util.h | 1 +
multipathd/main.c | 17 +++++++++++++++++
5 files changed, 32 insertions(+)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 213dbe4..7a8eef3 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1159,6 +1159,12 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
vector_foreach_slot (pathvec, pp1, k) {
int invalid;
+
+ if (should_exit()) {
+ ret = CP_FAIL;
+ goto out;
+ }
+
/* skip this path for some reason */
/* 1. if path has no unique id or wwid blacklisted */
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3f1b1d7..000f4c9 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -200,6 +200,9 @@ path_discovery (vector pathvec, int flag)
const char *devtype;
const char *devpath;
+ if (should_exit())
+ break;
+
devpath = udev_list_entry_get_name(entry);
condlog(4, "Discover device %s", devpath);
udevice = udev_device_new_from_syspath(udev, devpath);
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 1512424..7a0b2eb 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -422,3 +422,8 @@ void _log_bitfield_overflow(const char *f, unsigned int bit, unsigned int len)
{
condlog(0, "%s: bitfield overflow: %u >= %u", f, bit, len);
}
+
+int should_exit(void)
+{
+ return 0;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index 045bbee..a0d3d50 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -27,6 +27,7 @@ int parse_prkey(const char *ptr, uint64_t *prkey);
int parse_prkey_flags(const char *ptr, uint64_t *prkey, uint8_t *flags);
int safe_write(int fd, const void *buf, size_t count);
void set_max_fds(rlim_t max_fds);
+int should_exit(void);
#define KERNEL_VERSION(maj, min, ptc) ((((maj) * 256) + (min)) * 256 + (ptc))
#define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))
diff --git a/multipathd/main.c b/multipathd/main.c
index f7229a7..d5e90b2 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -141,6 +141,11 @@ static inline enum daemon_status get_running_state(void)
return st;
}
+int should_exit(void)
+{
+ return get_running_state() == DAEMON_SHUTDOWN;
+}
+
/*
* global copy of vecs for use in sig handlers
*/
@@ -2580,6 +2585,9 @@ configure (struct vectors * vecs)
goto fail;
}
+ if (should_exit())
+ goto fail;
+
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
vector_foreach_slot (vecs->pathvec, pp, i){
@@ -2596,6 +2604,9 @@ configure (struct vectors * vecs)
goto fail;
}
+ if (should_exit())
+ goto fail;
+
/*
* create new set of maps & push changed ones into dm
* In the first call, use FORCE_RELOAD_WEAK to avoid making
@@ -2610,6 +2621,9 @@ configure (struct vectors * vecs)
goto fail;
}
+ if (should_exit())
+ goto fail;
+
/*
* may need to remove some maps which are no longer relevant
* e.g., due to blacklist changes in conf file
@@ -2621,6 +2635,9 @@ configure (struct vectors * vecs)
dm_lib_release();
+ if (should_exit())
+ goto fail;
+
sync_maps_state(mpvec);
vector_foreach_slot(mpvec, mpp, i){
if (remember_wwid(mpp->wwid) == 1)
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 02/19] multipathd: avoid sending "READY=1" to systemd on early shutdown
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
2020-09-16 15:37 ` [PATCH 01/19] multipathd: allow shutdown during configure() mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 03/19] multipathd: send "STOPPING=1" to systemd on shutdown mwilck
` (17 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
If multipathd gets a shutdown request during initial reconfigure(),
it shouldn't send "READY=1" to systemd. Ensure this by sending
"READY=1" via post_config_state().
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index d5e90b2..afc6c99 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -189,6 +189,8 @@ static void do_sd_notify(enum daemon_status old_state,
{
char notify_msg[MSG_SIZE];
const char *msg;
+ static bool startup_done = false;
+
/*
* Checkerloop switches back and forth between idle and running state.
* No need to tell systemd each time.
@@ -205,6 +207,11 @@ static void do_sd_notify(enum daemon_status old_state,
if (msg && !safe_sprintf(notify_msg, "STATUS=%s", msg))
sd_notify(0, notify_msg);
+
+ if (new_state == DAEMON_IDLE && !startup_done) {
+ sd_notify(0, "READY=1");
+ startup_done = true;
+ }
}
#endif
@@ -2913,9 +2920,6 @@ child (__attribute__((unused)) void *param)
struct vectors * vecs;
struct multipath * mpp;
int i;
-#ifdef USE_SYSTEMD
- int startup_done = 0;
-#endif
int rc;
int pid_fd = -1;
struct config *conf;
@@ -3075,12 +3079,6 @@ child (__attribute__((unused)) void *param)
}
lock_cleanup_pop(vecs->lock);
post_config_state(DAEMON_IDLE);
-#ifdef USE_SYSTEMD
- if (!startup_done) {
- sd_notify(0, "READY=1");
- startup_done = 1;
- }
-#endif
}
}
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 03/19] multipathd: send "STOPPING=1" to systemd on shutdown
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
2020-09-16 15:37 ` [PATCH 01/19] multipathd: allow shutdown during configure() mwilck
2020-09-16 15:37 ` [PATCH 02/19] multipathd: avoid sending "READY=1" to systemd on early shutdown mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 04/19] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state mwilck
` (16 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Inform systemd that the daemon is shutting down. See sd_notify(3).
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index afc6c99..2923c08 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -208,7 +208,9 @@ static void do_sd_notify(enum daemon_status old_state,
if (msg && !safe_sprintf(notify_msg, "STATUS=%s", msg))
sd_notify(0, notify_msg);
- if (new_state == DAEMON_IDLE && !startup_done) {
+ if (new_state == DAEMON_SHUTDOWN)
+ sd_notify(0, "STOPPING=1");
+ else if (new_state == DAEMON_IDLE && !startup_done) {
sd_notify(0, "READY=1");
startup_done = true;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 04/19] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (2 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 03/19] multipathd: send "STOPPING=1" to systemd on shutdown mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 05/19] multipathd: use volatile qualifier for running_state mwilck
` (15 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
The logic is as follows: child() sets DAEMON_IDLE status after
DAEMON_CONFIGURE when reconfigure() has finished. The only other state change
that can race with that is DAEMON_SHUTDOWN. Other state changes will wait for
DAEMON_IDLE first (see set_config_state()). When DAEMON_CONFIGURE is entered,
and we are not just starting up, send a "RELOADING=1" message to
systemd. After that, we must send "READY=1" when we're done reloading. Also
do that on startup, when DAEMON_IDLE is set for the first time.
See sd_notify(3).
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 2923c08..8cc9b74 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -210,10 +210,11 @@ static void do_sd_notify(enum daemon_status old_state,
if (new_state == DAEMON_SHUTDOWN)
sd_notify(0, "STOPPING=1");
- else if (new_state == DAEMON_IDLE && !startup_done) {
+ else if (new_state == DAEMON_IDLE && old_state == DAEMON_CONFIGURE) {
sd_notify(0, "READY=1");
startup_done = true;
- }
+ } else if (new_state == DAEMON_CONFIGURE && startup_done)
+ sd_notify(0, "RELOADING=1");
}
#endif
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 05/19] multipathd: use volatile qualifier for running_state
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (3 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 04/19] multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 06/19] multipathd: generalize and fix wait_for_state_change_if() mwilck
` (14 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
While we access running_state only under the config_lock,
we sometimes do in a loop. Use the volatile qualifier to make
sure compilers can't optimize away the loads.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 8cc9b74..451d68c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -126,7 +126,7 @@ int poll_dmevents = 0;
int poll_dmevents = 1;
#endif
/* Don't access this variable without holding config_lock */
-enum daemon_status running_state = DAEMON_INIT;
+volatile enum daemon_status running_state = DAEMON_INIT;
pid_t daemon_pid;
pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t config_cond;
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 06/19] multipathd: generalize and fix wait_for_state_change_if()
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (4 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 05/19] multipathd: use volatile qualifier for running_state mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 07/19] multipathd: set_config_state(): avoid code duplication mwilck
` (13 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
It's unlikely but not impossible that other threads change the state
while we're waiting, and if we grab the lock again, it's still not
what we wanted. We need to continue waiting until either the condition
is met, or time timeout expired.
Moreover, generalize this code so that it can also be used in
set_config_state().
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 451d68c..49809e0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -223,6 +223,23 @@ static void config_cleanup(__attribute__((unused)) void *arg)
pthread_mutex_unlock(&config_lock);
}
+#define __wait_for_state_change(condition, ms) \
+ ({ \
+ struct timespec tmo; \
+ int rc = 0; \
+ \
+ if (condition) { \
+ get_monotonic_time(&tmo); \
+ tmo.tv_nsec += (ms) * 1000 * 1000; \
+ normalize_timespec(&tmo); \
+ do \
+ rc = pthread_cond_timedwait( \
+ &config_cond, &config_lock, &tmo); \
+ while (rc == 0 && (condition)); \
+ } \
+ rc; \
+ })
+
/*
* If the current status is @oldstate, wait for at most @ms milliseconds
* for the state to change, and return the new state, which may still be
@@ -232,20 +249,14 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
unsigned long ms)
{
enum daemon_status st;
- struct timespec tmo;
if (oldstate == DAEMON_SHUTDOWN)
return DAEMON_SHUTDOWN;
pthread_mutex_lock(&config_lock);
pthread_cleanup_push(config_cleanup, NULL);
+ __wait_for_state_change(running_state == oldstate, ms);
st = running_state;
- if (st == oldstate && clock_gettime(CLOCK_MONOTONIC, &tmo) == 0) {
- tmo.tv_nsec += ms * 1000 * 1000;
- normalize_timespec(&tmo);
- (void)pthread_cond_timedwait(&config_cond, &config_lock, &tmo);
- st = running_state;
- }
pthread_cleanup_pop(1);
return st;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 07/19] multipathd: set_config_state(): avoid code duplication
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (5 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 06/19] multipathd: generalize and fix wait_for_state_change_if() mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-19 19:01 ` Benjamin Marzinski
2020-09-16 15:37 ` [PATCH 08/19] multipathd: cancel threads early during shutdown mwilck
` (12 subsequent siblings)
19 siblings, 1 reply; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Use __post_config_state() and __wait_for_state_change(). This
way __post_config_state() is the only place where running_state
is ever changed, and we avoid code duplication.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 23 +++++------------------
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 49809e0..a5c4031 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -292,27 +292,14 @@ int set_config_state(enum daemon_status state)
pthread_cleanup_push(config_cleanup, NULL);
pthread_mutex_lock(&config_lock);
if (running_state != state) {
-#ifdef USE_SYSTEMD
- enum daemon_status old_state = running_state;
-#endif
if (running_state == DAEMON_SHUTDOWN)
rc = EINVAL;
- else if (running_state != DAEMON_IDLE) {
- struct timespec ts;
-
- get_monotonic_time(&ts);
- ts.tv_sec += 1;
- rc = pthread_cond_timedwait(&config_cond,
- &config_lock, &ts);
- }
- if (!rc && (running_state != DAEMON_SHUTDOWN)) {
- running_state = state;
- pthread_cond_broadcast(&config_cond);
-#ifdef USE_SYSTEMD
- do_sd_notify(old_state, state);
-#endif
- }
+ else
+ rc = __wait_for_state_change(
+ running_state != DAEMON_IDLE, 1000);
+ if (!rc)
+ __post_config_state(state);
}
pthread_cleanup_pop(1);
return rc;
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 07/19] multipathd: set_config_state(): avoid code duplication
2020-09-16 15:37 ` [PATCH 07/19] multipathd: set_config_state(): avoid code duplication mwilck
@ 2020-09-19 19:01 ` Benjamin Marzinski
0 siblings, 0 replies; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-19 19:01 UTC (permalink / raw)
To: mwilck; +Cc: dm-devel
On Wed, Sep 16, 2020 at 05:37:06PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> Use __post_config_state() and __wait_for_state_change(). This
> way __post_config_state() is the only place where running_state
> is ever changed, and we avoid code duplication.
>
It's only tangentially related to this patch, but it's possible for
set_conf_state() to timeout, and we'd don't always retry it. That's
fine, be we don't always check for failure and notify the user that the
reconfigure isn't happening, and we probably should. But the patch
itself is fine.
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> multipathd/main.c | 23 +++++------------------
> 1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 49809e0..a5c4031 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -292,27 +292,14 @@ int set_config_state(enum daemon_status state)
> pthread_cleanup_push(config_cleanup, NULL);
> pthread_mutex_lock(&config_lock);
> if (running_state != state) {
> -#ifdef USE_SYSTEMD
> - enum daemon_status old_state = running_state;
> -#endif
>
> if (running_state == DAEMON_SHUTDOWN)
> rc = EINVAL;
> - else if (running_state != DAEMON_IDLE) {
> - struct timespec ts;
> -
> - get_monotonic_time(&ts);
> - ts.tv_sec += 1;
> - rc = pthread_cond_timedwait(&config_cond,
> - &config_lock, &ts);
> - }
> - if (!rc && (running_state != DAEMON_SHUTDOWN)) {
> - running_state = state;
> - pthread_cond_broadcast(&config_cond);
> -#ifdef USE_SYSTEMD
> - do_sd_notify(old_state, state);
> -#endif
> - }
> + else
> + rc = __wait_for_state_change(
> + running_state != DAEMON_IDLE, 1000);
> + if (!rc)
> + __post_config_state(state);
> }
> pthread_cleanup_pop(1);
> return rc;
> --
> 2.28.0
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 08/19] multipathd: cancel threads early during shutdown
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (6 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 07/19] multipathd: set_config_state(): avoid code duplication mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 09/19] multipath-tools: don't call dm_lib_release() any more mwilck
` (11 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski
Cc: Chongyun Wu, dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Cancel the other threads before taking vecs->lock. This avoids
delays during shutdown caused e.g. by the checker thread holding
the vecs lock.
Note: this makes it possible that cancelled threads leak memory,
because they can now be cancelled before having released the vecs
lock. I believe this is acceptable, as only threads are affected
that are cancelled during multipathd shutdown.
Cc: Chongyun Wu <wu.chongyun@h3c.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index a5c4031..812d162 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3083,23 +3083,24 @@ child (__attribute__((unused)) void *param)
}
}
- lock(&vecs->lock);
+ pthread_cancel(check_thr);
+ pthread_cancel(uevent_thr);
+ pthread_cancel(uxlsnr_thr);
+ pthread_cancel(uevq_thr);
+ if (poll_dmevents)
+ pthread_cancel(dmevent_thr);
+
conf = get_multipath_config();
queue_without_daemon = conf->queue_without_daemon;
put_multipath_config(conf);
+
+ lock(&vecs->lock);
if (queue_without_daemon == QUE_NO_DAEMON_OFF)
vector_foreach_slot(vecs->mpvec, mpp, i)
dm_queue_if_no_path(mpp->alias, 0);
remove_maps_and_stop_waiters(vecs);
unlock(&vecs->lock);
- pthread_cancel(check_thr);
- pthread_cancel(uevent_thr);
- pthread_cancel(uxlsnr_thr);
- pthread_cancel(uevq_thr);
- if (poll_dmevents)
- pthread_cancel(dmevent_thr);
-
pthread_join(check_thr, NULL);
pthread_join(uevent_thr, NULL);
pthread_join(uxlsnr_thr, NULL);
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 09/19] multipath-tools: don't call dm_lib_release() any more
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (7 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 08/19] multipathd: cancel threads early during shutdown mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 10/19] libmultipath: devmapper: refactor libdm version determination mwilck
` (10 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski
Cc: lixiaokeng, dm-devel, Martin Wilck, Zdenek Kabelac
From: Martin Wilck <mwilck@suse.com>
The purpose of dm_lib_release() is to release stacked device node
operations in libdevmapper. This is functionality we don't need and
use any more, as we rely on udev to set up device nodes and symlinks.
We always set DM_UDEV_DISABLE_LIBRARY_FALLBACK when we run dm tasks.
In the standard CREATE and REMOVE cases, libdevmapper doesn't
stack any operations if this flag is set. The only exceptions are
a) RESUME operations with DM_ADD_NODE_ON_RESUME set. This happens
implicity when we create new maps
b) RENAME operations
In both cases, we call dm_udev_wait() after the libdm operation, which
calls update_devs() and thus has the same effect as dm_lib_release(),
cleaning out stacked operations.
OTOH, dm_lib_releases() accesses static variables in libdevmapper, so
calling it might be racy.
Drop the calls to dm_lib_release().
Cc: lixiaokeng@huawei.com
Cc: Zdenek Kabelac <zkabelac@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
kpartx/kpartx.c | 1 -
libmpathpersist/mpath_persist.c | 1 -
multipath/main.c | 1 -
multipathd/cli_handlers.c | 2 --
multipathd/main.c | 15 ++-------------
5 files changed, 2 insertions(+), 18 deletions(-)
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 98f6176..8e5bc96 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -664,7 +664,6 @@ main(int argc, char **argv){
}
end:
- dm_lib_release();
dm_lib_exit();
return r;
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index e725604..729a8e4 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -56,7 +56,6 @@ mpath_lib_init (void)
int
mpath_lib_exit (struct config *conf)
{
- dm_lib_release();
dm_lib_exit();
cleanup_prio();
cleanup_checkers();
diff --git a/multipath/main.c b/multipath/main.c
index d227e0b..650977c 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -1066,7 +1066,6 @@ main (int argc, char *argv[])
condlog(3, "restart multipath configuration process");
out:
- dm_lib_release();
dm_lib_exit();
cleanup_foreign();
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 8db3796..8f70593 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -860,7 +860,6 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
!= CP_OK)
condlog(2, "%s: coalesce_paths failed",
param);
- dm_lib_release();
FREE(refwwid);
}
} /*we attempt to create device only once*/
@@ -1032,7 +1031,6 @@ cli_resize(void *v, char **reply, int *len, void *data)
if (resize_map(mpp, size, vecs) != 0)
return 1;
- dm_lib_release();
if (setup_multipath(vecs, mpp) != 0)
return 1;
sync_map_state(mpp);
diff --git a/multipathd/main.c b/multipathd/main.c
index 812d162..affe706 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -510,7 +510,6 @@ retry:
sleep(1);
goto retry;
}
- dm_lib_release();
fail:
if (new_map && (retries < 0 || wait_for_events(mpp, vecs))) {
@@ -611,10 +610,8 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
vector_del_slot(ompv, i);
i--;
}
- else {
- dm_lib_release();
+ else
condlog(2, "%s devmap removed", ompp->alias);
- }
} else if (reassign_maps) {
condlog(3, "%s: Reassign existing device-mapper"
" devices", ompp->alias);
@@ -660,10 +657,8 @@ flush_map(struct multipath * mpp, struct vectors * vecs, int nopaths)
}
return r;
}
- else {
- dm_lib_release();
+ else
condlog(2, "%s: map flushed", mpp->alias);
- }
orphan_paths(vecs->pathvec, mpp, "map flushed");
remove_map_and_stop_waiter(mpp, vecs);
@@ -1080,7 +1075,6 @@ rescan:
else
goto fail_map;
}
- dm_lib_release();
if ((mpp->action == ACT_CREATE ||
(mpp->action == ACT_NOTHING && start_waiter && !mpp->waiter)) &&
@@ -1957,8 +1951,6 @@ int reload_and_sync_map(struct multipath *mpp,
{
if (reload_map(vecs, mpp, refresh, 1))
return 1;
-
- dm_lib_release();
if (setup_multipath(vecs, mpp) != 0)
return 2;
sync_map_state(mpp);
@@ -2641,8 +2633,6 @@ configure (struct vectors * vecs)
goto fail;
}
- dm_lib_release();
-
if (should_exit())
goto fail;
@@ -3125,7 +3115,6 @@ child (__attribute__((unused)) void *param)
if (poll_dmevents)
cleanup_dmevent_waiter();
- dm_lib_release();
dm_lib_exit();
/* We're done here */
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 10/19] libmultipath: devmapper: refactor libdm version determination
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (8 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 09/19] multipath-tools: don't call dm_lib_release() any more mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-19 22:14 ` Benjamin Marzinski
2020-09-16 15:37 ` [PATCH 11/19] libmultipath: protect racy libdevmapper calls with a mutex mwilck
` (9 subsequent siblings)
19 siblings, 1 reply; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski
Cc: lixiaokeng, dm-devel, Martin Wilck, Zdenek Kabelac
From: Martin Wilck <mwilck@suse.com>
As one step towards bundling all possibly racy libdm init calls into a single
function, split the code for determining and checking versions of
libdm and kernel components. Provide a generic helper
libmp_get_version() that makes sure the versions are "lazily" initialized.
Note that retrieving the versions requires libdm calls, thus the
version initialization calls libdm initialization, which might call
version initialization recursively. But that's not an issue, as
the initialization is protected by pthread_once().
External callers may use dm_prereq(), like before.
Minor API change: dm_prereq() does not nullify the argument any more
if an error is encountered.
Remove the conf->version field, which isn't needed any more after this
change.
Cc: lixiaokeng@huawei.com
Cc: Zdenek Kabelac <zkabelac@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/config.h | 1 -
libmultipath/devmapper.c | 155 ++++++++++++++++++++++++++-------------
libmultipath/devmapper.h | 11 ++-
libmultipath/propsel.c | 10 ++-
multipathd/dmevents.c | 2 +-
multipathd/main.c | 1 -
6 files changed, 120 insertions(+), 60 deletions(-)
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 2bb7153..8e13ae9 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -191,7 +191,6 @@ struct config {
int find_multipaths_timeout;
int marginal_pathgroups;
int skip_delegate;
- unsigned int version[3];
unsigned int sequence_nr;
char * multipath_dir;
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index cc2de1d..3694cc8 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -26,6 +26,7 @@
#include "sysfs.h"
#include "config.h"
#include "wwids.h"
+#include "version.h"
#include "log_pthread.h"
#include <sys/types.h>
@@ -34,7 +35,13 @@
#define MAX_WAIT 5
#define LOOPS_PER_SEC 5
+#define INVALID_VERSION ~0U
+static unsigned int dm_library_version[3] = { INVALID_VERSION, };
+static unsigned int dm_kernel_version[3] = { INVALID_VERSION, };
+static unsigned int dm_mpath_target_version[3] = { INVALID_VERSION, };
+
static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT;
+static pthread_once_t versions_initialized = PTHREAD_ONCE_INIT;
static int dm_conf_verbosity;
@@ -102,7 +109,7 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
return;
}
-void dm_init(int v)
+static void dm_init(int v)
{
/*
* This maps libdm's standard loglevel _LOG_WARN (= 4), which is rather
@@ -112,59 +119,66 @@ void dm_init(int v)
dm_log_init(&dm_write_log);
}
+static void init_dm_library_version(void)
+{
+ char version[64];
+ unsigned int v[3];
+
+ dm_get_library_version(version, sizeof(version));
+ if (sscanf(version, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
+ condlog(0, "invalid libdevmapper version %s", version);
+ return;
+ }
+ memcpy(dm_library_version, v, sizeof(dm_library_version));
+ condlog(3, "libdevmapper version %u.%.2u.%.2u",
+ dm_library_version[0], dm_library_version[1],
+ dm_library_version[2]);
+}
+
static int
dm_lib_prereq (void)
{
- char version[64];
- int v[3];
+
#if defined(LIBDM_API_HOLD_CONTROL)
- int minv[3] = {1, 2, 111};
+ unsigned int minv[3] = {1, 2, 111};
#elif defined(LIBDM_API_DEFERRED)
- int minv[3] = {1, 2, 89};
+ unsigned int minv[3] = {1, 2, 89};
#elif defined(DM_SUBSYSTEM_UDEV_FLAG0)
- int minv[3] = {1, 2, 82};
+ unsigned int minv[3] = {1, 2, 82};
#elif defined(LIBDM_API_COOKIE)
- int minv[3] = {1, 2, 38};
+ unsigned int minv[3] = {1, 2, 38};
#else
- int minv[3] = {1, 2, 8};
+ unsigned int minv[3] = {1, 2, 8};
#endif
- dm_get_library_version(version, sizeof(version));
- condlog(3, "libdevmapper version %s", version);
- if (sscanf(version, "%d.%d.%d ", &v[0], &v[1], &v[2]) != 3) {
- condlog(0, "invalid libdevmapper version %s", version);
- return 1;
- }
-
- if VERSION_GE(v, minv)
+ if (VERSION_GE(dm_library_version, minv))
return 0;
- condlog(0, "libdevmapper version must be >= %d.%.2d.%.2d",
+ condlog(0, "libdevmapper version must be >= %u.%.2u.%.2u",
minv[0], minv[1], minv[2]);
return 1;
}
-int
-dm_drv_version(unsigned int *v)
+static void init_dm_drv_version(void)
{
char buff[64];
-
- v[0] = 0;
- v[1] = 0;
- v[2] = 0;
+ unsigned int v[3];
if (!dm_driver_version(buff, sizeof(buff))) {
condlog(0, "cannot get kernel dm version");
- return 1;
+ return;
}
if (sscanf(buff, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
condlog(0, "invalid kernel dm version '%s'", buff);
- return 1;
+ return;
}
- return 0;
+ memcpy(dm_kernel_version, v, sizeof(dm_library_version));
+ condlog(3, "kernel device mapper v%u.%u.%u",
+ dm_kernel_version[0],
+ dm_kernel_version[1],
+ dm_kernel_version[2]);
}
-int
-dm_tgt_version (unsigned int * version, char * str)
+static int dm_tgt_version (unsigned int *version, char *str)
{
int r = 2;
struct dm_task *dmt;
@@ -172,10 +186,6 @@ dm_tgt_version (unsigned int * version, char * str)
struct dm_versions *last_target;
unsigned int *v;
- version[0] = 0;
- version[1] = 0;
- version[2] = 0;
-
if (!(dmt = dm_task_create(DM_DEVICE_LIST_VERSIONS)))
return 1;
@@ -211,26 +221,25 @@ out:
return r;
}
-static int
-dm_tgt_prereq (unsigned int *ver)
+static void init_dm_mpath_version(void)
{
- unsigned int minv[3] = {1, 0, 3};
- unsigned int version[3] = {0, 0, 0};
- unsigned int * v = version;
-
- if (dm_tgt_version(v, TGT_MPATH)) {
- /* in doubt return not capable */
- return 1;
- }
-
- /* test request based multipath capability */
+ dm_tgt_version(dm_mpath_target_version, TGT_MPATH);
condlog(3, "DM multipath kernel driver v%u.%u.%u",
- v[0], v[1], v[2]);
+ dm_mpath_target_version[0],
+ dm_mpath_target_version[1],
+ dm_mpath_target_version[2]);
+}
+
+static int dm_tgt_prereq (unsigned int *ver)
+{
+ unsigned int minv[3] = {1, 0, 3};
- if (VERSION_GE(v, minv)) {
- ver[0] = v[0];
- ver[1] = v[1];
- ver[2] = v[2];
+ if (VERSION_GE(dm_mpath_target_version, minv)) {
+ if (ver) {
+ ver[0] = dm_mpath_target_version[0];
+ ver[1] = dm_mpath_target_version[1];
+ ver[2] = dm_mpath_target_version[2];
+ }
return 0;
}
@@ -239,13 +248,60 @@ dm_tgt_prereq (unsigned int *ver)
return 1;
}
+static void _init_versions(void)
+{
+ dlog(logsink, 3, VERSION_STRING);
+ init_dm_library_version();
+ init_dm_drv_version();
+ init_dm_mpath_version();
+}
+
+static int init_versions(void) {
+ pthread_once(&versions_initialized, _init_versions);
+ return (dm_library_version[0] == INVALID_VERSION ||
+ dm_kernel_version[0] == INVALID_VERSION ||
+ dm_mpath_target_version[0] == INVALID_VERSION);
+}
+
int dm_prereq(unsigned int *v)
{
+ if (init_versions())
+ return 1;
if (dm_lib_prereq())
return 1;
return dm_tgt_prereq(v);
}
+int libmp_get_version(int which, unsigned int version[3])
+{
+ unsigned int *src_version;
+
+ init_versions();
+ switch (which) {
+ case DM_LIBRARY_VERSION:
+ src_version = dm_library_version;
+ break;
+ case DM_KERNEL_VERSION:
+ src_version = dm_kernel_version;
+ break;
+ case DM_MPATH_TARGET_VERSION:
+ src_version = dm_mpath_target_version;
+ break;
+ case MULTIPATH_VERSION:
+ version[0] = (VERSION_CODE >> 16) & 0xff;
+ version[1] = (VERSION_CODE >> 8) & 0xff;
+ version[2] = VERSION_CODE & 0xff;
+ return 0;
+ default:
+ condlog(0, "%s: invalid value for 'which'", __func__);
+ return 1;
+ }
+ if (src_version[0] == INVALID_VERSION)
+ return 1;
+ memcpy(version, src_version, 3 * sizeof(*version));
+ return 0;
+}
+
static int libmp_dm_udev_sync = 0;
void libmp_udev_set_sync_support(int on)
@@ -263,7 +319,6 @@ static void libmp_dm_init(void)
exit(1);
conf = get_multipath_config();
verbosity = conf->verbosity;
- memcpy(conf->version, version, sizeof(version));
put_multipath_config(conf);
dm_init(verbosity);
#ifdef LIBDM_API_HOLD_CONTROL
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index f568ab5..c8b37e1 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -33,13 +33,10 @@ enum {
DMP_NOT_FOUND,
};
-void dm_init(int verbosity);
int dm_prereq(unsigned int *v);
void skip_libmp_dm_init(void);
void libmp_udev_set_sync_support(int on);
struct dm_task *libmp_dm_task_create(int task);
-int dm_drv_version (unsigned int * version);
-int dm_tgt_version (unsigned int * version, char * str);
int dm_simplecmd_flush (int, const char *, uint16_t);
int dm_simplecmd_noflush (int, const char *, uint16_t);
int dm_addmap_create (struct multipath *mpp, char *params);
@@ -85,6 +82,14 @@ struct multipath *dm_get_multipath(const char *name);
((v[0] == minv[0]) && (v[1] == minv[1]) && (v[2] >= minv[2])) \
)
+enum {
+ DM_LIBRARY_VERSION,
+ DM_KERNEL_VERSION,
+ DM_MPATH_TARGET_VERSION,
+ MULTIPATH_VERSION
+};
+int libmp_get_version(int which, unsigned int version[3]);
+
#define dm_log_error(lvl, cmd, dmt) \
condlog(lvl, "%s: libdm task=%d error: %s", __func__, \
cmd, strerror(dm_task_get_errno(dmt))) \
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 7e6e0d6..781515e 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -737,9 +737,10 @@ out:
int select_minio(struct config *conf, struct multipath *mp)
{
- unsigned int minv_dmrq[3] = {1, 1, 0};
+ unsigned int minv_dmrq[3] = {1, 1, 0}, version[3];
- if (VERSION_GE(conf->version, minv_dmrq))
+ if (!libmp_get_version(DM_MPATH_TARGET_VERSION, version)
+ && VERSION_GE(version, minv_dmrq))
return select_minio_rq(conf, mp);
else
return select_minio_bio(conf, mp);
@@ -822,9 +823,10 @@ out:
int select_retain_hwhandler(struct config *conf, struct multipath *mp)
{
const char *origin;
- unsigned int minv_dm_retain[3] = {1, 5, 0};
+ unsigned int minv_dm_retain[3] = {1, 5, 0}, version[3];
- if (!VERSION_GE(conf->version, minv_dm_retain)) {
+ if (!libmp_get_version(DM_MPATH_TARGET_VERSION, version) &&
+ !VERSION_GE(version, minv_dm_retain)) {
mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
origin = "(setting: WARNING, requires kernel dm-mpath version >= 1.5.0)";
goto out;
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index 5f2d210..fc97c8a 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -60,7 +60,7 @@ int dmevent_poll_supported(void)
{
unsigned int v[3];
- if (dm_drv_version(v))
+ if (libmp_get_version(DM_KERNEL_VERSION, v))
return 0;
if (VERSION_GE(v, DM_VERSION_FOR_ARM_POLL))
diff --git a/multipathd/main.c b/multipathd/main.c
index affe706..53bacac 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2719,7 +2719,6 @@ reconfigure (struct vectors * vecs)
/* Re-read any timezone changes */
tzset();
- dm_tgt_version(conf->version, TGT_MPATH);
if (verbosity)
conf->verbosity = verbosity;
if (bindings_read_only)
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 10/19] libmultipath: devmapper: refactor libdm version determination
2020-09-16 15:37 ` [PATCH 10/19] libmultipath: devmapper: refactor libdm version determination mwilck
@ 2020-09-19 22:14 ` Benjamin Marzinski
2020-09-21 8:35 ` Martin Wilck
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-19 22:14 UTC (permalink / raw)
To: mwilck; +Cc: lixiaokeng, dm-devel, Zdenek Kabelac
On Wed, Sep 16, 2020 at 05:37:09PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> As one step towards bundling all possibly racy libdm init calls into a single
> function, split the code for determining and checking versions of
> libdm and kernel components. Provide a generic helper
> libmp_get_version() that makes sure the versions are "lazily" initialized.
> Note that retrieving the versions requires libdm calls, thus the
> version initialization calls libdm initialization, which might call
> version initialization recursively. But that's not an issue, as
> the initialization is protected by pthread_once().
This is confusing me. dm_tgt_version will call DM_DEVICE_LIST_VERSIONS,
but it doesn't call libmp_dm_task_create(), so I don't see the recursion
possiblity. That's good because according to the man page:
"If you specify a routine that directly or indirectly results in a
recursive call to pthread_once(3) and that specifies the same routine
argument, the recursive call can result in a deadlock."
-Ben
> External callers may use dm_prereq(), like before.
> Minor API change: dm_prereq() does not nullify the argument any more
> if an error is encountered.
>
> Remove the conf->version field, which isn't needed any more after this
> change.
>
> Cc: lixiaokeng@huawei.com
> Cc: Zdenek Kabelac <zkabelac@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/config.h | 1 -
> libmultipath/devmapper.c | 155 ++++++++++++++++++++++++++-------------
> libmultipath/devmapper.h | 11 ++-
> libmultipath/propsel.c | 10 ++-
> multipathd/dmevents.c | 2 +-
> multipathd/main.c | 1 -
> 6 files changed, 120 insertions(+), 60 deletions(-)
>
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 2bb7153..8e13ae9 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -191,7 +191,6 @@ struct config {
> int find_multipaths_timeout;
> int marginal_pathgroups;
> int skip_delegate;
> - unsigned int version[3];
> unsigned int sequence_nr;
>
> char * multipath_dir;
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index cc2de1d..3694cc8 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -26,6 +26,7 @@
> #include "sysfs.h"
> #include "config.h"
> #include "wwids.h"
> +#include "version.h"
>
> #include "log_pthread.h"
> #include <sys/types.h>
> @@ -34,7 +35,13 @@
> #define MAX_WAIT 5
> #define LOOPS_PER_SEC 5
>
> +#define INVALID_VERSION ~0U
> +static unsigned int dm_library_version[3] = { INVALID_VERSION, };
> +static unsigned int dm_kernel_version[3] = { INVALID_VERSION, };
> +static unsigned int dm_mpath_target_version[3] = { INVALID_VERSION, };
> +
> static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT;
> +static pthread_once_t versions_initialized = PTHREAD_ONCE_INIT;
>
> static int dm_conf_verbosity;
>
> @@ -102,7 +109,7 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
> return;
> }
>
> -void dm_init(int v)
> +static void dm_init(int v)
> {
> /*
> * This maps libdm's standard loglevel _LOG_WARN (= 4), which is rather
> @@ -112,59 +119,66 @@ void dm_init(int v)
> dm_log_init(&dm_write_log);
> }
>
> +static void init_dm_library_version(void)
> +{
> + char version[64];
> + unsigned int v[3];
> +
> + dm_get_library_version(version, sizeof(version));
> + if (sscanf(version, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
> + condlog(0, "invalid libdevmapper version %s", version);
> + return;
> + }
> + memcpy(dm_library_version, v, sizeof(dm_library_version));
> + condlog(3, "libdevmapper version %u.%.2u.%.2u",
> + dm_library_version[0], dm_library_version[1],
> + dm_library_version[2]);
> +}
> +
> static int
> dm_lib_prereq (void)
> {
> - char version[64];
> - int v[3];
> +
> #if defined(LIBDM_API_HOLD_CONTROL)
> - int minv[3] = {1, 2, 111};
> + unsigned int minv[3] = {1, 2, 111};
> #elif defined(LIBDM_API_DEFERRED)
> - int minv[3] = {1, 2, 89};
> + unsigned int minv[3] = {1, 2, 89};
> #elif defined(DM_SUBSYSTEM_UDEV_FLAG0)
> - int minv[3] = {1, 2, 82};
> + unsigned int minv[3] = {1, 2, 82};
> #elif defined(LIBDM_API_COOKIE)
> - int minv[3] = {1, 2, 38};
> + unsigned int minv[3] = {1, 2, 38};
> #else
> - int minv[3] = {1, 2, 8};
> + unsigned int minv[3] = {1, 2, 8};
> #endif
>
> - dm_get_library_version(version, sizeof(version));
> - condlog(3, "libdevmapper version %s", version);
> - if (sscanf(version, "%d.%d.%d ", &v[0], &v[1], &v[2]) != 3) {
> - condlog(0, "invalid libdevmapper version %s", version);
> - return 1;
> - }
> -
> - if VERSION_GE(v, minv)
> + if (VERSION_GE(dm_library_version, minv))
> return 0;
> - condlog(0, "libdevmapper version must be >= %d.%.2d.%.2d",
> + condlog(0, "libdevmapper version must be >= %u.%.2u.%.2u",
> minv[0], minv[1], minv[2]);
> return 1;
> }
>
> -int
> -dm_drv_version(unsigned int *v)
> +static void init_dm_drv_version(void)
> {
> char buff[64];
> -
> - v[0] = 0;
> - v[1] = 0;
> - v[2] = 0;
> + unsigned int v[3];
>
> if (!dm_driver_version(buff, sizeof(buff))) {
> condlog(0, "cannot get kernel dm version");
> - return 1;
> + return;
> }
> if (sscanf(buff, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
> condlog(0, "invalid kernel dm version '%s'", buff);
> - return 1;
> + return;
> }
> - return 0;
> + memcpy(dm_kernel_version, v, sizeof(dm_library_version));
> + condlog(3, "kernel device mapper v%u.%u.%u",
> + dm_kernel_version[0],
> + dm_kernel_version[1],
> + dm_kernel_version[2]);
> }
>
> -int
> -dm_tgt_version (unsigned int * version, char * str)
> +static int dm_tgt_version (unsigned int *version, char *str)
> {
> int r = 2;
> struct dm_task *dmt;
> @@ -172,10 +186,6 @@ dm_tgt_version (unsigned int * version, char * str)
> struct dm_versions *last_target;
> unsigned int *v;
>
> - version[0] = 0;
> - version[1] = 0;
> - version[2] = 0;
> -
> if (!(dmt = dm_task_create(DM_DEVICE_LIST_VERSIONS)))
> return 1;
>
> @@ -211,26 +221,25 @@ out:
> return r;
> }
>
> -static int
> -dm_tgt_prereq (unsigned int *ver)
> +static void init_dm_mpath_version(void)
> {
> - unsigned int minv[3] = {1, 0, 3};
> - unsigned int version[3] = {0, 0, 0};
> - unsigned int * v = version;
> -
> - if (dm_tgt_version(v, TGT_MPATH)) {
> - /* in doubt return not capable */
> - return 1;
> - }
> -
> - /* test request based multipath capability */
> + dm_tgt_version(dm_mpath_target_version, TGT_MPATH);
> condlog(3, "DM multipath kernel driver v%u.%u.%u",
> - v[0], v[1], v[2]);
> + dm_mpath_target_version[0],
> + dm_mpath_target_version[1],
> + dm_mpath_target_version[2]);
> +}
> +
> +static int dm_tgt_prereq (unsigned int *ver)
> +{
> + unsigned int minv[3] = {1, 0, 3};
>
> - if (VERSION_GE(v, minv)) {
> - ver[0] = v[0];
> - ver[1] = v[1];
> - ver[2] = v[2];
> + if (VERSION_GE(dm_mpath_target_version, minv)) {
> + if (ver) {
> + ver[0] = dm_mpath_target_version[0];
> + ver[1] = dm_mpath_target_version[1];
> + ver[2] = dm_mpath_target_version[2];
> + }
> return 0;
> }
>
> @@ -239,13 +248,60 @@ dm_tgt_prereq (unsigned int *ver)
> return 1;
> }
>
> +static void _init_versions(void)
> +{
> + dlog(logsink, 3, VERSION_STRING);
> + init_dm_library_version();
> + init_dm_drv_version();
> + init_dm_mpath_version();
> +}
> +
> +static int init_versions(void) {
> + pthread_once(&versions_initialized, _init_versions);
> + return (dm_library_version[0] == INVALID_VERSION ||
> + dm_kernel_version[0] == INVALID_VERSION ||
> + dm_mpath_target_version[0] == INVALID_VERSION);
> +}
> +
> int dm_prereq(unsigned int *v)
> {
> + if (init_versions())
> + return 1;
> if (dm_lib_prereq())
> return 1;
> return dm_tgt_prereq(v);
> }
>
> +int libmp_get_version(int which, unsigned int version[3])
> +{
> + unsigned int *src_version;
> +
> + init_versions();
> + switch (which) {
> + case DM_LIBRARY_VERSION:
> + src_version = dm_library_version;
> + break;
> + case DM_KERNEL_VERSION:
> + src_version = dm_kernel_version;
> + break;
> + case DM_MPATH_TARGET_VERSION:
> + src_version = dm_mpath_target_version;
> + break;
> + case MULTIPATH_VERSION:
> + version[0] = (VERSION_CODE >> 16) & 0xff;
> + version[1] = (VERSION_CODE >> 8) & 0xff;
> + version[2] = VERSION_CODE & 0xff;
> + return 0;
> + default:
> + condlog(0, "%s: invalid value for 'which'", __func__);
> + return 1;
> + }
> + if (src_version[0] == INVALID_VERSION)
> + return 1;
> + memcpy(version, src_version, 3 * sizeof(*version));
> + return 0;
> +}
> +
> static int libmp_dm_udev_sync = 0;
>
> void libmp_udev_set_sync_support(int on)
> @@ -263,7 +319,6 @@ static void libmp_dm_init(void)
> exit(1);
> conf = get_multipath_config();
> verbosity = conf->verbosity;
> - memcpy(conf->version, version, sizeof(version));
> put_multipath_config(conf);
> dm_init(verbosity);
> #ifdef LIBDM_API_HOLD_CONTROL
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index f568ab5..c8b37e1 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -33,13 +33,10 @@ enum {
> DMP_NOT_FOUND,
> };
>
> -void dm_init(int verbosity);
> int dm_prereq(unsigned int *v);
> void skip_libmp_dm_init(void);
> void libmp_udev_set_sync_support(int on);
> struct dm_task *libmp_dm_task_create(int task);
> -int dm_drv_version (unsigned int * version);
> -int dm_tgt_version (unsigned int * version, char * str);
> int dm_simplecmd_flush (int, const char *, uint16_t);
> int dm_simplecmd_noflush (int, const char *, uint16_t);
> int dm_addmap_create (struct multipath *mpp, char *params);
> @@ -85,6 +82,14 @@ struct multipath *dm_get_multipath(const char *name);
> ((v[0] == minv[0]) && (v[1] == minv[1]) && (v[2] >= minv[2])) \
> )
>
> +enum {
> + DM_LIBRARY_VERSION,
> + DM_KERNEL_VERSION,
> + DM_MPATH_TARGET_VERSION,
> + MULTIPATH_VERSION
> +};
> +int libmp_get_version(int which, unsigned int version[3]);
> +
> #define dm_log_error(lvl, cmd, dmt) \
> condlog(lvl, "%s: libdm task=%d error: %s", __func__, \
> cmd, strerror(dm_task_get_errno(dmt))) \
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 7e6e0d6..781515e 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -737,9 +737,10 @@ out:
>
> int select_minio(struct config *conf, struct multipath *mp)
> {
> - unsigned int minv_dmrq[3] = {1, 1, 0};
> + unsigned int minv_dmrq[3] = {1, 1, 0}, version[3];
>
> - if (VERSION_GE(conf->version, minv_dmrq))
> + if (!libmp_get_version(DM_MPATH_TARGET_VERSION, version)
> + && VERSION_GE(version, minv_dmrq))
> return select_minio_rq(conf, mp);
> else
> return select_minio_bio(conf, mp);
> @@ -822,9 +823,10 @@ out:
> int select_retain_hwhandler(struct config *conf, struct multipath *mp)
> {
> const char *origin;
> - unsigned int minv_dm_retain[3] = {1, 5, 0};
> + unsigned int minv_dm_retain[3] = {1, 5, 0}, version[3];
>
> - if (!VERSION_GE(conf->version, minv_dm_retain)) {
> + if (!libmp_get_version(DM_MPATH_TARGET_VERSION, version) &&
> + !VERSION_GE(version, minv_dm_retain)) {
> mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
> origin = "(setting: WARNING, requires kernel dm-mpath version >= 1.5.0)";
> goto out;
> diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
> index 5f2d210..fc97c8a 100644
> --- a/multipathd/dmevents.c
> +++ b/multipathd/dmevents.c
> @@ -60,7 +60,7 @@ int dmevent_poll_supported(void)
> {
> unsigned int v[3];
>
> - if (dm_drv_version(v))
> + if (libmp_get_version(DM_KERNEL_VERSION, v))
> return 0;
>
> if (VERSION_GE(v, DM_VERSION_FOR_ARM_POLL))
> diff --git a/multipathd/main.c b/multipathd/main.c
> index affe706..53bacac 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2719,7 +2719,6 @@ reconfigure (struct vectors * vecs)
> /* Re-read any timezone changes */
> tzset();
>
> - dm_tgt_version(conf->version, TGT_MPATH);
> if (verbosity)
> conf->verbosity = verbosity;
> if (bindings_read_only)
> --
> 2.28.0
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 10/19] libmultipath: devmapper: refactor libdm version determination
2020-09-19 22:14 ` Benjamin Marzinski
@ 2020-09-21 8:35 ` Martin Wilck
0 siblings, 0 replies; 31+ messages in thread
From: Martin Wilck @ 2020-09-21 8:35 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Zdenek Kabelac
On Sat, 2020-09-19 at 17:14 -0500, Benjamin Marzinski wrote:
> On Wed, Sep 16, 2020 at 05:37:09PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> >
> > As one step towards bundling all possibly racy libdm init calls
> > into a single
> > function, split the code for determining and checking versions of
> > libdm and kernel components. Provide a generic helper
> > libmp_get_version() that makes sure the versions are "lazily"
> > initialized.
> > Note that retrieving the versions requires libdm calls, thus the
> > version initialization calls libdm initialization, which might call
> > version initialization recursively. But that's not an issue, as
> > the initialization is protected by pthread_once().
>
> This is confusing me. dm_tgt_version will call
> DM_DEVICE_LIST_VERSIONS,
> but it doesn't call libmp_dm_task_create(), so I don't see the
> recursion
> possiblity.
You are right, I was confused.
The callstack for "lazy" initialization looks like this:
libmp_dm_task_create
pthread_once(&dm_initialized, libmp_dm_init);
libmp_dm_init()
=> dm_prereq()
init_versions
pthread_once(&versions_initialized, _init_versions);
_init_versions()
init_dm_library_version()
init_dm_drv_version()
init_dm_mpath_version()
dm_tgt_version
** dm_task_create
libmp_task_run()
pthread_mutex_lock(&libmp_dm_lock);
dm_task_run()
(If an application called dm_prereq() explicitly, it would enter
the stack at "=>", which would be less of a problem).
I suppose I should add a comment at "**" saying that we must not call
libmp_dm_task_create() there to avoid recursion. I also notice that my
function naming I used is inconsistent; it should be
libmp_dm_task_run() rather than just libmp_task_run().
> That's good because according to the man page:
>
> "If you specify a routine that directly or indirectly results in a
> recursive call to pthread_once(3) and that specifies the same routine
> argument, the recursive call can result in a deadlock."
Thanks for pointing this out. I had missed that indeed.
I see this paragraph only in some old DECThreads man pages on the web.
There's a slighly different "APPLICATION USAGE" statement under
https://pubs.opengroup.org/onlinepubs/9699919799/. The pragraph is
missing in https://man7.org/linux/man-pages/man3/pthread_once.3p.html,
which is still based on POSIX.1-2008, 2013 edition - that's why I
missed it.
Anyway, it makes sense, because pthread_once() guarantees that the init
routine has _finished_ when it returns, which can't work with recursive
calls.
Anyway, I'll send a v2.
Regards
Martin
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 11/19] libmultipath: protect racy libdevmapper calls with a mutex
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (9 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 10/19] libmultipath: devmapper: refactor libdm version determination mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 12/19] libmultipath: constify file argument in config parser mwilck
` (8 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski
Cc: lixiaokeng, dm-devel, Martin Wilck, Zdenek Kabelac
From: Martin Wilck <mwilck@suse.com>
dm_udev_wait() and dm_task_run() may access global / static state
in libdevmapper. They need to be protected by a lock in in our
multithreaded library.
Cc: lixiaokeng@huawei.com
Cc: Zdenek Kabelac <zkabelac@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/devmapper.c | 73 +++++++++++++++++++++++++---------------
libmultipath/devmapper.h | 2 ++
libmultipath/util.c | 5 +++
libmultipath/util.h | 1 +
multipathd/dmevents.c | 2 +-
multipathd/waiter.c | 2 +-
6 files changed, 56 insertions(+), 29 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 3694cc8..34f6ade 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -42,6 +42,7 @@ static unsigned int dm_mpath_target_version[3] = { INVALID_VERSION, };
static pthread_once_t dm_initialized = PTHREAD_ONCE_INIT;
static pthread_once_t versions_initialized = PTHREAD_ONCE_INIT;
+static pthread_mutex_t libmp_dm_lock = PTHREAD_MUTEX_INITIALIZER;
static int dm_conf_verbosity;
@@ -59,16 +60,34 @@ static inline int dm_task_set_cookie(struct dm_task *dmt, uint32_t *c, int a)
return 1;
}
-void dm_udev_wait(unsigned int c)
+static void libmp_udev_wait(unsigned int c)
{
}
-void dm_udev_set_sync_support(int c)
+static void dm_udev_set_sync_support(int c)
{
}
-
+#else
+static void libmp_udev_wait(unsigned int c)
+{
+ pthread_mutex_lock(&libmp_dm_lock);
+ pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock);
+ dm_udev_wait(c);
+ pthread_cleanup_pop(1);
+}
#endif
+int libmp_task_run(struct dm_task *dmt)
+{
+ int r;
+
+ pthread_mutex_lock(&libmp_dm_lock);
+ pthread_cleanup_push(cleanup_mutex, &libmp_dm_lock);
+ r = dm_task_run(dmt);
+ pthread_cleanup_pop(1);
+ return r;
+}
+
__attribute__((format(printf, 4, 5))) static void
dm_write_log (int level, const char *file, int line, const char *f, ...)
{
@@ -191,7 +210,7 @@ static int dm_tgt_version (unsigned int *version, char *str)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(2, DM_DEVICE_LIST_VERSIONS, dmt);
condlog(0, "Can not communicate with kernel DM");
goto out;
@@ -375,12 +394,12 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags))
goto out;
- r = dm_task_run (dmt);
+ r = libmp_task_run (dmt);
if (!r)
dm_log_error(2, task, dmt);
if (udev_wait_flag)
- dm_udev_wait(cookie);
+ libmp_udev_wait(cookie);
out:
dm_task_destroy (dmt);
return r;
@@ -467,12 +486,12 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
!dm_task_set_cookie(dmt, &cookie, udev_flags))
goto freeout;
- r = dm_task_run (dmt);
+ r = libmp_task_run (dmt);
if (!r)
dm_log_error(2, task, dmt);
if (task == DM_DEVICE_CREATE)
- dm_udev_wait(cookie);
+ libmp_udev_wait(cookie);
freeout:
if (prefixed_uuid)
FREE(prefixed_uuid);
@@ -582,7 +601,7 @@ do_get_info(const char *name, struct dm_info *info)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_INFO, dmt);
goto out;
}
@@ -623,7 +642,7 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams)
dm_task_no_open_count(dmt);
errno = 0;
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
if (dm_task_get_errno(dmt) == ENXIO)
r = DMP_NOT_FOUND;
@@ -665,7 +684,7 @@ dm_get_prefixed_uuid(const char *name, char *uuid, int uuid_len)
if (!dm_task_set_name (dmt, name))
goto uuidout;
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_INFO, dmt);
goto uuidout;
}
@@ -736,7 +755,7 @@ int dm_get_status(const char *name, char *outstatus)
dm_task_no_open_count(dmt);
errno = 0;
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_STATUS, dmt);
if (dm_task_get_errno(dmt) == ENXIO)
r = DMP_NOT_FOUND;
@@ -789,7 +808,7 @@ int dm_type(const char *name, char *type)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
goto out;
}
@@ -833,7 +852,7 @@ int dm_is_mpath(const char *name)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
goto out_task;
}
@@ -897,7 +916,7 @@ dm_map_present_by_uuid(const char *uuid)
if (!dm_task_set_uuid(dmt, prefixed_uuid))
goto out_task;
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_INFO, dmt);
goto out_task;
}
@@ -943,7 +962,7 @@ dm_get_opencount (const char * mapname)
if (!dm_task_set_name(dmt, mapname))
goto out;
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_INFO, dmt);
goto out;
}
@@ -1103,7 +1122,7 @@ int dm_flush_maps (int need_suspend, int retries)
dm_task_no_open_count(dmt);
- if (!dm_task_run (dmt)) {
+ if (!libmp_task_run (dmt)) {
dm_log_error(3, DM_DEVICE_LIST, dmt);
goto out;
}
@@ -1149,7 +1168,7 @@ dm_message(const char * mapname, char * message)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(2, DM_DEVICE_TARGET_MSG, dmt);
goto out;
}
@@ -1269,7 +1288,7 @@ dm_get_maps (vector mp)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_LIST, dmt);
goto out;
}
@@ -1354,7 +1373,7 @@ dm_mapname(int major, int minor)
* daemon uev_trigger -> uev_add_map
*/
while (--loop) {
- r = dm_task_run(dmt);
+ r = libmp_task_run(dmt);
if (r)
break;
@@ -1399,7 +1418,7 @@ do_foreach_partmaps (const char * mapname,
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_LIST, dmt);
goto out;
}
@@ -1634,11 +1653,11 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
if (!dm_task_set_cookie(dmt, &cookie, udev_flags))
goto out;
- r = dm_task_run(dmt);
+ r = libmp_task_run(dmt);
if (!r)
dm_log_error(2, DM_DEVICE_RENAME, dmt);
- dm_udev_wait(cookie);
+ libmp_udev_wait(cookie);
out:
dm_task_destroy(dmt);
@@ -1680,7 +1699,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
goto out;
}
@@ -1713,7 +1732,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
if (modified) {
dm_task_no_open_count(reload_dmt);
- if (!dm_task_run(reload_dmt)) {
+ if (!libmp_task_run(reload_dmt)) {
dm_log_error(3, DM_DEVICE_RELOAD, reload_dmt);
condlog(3, "%s: failed to reassign targets", name);
goto out_reload;
@@ -1760,7 +1779,7 @@ int dm_reassign(const char *mapname)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_DEPS, dmt);
goto out;
}
@@ -1828,7 +1847,7 @@ int dm_setgeometry(struct multipath *mpp)
goto out;
}
- r = dm_task_run(dmt);
+ r = libmp_task_run(dmt);
if (!r)
dm_log_error(3, DM_DEVICE_SET_GEOMETRY, dmt);
out:
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index c8b37e1..75fbc33 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -89,6 +89,8 @@ enum {
MULTIPATH_VERSION
};
int libmp_get_version(int which, unsigned int version[3]);
+struct dm_task;
+int libmp_task_run(struct dm_task *dmt);
#define dm_log_error(lvl, cmd, dmt) \
condlog(lvl, "%s: libdm task=%d error: %s", __func__, \
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 7a0b2eb..8412d30 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -401,6 +401,11 @@ void close_fd(void *arg)
close((long)arg);
}
+void cleanup_mutex(void *arg)
+{
+ pthread_mutex_unlock(arg);
+}
+
struct bitfield *alloc_bitfield(unsigned int maxbit)
{
unsigned int n;
diff --git a/libmultipath/util.h b/libmultipath/util.h
index a0d3d50..94ed872 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -48,6 +48,7 @@ int should_exit(void);
pthread_cleanup_push(((void (*)(void *))&f), (arg))
void close_fd(void *arg);
+void cleanup_mutex(void *arg);
struct scandir_result {
struct dirent **di;
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index fc97c8a..f8ed4e4 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -156,7 +156,7 @@ static int dm_get_events(void)
dm_task_no_open_count(dmt);
- if (!dm_task_run(dmt)) {
+ if (!libmp_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_LIST, dmt);
goto fail;
}
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index 16fbdc8..ef57765 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -118,7 +118,7 @@ static int waiteventloop (struct event_thread *waiter)
pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
pthread_testcancel();
- r = dm_task_run(waiter->dmt);
+ r = libmp_task_run(waiter->dmt);
if (!r)
dm_log_error(2, DM_DEVICE_WAITEVENT, waiter->dmt);
pthread_testcancel();
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 12/19] libmultipath: constify file argument in config parser
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (10 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 11/19] libmultipath: protect racy libdevmapper calls with a mutex mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 13/19] libmultipath: provide defaults for {get, put}_multipath_config mwilck
` (7 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/config.c | 3 +--
libmultipath/config.h | 2 +-
libmultipath/parser.c | 9 +++++----
libmultipath/parser.h | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index b9bdbdb..2011a29 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -719,8 +719,7 @@ static void set_max_checkint_from_watchdog(struct config *conf)
}
#endif
-struct config *
-load_config (char * file)
+struct config *load_config(const char *file)
{
struct config *conf = alloc_config();
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 8e13ae9..116fe37 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -250,7 +250,7 @@ void free_mptable (vector mptable);
int store_hwe (vector hwtable, struct hwentry *);
-struct config *load_config (char * file);
+struct config *load_config (const char *file);
struct config * alloc_config (void);
void free_config (struct config * conf);
extern struct config *get_multipath_config(void);
diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index ed6d5d6..163ffbc 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -390,7 +390,7 @@ oom:
/* non-recursive configuration stream handler */
static int kw_level = 0;
-int warn_on_duplicates(vector uniques, char *str, char *file)
+int warn_on_duplicates(vector uniques, char *str, const char *file)
{
char *tmp;
int i;
@@ -434,7 +434,7 @@ is_sublevel_keyword(char *str)
}
int
-validate_config_strvec(vector strvec, char *file)
+validate_config_strvec(vector strvec, const char *file)
{
char *str = NULL;
int i;
@@ -499,7 +499,8 @@ validate_config_strvec(vector strvec, char *file)
}
static int
-process_stream(struct config *conf, FILE *stream, vector keywords, char *file)
+process_stream(struct config *conf, FILE *stream, vector keywords,
+ const char *file)
{
int i;
int r = 0, t;
@@ -584,7 +585,7 @@ out:
/* Data initialization */
int
-process_file(struct config *conf, char *file)
+process_file(struct config *conf, const char *file)
{
int r;
FILE *stream;
diff --git a/libmultipath/parser.h b/libmultipath/parser.h
index 62906e9..06666cc 100644
--- a/libmultipath/parser.h
+++ b/libmultipath/parser.h
@@ -77,7 +77,7 @@ extern void dump_keywords(vector keydump, int level);
extern void free_keywords(vector keywords);
extern vector alloc_strvec(char *string);
extern void *set_value(vector strvec);
-extern int process_file(struct config *conf, char *conf_file);
+extern int process_file(struct config *conf, const char *conf_file);
extern struct keyword * find_keyword(vector keywords, vector v, char * name);
int snprint_keyword(char *buff, int len, char *fmt, struct keyword *kw,
const void *data);
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 13/19] libmultipath: provide defaults for {get, put}_multipath_config
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (11 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 12/19] libmultipath: constify file argument in config parser mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-21 19:08 ` Benjamin Marzinski
2020-09-16 15:37 ` [PATCH 14/19] libmpathpersist: allow using libmultipath " mwilck
` (6 subsequent siblings)
19 siblings, 1 reply; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Add an implementation of get_multipath_config() and put_multipath_config()
to libmultipath. The linker's symbol lookup rules will make sure that
applications can override these functions if they need to. Defining
these functions in libmultipath avoids the need to provide stubs
for these functions in every appliation linking to libmultipath.
libmultipath's internal functions simply refer to a static "struct config".
It must be initialized with init_config() rather than load_config(),
which always allocates a new struct for backward compatibility.
free_config() can be used in both cases.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/config.c | 70 ++++++++++++++++++++++++++++++++++++-------
libmultipath/config.h | 21 +++++++++++--
2 files changed, 78 insertions(+), 13 deletions(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 2011a29..b83e5cd 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -27,6 +27,23 @@
#include "mpath_cmd.h"
#include "propsel.h"
+static struct config __internal_config;
+struct config *libmp_get_multipath_config(void)
+{
+ return &__internal_config;
+}
+
+struct config *get_multipath_config(void)
+ __attribute__((weak, alias("libmp_get_multipath_config")));
+
+void libmp_put_multipath_config(void *conf __attribute__((unused)))
+{
+ /* empty */
+}
+
+void put_multipath_config(void *conf)
+ __attribute__((weak, alias("libmp_put_multipath_config")));
+
static int
hwe_strmatch (const struct hwentry *hwe1, const struct hwentry *hwe2)
{
@@ -574,17 +591,15 @@ restart:
return;
}
-struct config *
-alloc_config (void)
+static struct config *alloc_config (void)
{
return (struct config *)MALLOC(sizeof(struct config));
}
-void
-free_config (struct config * conf)
+static void _uninit_config(struct config *conf)
{
if (!conf)
- return;
+ conf = &__internal_config;
if (conf->multipath_dir)
FREE(conf->multipath_dir);
@@ -650,7 +665,25 @@ free_config (struct config * conf)
free_hwtable(conf->hwtable);
free_hwe(conf->overrides);
free_keywords(conf->keywords);
- FREE(conf);
+}
+
+void uninit_config(void)
+{
+ _uninit_config(&__internal_config);
+}
+
+void free_config(struct config *conf)
+{
+ if (!conf)
+ return;
+ else if (conf == &__internal_config) {
+ condlog(0, "ERROR: %s called for internal config. Use uninit_config() instead",
+ __func__);
+ return;
+ }
+
+ _uninit_config(conf);
+ free(conf);
}
/* if multipath fails to process the config directory, it should continue,
@@ -719,12 +752,29 @@ static void set_max_checkint_from_watchdog(struct config *conf)
}
#endif
+static int _init_config (const char *file, struct config *conf);
+
+int init_config(const char *file)
+{
+ return _init_config(file, &__internal_config);
+}
+
struct config *load_config(const char *file)
{
struct config *conf = alloc_config();
+ if (conf && !_init_config(file, conf))
+ return conf;
+
+ free(conf);
+ return NULL;
+}
+
+int _init_config (const char *file, struct config *conf)
+{
+
if (!conf)
- return NULL;
+ conf = &__internal_config;
/*
* internal defaults
@@ -896,10 +946,10 @@ struct config *load_config(const char *file)
!conf->wwids_file || !conf->prkeys_file)
goto out;
- return conf;
+ return 0;
out:
- free_config(conf);
- return NULL;
+ _uninit_config(conf);
+ return 1;
}
char *get_uid_attribute_by_attrs(struct config *conf,
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 116fe37..5997b71 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -251,10 +251,25 @@ void free_mptable (vector mptable);
int store_hwe (vector hwtable, struct hwentry *);
struct config *load_config (const char *file);
-struct config * alloc_config (void);
void free_config (struct config * conf);
-extern struct config *get_multipath_config(void);
-extern void put_multipath_config(void *);
+int init_config(const char *file);
+void uninit_config(void);
+
+/*
+ * libmultipath provides default implementations of
+ * get_multipath_config() and put_multipath_config().
+ * Applications using these should use init_config(file, NULL)
+ * to load the configuration, rather than load_config(file).
+ * Likewise, uninit_config() should be used for teardown, but
+ * using free_config() for that is supported, too.
+ * Applications can define their own {get,put}_multipath_config()
+ * functions, which override the library-internal ones, but
+ * could still call libmp_{get,put}_multipath_config().
+ */
+struct config *libmp_get_multipath_config(void);
+struct config *get_multipath_config(void);
+void libmp_put_multipath_config(void *);
+void put_multipath_config(void *);
int parse_uid_attrs(char *uid_attrs, struct config *conf);
char *get_uid_attribute_by_attrs(struct config *conf,
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 13/19] libmultipath: provide defaults for {get, put}_multipath_config
2020-09-16 15:37 ` [PATCH 13/19] libmultipath: provide defaults for {get, put}_multipath_config mwilck
@ 2020-09-21 19:08 ` Benjamin Marzinski
2020-09-21 19:42 ` Martin Wilck
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-21 19:08 UTC (permalink / raw)
To: mwilck; +Cc: dm-devel
On Wed, Sep 16, 2020 at 05:37:12PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> Add an implementation of get_multipath_config() and put_multipath_config()
> to libmultipath. The linker's symbol lookup rules will make sure that
> applications can override these functions if they need to. Defining
> these functions in libmultipath avoids the need to provide stubs
> for these functions in every appliation linking to libmultipath.
>
> libmultipath's internal functions simply refer to a static "struct config".
> It must be initialized with init_config() rather than load_config(),
> which always allocates a new struct for backward compatibility.
> free_config() can be used in both cases.
free_config() doesn't actually work for configs that are initialized by
init_config(). That's fine, but the commit message is wrong. Also, I
wonder if uninit_config() should zero out __internal_config, so that
it's in the same state as it was before init_config() is called.
-Ben
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/config.c | 70 ++++++++++++++++++++++++++++++++++++-------
> libmultipath/config.h | 21 +++++++++++--
> 2 files changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 2011a29..b83e5cd 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -27,6 +27,23 @@
> #include "mpath_cmd.h"
> #include "propsel.h"
>
> +static struct config __internal_config;
> +struct config *libmp_get_multipath_config(void)
> +{
> + return &__internal_config;
> +}
> +
> +struct config *get_multipath_config(void)
> + __attribute__((weak, alias("libmp_get_multipath_config")));
> +
> +void libmp_put_multipath_config(void *conf __attribute__((unused)))
> +{
> + /* empty */
> +}
> +
> +void put_multipath_config(void *conf)
> + __attribute__((weak, alias("libmp_put_multipath_config")));
> +
> static int
> hwe_strmatch (const struct hwentry *hwe1, const struct hwentry *hwe2)
> {
> @@ -574,17 +591,15 @@ restart:
> return;
> }
>
> -struct config *
> -alloc_config (void)
> +static struct config *alloc_config (void)
> {
> return (struct config *)MALLOC(sizeof(struct config));
> }
>
> -void
> -free_config (struct config * conf)
> +static void _uninit_config(struct config *conf)
> {
> if (!conf)
> - return;
> + conf = &__internal_config;
>
> if (conf->multipath_dir)
> FREE(conf->multipath_dir);
> @@ -650,7 +665,25 @@ free_config (struct config * conf)
> free_hwtable(conf->hwtable);
> free_hwe(conf->overrides);
> free_keywords(conf->keywords);
> - FREE(conf);
> +}
> +
> +void uninit_config(void)
> +{
> + _uninit_config(&__internal_config);
> +}
> +
> +void free_config(struct config *conf)
> +{
> + if (!conf)
> + return;
> + else if (conf == &__internal_config) {
> + condlog(0, "ERROR: %s called for internal config. Use uninit_config() instead",
> + __func__);
> + return;
> + }
> +
> + _uninit_config(conf);
> + free(conf);
> }
>
> /* if multipath fails to process the config directory, it should continue,
> @@ -719,12 +752,29 @@ static void set_max_checkint_from_watchdog(struct config *conf)
> }
> #endif
>
> +static int _init_config (const char *file, struct config *conf);
> +
> +int init_config(const char *file)
> +{
> + return _init_config(file, &__internal_config);
> +}
> +
> struct config *load_config(const char *file)
> {
> struct config *conf = alloc_config();
>
> + if (conf && !_init_config(file, conf))
> + return conf;
> +
> + free(conf);
> + return NULL;
> +}
> +
> +int _init_config (const char *file, struct config *conf)
> +{
> +
> if (!conf)
> - return NULL;
> + conf = &__internal_config;
>
> /*
> * internal defaults
> @@ -896,10 +946,10 @@ struct config *load_config(const char *file)
> !conf->wwids_file || !conf->prkeys_file)
> goto out;
>
> - return conf;
> + return 0;
> out:
> - free_config(conf);
> - return NULL;
> + _uninit_config(conf);
> + return 1;
> }
>
> char *get_uid_attribute_by_attrs(struct config *conf,
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 116fe37..5997b71 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -251,10 +251,25 @@ void free_mptable (vector mptable);
> int store_hwe (vector hwtable, struct hwentry *);
>
> struct config *load_config (const char *file);
> -struct config * alloc_config (void);
> void free_config (struct config * conf);
> -extern struct config *get_multipath_config(void);
> -extern void put_multipath_config(void *);
> +int init_config(const char *file);
> +void uninit_config(void);
> +
> +/*
> + * libmultipath provides default implementations of
> + * get_multipath_config() and put_multipath_config().
> + * Applications using these should use init_config(file, NULL)
> + * to load the configuration, rather than load_config(file).
> + * Likewise, uninit_config() should be used for teardown, but
> + * using free_config() for that is supported, too.
> + * Applications can define their own {get,put}_multipath_config()
> + * functions, which override the library-internal ones, but
> + * could still call libmp_{get,put}_multipath_config().
> + */
> +struct config *libmp_get_multipath_config(void);
> +struct config *get_multipath_config(void);
> +void libmp_put_multipath_config(void *);
> +void put_multipath_config(void *);
>
> int parse_uid_attrs(char *uid_attrs, struct config *conf);
> char *get_uid_attribute_by_attrs(struct config *conf,
> --
> 2.28.0
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 13/19] libmultipath: provide defaults for {get, put}_multipath_config
2020-09-21 19:08 ` Benjamin Marzinski
@ 2020-09-21 19:42 ` Martin Wilck
0 siblings, 0 replies; 31+ messages in thread
From: Martin Wilck @ 2020-09-21 19:42 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: dm-devel
On Mon, 2020-09-21 at 14:08 -0500, Benjamin Marzinski wrote:
>
> free_config() doesn't actually work for configs that are initialized
> by
> init_config(). That's fine, but the commit message is wrong. Also, I
> wonder if uninit_config() should zero out __internal_config, so that
> it's in the same state as it was before init_config() is called.
argh, did it that way first, then changed my mind, and forgot to fix
the commit message .... sorry. Good point about zeroing it out.
Martin
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 14/19] libmpathpersist: allow using libmultipath {get, put}_multipath_config
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (12 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 13/19] libmultipath: provide defaults for {get, put}_multipath_config mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 15/19] multipath: use {get_put}_multipath_config from libmultipath mwilck
` (5 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Provide an alternative init function libmpathpersist_init() which
avoids allocating a new struct config, simply using libmultipath's
internal implementation.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmpathpersist/mpath_persist.c | 42 ++++++++++++++++++++++++++++-----
libmpathpersist/mpath_persist.h | 28 ++++++++++++++++++++++
2 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 729a8e4..fd113cc 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -37,6 +37,27 @@
extern struct udev *udev;
+static void adapt_config(struct config *conf)
+{
+ conf->force_sync = 1;
+ set_max_fds(conf->max_fds);
+}
+
+int libmpathpersist_init(void)
+{
+ struct config *conf;
+ int rc = 0;
+
+ if (init_config(DEFAULT_CONFIGFILE)) {
+ condlog(0, "Failed to initialize multipath config.");
+ return 1;
+ }
+ conf = libmp_get_multipath_config();
+ adapt_config(conf);
+ libmp_put_multipath_config(conf);
+ return rc;
+}
+
struct config *
mpath_lib_init (void)
{
@@ -47,20 +68,29 @@ mpath_lib_init (void)
condlog(0, "Failed to initialize multipath config.");
return NULL;
}
- conf->force_sync = 1;
- set_max_fds(conf->max_fds);
-
+ adapt_config(conf);
return conf;
}
-int
-mpath_lib_exit (struct config *conf)
+static void libmpathpersist_cleanup(void)
{
dm_lib_exit();
cleanup_prio();
cleanup_checkers();
+}
+
+int
+mpath_lib_exit (struct config *conf)
+{
+ libmpathpersist_cleanup();
free_config(conf);
- conf = NULL;
+ return 0;
+}
+
+int libmpathpersist_exit(void)
+{
+ libmpathpersist_cleanup();
+ uninit_config();
return 0;
}
diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index 7cf4faf..91606ef 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -175,6 +175,22 @@ struct prout_param_descriptor { /* PROUT parameter descriptor */
* DESCRIPTION :
* Initialize device mapper multipath configuration. This function must be invoked first
* before performing reservation management functions.
+ * Either this function or mpath_lib_init() may be used.
+ * Use this function to work with libmultipath's internal "struct config".
+ * Call libmpathpersist_exit() for cleanup.
+ * RESTRICTIONS:
+ *
+ * RETURNS: 0->Success, 1->Failed.
+ */
+extern int libmpathpersist_init (void);
+
+/*
+ * DESCRIPTION :
+ * Initialize device mapper multipath configuration. This function must be invoked first
+ * before performing reservation management functions.
+ * Either this function or libmpathpersist_init() may be used.
+ * Use this function to work with an application-specific "struct config".
+ * Call mpath_lib_exit() for cleanup.
* RESTRICTIONS:
*
* RETURNS: struct config ->Success, NULL->Failed.
@@ -186,12 +202,24 @@ extern struct config * mpath_lib_init (void);
* DESCRIPTION :
* Release device mapper multipath configuration. This function must be invoked after
* performing reservation management functions.
+ * Use this after initialization with mpath_lib_init().
* RESTRICTIONS:
*
* RETURNS: 0->Success, 1->Failed.
*/
extern int mpath_lib_exit (struct config *conf);
+/*
+ * DESCRIPTION :
+ * Release device mapper multipath configuration. This function must be invoked after
+ * performing reservation management functions.
+ * Use this after initialization with libmpathpersist_init().
+ * RESTRICTIONS:
+ *
+ * RETURNS: 0->Success, 1->Failed.
+ */
+extern int libmpathpersist_exit (void);
+
/*
* DESCRIPTION :
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 15/19] multipath: use {get_put}_multipath_config from libmultipath
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (13 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 14/19] libmpathpersist: allow using libmultipath " mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 16/19] mpathpersist: use {get, put}_multipath_config() " mwilck
` (4 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipath/main.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/multipath/main.c b/multipath/main.c
index 650977c..2f5ddd9 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -68,7 +68,6 @@
int logsink;
struct udev *udev;
-struct config *multipath_conf;
/*
* Return values of configure(), check_path_valid(), and main().
@@ -79,16 +78,6 @@ enum {
RTVL_RETRY, /* returned by configure(), not by main() */
};
-struct config *get_multipath_config(void)
-{
- return multipath_conf;
-}
-
-void put_multipath_config(__attribute__((unused)) void *arg)
-{
- /* Noop for now */
-}
-
static int
dump_config (struct config *conf, vector hwes, vector mpvec)
{
@@ -826,10 +815,9 @@ main (int argc, char *argv[])
udev = udev_new();
logsink = 0;
- conf = load_config(DEFAULT_CONFIGFILE);
- if (!conf)
+ if (init_config(DEFAULT_CONFIGFILE))
exit(RTVL_FAIL);
- multipath_conf = conf;
+ conf = get_multipath_config();
conf->retrigger_tries = 0;
conf->force_sync = 1;
while ((arg = getopt(argc, argv, ":adDcChl::eFfM:v:p:b:BrR:itTquUwW")) != EOF ) {
@@ -1081,8 +1069,8 @@ out_free_config:
* the logging function (dm_write_log()), which is called there,
* references the config.
*/
- free_config(conf);
- conf = NULL;
+ put_multipath_config(conf);
+ uninit_config();
udev_unref(udev);
if (dev)
FREE(dev);
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 16/19] mpathpersist: use {get, put}_multipath_config() from libmultipath
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (14 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 15/19] multipath: use {get_put}_multipath_config from libmultipath mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 17/19] libmultipath: add udev and logsink symbols mwilck
` (3 subsequent siblings)
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
mpathpersist/main.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 28bfe41..0f0db4b 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -43,17 +43,6 @@ void mpath_print_transport_id(struct prin_fulldescr *fdesc);
int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
int logsink;
-struct config *multipath_conf;
-
-struct config *get_multipath_config(void)
-{
- return multipath_conf;
-}
-
-void put_multipath_config(__attribute__((unused)) void * arg)
-{
- /* Noop for now */
-}
void rcu_register_thread_memb(void) {}
@@ -620,15 +609,14 @@ int main(int argc, char *argv[])
}
udev = udev_new();
- multipath_conf = mpath_lib_init();
- if(!multipath_conf) {
+ if (libmpathpersist_init()) {
udev_unref(udev);
exit(1);
}
ret = handle_args(argc, argv, 0);
- mpath_lib_exit(multipath_conf);
+ libmpathpersist_exit();
udev_unref(udev);
return (ret >= 0) ? ret : MPATH_PR_OTHER;
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 17/19] libmultipath: add udev and logsink symbols
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (15 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 16/19] mpathpersist: use {get, put}_multipath_config() " mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-21 20:10 ` Benjamin Marzinski
2020-09-16 15:37 ` [PATCH 18/19] multipath: remove logsink and udev mwilck
` (2 subsequent siblings)
19 siblings, 1 reply; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
With these symbols added, applications using libmultipath don't
need to define global variables "udev" and "logsink" any more.
This comes at the cost of having to call an init function.
Currently, libmultipath_init() does nothing but initialize
"udev".
The linker's symbol lookup order still allows applications to use
their own "logsink" and "udev" variables, which will take precendence
over libmultipath's internal ones. In this case, calling
libmultipath_init() can be skipped, but like before,
udev should be initialized (using udev_new()) before making any
libmultipath calls.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/config.c | 22 ++++++++++++++++++++++
libmultipath/config.h | 4 +++-
libmultipath/debug.c | 2 ++
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/libmultipath/config.c b/libmultipath/config.c
index b83e5cd..4b48b27 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -27,6 +27,28 @@
#include "mpath_cmd.h"
#include "propsel.h"
+static pthread_once_t _udev_once = PTHREAD_ONCE_INIT;
+struct udev *udev;
+
+void _udev_init(void)
+{
+ udev = udev_new();
+ if (!udev)
+ condlog(0, "%s: failed to initialize udev", __func__);
+}
+
+int libmultipath_init(void)
+{
+ if (!udev)
+ pthread_once(&_udev_once, _udev_init);
+ return udev ? 0 : 1;
+}
+
+void libmultipath_exit(void)
+{
+ udev_unref(udev);
+}
+
static struct config __internal_config;
struct config *libmp_get_multipath_config(void)
{
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 5997b71..541b2e4 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -232,7 +232,9 @@ struct config {
char *enable_foreign;
};
-extern struct udev * udev;
+extern struct udev *udev;
+int libmultipath_init(void);
+void libmultipath_exit(void);
int find_hwe (const struct _vector *hwtable,
const char * vendor, const char * product, const char *revision,
diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index 4128cb9..b3a1de9 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -15,6 +15,8 @@
#include "defaults.h"
#include "debug.h"
+int logsink;
+
void dlog (int sink, int prio, const char * fmt, ...)
{
va_list ap;
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 17/19] libmultipath: add udev and logsink symbols
2020-09-16 15:37 ` [PATCH 17/19] libmultipath: add udev and logsink symbols mwilck
@ 2020-09-21 20:10 ` Benjamin Marzinski
2020-09-23 8:16 ` Martin Wilck
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-21 20:10 UTC (permalink / raw)
To: mwilck; +Cc: dm-devel
On Wed, Sep 16, 2020 at 05:37:16PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> With these symbols added, applications using libmultipath don't
> need to define global variables "udev" and "logsink" any more.
> This comes at the cost of having to call an init function.
> Currently, libmultipath_init() does nothing but initialize
> "udev".
>
> The linker's symbol lookup order still allows applications to use
> their own "logsink" and "udev" variables, which will take precendence
> over libmultipath's internal ones. In this case, calling
> libmultipath_init() can be skipped, but like before,
> udev should be initialized (using udev_new()) before making any
> libmultipath calls.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/config.c | 22 ++++++++++++++++++++++
> libmultipath/config.h | 4 +++-
> libmultipath/debug.c | 2 ++
> 3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index b83e5cd..4b48b27 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -27,6 +27,28 @@
> #include "mpath_cmd.h"
> #include "propsel.h"
>
> +static pthread_once_t _udev_once = PTHREAD_ONCE_INIT;
> +struct udev *udev;
> +
> +void _udev_init(void)
> +{
> + udev = udev_new();
> + if (!udev)
> + condlog(0, "%s: failed to initialize udev", __func__);
> +}
> +
> +int libmultipath_init(void)
> +{
> + if (!udev)
> + pthread_once(&_udev_once, _udev_init);
> + return udev ? 0 : 1;
> +}
> +
> +void libmultipath_exit(void)
> +{
> + udev_unref(udev);
> +}
After calling libmultipath_exit(), you can never reinitialized the udev
device. That seems fine, but it should probably set udev to null, so
that future calls to libmultipath_init() don't return success. Either
that or multipath_init() should use a mutex instead of pthread_once() to
avoid races, so that you can reinitialize udev after a call to
libmultipath_exit().
-Ben
> +
> static struct config __internal_config;
> struct config *libmp_get_multipath_config(void)
> {
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 5997b71..541b2e4 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -232,7 +232,9 @@ struct config {
> char *enable_foreign;
> };
>
> -extern struct udev * udev;
> +extern struct udev *udev;
> +int libmultipath_init(void);
> +void libmultipath_exit(void);
>
> int find_hwe (const struct _vector *hwtable,
> const char * vendor, const char * product, const char *revision,
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index 4128cb9..b3a1de9 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -15,6 +15,8 @@
> #include "defaults.h"
> #include "debug.h"
>
> +int logsink;
> +
> void dlog (int sink, int prio, const char * fmt, ...)
> {
> va_list ap;
> --
> 2.28.0
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 17/19] libmultipath: add udev and logsink symbols
2020-09-21 20:10 ` Benjamin Marzinski
@ 2020-09-23 8:16 ` Martin Wilck
2020-09-23 16:05 ` Benjamin Marzinski
0 siblings, 1 reply; 31+ messages in thread
From: Martin Wilck @ 2020-09-23 8:16 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: dm-devel
On Mon, 2020-09-21 at 15:10 -0500, Benjamin Marzinski wrote:
>
> After calling libmultipath_exit(), you can never reinitialized the
> udev
> device. That seems fine, but it should probably set udev to null, so
> that future calls to libmultipath_init() don't return success. Either
> that or multipath_init() should use a mutex instead of pthread_once()
> to
> avoid races, so that you can reinitialize udev after a call to
> libmultipath_exit().
I've been thinking about this some more. It makes a lot of sense to
move more cleanup code into libmultipath_exit() in the future; thus
this function will do more than just clean up "udev". I believe calling
libmultipath_exit() will become the only supported way to clean up
libmultipath, and basically mandatory, rather sooner than later.
The handling of "udev" is the main cause of headache, because we don't
know whether the application wants to continue to use the variable
after libmultipath_exit(). In libmultipath_exit(), we can't determine
if "udev" is the symbol from libmultipath or from some other object
file. It's also impossible to tell by the return value of udev_unref()
whether or not it destroyed the variable. Setting udev to NULL is
dangerous if the uevent listener thread is still running, or if the
application needs to use the variable further.
So this is my current idea for a "robust" design:
1. libmultipath_init() initializes udev if it's NULL; otherwise, it
simply takes an additional ref. IOW, applications may (but
don't have to) initialize and use udev before calling
libmultipath_init().
2. libmultipath_exit() calls udev_unref(udev), without nullifying it.
Thus applications may continue using udev afterwards, if they own an
additional reference.
3. libmultipath_init() always fails after calling libmultipath_exit().
4. Other libmultipath calls after libmultipath_exit() may cause
undefined behavior.
5. Normal usage would be to call libmultipath_exit() at program exit.
6. Calling libmultipath_init() is currently only mandatory for
programs that don't initialize "udev" themselves. This may change
in the future.
7. Calling libmultipath_exit() will be mandatory for programs that
wish to avoid memory leaks.
The only downside that I see is that the application can't test whether
"udev" is valid by checking if it's NULL. But that's by design of
udev_unref(). The application needs to track its own references.
There is no rigorous reason for (3.). In principle, we could just
handle re-initialization like (1.). But I don't think it's worth the
effort of figuring out all possible ways in which re-initialization
could go wrong, in particular if we want to initialize more stuff
later.
Does this make sense?
Martin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 17/19] libmultipath: add udev and logsink symbols
2020-09-23 8:16 ` Martin Wilck
@ 2020-09-23 16:05 ` Benjamin Marzinski
0 siblings, 0 replies; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-23 16:05 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Wed, Sep 23, 2020 at 10:16:48AM +0200, Martin Wilck wrote:
> On Mon, 2020-09-21 at 15:10 -0500, Benjamin Marzinski wrote:
> >
> > After calling libmultipath_exit(), you can never reinitialized the
> > udev
> > device. That seems fine, but it should probably set udev to null, so
> > that future calls to libmultipath_init() don't return success. Either
> > that or multipath_init() should use a mutex instead of pthread_once()
> > to
> > avoid races, so that you can reinitialize udev after a call to
> > libmultipath_exit().
>
> I've been thinking about this some more. It makes a lot of sense to
> move more cleanup code into libmultipath_exit() in the future; thus
> this function will do more than just clean up "udev". I believe calling
> libmultipath_exit() will become the only supported way to clean up
> libmultipath, and basically mandatory, rather sooner than later.
>
> The handling of "udev" is the main cause of headache, because we don't
> know whether the application wants to continue to use the variable
> after libmultipath_exit(). In libmultipath_exit(), we can't determine
> if "udev" is the symbol from libmultipath or from some other object
> file. It's also impossible to tell by the return value of udev_unref()
> whether or not it destroyed the variable. Setting udev to NULL is
> dangerous if the uevent listener thread is still running, or if the
> application needs to use the variable further.
>
> So this is my current idea for a "robust" design:
>
> 1. libmultipath_init() initializes udev if it's NULL; otherwise, it
> simply takes an additional ref. IOW, applications may (but
> don't have to) initialize and use udev before calling
> libmultipath_init().
> 2. libmultipath_exit() calls udev_unref(udev), without nullifying it.
> Thus applications may continue using udev afterwards, if they own an
> additional reference.
> 3. libmultipath_init() always fails after calling libmultipath_exit().
> 4. Other libmultipath calls after libmultipath_exit() may cause
> undefined behavior.
> 5. Normal usage would be to call libmultipath_exit() at program exit.
> 6. Calling libmultipath_init() is currently only mandatory for
> programs that don't initialize "udev" themselves. This may change
> in the future.
> 7. Calling libmultipath_exit() will be mandatory for programs that
> wish to avoid memory leaks.
>
> The only downside that I see is that the application can't test whether
> "udev" is valid by checking if it's NULL. But that's by design of
> udev_unref(). The application needs to track its own references.
>
> There is no rigorous reason for (3.). In principle, we could just
> handle re-initialization like (1.). But I don't think it's worth the
> effort of figuring out all possible ways in which re-initialization
> could go wrong, in particular if we want to initialize more stuff
> later.
>
> Does this make sense?
Yeah. After I sent off my message, I realized that there's no way to
know if you dropped the last reference, which means that NULLing out
udev doesn't make sense.
-Ben
> Martin
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 18/19] multipath: remove logsink and udev
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (16 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 17/19] libmultipath: add udev and logsink symbols mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-16 15:37 ` [PATCH 19/19] mpathpersist: " mwilck
2020-09-21 20:20 ` [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals Benjamin Marzinski
19 siblings, 0 replies; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
We can use libmultipath's symbols now.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipath/main.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/multipath/main.c b/multipath/main.c
index 2f5ddd9..6a2a188 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -66,9 +66,6 @@
#include "valid.h"
#include "alias.h"
-int logsink;
-struct udev *udev;
-
/*
* Return values of configure(), check_path_valid(), and main().
*/
@@ -813,7 +810,7 @@ main (int argc, char *argv[])
int retries = -1;
bool enable_foreign = false;
- udev = udev_new();
+ libmultipath_init();
logsink = 0;
if (init_config(DEFAULT_CONFIGFILE))
exit(RTVL_FAIL);
@@ -1071,7 +1068,7 @@ out_free_config:
*/
put_multipath_config(conf);
uninit_config();
- udev_unref(udev);
+ libmultipath_exit();
if (dev)
FREE(dev);
#ifdef _DEBUG_
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 19/19] mpathpersist: remove logsink and udev
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (17 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 18/19] multipath: remove logsink and udev mwilck
@ 2020-09-16 15:37 ` mwilck
2020-09-21 20:15 ` Benjamin Marzinski
2020-09-21 20:20 ` [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals Benjamin Marzinski
19 siblings, 1 reply; 31+ messages in thread
From: mwilck @ 2020-09-16 15:37 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
We can use libmultipath's internal symbols now.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
mpathpersist/main.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 0f0db4b..729857f 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -42,13 +42,10 @@ void * mpath_alloc_prin_response(int prin_sa);
void mpath_print_transport_id(struct prin_fulldescr *fdesc);
int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
-int logsink;
-
void rcu_register_thread_memb(void) {}
void rcu_unregister_thread_memb(void) {}
-struct udev *udev;
static int verbose, loglevel, noisy;
@@ -608,16 +605,17 @@ int main(int argc, char *argv[])
exit (1);
}
- udev = udev_new();
+ if (libmultipath_init())
+ exit(1);
if (libmpathpersist_init()) {
- udev_unref(udev);
+ libmultipath_exit();
exit(1);
}
ret = handle_args(argc, argv, 0);
libmpathpersist_exit();
- udev_unref(udev);
+ libmultipath_exit();
return (ret >= 0) ? ret : MPATH_PR_OTHER;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 19/19] mpathpersist: remove logsink and udev
2020-09-16 15:37 ` [PATCH 19/19] mpathpersist: " mwilck
@ 2020-09-21 20:15 ` Benjamin Marzinski
2020-09-22 11:32 ` Martin Wilck
0 siblings, 1 reply; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-21 20:15 UTC (permalink / raw)
To: mwilck; +Cc: dm-devel
On Wed, Sep 16, 2020 at 05:37:18PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> We can use libmultipath's internal symbols now.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> mpathpersist/main.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 0f0db4b..729857f 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
I'm pretty sure that without this patch, mpathpersist is not directly
calling functions from libmultipath, and if so, it should stay that way.
I don't see any problem with adding the libmultipath_init() and
libmultipath_exit() calls to libmpathpersist_init() and
libmpathpersist_exit().
-Ben
> @@ -42,13 +42,10 @@ void * mpath_alloc_prin_response(int prin_sa);
> void mpath_print_transport_id(struct prin_fulldescr *fdesc);
> int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
>
> -int logsink;
> -
> void rcu_register_thread_memb(void) {}
>
> void rcu_unregister_thread_memb(void) {}
>
> -struct udev *udev;
>
> static int verbose, loglevel, noisy;
>
> @@ -608,16 +605,17 @@ int main(int argc, char *argv[])
> exit (1);
> }
>
> - udev = udev_new();
> + if (libmultipath_init())
> + exit(1);
> if (libmpathpersist_init()) {
> - udev_unref(udev);
> + libmultipath_exit();
> exit(1);
> }
>
> ret = handle_args(argc, argv, 0);
>
> libmpathpersist_exit();
> - udev_unref(udev);
> + libmultipath_exit();
>
> return (ret >= 0) ? ret : MPATH_PR_OTHER;
> }
> --
> 2.28.0
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 19/19] mpathpersist: remove logsink and udev
2020-09-21 20:15 ` Benjamin Marzinski
@ 2020-09-22 11:32 ` Martin Wilck
0 siblings, 0 replies; 31+ messages in thread
From: Martin Wilck @ 2020-09-22 11:32 UTC (permalink / raw)
To: Benjamin Marzinski; +Cc: dm-devel
On Mon, 2020-09-21 at 15:15 -0500, Benjamin Marzinski wrote:
> On Wed, Sep 16, 2020 at 05:37:18PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> >
> > We can use libmultipath's internal symbols now.
> >
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> > mpathpersist/main.c | 10 ++++------
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> > index 0f0db4b..729857f 100644
> > --- a/mpathpersist/main.c
> > +++ b/mpathpersist/main.c
>
> I'm pretty sure that without this patch, mpathpersist is not directly
> calling functions from libmultipath
Yes, this is so.
> , and if so, it should stay that way.
>
> I don't see any problem with adding the libmultipath_init() and
> libmultipath_exit() calls to libmpathpersist_init() and
> libmpathpersist_exit().
Will do.
Martin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals
2020-09-16 15:36 [PATCH 00/19] multipath-tools: shutdown, libdevmapper races, globals mwilck
` (18 preceding siblings ...)
2020-09-16 15:37 ` [PATCH 19/19] mpathpersist: " mwilck
@ 2020-09-21 20:20 ` Benjamin Marzinski
19 siblings, 0 replies; 31+ messages in thread
From: Benjamin Marzinski @ 2020-09-21 20:20 UTC (permalink / raw)
To: mwilck; +Cc: lixiaokeng, dm-devel
On Wed, Sep 16, 2020 at 05:36:59PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> Hi Christophe, hi Ben,
>
> this series contains a number of patches I've wanted to do for some
> time. As usual, it's based on my "upstream-queue" tree
> (https://github.com/openSUSE/multipath-tools/tree/upstream-queue).
>
> Patch 1-8 are related to multipathd shutdown and systemd notifcation.
> Patch 1 and 8 have been part of my earlier series "multipath-tools:
> Fix remaining shutdown delay issues" from Jan, 2019. But this is so
> long ago that I didn't mark this as a v2. I have tried to address
> Ben's issues with #1 (size_mismatch_seen leak, and premature sd_notify)
> (https://www.redhat.com/archives/dm-devel/2019-January/msg00097.html).
> #8 is just resent, after the recent discussion
> (https://www.redhat.com/archives/dm-devel/2020-August/msg00342.html).
> Ben's remark about sd_notify() drove me to reexamine that feature,
> and actually improve a little by informing systemd of shutdown and
> reconfigure operations.
>
> Patch 9-11 are an attempt to fix races in libdevmapper, as discussed
> a while ago in the "fix fd leak when iscsi device logs in" thread
> (https://www.redhat.com/archives/dm-devel/2020-July/msg00321.html and
> references).
>
> Patch 12ff. add definitions of the symbol get_multipath_config(),
> put_multipath_config(), udev, and logsink to libmultipath. This way
> callers won't have to bother with defining these global symbols any
> more in the future (but they still can).
Thanks for doing this. I really like these cleanups. I'll be resending
my libmpathvalid library code on top of this set.
Reviewed-by: Benjamin Marzinski <bmarizns@redhatc.com>
For all the patches I didn't comment on.
-Ben
> Regards,
> Martin
>
> Cc: lixiaokeng@huawei.com
>
> Martin Wilck (19):
> multipathd: allow shutdown during configure()
> multipathd: avoid sending "READY=1" to systemd on early shutdown
> multipathd: send "STOPPING=1" to systemd on shutdown
> multipathd: send "RELOADING=1" to systemd on DAEMON_CONFIGURE state
> multipathd: use volatile qualifier for running_state
> multipathd: generalize and fix wait_for_state_change_if()
> multipathd: set_config_state(): avoid code duplication
> multipathd: cancel threads early during shutdown
> multipath-tools: don't call dm_lib_release() any more
> libmultipath: devmapper: refactor libdm version determination
> libmultipath: protect racy libdevmapper calls with a mutex
> libmultipath: constify file argument in config parser
> libmultipath: provide defaults for {get,put}_multipath_config
> libmpathpersist: allow using libmultipath {get,put}_multipath_config
> multipath: use {get_put}_multipath_config from libmultipath
> mpathpersist: use {get,put}_multipath_config() from libmultipath
> libmultipath: add udev and logsink symbols
> multipath: remove logsink and udev
> mpathpersist: remove logsink and udev
>
> kpartx/kpartx.c | 1 -
> libmpathpersist/mpath_persist.c | 43 +++++-
> libmpathpersist/mpath_persist.h | 28 ++++
> libmultipath/config.c | 95 +++++++++++--
> libmultipath/config.h | 28 +++-
> libmultipath/configure.c | 6 +
> libmultipath/debug.c | 2 +
> libmultipath/devmapper.c | 228 +++++++++++++++++++++-----------
> libmultipath/devmapper.h | 13 +-
> libmultipath/discovery.c | 3 +
> libmultipath/parser.c | 9 +-
> libmultipath/parser.h | 2 +-
> libmultipath/propsel.c | 10 +-
> libmultipath/util.c | 10 ++
> libmultipath/util.h | 2 +
> mpathpersist/main.c | 26 +---
> multipath/main.c | 28 +---
> multipathd/cli_handlers.c | 2 -
> multipathd/dmevents.c | 4 +-
> multipathd/main.c | 117 ++++++++--------
> multipathd/waiter.c | 2 +-
> 21 files changed, 441 insertions(+), 218 deletions(-)
>
> --
> 2.28.0
^ permalink raw reply [flat|nested] 31+ messages in thread