[PATCH 0/3] multipathd per-device waiter fixes

[PATCH 0/3] multipathd per-device waiter fixes

[Benjamin Marzinski]

These patches fix some issues in the old waiter code that I noticed
when writing my the polling waiter code. Neither of them is particularly
likely to cause a problem in real world situations, but I don't think
the fixes should be that controversial.

These patches are based on top of my "multipath: new and rebased patches"
set.

Benjamin Marzinski (3):
  multipathd: fix waiter thread cancelling
  multipathd: move __setup_multipath to multipathd
  multipathd: stop waiter in __setup_multipath

 libmultipath/structs_vec.c | 103 ---------------------------------------------
 libmultipath/structs_vec.h |   4 --
 multipathd/dmevents.c      |   1 +
 multipathd/main.c          | 103 +++++++++++++++++++++++++++++++++++++++++++++
 multipathd/main.h          |   4 ++
 multipathd/waiter.c        |  22 ++++++++--
 6 files changed, 127 insertions(+), 110 deletions(-)

-- 
2.7.4

[PATCH 1/3] multipathd: fix waiter thread cancelling

[Benjamin Marzinski]

multipathd was sending a signal to the per-device waiter thread after
cancelling it.  Since these threads are detached, there is a window
where a new thread could have started with the old thread id between the
cancel and the signalling, causing the signal to be delivered to the
wrong thread. Simply reversing the order doesn't fix the issue, since
the waiter threads exit immediately if they receive a signal, again
opening a window for the cancel to be delivered to the wrong thread.

To fix this, multipathd does reverse the order, so that it signals the
thread first (it needs to signal the thread, since the dm_task_run ioctl
isn't a cancellation point) and then cancels it. However it does this
while holding a new mutex.

The waiter thread can only exit without being cancelled for two reasons.
1. When it fails in update_multipath, which removes the device while
   holding the vecs lock.
2. If it receives a SIGUSR2 signal while waiting for a dm event.

Case 1 can never race with another thread removing the device, since
removing a device always happens while holding the vecs lock.  This
means that if the device exists to be removed, then the waiter thread
can't exit this way during the removal.

Case 2 is now solved by grabbing the new mutex after failing
dm_task_run(). With the mutex held, the thread checks if it has been
cancelled. If it wasn't cancelled, the thread continues.

The reason that this uses a new mutex, instead of the vecs lock, is that
using the vecs lock would keep the thread from ending until the vecs
lock was released.  Normally, this isn't a problem. But during
reconfigure, the waiter threads for all devices are stopped, and new
ones started, all while holding the vecs lock.  For systems with a large
number of multipath devices, this will cause multipathd do have double
its already large number of waiter threads during reconfigure, all locked
into memory.

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

diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index cb9708b..e894294 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -23,6 +23,7 @@
 #include "waiter.h"
 
 pthread_attr_t waiter_attr;
+struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER };
 
 static struct event_thread *alloc_waiter (void)
 {
@@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 		mpp->waiter);
 	thread = mpp->waiter;
 	mpp->waiter = (pthread_t)0;
-	pthread_cancel(thread);
+	pthread_cleanup_push(cleanup_lock, &waiter_lock);
+	lock(&waiter_lock);
+	pthread_testcancel();
 	pthread_kill(thread, SIGUSR2);
+	pthread_cancel(thread);
+	lock_cleanup_pop(&waiter_lock);
 }
 
 /*
@@ -114,8 +119,13 @@ static int waiteventloop (struct event_thread *waiter)
 	dm_task_destroy(waiter->dmt);
 	waiter->dmt = NULL;
 
-	if (!r)	/* wait interrupted by signal */
-		return -1;
+	if (!r)	{ /* wait interrupted by signal. check for cancellation */
+		pthread_cleanup_push(cleanup_lock, &waiter_lock);
+		lock(&waiter_lock);
+		pthread_testcancel();
+		lock_cleanup_pop(&waiter_lock);
+		return 1; /* If we weren't cancelled, just reschedule */
+	}
 
 	waiter->event_nr++;
 
