[PATCH 0/7] multipath: miscellaneous bug fixes

[PATCH 0/7] multipath: miscellaneous bug fixes

[Benjamin Marzinski]

This is just a set of fixes for bugs I've run into.

Benjamin Marzinski (7):
  multipath: fix tur checker locking
  multipath: fix DEF_TIMEOUT use
  multipathd: remove coalesce_paths from ev_add_map
  multipathd: remove unused configure parameter
  Fix set_no_path_retry() regression
  multipathd: change spurious uevent msg priority
  multipath: print sysfs state in fast list mode

 libmultipath/checkers/tur.c | 66 ++++++++++++++++++++++++++++++++-------------
 libmultipath/discovery.c    |  2 +-
 libmultipath/structs_vec.c  |  5 ++--
 multipath/main.c            |  9 ++++---
 multipathd/main.c           | 55 +++++++++++--------------------------
 5 files changed, 73 insertions(+), 64 deletions(-)

-- 
2.7.4

[PATCH 1/7] multipath: fix tur checker locking

[Benjamin Marzinski]

Commit 6e2423fd fixed a bug where the tur checker could cancel a
detached thread after it had exitted. However in fixing this, the new
code grabbed a mutex (to call condlog) while holding a spin_lock. To
deal with that, and to try to keep with the maixim "lock data, not
code", I've changed how the tur checker synchronizes with its thread.

Now, the tur checker creates joinable threads, and detaches them when
the thread is finished or has timed out. To track the state of the
threads, I've added a new variable to the checker context, ct->attached.
When a thread starts, attached is set to 1. When the thread finishes, it
saves the value of attached, and then zeros it out, while locked. If
attached was set, it detaches itself.

When the tur checker gives up on a thread, it also saves and decrements
ct->attached, while locked. At the same time it saves the value of
ct->thread.  If attached was set, it cancels the thread, and then
detaches it.

So the values that are protected by the spin lock are now ct->holders,
ct->thread, and ct->attached. There are cases where the tur checker just
wants to know if the thread is running. If so, its safe to simply read
ct->thread without locking.  Also, if it knows that the thread isn't
running, it can freely access all of these variables. I've left the
locking in-place in these cases to make the static analyzers happy.
However I have added comments stating when the locking isn't actually
necessary.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/tur.c | 66 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index b4a5cb2..6ae9700 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -46,6 +46,7 @@ struct tur_checker_context {
 	pthread_cond_t active;
 	pthread_spinlock_t hldr_lock;
 	int holders;
+	int attached;
 	char message[CHECKER_MSG_LEN];
 };
 