-- 
2.7.4

[PATCH 2/3] multipathd: move __setup_multipath to multipathd

[Benjamin Marzinski]

__setup_multipath is only called from multipathd, so it shouldn't be in
libmultipath.  Move it, update_multpath (which calls it) and
set_no_path_retry (which is a helper function for it) into multipathd.
None of these functions were changed, only copied.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 103 ---------------------------------------------
 libmultipath/structs_vec.h |   4 --
 multipathd/dmevents.c      |   1 +
 multipathd/main.c          | 103 +++++++++++++++++++++++++++++++++++++++++++++
 multipathd/main.h          |   4 ++
 multipathd/waiter.c        |   1 +
 6 files changed, 109 insertions(+), 107 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index e9a0274..8c8fb25 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -290,61 +290,6 @@ void enter_recovery_mode(struct multipath *mpp)
 	put_multipath_config(conf);
 }
 
-static void set_no_path_retry(struct multipath *mpp)
-{
-	char is_queueing = 0;
-
-	mpp->nr_active = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
-	if (mpp->features && strstr(mpp->features, "queue_if_no_path"))
-		is_queueing = 1;
-
-	switch (mpp->no_path_retry) {
-	case NO_PATH_RETRY_UNDEF:
-		break;
-	case NO_PATH_RETRY_FAIL:
-		if (is_queueing)
-			dm_queue_if_no_path(mpp->alias, 0);
-		break;
-	case NO_PATH_RETRY_QUEUE:
-		if (!is_queueing)
-			dm_queue_if_no_path(mpp->alias, 1);
-		break;
-	default:
-		if (mpp->nr_active > 0) {
-			mpp->retry_tick = 0;
-			dm_queue_if_no_path(mpp->alias, 1);
-		} else if (is_queueing && mpp->retry_tick == 0)
-			enter_recovery_mode(mpp);
-		break;
-	}
-}
-
-int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
-		      int reset)
-{
-	if (dm_get_info(mpp->alias, &mpp->dmi)) {
-		/* Error accessing table */
-		condlog(3, "%s: cannot access table", mpp->alias);
-		goto out;
-	}
-
-	if (update_multipath_strings(mpp, vecs->pathvec, 1)) {
-		condlog(0, "%s: failed to setup multipath", mpp->alias);
-		goto out;
-	}
-
-	if (reset) {
-		set_no_path_retry(mpp);
-		if (VECTOR_SIZE(mpp->paths) != 0)
-			dm_cancel_deferred_remove(mpp);
-	}
-
-	return 0;
-out:
-	remove_map(mpp, vecs, PURGE_VEC);
-	return 1;
-}
-
 void
 sync_map_state(struct multipath *mpp)
 {
@@ -468,54 +413,6 @@ int verify_paths(struct multipath *mpp, struct vectors *vecs)
 	return count;
 }
 
-int update_multipath (struct vectors *vecs, char *mapname, int reset)
-{
-	struct multipath *mpp;
-	struct pathgroup  *pgp;
-	struct path *pp;
-	int i, j;
-
-	mpp = find_mp_by_alias(vecs->mpvec, mapname);
-
-	if (!mpp) {
-		condlog(3, "%s: multipath map not found", mapname);
-		return 2;
-	}
-
-	if (__setup_multipath(vecs, mpp, reset))
-		return 1; /* mpp freed in setup_multipath */
-
-	/*
-	 * compare checkers states with DM states
-	 */
-	vector_foreach_slot (mpp->pg, pgp, i) {
-		vector_foreach_slot (pgp->paths, pp, j) {
-			if (pp->dmstate != PSTATE_FAILED)
-				continue;
-
-			if (pp->state != PATH_DOWN) {
-				struct config *conf = get_multipath_config();
-				int oldstate = pp->state;
-				condlog(2, "%s: mark as failed", pp->dev);
-				mpp->stat_path_failures++;
-				pp->state = PATH_DOWN;
-				if (oldstate == PATH_UP ||
-				    oldstate == PATH_GHOST)
-					update_queue_mode_del_path(mpp);
-
-				/*
-				 * if opportune,
-				 * schedule the next check earlier
-				 */
-				if (pp->tick > conf->checkint)
-					pp->tick = conf->checkint;
-				put_multipath_config(conf);
-			}
-		}
-	}
-	return 0;
-}
-
 /*
  * mpp->no_path_retry:
  *   -2 (QUEUE) : queue_if_no_path enabled, never turned off
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 0adba17..4220ea3 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -19,9 +19,6 @@ void orphan_path (struct path * pp, const char *reason);
 
 int verify_paths(struct multipath * mpp, struct vectors * vecs);
 int update_mpp_paths(struct multipath * mpp, vector pathvec);
-int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
-		       int reset);
-#define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
 int update_multipath_strings (struct multipath *mpp, vector pathvec,
 			      int is_daemon);
 void extract_hwe_from_path(struct multipath * mpp);
@@ -36,7 +33,6 @@ void remove_maps (struct vectors * vecs);
 void sync_map_state (struct multipath *);
 struct multipath * add_map_with_path (struct vectors * vecs,
 				struct path * pp, int add_vec);
-int update_multipath (struct vectors *vecs, char *mapname, int reset);
 void update_queue_mode_del_path(struct multipath *mpp);
 void update_queue_mode_add_path(struct multipath *mpp);
 int update_multipath_table (struct multipath *mpp, vector pathvec,
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index 1ef811e..2281a10 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -22,6 +22,7 @@
 #include "structs_vec.h"
 #include "devmapper.h"
 #include "debug.h"
+#include "main.h"
 #include "dmevents.h"
 
 #ifndef DM_DEV_ARM_POLL
diff --git a/multipathd/main.c b/multipathd/main.c
index e35231e..70aff5d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -347,6 +347,109 @@ set_multipath_wwid (struct multipath * mpp)
 	dm_get_uuid(mpp->alias, mpp->wwid);
 }
 
+static void set_no_path_retry(struct multipath *mpp)
+{
+	char is_queueing = 0;
+
+	mpp->nr_active = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
+	if (mpp->features && strstr(mpp->features, "queue_if_no_path"))
+		is_queueing = 1;
+
+	switch (mpp->no_path_retry) {
+	case NO_PATH_RETRY_UNDEF:
+		break;
+	case NO_PATH_RETRY_FAIL:
+		if (is_queueing)
+			dm_queue_if_no_path(mpp->alias, 0);
+		break;
+	case NO_PATH_RETRY_QUEUE:
+		if (!is_queueing)
+			dm_queue_if_no_path(mpp->alias, 1);
+		break;
+	default:
+		if (mpp->nr_active > 0) {
+			mpp->retry_tick = 0;
+			dm_queue_if_no_path(mpp->alias, 1);
+		} else if (is_queueing && mpp->retry_tick == 0)
+			enter_recovery_mode(mpp);
+		break;
+	}
+}
+
+int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
+		      int reset)
+{
+	if (dm_get_info(mpp->alias, &mpp->dmi)) {
+		/* Error accessing table */
+		condlog(3, "%s: cannot access table", mpp->alias);
+		goto out;
+	}
+
+	if (update_multipath_strings(mpp, vecs->pathvec, 1)) {
+		condlog(0, "%s: failed to setup multipath", mpp->alias);
+		goto out;
+	}
+
+	if (reset) {
+		set_no_path_retry(mpp);
+		if (VECTOR_SIZE(mpp->paths) != 0)
+			dm_cancel_deferred_remove(mpp);
+	}
+
+	return 0;
+out:
+	remove_map(mpp, vecs, PURGE_VEC);
+	return 1;
+}
+
+int update_multipath (struct vectors *vecs, char *mapname, int reset)
+{
+	struct multipath *mpp;
+	struct pathgroup  *pgp;
+	struct path *pp;
+	int i, j;
+
+	mpp = find_mp_by_alias(vecs->mpvec, mapname);
+
+	if (!mpp) {
+		condlog(3, "%s: multipath map not found", mapname);
+		return 2;
+	}
+
+	if (__setup_multipath(vecs, mpp, reset))
+		return 1; /* mpp freed in setup_multipath */
+
+	/*
+	 * compare checkers states with DM states
+	 */
+	vector_foreach_slot (mpp->pg, pgp, i) {
+		vector_foreach_slot (pgp->paths, pp, j) {
+			if (pp->dmstate != PSTATE_FAILED)
+				continue;
+
+			if (pp->state != PATH_DOWN) {
+				struct config *conf = get_multipath_config();
+				int oldstate = pp->state;
+				condlog(2, "%s: mark as failed", pp->dev);
+				mpp->stat_path_failures++;
+				pp->state = PATH_DOWN;
+				if (oldstate == PATH_UP ||
+				    oldstate == PATH_GHOST)
+					update_queue_mode_del_path(mpp);
+
+				/*
+				 * if opportune,
+				 * schedule the next check earlier
+				 */
+				if (pp->tick > conf->checkint)
+					pp->tick = conf->checkint;
+				put_multipath_config(conf);
+			}
+		}
+	}
+	return 0;
+}
+
 static int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