@@ -98,17 +99,21 @@ void libcheck_free (struct checker * c)
 {
 	if (c->context) {
 		struct tur_checker_context *ct = c->context;
-		int holders;
+		int attached, holders;
 		pthread_t thread;
 
 		pthread_spin_lock(&ct->hldr_lock);
 		ct->holders--;
 		holders = ct->holders;
+		attached = ct->attached;
+		ct->attached = 0;
 		thread = ct->thread;
 		pthread_spin_unlock(&ct->hldr_lock);
-		if (holders)
+		if (attached) {
 			pthread_cancel(thread);
-		else
+			pthread_detach(thread);
+		}
+		if (!holders)
 			cleanup_context(ct);
 		c->context = NULL;
 	}
@@ -218,15 +223,21 @@ retry:
 
 static void cleanup_func(void *data)
 {
-	int holders;
+	int attached, holders;
+	pthread_t thread;
 	struct tur_checker_context *ct = data;
 	pthread_spin_lock(&ct->hldr_lock);
 	ct->holders--;
 	holders = ct->holders;
+	attached = ct->attached;
+	ct->attached = 0;
+	thread = ct->thread;
 	ct->thread = 0;
 	pthread_spin_unlock(&ct->hldr_lock);
 	if (!holders)
 		cleanup_context(ct);
+	if (attached)
+		pthread_detach(thread);
 }
 
 static int tur_running(struct tur_checker_context *ct)
@@ -324,7 +335,8 @@ int libcheck_check(struct checker * c)
 	pthread_attr_t attr;
 	int tur_status, r;
 	char devt[32];
-
+	pthread_t thread;
+	int timed_out, attached;
 
 	if (!ct)
 		return PATH_UNCHECKED;
@@ -350,18 +362,27 @@ int libcheck_check(struct checker * c)
 	}
 
 	if (ct->running) {
-		/*
-		 * Check if TUR checker is still running. Hold hldr_lock
-		 * around the pthread_cancel() call to avoid that
-		 * pthread_cancel() gets called after the (detached) TUR
-		 * thread has exited.
-		 */
-		pthread_spin_lock(&ct->hldr_lock);
-		if (ct->thread) {
-			if (tur_check_async_timeout(c)) {
+		timed_out = tur_check_async_timeout(c);
+		if (timed_out) {
+			pthread_spin_lock(&ct->hldr_lock);
+			attached = ct->attached;
+			ct->attached = 0;
+			thread = ct->thread;
+			pthread_spin_unlock(&ct->hldr_lock);
+			if (attached) {
+				pthread_cancel(thread);
+				pthread_detach(thread);
+			}
+		} else {
+			/* locking here solely to make static analyzers happy */
+			pthread_spin_lock(&ct->hldr_lock);
+			thread = ct->thread;
+			pthread_spin_unlock(&ct->hldr_lock);
+		}
+		if (thread) {
+			if (timed_out) {
 				condlog(3, "%s: tur checker timeout",
 					tur_devt(devt, sizeof(devt), ct));
-				pthread_cancel(ct->thread);
 				ct->running = 0;
 				MSG(c, MSG_TUR_TIMEOUT);
 				tur_status = PATH_TIMEOUT;
@@ -377,9 +398,13 @@ int libcheck_check(struct checker * c)
 			tur_status = ct->state;
 			strlcpy(c->message, ct->message, sizeof(c->message));
 		}
-		pthread_spin_unlock(&ct->hldr_lock);
 		pthread_mutex_unlock(&ct->lock);
 	} else {
+		/*
+		 * locking is necessary here, so that we know that the
+		 * thread finished all access to the context before we
+		 * delare it not running
+		 */
 		if (tur_running(ct)) {
 			/* pthread cancel failed. continue in sync mode */
 			pthread_mutex_unlock(&ct->lock);
@@ -391,19 +416,23 @@ int libcheck_check(struct checker * c)
 		ct->state = PATH_UNCHECKED;
 		ct->fd = c->fd;
 		ct->timeout = c->timeout;
+		/* locking here solely to make static analyzers happy */
 		pthread_spin_lock(&ct->hldr_lock);
 		ct->holders++;
+		ct->attached = 1;
 		pthread_spin_unlock(&ct->hldr_lock);
 		tur_set_async_timeout(c);
-		setup_thread_attr(&attr, 32 * 1024, 1);
+		setup_thread_attr(&attr, 32 * 1024, 0);
 		r = pthread_create(&ct->thread, &attr, tur_thread, ct);
 		pthread_attr_destroy(&attr);
 		if (r) {
+			/* locking here solely to make static analyzers happy */
 			pthread_spin_lock(&ct->hldr_lock);
 			ct->holders--;
+			ct->attached = 0;
+			ct->thread = 0;
 			pthread_spin_unlock(&ct->hldr_lock);
 			pthread_mutex_unlock(&ct->lock);
-			ct->thread = 0;
 			condlog(3, "%s: failed to start tur thread, using"
 				" sync mode", tur_devt(devt, sizeof(devt), ct));
 			return tur_check(c->fd, c->timeout,
@@ -414,6 +443,7 @@ int libcheck_check(struct checker * c)
 		tur_status = ct->state;
 		strlcpy(c->message, ct->message, sizeof(c->message));
 		pthread_mutex_unlock(&ct->lock);
+		/* locking here solely to make static analyzers happy */
 		if (tur_running(ct) &&
 		    (tur_status == PATH_PENDING || tur_status == PATH_UNCHECKED)) {
 			condlog(3, "%s: tur checker still running",
-- 
2.7.4

[PATCH 2/7] multipath: fix DEF_TIMEOUT use

[Benjamin Marzinski]

DEF_TIMEOUT is specified in seconds. The io_hdr timeout is specified in
milliseconds, so we need to convert it. Multipath should be waiting
longer than 30 milliseconds here. If there are concerns that 30 seconds
may be too long, we could always make this configurable, using
conf->checker_timeout if set.

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 4b31dde..f118800 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -766,7 +766,7 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
 	io_hdr.dxferp = resp;
 	io_hdr.cmdp = inqCmdBlk;
 	io_hdr.sbp = sense_b;
-	io_hdr.timeout = DEF_TIMEOUT;
+	io_hdr.timeout = DEF_TIMEOUT * 1000;
 
 	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
 		return -1;
-- 
2.7.4

[PATCH 3/7] multipathd: remove coalesce_paths from ev_add_map

[Benjamin Marzinski]

If ev_add_map is called for a multipath device that doesn't exist in
device-mapper, it will call coalesce_paths to add it.  This doesn't work
and hasn't for years. It doesn't add the map to the mpvec, or start up
waiters, or do any of the necessary things that do get done when you
call ev_add_map for a map that does exist in device mapper.

Fortunately, there are only two things that call ev_add_map. uev_add_map
makes sure that the device does exist in device-mapper before calling
ev_add_map, and cli_add_map creates the device first and then calls
ev_add_map, if the device doesn't exist.

So, there is no reason for coalesce_paths to be in ev_add_map. This
removes it.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 27cf234..cf76241 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -415,15 +415,11 @@ uev_add_map (struct uevent * uev, struct vectors * vecs)
 int
 ev_add_map (char * dev, char * alias, struct vectors * vecs)
 {
-	char * refwwid;
 	struct multipath * mpp;
-	int map_present;
-	int r = 1, delayed_reconfig, reassign_maps;
+	int delayed_reconfig, reassign_maps;
 	struct config *conf;
 
-	map_present = dm_map_present(alias);
-
-	if (map_present && !dm_is_mpath(alias)) {
+	if (!dm_is_mpath(alias)) {
 		condlog(4, "%s: not a multipath map", alias);
 		return 0;
 	}
@@ -468,33 +464,14 @@ ev_add_map (char * dev, char * alias, struct vectors * vecs)
 	/*
 	 * now we can register the map
 	 */
-	if (map_present) {
-		if ((mpp = add_map_without_path(vecs, alias))) {
-			sync_map_state(mpp);
-			condlog(2, "%s: devmap %s registered", alias, dev);
-			return 0;
-		} else {
-			condlog(2, "%s: uev_add_map failed", dev);
-			return 1;
-		}
-	}
-	r = get_refwwid(CMD_NONE, dev, DEV_DEVMAP, vecs->pathvec, &refwwid);
-
-	if (refwwid) {
-		r = coalesce_paths(vecs, NULL, refwwid, FORCE_RELOAD_NONE,
-				   CMD_NONE);
-		dm_lib_release();
+	if ((mpp = add_map_without_path(vecs, alias))) {
+		sync_map_state(mpp);
+		condlog(2, "%s: devmap %s registered", alias, dev);
+		return 0;
+	} else {
+		condlog(2, "%s: uev_add_map failed", dev);
+		return 1;
 	}
-
-	if (!r)
-		condlog(2, "%s: devmap %s added", alias, dev);
-	else if (r == 2)
-		condlog(2, "%s: uev_add_map %s blacklisted", alias, dev);
-	else
-		condlog(0, "%s: uev_add_map %s failed", alias, dev);
-
-	FREE(refwwid);
-	return r;
 }
 
 static int
-- 
2.7.4

[PATCH 4/7] multipathd: remove unused configure parameter

[Benjamin Marzinski]

configure() is always called with start_waiters=1, so there is no point
in having the parameter. Remove it.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index cf76241..233d09f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1950,7 +1950,7 @@ checkerloop (void *ap)
 }
 
 int
-configure (struct vectors * vecs, int start_waiters)
+configure (struct vectors * vecs)
 {
 	struct multipath * mpp;
 	struct path * pp;
@@ -2049,11 +2049,9 @@ configure (struct vectors * vecs, int start_waiters)
 			i--;
 			continue;
 		}
-		if (start_waiters) {
-			if (start_waiter_thread(mpp, vecs)) {
-				remove_map(mpp, vecs, 1);
-				i--;
-			}
+		if (start_waiter_thread(mpp, vecs)) {
+			remove_map(mpp, vecs, 1);
+			i--;
 		}
 	}
 	return 0;
@@ -2120,7 +2118,7 @@ reconfigure (struct vectors * vecs)
 	rcu_assign_pointer(multipath_conf, conf);
 	call_rcu(&old->rcu, rcu_free_config);
 
-	configure(vecs, 1);
+	configure(vecs);
 
 
 	return 0;
-- 
2.7.4

[PATCH 5/7] Fix set_no_path_retry() regression

[Benjamin Marzinski]

commit 0f850db7fceb6b2bf4968f3831efd250c17c6138 "multipathd: clean up
set_no_path_retry" has a bug in it. It made set_no_path_retry
never reset mpp->retry_ticks, even if the device was in recovery mode,
and there were valid paths. This meant that adding new paths didn't
remove a device from recovery mode, and queueing could get disabled,
even while there were valid paths. This patch fixes that.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index fbab61f..0de2221 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -343,9 +343,10 @@ static void set_no_path_retry(struct multipath *mpp)
 			dm_queue_if_no_path(mpp->alias, 1);
 		break;
 	default:
-		if (mpp->nr_active > 0)
+		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)
+		} else if (is_queueing && mpp->retry_tick == 0)
 			enter_recovery_mode(mpp);
 		break;
 	}
-- 
2.7.4

[PATCH 6/7] multipathd: change spurious uevent msg priority

[Benjamin Marzinski]

The "spurious uevent, path already in pathvec" is not anything to worry
about, so it should not have the error priority.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 233d09f..cfd0363 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -557,7 +557,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	if (pp) {
 		int r;
 
-		condlog(0, "%s: spurious uevent, path already in pathvec",
+		condlog(2, "%s: spurious uevent, path already in pathvec",
 			uev->kernel);
 		if (!pp->mpp && !strlen(pp->wwid)) {
 			condlog(3, "%s: reinitialize path", uev->kernel);
-- 
2.7.4

[PATCH 7/7] multipath: print sysfs state in fast list mode

[Benjamin Marzinski]

commit b123e711ea2a4b471a98ff5e26815df3773636b5 "libmultipath: cleanup
orphan device states" stopped multipathd from showing old state for
orphan paths by checking if pp->mpp was set, and only printing the state
if it was.   Unfortunately, when "multipath -l" is run, pp->mpp isn't
set. While the checker state isn't checked and shouldn't be printed with
"-l", the sysfs state can be, and was before b123e711. This patch sets
pp->mpp in fast list mode, so that the sysfs state gets printed.

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

diff --git a/multipath/main.c b/multipath/main.c
index bffe065..1799fd5 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -149,7 +149,7 @@ usage (char * progname)
 }
 
 static int
-update_paths (struct multipath * mpp)
+update_paths (struct multipath * mpp, int quick)
 {
 	int i, j;
 	struct pathgroup * pgp;
@@ -164,6 +164,10 @@ update_paths (struct multipath * mpp)
 			continue;
 
 		vector_foreach_slot (pgp->paths, pp, j) {
+			if (quick) {
+				pp->mpp = mpp;
+				continue;
+			}
 			if (!strlen(pp->dev)) {
 				if (devt2devname(pp->dev, FILE_NAME_SIZE,
 						 pp->dev_t)) {
@@ -238,8 +242,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 		 * If not in "fast list mode", we need to fetch information
 		 * about them
 		 */
-		if (cmd != CMD_LIST_SHORT)
-			update_paths(mpp);
+		update_paths(mpp, (cmd == CMD_LIST_SHORT));
 
 		if (cmd == CMD_LIST_LONG)
 			mpp->bestpg = select_path_group(mpp);
-- 
2.7.4

Re: [PATCH 1/7] multipath: fix tur checker locking

[Martin Wilck]

Hello Ben,

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> Commit 6e2423fd fixed a bug where the tur checker could cancel a
> detached thread after it had exitted. However in fixing this, the new
> code grabbed a mutex (to call condlog) while holding a spin_lock. To
> deal with that, and to try to keep with the maixim "lock data, not
> code", I've changed how the tur checker synchronizes with its thread.

Hmm, if it was only for the condlog issue, it'd certainly be possible
to find a simpler solution (just move the log message out of the spin-
locked code). And please explain some more why your patch implements
"lock data not code" better than what we did before - it's not obvious
to me.

> Now, the tur checker creates joinable threads, and detaches them when
> the thread is finished or has timed out. To track the state of the
> threads, I've added a new variable to the checker context, ct-
> >attached.

Hmm, again. This adds more complexity to an already complex contruct,
because now we don't know in the first place whether the checker thread
is joinable or detached. IMO it makes a lot of sense for the checker to
run in detached mode right from the start. If multipathd is terminated
while checkers are blocked, it'll have to detach these threads in the
process of terminating - I find that a bit weird.

While I didn't spot obvious errors in your patch, changing the locking
fundamentally is always risky to some extent, and I'm not yet convinced
that the problem you're trying to solve justifies this risk.

> When a thread starts, attached is set to 1. When the thread finishes,
> it
> saves the value of attached, and then zeros it out, while locked. If
> attached was set, it detaches itself.

Why aren't you simply using pthread_attr_getdetachstate()?

> 
> When the tur checker gives up on a thread, it also saves and
> decrements
> ct->attached, while locked. At the same time it saves the value of
> ct->thread.  If attached was set, it cancels the thread, and then
> detaches it.

Have you thought about using uatomic_add_return(), which we have
available anyway through liburcu?
https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md

We might actually be able to get rid of the hldr_lock altogether, which
would solve the initial spin_lock/mutex problem without any additional
code.

Some minor remarks below,
Regards, Martin

> So the values that are protected by the spin lock are now ct-
> >holders,
> ct->thread, and ct->attached. There are cases where the tur checker
> just
> wants to know if the thread is running. If so, its safe to simply
> read
> ct->thread without locking.  Also, if it knows that the thread isn't
> running, it can freely access all of these variables. I've left the
> locking in-place in these cases to make the static analyzers happy.
> However I have added comments stating when the locking isn't actually
> necessary.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/checkers/tur.c | 66 ++++++++++++++++++++++++++++++++---
> ----------
>  1 file changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/libmultipath/checkers/tur.c
> b/libmultipath/checkers/tur.c
> index b4a5cb2..6ae9700 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -46,6 +46,7 @@ struct tur_checker_context {
>  	pthread_cond_t active;
>  	pthread_spinlock_t hldr_lock;
>  	int holders;
> +	int attached;
>  	char message[CHECKER_MSG_LEN];
>  };
>  
> @@ -98,17 +99,21 @@ void libcheck_free (struct checker * c)
>  {
>  	if (c->context) {
>  		struct tur_checker_context *ct = c->context;
> -		int holders;
> +		int attached, holders;
>  		pthread_t thread;
>  
>  		pthread_spin_lock(&ct->hldr_lock);
>  		ct->holders--;
>  		holders = ct->holders;
> +		attached = ct->attached;
> +		ct->attached = 0;
>  		thread = ct->thread;
>  		pthread_spin_unlock(&ct->hldr_lock);
> -		if (holders)
> +		if (attached) {
>  			pthread_cancel(thread);
> -		else
> +			pthread_detach(thread);
> +		}
> +		if (!holders)
>  			cleanup_context(ct);
>  		c->context = NULL;
>  	}
> @@ -218,15 +223,21 @@ retry:
>  
>  static void cleanup_func(void *data)
>  {
> -	int holders;
> +	int attached, holders;
> +	pthread_t thread;
>  	struct tur_checker_context *ct = data;
>  	pthread_spin_lock(&ct->hldr_lock);
>  	ct->holders--;
>  	holders = ct->holders;
> +	attached = ct->attached;
> +	ct->attached = 0;
> +	thread = ct->thread;
>  	ct->thread = 0;
>  	pthread_spin_unlock(&ct->hldr_lock);
>  	if (!holders)
>  		cleanup_context(ct);
> +	if (attached)
> +		pthread_detach(thread);
>  }
>  
>  static int tur_running(struct tur_checker_context *ct)
> @@ -324,7 +335,8 @@ int libcheck_check(struct checker * c)
>  	pthread_attr_t attr;
>  	int tur_status, r;
>  	char devt[32];
> -
> +	pthread_t thread;
> +	int timed_out, attached;
>  
>  	if (!ct)
>  		return PATH_UNCHECKED;
> @@ -350,18 +362,27 @@ int libcheck_check(struct checker * c)
>  	}
>  
>  	if (ct->running) {
> -		/*
> -		 * Check if TUR checker is still running. Hold
> hldr_lock
> -		 * around the pthread_cancel() call to avoid that
> -		 * pthread_cancel() gets called after the (detached)
> TUR
> -		 * thread has exited.
> -		 */
> -		pthread_spin_lock(&ct->hldr_lock);
> -		if (ct->thread) {
> -			if (tur_check_async_timeout(c)) {
> +		timed_out = tur_check_async_timeout(c);
> +		if (timed_out) {
> +			pthread_spin_lock(&ct->hldr_lock);
> +			attached = ct->attached;
> +			ct->attached = 0;
> +			thread = ct->thread;
> +			pthread_spin_unlock(&ct->hldr_lock);
> +			if (attached) {
> +				pthread_cancel(thread);
> +				pthread_detach(thread);
> +			}
> +		} else {
> +			/* locking here solely to make static
> analyzers happy */

Out of curiosity: which analyzers have you been using?

> +			pthread_spin_lock(&ct->hldr_lock);
> +			thread = ct->thread;
> +			pthread_spin_unlock(&ct->hldr_lock);
> +		}
> +		if (thread) {
> +			if (timed_out) {
>  				condlog(3, "%s: tur checker
> timeout",
>  					tur_devt(devt, sizeof(devt),
> ct));
> -				pthread_cancel(ct->thread);
>  				ct->running = 0;
>  				MSG(c, MSG_TUR_TIMEOUT);
>  				tur_status = PATH_TIMEOUT;
> @@ -377,9 +398,13 @@ int libcheck_check(struct checker * c)
>  			tur_status = ct->state;
>  			strlcpy(c->message, ct->message, sizeof(c-
> >message));
>  		}
> -		pthread_spin_unlock(&ct->hldr_lock);
>  		pthread_mutex_unlock(&ct->lock);
>  	} else {
> +		/*
> +		 * locking is necessary here, so that we know that
> the
> +		 * thread finished all access to the context before
> we
> +		 * delare it not running
> +		 */
>  		if (tur_running(ct)) {
>  			/* pthread cancel failed. continue in sync
> mode */
>  			pthread_mutex_unlock(&ct->lock);
> @@ -391,19 +416,23 @@ int libcheck_check(struct checker * c)
>  		ct->state = PATH_UNCHECKED;
>  		ct->fd = c->fd;
>  		ct->timeout = c->timeout;
> +		/* locking here solely to make static analyzers
> happy */
>  		pthread_spin_lock(&ct->hldr_lock);
>  		ct->holders++;
> +		ct->attached = 1;
>  		pthread_spin_unlock(&ct->hldr_lock);
>  		tur_set_async_timeout(c);
> -		setup_thread_attr(&attr, 32 * 1024, 1);
> +		setup_thread_attr(&attr, 32 * 1024, 0);
>  		r = pthread_create(&ct->thread, &attr, tur_thread,
> ct);
>  		pthread_attr_destroy(&attr);
>  		if (r) {
> +			/* locking here solely to make static
> analyzers happy */
>  			pthread_spin_lock(&ct->hldr_lock);
>  			ct->holders--;
> +			ct->attached = 0;
> +			ct->thread = 0;
>  			pthread_spin_unlock(&ct->hldr_lock);
>  			pthread_mutex_unlock(&ct->lock);
> -			ct->thread = 0;
>  			condlog(3, "%s: failed to start tur thread, 

This part (moving ct->thread =0 into the cricital section) is a bug
fix.


> using"
>  				" sync mode", tur_devt(devt,
> sizeof(devt), ct));
>  			return tur_check(c->fd, c->timeout,
> @@ -414,6 +443,7 @@ int libcheck_check(struct checker * c)
>  		tur_status = ct->state;
>  		strlcpy(c->message, ct->message, sizeof(c-
> >message));
>  		pthread_mutex_unlock(&ct->lock);
> +		/* locking here solely to make static analyzers
> happy */
>  		if (tur_running(ct) &&
>  		    (tur_status == PATH_PENDING || tur_status ==
> PATH_UNCHECKED)) {
>  			condlog(3, "%s: tur checker still running",

-- 
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/7] multipath: fix DEF_TIMEOUT use

[Martin Wilck]

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> DEF_TIMEOUT is specified in seconds. The io_hdr timeout is specified
> in
> milliseconds, so we need to convert it. Multipath should be waiting
> longer than 30 milliseconds here. If there are concerns that 30
> seconds
> may be too long, we could always make this configurable, using
> conf->checker_timeout if set.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 4b31dde..f118800 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -766,7 +766,7 @@ do_inq(int sg_fd, int cmddt, int evpd, unsigned
> int pg_op,
>  	io_hdr.dxferp = resp;
>  	io_hdr.cmdp = inqCmdBlk;
>  	io_hdr.sbp = sense_b;
> -	io_hdr.timeout = DEF_TIMEOUT;
> +	io_hdr.timeout = DEF_TIMEOUT * 1000;
>  
>  	if (ioctl(sg_fd, SG_IO, &io_hdr) < 0)
>  		return -1;

-- 
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/7] multipathd: remove coalesce_paths from ev_add_map

[Martin Wilck]

Hi Ben,

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> If ev_add_map is called for a multipath device that doesn't exist in
> device-mapper, it will call coalesce_paths to add it.  This doesn't
> work
> and hasn't for years. It doesn't add the map to the mpvec, or start
> up
> waiters, or do any of the necessary things that do get done when you
> call ev_add_map for a map that does exist in device mapper.
> 
> Fortunately, there are only two things that call ev_add_map.
> uev_add_map
> makes sure that the device does exist in device-mapper before calling
> ev_add_map, and cli_add_map creates the device first and then calls
> ev_add_map, if the device doesn't exist.
> 
> So, there is no reason for coalesce_paths to be in ev_add_map. This
> removes it.

Basically ACK, but see below.

I'd recommend to add a comment that ev_add_map() expects the map to be
set up already, and that callers need to take care of that.

> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 41 +++++++++--------------------------------
>  1 file changed, 9 insertions(+), 32 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 27cf234..cf76241 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -415,15 +415,11 @@ uev_add_map (struct uevent * uev, struct
> vectors * vecs)
>  int
>  ev_add_map (char * dev, char * alias, struct vectors * vecs)
>  {
> -	char * refwwid;
>  	struct multipath * mpp;
> -	int map_present;
> -	int r = 1, delayed_reconfig, reassign_maps;
> +	int delayed_reconfig, reassign_maps;
>  	struct config *conf;
>  
> -	map_present = dm_map_present(alias);
> -
> -	if (map_present && !dm_is_mpath(alias)) {
> +	if (!dm_is_mpath(alias)) {
>  		condlog(4, "%s: not a multipath map", alias);
>  		return 0;
>  	}
> @@ -468,33 +464,14 @@ ev_add_map (char * dev, char * alias, struct
> vectors * vecs)
>  	/*
>  	 * now we can register the map
>  	 */
> -	if (map_present) {
> -		if ((mpp = add_map_without_path(vecs, alias))) {
> -			sync_map_state(mpp);
> -			condlog(2, "%s: devmap %s registered",
> alias, dev);
> -			return 0;
> -		} else {
> -			condlog(2, "%s: uev_add_map failed", dev);
> -			return 1;
> -		}
> -	}
> -	r = get_refwwid(CMD_NONE, dev, DEV_DEVMAP, vecs->pathvec,
> &refwwid);
> -
> -	if (refwwid) {
> -		r = coalesce_paths(vecs, NULL, refwwid,
> FORCE_RELOAD_NONE,
> -				   CMD_NONE);
> -		dm_lib_release();
> +	if ((mpp = add_map_without_path(vecs, alias))) {
> +		sync_map_state(mpp);
> +		condlog(2, "%s: devmap %s registered", alias, dev);
> +		return 0;

This is unrelated to your patch, but add_map_without_path() calls
setup_map() and domap() without checking whether that's necessary.
That seems to be an odd thing to do when an uevent for existing map is
received. We should be adjusting multipathd's internal data structures
with the data from the kernel, not vice versa.

While working on this, we may want to fix that, too.

> +	} else {
> +		condlog(2, "%s: uev_add_map failed", dev);
> +		return 1;
>  	}

While you're at it, please fix this log message ("uev_add_map").

Martin

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

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

Re: [PATCH 4/7] multipathd: remove unused configure parameter

[Martin Wilck]

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> configure() is always called with start_waiters=1, so there is no
> point
> in having the parameter. Remove it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

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

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

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

Re: [PATCH 5/7] Fix set_no_path_retry() regression

[Martin Wilck]

Hi Ben,

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> commit 0f850db7fceb6b2bf4968f3831efd250c17c6138 "multipathd: clean up
> set_no_path_retry" has a bug in it. It made set_no_path_retry
> never reset mpp->retry_ticks, even if the device was in recovery
> mode,
> and there were valid paths. This meant that adding new paths didn't
> remove a device from recovery mode, and queueing could get disabled,
> even while there were valid paths. This patch fixes that.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/structs_vec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index fbab61f..0de2221 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -343,9 +343,10 @@ static void set_no_path_retry(struct multipath
> *mpp)
>  			dm_queue_if_no_path(mpp->alias, 1);
>  		break;
>  	default:
> -		if (mpp->nr_active > 0)
> +		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)
> +		} else if (is_queueing && mpp->retry_tick == 0)
>  			enter_recovery_mode(mpp);
>  		break;
>  	}

Please explain why it's sufficient to do this in the "default" case
only. Before 0f850db7, set_no_path_retry() reset retry_tick for any
value of no_path_retry.

Martin

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

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

Re: [PATCH 6/7] multipathd: change spurious uevent msg priority

[Martin Wilck]

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> The "spurious uevent, path already in pathvec" is not anything to
> worry
> about, so it should not have the error priority.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

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

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

Re: [PATCH 7/7] multipath: print sysfs state in fast list mode

[Martin Wilck]

Hi Ben,

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> commit b123e711ea2a4b471a98ff5e26815df3773636b5 "libmultipath:
> cleanup
> orphan device states" stopped multipathd from showing old state for
> orphan paths by checking if pp->mpp was set, and only printing the
> state
> if it was.   Unfortunately, when "multipath -l" is run, pp->mpp isn't
> set. While the checker state isn't checked and shouldn't be printed
> with
> "-l", the sysfs state can be, and was before b123e711. This patch
> sets
> pp->mpp in fast list mode, so that the sysfs state gets printed.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipath/main.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index bffe065..1799fd5 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -149,7 +149,7 @@ usage (char * progname)
>  }
>  
>  static int
> -update_paths (struct multipath * mpp)
> +update_paths (struct multipath * mpp, int quick)
>  {
>  	int i, j;
>  	struct pathgroup * pgp;
> @@ -164,6 +164,10 @@ update_paths (struct multipath * mpp)
>  			continue;
>  
>  		vector_foreach_slot (pgp->paths, pp, j) {
> +			if (quick) {
> +				pp->mpp = mpp;
> +				continue;
> +			}
>  			if (!strlen(pp->dev)) {
>  				if (devt2devname(pp->dev, 

Shouldn't you check whether the paths are still present in sysfs, like
the code for the general case (quick == false) does?

Regards
Martin

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

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

Re: [PATCH 1/7] multipath: fix tur checker locking

[Benjamin Marzinski]

On Thu, Feb 08, 2018 at 09:49:20AM +0100, Martin Wilck wrote:
> Hello Ben,
> 
> On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > Commit 6e2423fd fixed a bug where the tur checker could cancel a
> > detached thread after it had exitted. However in fixing this, the new
> > code grabbed a mutex (to call condlog) while holding a spin_lock. To
> > deal with that, and to try to keep with the maixim "lock data, not
> > code", I've changed how the tur checker synchronizes with its thread.
> 
> Hmm, if it was only for the condlog issue, it'd certainly be possible
> to find a simpler solution (just move the log message out of the spin-
> locked code). And please explain some more why your patch implements
> "lock data not code" better than what we did before - it's not obvious
> to me.

The way I view spin_locks is that they hold up a processor, so they
should only be locking a defined set of shared resources, and making
sure that they move from a defined state to another defined state
correctly.  The existing code calls pthread_cancel(),
tur_check_async_timeout(), and condlog() in the spinlock, and only
condlog() can be removed from it. While I don't believe that
pthread_cancel() and tur_check_async_timeout() can block, they certainly
don't need to be run under a spin_lock.

My code just makes sure that holders, thread, and attached are updated
atomically.

> 
> > Now, the tur checker creates joinable threads, and detaches them when
> > the thread is finished or has timed out. To track the state of the
> > threads, I've added a new variable to the checker context, ct-
> > >attached.
> 
> Hmm, again. This adds more complexity to an already complex contruct,
> because now we don't know in the first place whether the checker thread
> is joinable or detached. IMO it makes a lot of sense for the checker to
> run in detached mode right from the start. If multipathd is terminated
> while checkers are blocked, it'll have to detach these threads in the
> process of terminating - I find that a bit weird.

I'm not sure I get what's weird here. Calling pthread_cancel() and then
pthread_detach() is commonly recommended online as an alternative to
calling pthread_cancel() and then pthread_join() if you don't want to
wait for the results.

> While I didn't spot obvious errors in your patch, changing the locking
> fundamentally is always risky to some extent, and I'm not yet convinced
> that the problem you're trying to solve justifies this risk.
> 
> > When a thread starts, attached is set to 1. When the thread finishes,
> > it
> > saves the value of attached, and then zeros it out, while locked. If
> > attached was set, it detaches itself.
> 
> Why aren't you simply using pthread_attr_getdetachstate()?

I'm not sure how you would avoid races if you did this. ct->attach gets
set to 0 inside the spinlock to notify the other side that they don't
have to do the detach.  If you just check if the thread is currently
detached, without being able to atomically noitify the other side that
you are going to do the detach, what's to stop both sides from checking
at the same time and both doing the detach? Am I missing something here?
 
> > 
> > When the tur checker gives up on a thread, it also saves and
> > decrements
> > ct->attached, while locked. At the same time it saves the value of
> > ct->thread.  If attached was set, it cancels the thread, and then
> > detaches it.
> 
> Have you thought about using uatomic_add_return(), which we have
> available anyway through liburcu?
> https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
> 
> We might actually be able to get rid of the hldr_lock altogether, which
> would solve the initial spin_lock/mutex problem without any additional
> code.

No, I haven't. I can look into that.

> Some minor remarks below,
> Regards, Martin
> 
> > So the values that are protected by the spin lock are now ct-
> > >holders,
> > ct->thread, and ct->attached. There are cases where the tur checker
> > just
> > wants to know if the thread is running. If so, its safe to simply
> > read
> > ct->thread without locking.  Also, if it knows that the thread isn't
> > running, it can freely access all of these variables. I've left the
> > locking in-place in these cases to make the static analyzers happy.
> > However I have added comments stating when the locking isn't actually
> > necessary.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/checkers/tur.c | 66 ++++++++++++++++++++++++++++++++---
> > ----------
> >  1 file changed, 48 insertions(+), 18 deletions(-)
> > 
> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index b4a5cb2..6ae9700 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -46,6 +46,7 @@ struct tur_checker_context {
> >  	pthread_cond_t active;
> >  	pthread_spinlock_t hldr_lock;
> >  	int holders;
> > +	int attached;
> >  	char message[CHECKER_MSG_LEN];
> >  };
> >  
> > @@ -98,17 +99,21 @@ void libcheck_free (struct checker * c)
> >  {
> >  	if (c->context) {
> >  		struct tur_checker_context *ct = c->context;
> > -		int holders;
> > +		int attached, holders;
> >  		pthread_t thread;
> >  
> >  		pthread_spin_lock(&ct->hldr_lock);
> >  		ct->holders--;
> >  		holders = ct->holders;
> > +		attached = ct->attached;
> > +		ct->attached = 0;
> >  		thread = ct->thread;
> >  		pthread_spin_unlock(&ct->hldr_lock);
> > -		if (holders)
> > +		if (attached) {
> >  			pthread_cancel(thread);
> > -		else
> > +			pthread_detach(thread);
> > +		}
> > +		if (!holders)
> >  			cleanup_context(ct);
> >  		c->context = NULL;
> >  	}
> > @@ -218,15 +223,21 @@ retry:
> >  
> >  static void cleanup_func(void *data)
> >  {
> > -	int holders;
> > +	int attached, holders;
> > +	pthread_t thread;
> >  	struct tur_checker_context *ct = data;
> >  	pthread_spin_lock(&ct->hldr_lock);
> >  	ct->holders--;
> >  	holders = ct->holders;
> > +	attached = ct->attached;
> > +	ct->attached = 0;
> > +	thread = ct->thread;
> >  	ct->thread = 0;
> >  	pthread_spin_unlock(&ct->hldr_lock);
> >  	if (!holders)
> >  		cleanup_context(ct);
> > +	if (attached)
> > +		pthread_detach(thread);
> >  }
> >  
> >  static int tur_running(struct tur_checker_context *ct)
> > @@ -324,7 +335,8 @@ int libcheck_check(struct checker * c)
> >  	pthread_attr_t attr;
> >  	int tur_status, r;
> >  	char devt[32];
> > -
> > +	pthread_t thread;
> > +	int timed_out, attached;
> >  
> >  	if (!ct)
> >  		return PATH_UNCHECKED;
> > @@ -350,18 +362,27 @@ int libcheck_check(struct checker * c)
> >  	}
> >  
> >  	if (ct->running) {
> > -		/*
> > -		 * Check if TUR checker is still running. Hold
> > hldr_lock
> > -		 * around the pthread_cancel() call to avoid that
> > -		 * pthread_cancel() gets called after the (detached)
> > TUR
> > -		 * thread has exited.
> > -		 */
> > -		pthread_spin_lock(&ct->hldr_lock);
> > -		if (ct->thread) {
> > -			if (tur_check_async_timeout(c)) {
> > +		timed_out = tur_check_async_timeout(c);
> > +		if (timed_out) {
> > +			pthread_spin_lock(&ct->hldr_lock);
> > +			attached = ct->attached;
> > +			ct->attached = 0;
> > +			thread = ct->thread;
> > +			pthread_spin_unlock(&ct->hldr_lock);
> > +			if (attached) {
> > +				pthread_cancel(thread);
> > +				pthread_detach(thread);
> > +			}
> > +		} else {
> > +			/* locking here solely to make static
> > analyzers happy */
> 
> Out of curiosity: which analyzers have you been using?

You would have to ask Bart, who I forgot to CC on this patch. He added
the code to always read ct->thread in a spin_lock, and said it was to
make an analyzer happy.
 
> > +			pthread_spin_lock(&ct->hldr_lock);
> > +			thread = ct->thread;
> > +			pthread_spin_unlock(&ct->hldr_lock);
> > +		}
> > +		if (thread) {
> > +			if (timed_out) {
> >  				condlog(3, "%s: tur checker
> > timeout",
> >  					tur_devt(devt, sizeof(devt),
> > ct));
> > -				pthread_cancel(ct->thread);
> >  				ct->running = 0;
> >  				MSG(c, MSG_TUR_TIMEOUT);
> >  				tur_status = PATH_TIMEOUT;
> > @@ -377,9 +398,13 @@ int libcheck_check(struct checker * c)
> >  			tur_status = ct->state;
> >  			strlcpy(c->message, ct->message, sizeof(c-
> > >message));
> >  		}
> > -		pthread_spin_unlock(&ct->hldr_lock);
> >  		pthread_mutex_unlock(&ct->lock);
> >  	} else {
> > +		/*
> > +		 * locking is necessary here, so that we know that
> > the
> > +		 * thread finished all access to the context before
> > we
> > +		 * delare it not running
> > +		 */
> >  		if (tur_running(ct)) {
> >  			/* pthread cancel failed. continue in sync
> > mode */
> >  			pthread_mutex_unlock(&ct->lock);
> > @@ -391,19 +416,23 @@ int libcheck_check(struct checker * c)
> >  		ct->state = PATH_UNCHECKED;
> >  		ct->fd = c->fd;
> >  		ct->timeout = c->timeout;
> > +		/* locking here solely to make static analyzers
> > happy */
> >  		pthread_spin_lock(&ct->hldr_lock);
> >  		ct->holders++;
> > +		ct->attached = 1;
> >  		pthread_spin_unlock(&ct->hldr_lock);
> >  		tur_set_async_timeout(c);
> > -		setup_thread_attr(&attr, 32 * 1024, 1);
> > +		setup_thread_attr(&attr, 32 * 1024, 0);
> >  		r = pthread_create(&ct->thread, &attr, tur_thread,
> > ct);
> >  		pthread_attr_destroy(&attr);
> >  		if (r) {
> > +			/* locking here solely to make static
> > analyzers happy */
> >  			pthread_spin_lock(&ct->hldr_lock);
> >  			ct->holders--;
> > +			ct->attached = 0;
> > +			ct->thread = 0;
> >  			pthread_spin_unlock(&ct->hldr_lock);
> >  			pthread_mutex_unlock(&ct->lock);
> > -			ct->thread = 0;
> >  			condlog(3, "%s: failed to start tur thread, 
> 
> This part (moving ct->thread =0 into the cricital section) is a bug
> fix.

Well, we've already determined that another thread wasn't running, and
then we failed to start a new thread, so really all the locking here is
unnecessary, but I agree that we should either lock all the variables or
none of them.

> 
> > using"
> >  				" sync mode", tur_devt(devt,
> > sizeof(devt), ct));
> >  			return tur_check(c->fd, c->timeout,
> > @@ -414,6 +443,7 @@ int libcheck_check(struct checker * c)
> >  		tur_status = ct->state;
> >  		strlcpy(c->message, ct->message, sizeof(c-
> > >message));
> >  		pthread_mutex_unlock(&ct->lock);
> > +		/* locking here solely to make static analyzers
> > happy */
> >  		if (tur_running(ct) &&
> >  		    (tur_status == PATH_PENDING || tur_status ==
> > PATH_UNCHECKED)) {
> >  			condlog(3, "%s: tur checker still running",
> 
> -- 
> 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 3/7] multipathd: remove coalesce_paths from ev_add_map

[Benjamin Marzinski]

On Thu, Feb 08, 2018 at 10:10:38AM +0100, Martin Wilck wrote:
> Hi Ben,
> 
> On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > If ev_add_map is called for a multipath device that doesn't exist in
> > device-mapper, it will call coalesce_paths to add it.  This doesn't
> > work
> > and hasn't for years. It doesn't add the map to the mpvec, or start
> > up
> > waiters, or do any of the necessary things that do get done when you
> > call ev_add_map for a map that does exist in device mapper.
> > 
> > Fortunately, there are only two things that call ev_add_map.
> > uev_add_map
> > makes sure that the device does exist in device-mapper before calling
> > ev_add_map, and cli_add_map creates the device first and then calls
> > ev_add_map, if the device doesn't exist.
> > 
> > So, there is no reason for coalesce_paths to be in ev_add_map. This
> > removes it.
> 
> Basically ACK, but see below.
> 
> I'd recommend to add a comment that ev_add_map() expects the map to be
> set up already, and that callers need to take care of that.

Sure.
 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  multipathd/main.c | 41 +++++++++--------------------------------
> >  1 file changed, 9 insertions(+), 32 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 27cf234..cf76241 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -415,15 +415,11 @@ uev_add_map (struct uevent * uev, struct
> > vectors * vecs)
> >  int
> >  ev_add_map (char * dev, char * alias, struct vectors * vecs)
> >  {
> > -	char * refwwid;
> >  	struct multipath * mpp;
> > -	int map_present;
> > -	int r = 1, delayed_reconfig, reassign_maps;
> > +	int delayed_reconfig, reassign_maps;
> >  	struct config *conf;
> >  
> > -	map_present = dm_map_present(alias);
> > -
> > -	if (map_present && !dm_is_mpath(alias)) {
> > +	if (!dm_is_mpath(alias)) {
> >  		condlog(4, "%s: not a multipath map", alias);
> >  		return 0;
> >  	}
> > @@ -468,33 +464,14 @@ ev_add_map (char * dev, char * alias, struct
> > vectors * vecs)
> >  	/*
> >  	 * now we can register the map
> >  	 */
> > -	if (map_present) {
> > -		if ((mpp = add_map_without_path(vecs, alias))) {
> > -			sync_map_state(mpp);
> > -			condlog(2, "%s: devmap %s registered",
> > alias, dev);
> > -			return 0;
> > -		} else {
> > -			condlog(2, "%s: uev_add_map failed", dev);
> > -			return 1;
> > -		}
> > -	}
> > -	r = get_refwwid(CMD_NONE, dev, DEV_DEVMAP, vecs->pathvec,
> > &refwwid);
> > -
> > -	if (refwwid) {
> > -		r = coalesce_paths(vecs, NULL, refwwid,
> > FORCE_RELOAD_NONE,
> > -				   CMD_NONE);
> > -		dm_lib_release();
> > +	if ((mpp = add_map_without_path(vecs, alias))) {
> > +		sync_map_state(mpp);
> > +		condlog(2, "%s: devmap %s registered", alias, dev);
> > +		return 0;
> 
> This is unrelated to your patch, but add_map_without_path() calls
> setup_map() and domap() without checking whether that's necessary.
> That seems to be an odd thing to do when an uevent for existing map is
> received. We should be adjusting multipathd's internal data structures
> with the data from the kernel, not vice versa.

I'm not convinced that this is correct. First, as soon as another path
is added or removed, or multipathd is reconfigured, we will reset the
device to the way that multipathd thinks it should be. Second multipathd
might really know better how to set up a device right away. For
instance, multipath may not know about some paths because they
currently are down, but multipathd will know that they belong to the
device and adopt them in setup_map(). 
 
> While working on this, we may want to fix that, too.
> 
> > +	} else {
> > +		condlog(2, "%s: uev_add_map failed", dev);
> > +		return 1;
> >  	}
> 
> While you're at it, please fix this log message ("uev_add_map").

Sure.
 
> 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)

Re: [PATCH 1/7] multipath: fix tur checker locking

[Bart Van Assche]

On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> Commit 6e2423fd fixed a bug where the tur checker could cancel a
> detached thread after it had exitted. However in fixing this, the new
> code grabbed a mutex (to call condlog) while holding a spin_lock. To
> deal with that, and to try to keep with the maixim "lock data, not
> code", I've changed how the tur checker synchronizes with its thread.
> 
> Now, the tur checker creates joinable threads, and detaches them when
> the thread is finished or has timed out. To track the state of the
> threads, I've added a new variable to the checker context, ct->attached.
> When a thread starts, attached is set to 1. When the thread finishes, it
> saves the value of attached, and then zeros it out, while locked. If
> attached was set, it detaches itself.
> 
> When the tur checker gives up on a thread, it also saves and decrements
> ct->attached, while locked. At the same time it saves the value of
> ct->thread.  If attached was set, it cancels the thread, and then
> detaches it.
> 
> So the values that are protected by the spin lock are now ct->holders,
> ct->thread, and ct->attached. There are cases where the tur checker just
> wants to know if the thread is running. If so, its safe to simply read
> ct->thread without locking.  Also, if it knows that the thread isn't
> running, it can freely access all of these variables. I've left the
> locking in-place in these cases to make the static analyzers happy.
> However I have added comments stating when the locking isn't actually
> necessary.

Hello Ben,

Have you considered to move the condlog() statements out of the spinlock
section? I think that would lead to a much smaller and less contrived change
than the patch you posted.

Thanks,

Bart.

Re: [PATCH 1/7] multipath: fix tur checker locking

[Bart Van Assche]

On Thu, 2018-02-08 at 11:52 -0600, Benjamin Marzinski wrote:
> On Thu, Feb 08, 2018 at 09:49:20AM +0100, Martin Wilck wrote:
> > On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > > +		} else {
> > > +			/* locking here solely to make static
> > > analyzers happy */
> > 
> > Out of curiosity: which analyzers have you been using?
> 
> You would have to ask Bart, who I forgot to CC on this patch. He added
> the code to always read ct->thread in a spin_lock, and said it was to
> make an analyzer happy.

The analyzer I used is a dynamic analyzer, namely drd. See also
http://valgrind.org/docs/manual/drd-manual.html.

Bart.

Re: [PATCH 5/7] Fix set_no_path_retry() regression

[Benjamin Marzinski]

On Thu, Feb 08, 2018 at 10:21:45AM +0100, Martin Wilck wrote:
> Hi Ben,
> 
> On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > commit 0f850db7fceb6b2bf4968f3831efd250c17c6138 "multipathd: clean up
> > set_no_path_retry" has a bug in it. It made set_no_path_retry
> > never reset mpp->retry_ticks, even if the device was in recovery
> > mode,
> > and there were valid paths. This meant that adding new paths didn't
> > remove a device from recovery mode, and queueing could get disabled,
> > even while there were valid paths. This patch fixes that.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/structs_vec.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> > index fbab61f..0de2221 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > @@ -343,9 +343,10 @@ static void set_no_path_retry(struct multipath
> > *mpp)
> >  			dm_queue_if_no_path(mpp->alias, 1);
> >  		break;
> >  	default:
> > -		if (mpp->nr_active > 0)
> > +		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)
> > +		} else if (is_queueing && mpp->retry_tick == 0)
> >  			enter_recovery_mode(mpp);
> >  		break;
> >  	}
> 
> Please explain why it's sufficient to do this in the "default" case
> only. Before 0f850db7, set_no_path_retry() reset retry_tick for any
> value of no_path_retry.

before 0f850db7, set_no_path_retry() was doing this wrong.  it was
resetting the timeout whenever __setup_multipath() was called with
reset, even if there were no usable paths.  This could keep devices from
disabling queueing like they were supposed to, since retry_count_tick()
would ignore them if retry_tick was 0.

But, to go throught the current options: It makes no sense to reset
retry_tick if not_path_retry is set to NO_PATH_RETRY_UNDEF,
NO_PATH_RETRY_FAIL or NO_PATH_RETRY_QUEUE, because we never go into
recovery move... Well, actually that's not true. I just noticed a bug in
cli_restore_queueing() and cli_restore_all_queueing(), where we can go
into recovery mode if we are set to NO_PATH_RETRY_QUEUE. This isn't
actually a problem, since that sets retry_ticks to a negative number,
which means it will get ignored and we will never actually stop
queueing. But that obivously incorrect case aside, we should never be in
recovery mode in the first place unless no_path_retry is set to a
positive number.

The remaining cases where retry_tick was set before 0f850db7 and isn't
now are in the default case when there are no valid paths. In that case,
if we aren't in recovery mode, we should go into it (that's what the
"else if" code does), which means setting the retry_tick to something
other than 0.  If we have already timed out of recovery mode and queuing
is disabled, mpp->retry_tick already is 0. Finally, if we are currently
in recovery mode, and retry_tick isn't 0, then we should leave it alone.
Otherwise we are simply resetting the no_path_retry timer, when we still
don't have any paths, which is one of the bugs the original code was
supposed to fix, like I mentioned above.

> 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)

Re: [PATCH 7/7] multipath: print sysfs state in fast list mode

[Benjamin Marzinski]

On Thu, Feb 08, 2018 at 10:32:06AM +0100, Martin Wilck wrote:
> Hi Ben,
> 
> On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > commit b123e711ea2a4b471a98ff5e26815df3773636b5 "libmultipath:
> > cleanup
> > orphan device states" stopped multipathd from showing old state for
> > orphan paths by checking if pp->mpp was set, and only printing the
> > state
> > if it was.   Unfortunately, when "multipath -l" is run, pp->mpp isn't
> > set. While the checker state isn't checked and shouldn't be printed
> > with
> > "-l", the sysfs state can be, and was before b123e711. This patch
> > sets
> > pp->mpp in fast list mode, so that the sysfs state gets printed.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  multipath/main.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/multipath/main.c b/multipath/main.c
> > index bffe065..1799fd5 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -149,7 +149,7 @@ usage (char * progname)
> >  }
> >  
> >  static int
> > -update_paths (struct multipath * mpp)
> > +update_paths (struct multipath * mpp, int quick)
> >  {
> >  	int i, j;
> >  	struct pathgroup * pgp;
> > @@ -164,6 +164,10 @@ update_paths (struct multipath * mpp)
> >  			continue;
> >  
> >  		vector_foreach_slot (pgp->paths, pp, j) {
> > +			if (quick) {
> > +				pp->mpp = mpp;
> > +				continue;
> > +			}
> >  			if (!strlen(pp->dev)) {
> >  				if (devt2devname(pp->dev, 
> 
> Shouldn't you check whether the paths are still present in sysfs, like
> the code for the general case (quick == false) does?

Sure. It will make multipath paths that don't exist in sysfs show up
with a checker state of "faulty", instead of "undef" like was previously
the case for all paths in the quick list, but that seems like an
improvement.

> 
> 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)

Re: [PATCH 1/7] multipath: fix tur checker locking

[Benjamin Marzinski]

On Thu, Feb 08, 2018 at 06:19:32PM +0000, Bart Van Assche wrote:
> On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > Commit 6e2423fd fixed a bug where the tur checker could cancel a
> > detached thread after it had exitted. However in fixing this, the new
> > code grabbed a mutex (to call condlog) while holding a spin_lock. To
> > deal with that, and to try to keep with the maixim "lock data, not
> > code", I've changed how the tur checker synchronizes with its thread.
> > 
> > Now, the tur checker creates joinable threads, and detaches them when
> > the thread is finished or has timed out. To track the state of the
> > threads, I've added a new variable to the checker context, ct->attached.
> > When a thread starts, attached is set to 1. When the thread finishes, it
> > saves the value of attached, and then zeros it out, while locked. If
> > attached was set, it detaches itself.
> > 
> > When the tur checker gives up on a thread, it also saves and decrements
> > ct->attached, while locked. At the same time it saves the value of
> > ct->thread.  If attached was set, it cancels the thread, and then
> > detaches it.
> > 
> > So the values that are protected by the spin lock are now ct->holders,
> > ct->thread, and ct->attached. There are cases where the tur checker just
> > wants to know if the thread is running. If so, its safe to simply read
> > ct->thread without locking.  Also, if it knows that the thread isn't
> > running, it can freely access all of these variables. I've left the
> > locking in-place in these cases to make the static analyzers happy.
> > However I have added comments stating when the locking isn't actually
> > necessary.
> 
> Hello Ben,
> 
> Have you considered to move the condlog() statements out of the spinlock
> section? I think that would lead to a much smaller and less contrived change
> than the patch you posted.

Yes. I thought it was better to keep the amount of code locked under a
spin_lock as small as possible. But it seems that I am alone in thinking
my trade-off was worth it.  If both you and Martin object, I'm fine with
redoing the patch to simply move the condlog(), since I don't believe
that either tur_check_async_timeout() or pthread_cancel() can block.

-Ben

> Thanks,
> 
> Bart.
> 
> 

Re: [PATCH 1/7] multipath: fix tur checker locking

[Bart Van Assche]

On Thu, 2018-02-08 at 13:27 -0600, Benjamin Marzinski wrote:
> On Thu, Feb 08, 2018 at 06:19:32PM +0000, Bart Van Assche wrote:
> > On Wed, 2018-02-07 at 16:49 -0600, Benjamin Marzinski wrote:
> > > Commit 6e2423fd fixed a bug where the tur checker could cancel a
> > > detached thread after it had exitted. However in fixing this, the new
> > > code grabbed a mutex (to call condlog) while holding a spin_lock. To
> > > deal with that, and to try to keep with the maixim "lock data, not
> > > code", I've changed how the tur checker synchronizes with its thread.
> > > 
> > > Now, the tur checker creates joinable threads, and detaches them when
> > > the thread is finished or has timed out. To track the state of the
> > > threads, I've added a new variable to the checker context, ct->attached.
> > > When a thread starts, attached is set to 1. When the thread finishes, it
> > > saves the value of attached, and then zeros it out, while locked. If
> > > attached was set, it detaches itself.
> > > 
> > > When the tur checker gives up on a thread, it also saves and decrements
> > > ct->attached, while locked. At the same time it saves the value of
> > > ct->thread.  If attached was set, it cancels the thread, and then
> > > detaches it.
> > > 
> > > So the values that are protected by the spin lock are now ct->holders,
> > > ct->thread, and ct->attached. There are cases where the tur checker just
> > > wants to know if the thread is running. If so, its safe to simply read
> > > ct->thread without locking.  Also, if it knows that the thread isn't
> > > running, it can freely access all of these variables. I've left the
> > > locking in-place in these cases to make the static analyzers happy.
> > > However I have added comments stating when the locking isn't actually
> > > necessary.
> > 
> > Hello Ben,
> > 
> > Have you considered to move the condlog() statements out of the spinlock
> > section? I think that would lead to a much smaller and less contrived change
> > than the patch you posted.
> 
> Yes. I thought it was better to keep the amount of code locked under a
> spin_lock as small as possible. But it seems that I am alone in thinking
> my trade-off was worth it.  If both you and Martin object, I'm fine with
> redoing the patch to simply move the condlog(), since I don't believe
> that either tur_check_async_timeout() or pthread_cancel() can block.

Hello Ben,

Have you considered to convert the hldr_lock spinlock into a mutex? I think
that the hldr_lock spinlock got introduced by commit 892f7b333a03 ("multipath:
fix scsi async tur checker corruption"). However, I have not found an
explanation in the description of that commit why hldr_lock is a spinlock
and not any other type of synchronization primitive.

Thanks,

Bart.