diff --git a/multipathd/main.h b/multipathd/main.h
index 0e9c5e3..af39558 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -39,5 +39,9 @@ void * mpath_pr_event_handler_fn (void * );
 int update_map_pr(struct multipath *mpp);
 void * mpath_pr_event_handler_fn (void * pathp );
 void handle_signals(bool);
+int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
+		       int reset);
+#define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
+int update_multipath (struct vectors *vecs, char *mapname, int reset);
 
 #endif /* MAIN_H */
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index e894294..c70ad21 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -21,6 +21,7 @@
 #include "debug.h"
 #include "lock.h"
 #include "waiter.h"
+#include "main.h"
 
 pthread_attr_t waiter_attr;
 struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER };
-- 
2.7.4

[PATCH 3/3] multipathd: stop waiter in __setup_multipath

[Benjamin Marzinski]

__setup_multipath can remove a multipath device from multipathd, and it
can be called by either by the waiter thread or another thread.
Previously, it dealt with this by never stopping the waiter thread.  It
simply relied on the waiter thread to notice and stop itself.  Now, when
called by another thread, it explicitly stops the waiter thread.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c   | 2 +-
 multipathd/waiter.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 70aff5d..3ae0442 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -398,7 +398,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 
 	return 0;
 out:
-	remove_map(mpp, vecs, PURGE_VEC);
+	remove_map_and_stop_waiter(mpp, vecs, PURGE_VEC);
 	return 1;
 }
 
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index c70ad21..595c69a 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -57,6 +57,11 @@ void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 			mpp->alias);
 		return;
 	}
+	/* Don't cancel yourself. __setup_multipath is called by
+	   by the waiter thread, and may remove a multipath device */
+	if (pthread_equal(mpp->waiter, pthread_self()))
+		return;
+
 	condlog(2, "%s: stop event checker thread (%lu)", mpp->alias,
 		mpp->waiter);
 	thread = mpp->waiter;
-- 
2.7.4

Re: [PATCH 1/3] multipathd: fix waiter thread cancelling

[Martin Wilck]

On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> multipathd was sending a signal to the per-device waiter thread after
> cancelling it.  Since these threads are detached, there is a window
> where a new thread could have started with the old thread id between
> the
> cancel and the signalling, causing the signal to be delivered to the
> wrong thread. Simply reversing the order doesn't fix the issue, since
> the waiter threads exit immediately if they receive a signal, again
> opening a window for the cancel to be delivered to the wrong thread.
> 
> To fix this, multipathd does reverse the order, so that it signals
> the
> thread first (it needs to signal the thread, since the dm_task_run
> ioctl
> isn't a cancellation point) and then cancels it. However it does this
> while holding a new mutex.
> 
> The waiter thread can only exit without being cancelled for two
> reasons.
> 1. When it fails in update_multipath, which removes the device while
>    holding the vecs lock.
> 2. If it receives a SIGUSR2 signal while waiting for a dm event.
> 
> Case 1 can never race with another thread removing the device, since
> removing a device always happens while holding the vecs lock.  This
> means that if the device exists to be removed, then the waiter thread
> can't exit this way during the removal.
> 
> Case 2 is now solved by grabbing the new mutex after failing
> dm_task_run(). With the mutex held, the thread checks if it has been
> cancelled. If it wasn't cancelled, the thread continues.
> 
> The reason that this uses a new mutex, instead of the vecs lock, is
> that
> using the vecs lock would keep the thread from ending until the vecs
> lock was released.  Normally, this isn't a problem. But during
> reconfigure, the waiter threads for all devices are stopped, and new
> ones started, all while holding the vecs lock.  For systems with a
> large
> number of multipath devices, this will cause multipathd do have
> double
> its already large number of waiter threads during reconfigure, all
> locked
> into memory.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/waiter.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> index cb9708b..e894294 100644
> --- a/multipathd/waiter.c
> +++ b/multipathd/waiter.c
> @@ -23,6 +23,7 @@
>  #include "waiter.h"
>  
>  pthread_attr_t waiter_attr;
> +struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER
> };
>  
>  static struct event_thread *alloc_waiter (void)
>  {
> @@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath *mpp,
> struct vectors *vecs)
>  		mpp->waiter);
>  	thread = mpp->waiter;
>  	mpp->waiter = (pthread_t)0;
> -	pthread_cancel(thread);
> +	pthread_cleanup_push(cleanup_lock, &waiter_lock);
> +	lock(&waiter_lock);
> +	pthread_testcancel();

What's the purpose of this pthread_testcancel() call?

>  	pthread_kill(thread, SIGUSR2);
> +	pthread_cancel(thread);
> +	lock_cleanup_pop(&waiter_lock);
>  }
>  
>  /*
> @@ -114,8 +119,13 @@ static int waiteventloop (struct event_thread
> *waiter)
>  	dm_task_destroy(waiter->dmt);
>  	waiter->dmt = NULL;
>  
> -	if (!r)	/* wait interrupted by signal */
> -		return -1;
> +	if (!r)	{ /* wait interrupted by signal. check for
> cancellation */
> +		pthread_cleanup_push(cleanup_lock, &waiter_lock);
> +		lock(&waiter_lock);
> +		pthread_testcancel();
> +		lock_cleanup_pop(&waiter_lock);

Nitpick: I'd prefer pthread_cleanup_pop(1) here.

Regards,
Martin


> +		return 1; /* If we weren't cancelled, just
> reschedule */
> +	}
>  
>  	waiter->event_nr++;
>  

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

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

Re: [PATCH 2/3] multipathd: move __setup_multipath to multipathd

[Martin Wilck]

On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> __setup_multipath is only called from multipathd, so it shouldn't be
> in
> libmultipath.  Move it, update_multpath (which calls it) and
> set_no_path_retry (which is a helper function for it) into
> multipathd.
> None of these functions were changed, only copied.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

One day we may need to split up multipathd/main.c a bit...

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

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

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

Re: [PATCH 3/3] multipathd: stop waiter in __setup_multipath

[Martin Wilck]

On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> __setup_multipath can remove a multipath device from multipathd, and
> it
> can be called by either by the waiter thread or another thread.
> Previously, it dealt with this by never stopping the waiter
> thread.  It
> simply relied on the waiter thread to notice and stop itself.  Now,
> when
> called by another thread, it explicitly stops the waiter thread.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

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

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

Re: [PATCH 1/3] multipathd: fix waiter thread cancelling

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 01:59:24PM +0100, Martin Wilck wrote:
> On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> > diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> > index cb9708b..e894294 100644
> > --- a/multipathd/waiter.c
> > +++ b/multipathd/waiter.c
> > @@ -23,6 +23,7 @@
> >  #include "waiter.h"
> >  
> >  pthread_attr_t waiter_attr;
> > +struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER
> > };
> >  
> >  static struct event_thread *alloc_waiter (void)
> >  {
> > @@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath *mpp,
> > struct vectors *vecs)
> >  		mpp->waiter);
> >  	thread = mpp->waiter;
> >  	mpp->waiter = (pthread_t)0;
> > -	pthread_cancel(thread);
> > +	pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > +	lock(&waiter_lock);
> > +	pthread_testcancel();
> 
> What's the purpose of this pthread_testcancel() call?
> 

It's not necessary. I feel like in general, after you get done waiting
on a lock, it's good form to see if you've been cancelled while you were
waiting. However, it's really unlikely that a thread will be waiting
here long, and this pthread_cancel isn't protecting it form accessing
any freed data. If you think it's likely to confuse people, I have no
problem with removing it.

> >  	pthread_kill(thread, SIGUSR2);
> > +	pthread_cancel(thread);
> > +	lock_cleanup_pop(&waiter_lock);
> >  }
> >  
> >  /*
> > @@ -114,8 +119,13 @@ static int waiteventloop (struct event_thread
> > *waiter)
> >  	dm_task_destroy(waiter->dmt);
> >  	waiter->dmt = NULL;
> >  
> > -	if (!r)	/* wait interrupted by signal */
> > -		return -1;
> > +	if (!r)	{ /* wait interrupted by signal. check for
> > cancellation */
> > +		pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > +		lock(&waiter_lock);
> > +		pthread_testcancel();
> > +		lock_cleanup_pop(&waiter_lock);
> 
> Nitpick: I'd prefer pthread_cleanup_pop(1) here.

I have no problem with changing that.

-Ben

> Regards,
> Martin
> 
> 
> > +		return 1; /* If we weren't cancelled, just
> > reschedule */
> > +	}
> >  
> >  	waiter->event_nr++;
> >  
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH 1/3] multipathd: fix waiter thread cancelling

[Martin Wilck]

On Mon, 2018-03-19 at 11:25 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 19, 2018 at 01:59:24PM +0100, Martin Wilck wrote:
> > On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> > > diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> > > index cb9708b..e894294 100644
> > > --- a/multipathd/waiter.c
> > > +++ b/multipathd/waiter.c
> > > @@ -23,6 +23,7 @@
> > >  #include "waiter.h"
> > >  
> > >  pthread_attr_t waiter_attr;
> > > +struct mutex_lock waiter_lock = { .mutex =
> > > PTHREAD_MUTEX_INITIALIZER
> > > };
> > >  
> > >  static struct event_thread *alloc_waiter (void)
> > >  {
> > > @@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath
> > > *mpp,
> > > struct vectors *vecs)
> > >  		mpp->waiter);
> > >  	thread = mpp->waiter;
> > >  	mpp->waiter = (pthread_t)0;
> > > -	pthread_cancel(thread);
> > > +	pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > > +	lock(&waiter_lock);
> > > +	pthread_testcancel();
> > 
> > What's the purpose of this pthread_testcancel() call?
> > 
> 
> It's not necessary. I feel like in general, after you get done
> waiting
> on a lock, it's good form to see if you've been cancelled while you
> were
> waiting. However, it's really unlikely that a thread will be waiting
> here long, and this pthread_cancel isn't protecting it form accessing
> any freed data. If you think it's likely to confuse people, I have no
> problem with removing it.

My thinking was: this thread is just about to cancel another thread. If
it's cancelled itself just before doing that, the other one won't be
cancelled. I find it hard to imagine conditions where that would be the
right thing. So if at all, I'd call pthread_testcancel() after
releasing the lock. But probably it's best just to remove it here.

Regards,
Martin

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

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