[PATCH 0/3] Some miscellaneous dm-multipath patches

[PATCH 0/3] Some miscellaneous dm-multipath patches

[Benjamin Marzinski]

Here are a couple of my local multipath patches, that have been waiting to go
upstream.

-Ben

[PATCH 1/3] remove deleted path from pathvec

[Benjamin Marzinski]

When the last path in a multipath map was removed, the path wasn't getting
deleted from the pathvec before it was getting freed.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 36aa93c..98153df 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -488,7 +488,7 @@ ev_remove_path (char * devname, struct vectors * vecs)
 		if (update_mpp_paths(mpp, vecs->pathvec)) {
 			condlog(0, "%s: failed to update paths",
 				mpp->alias);
-			goto out;
+			goto fail;
 		}
 		if ((i = find_slot(mpp->paths, (void *)pp)) != -1)
 			vector_del_slot(mpp->paths, i);
@@ -507,8 +507,8 @@ ev_remove_path (char * devname, struct vectors * vecs)
 				condlog(2, "%s: removed map after"
 					" removing all paths",
 					alias);
-				free_path(pp);
-				return 0;
+				retval = 0;
+				goto out;
 			}
 			/*
 			 * Not an error, continue
@@ -519,7 +519,7 @@ ev_remove_path (char * devname, struct vectors * vecs)
 			condlog(0, "%s: failed to setup map for"
 				" removal of path %s", mpp->alias,
 				devname);
-			goto out;
+			goto fail;
 		}
 		/*
 		 * reload the map
@@ -535,7 +535,7 @@ ev_remove_path (char * devname, struct vectors * vecs)
 			 * update our state from kernel
 			 */
 			if (setup_multipath(vecs, mpp)) {
-				goto out;
+				goto fail;
 			}
 			sync_map_state(mpp);
 
@@ -544,6 +544,7 @@ ev_remove_path (char * devname, struct vectors * vecs)
 		}
 	}
 
+out:
 	if ((i = find_slot(vecs->pathvec, (void *)pp)) != -1)
 		vector_del_slot(vecs->pathvec, i);
 
@@ -551,7 +552,7 @@ ev_remove_path (char * devname, struct vectors * vecs)
 
 	return retval;
 
-out:
+fail:
 	remove_map_and_stop_waiter(mpp, vecs, 1);
 	return 1;
 }
-- 
1.5.3.3

[PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Benjamin Marzinski]

Attempting to set the stacksize of a pthread to below
PTHREAD_STACK_MIN causes pthread_attr_setstacksize() to fail, which
means that the thread will use the default stack size.  This fix
makes sure that multipathd never requests a stack size less than
PTHREAD_STACK_MIN.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/log_pthread.c |    6 +++++-
 libmultipath/waiter.c      |    5 ++++-
 multipathd/main.c          |    5 ++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index a1d4a10..5d2fe76 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -6,6 +6,7 @@
 #include <stdarg.h>
 #include <pthread.h>
 #include <sys/mman.h>
+#include <limits.h>
 
 #include <memory.h>
 
@@ -52,6 +53,7 @@ static void * log_thread (void * et)
 
 void log_thread_start (void)
 {
+	size_t stacksize = 64 * 1024;
 	pthread_attr_t attr;
 
 	logdbg(stderr,"enter log_thread_start\n");
@@ -65,7 +67,9 @@ void log_thread_start (void)
 	pthread_cond_init(logev_cond, NULL);
 
 	pthread_attr_init(&attr);
-	pthread_attr_setstacksize(&attr, 64 * 1024);
+	if (stacksize < PTHREAD_STACK_MIN)
+		stacksize = PTHREAD_STACK_MIN;
+	pthread_attr_setstacksize(&attr, stacksize);
 
 	if (log_init("multipathd", 0)) {
 		fprintf(stderr,"can't initialize log buffer\n");
diff --git a/libmultipath/waiter.c b/libmultipath/waiter.c
index 54cd19f..4d449e1 100644
--- a/libmultipath/waiter.c
+++ b/libmultipath/waiter.c
@@ -194,6 +194,7 @@ void *waitevent (void *et)
 
 int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 {
+	size_t stacksize = 32 * 1024;
 	pthread_attr_t attr;
 	struct event_thread *wp;
 
@@ -203,7 +204,9 @@ int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 	if (pthread_attr_init(&attr))
 		goto out;
 
-	pthread_attr_setstacksize(&attr, 32 * 1024);
+	if (stacksize < PTHREAD_STACK_MIN)
+		stacksize = PTHREAD_STACK_MIN;
+	pthread_attr_setstacksize(&attr, stacksize);
 	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
 
 	wp = alloc_waiter();
diff --git a/multipathd/main.c b/multipathd/main.c
index 98153df..dae9152 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1267,6 +1267,7 @@ set_oom_adj (int val)
 static int
 child (void * param)
 {
+	size_t stacksize = 64 * 1024;
 	pthread_t check_thr, uevent_thr, uxlsnr_thr;
 	pthread_attr_t attr;
 	struct vectors * vecs;
@@ -1347,7 +1348,9 @@ child (void * param)
 	 * start threads
 	 */
 	pthread_attr_init(&attr);
-	pthread_attr_setstacksize(&attr, 64 * 1024);
+	if (stacksize < PTHREAD_STACK_MIN)
+		stacksize = PTHREAD_STACK_MIN;
+	pthread_attr_setstacksize(&attr, stacksize);
 	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
 
 	pthread_create(&check_thr, &attr, checkerloop, vecs);
-- 
1.5.3.3

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Hannes Reinecke]

Benjamin Marzinski wrote:
> Attempting to set the stacksize of a pthread to below
> PTHREAD_STACK_MIN causes pthread_attr_setstacksize() to fail, which
> means that the thread will use the default stack size.  This fix
> makes sure that multipathd never requests a stack size less than
> PTHREAD_STACK_MIN.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/log_pthread.c |    6 +++++-
>  libmultipath/waiter.c      |    5 ++++-
>  multipathd/main.c          |    5 ++++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index a1d4a10..5d2fe76 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -6,6 +6,7 @@
>  #include <stdarg.h>
>  #include <pthread.h>
>  #include <sys/mman.h>
> +#include <limits.h>
>  
>  #include <memory.h>
>  
> @@ -52,6 +53,7 @@ static void * log_thread (void * et)
>  
>  void log_thread_start (void)
>  {
> +	size_t stacksize = 64 * 1024;
>  	pthread_attr_t attr;
>  
>  	logdbg(stderr,"enter log_thread_start\n");
> @@ -65,7 +67,9 @@ void log_thread_start (void)
>  	pthread_cond_init(logev_cond, NULL);
>  
>  	pthread_attr_init(&attr);
> -	pthread_attr_setstacksize(&attr, 64 * 1024);
> +	if (stacksize < PTHREAD_STACK_MIN)
> +		stacksize = PTHREAD_STACK_MIN;
> +	pthread_attr_setstacksize(&attr, stacksize);
>  
>  	if (log_init("multipathd", 0)) {
>  		fprintf(stderr,"can't initialize log buffer\n");
[ .. ]
Hmm. I don't quite agree. I run into the same problem, but having
discovered that we're not checking any return values at all here
we should rather do the prudent thing and check them once and for all.

I've chosen this approach:

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 9e9aebe..c33480e 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -53,9 +53,30 @@ static void * log_thread (void * et)
 void log_thread_start (void)
 {
        pthread_attr_t attr;
+       size_t stacksize;
 
        logdbg(stderr,"enter log_thread_start\n");
 
+       if (pthread_attr_init(&attr)) {
+               fprintf(stderr,"can't initialize log thread\n");
+               exit(1);
+       }
+
+       if (pthread_attr_getstacksize(&attr, &stacksize) != 0)
+               stacksize = PTHREAD_STACK_MIN:
+
+       /* Check if the stacksize is large enough */
+       if (stacksize < (64 * 1024))
+               stacksize = 64 * 1024;
+
+       /* Set stacksize and try to reinitialize attr if failed */
+       if (stacksize > PTHREAD_STACK_MIN &&
+           pthread_attr_setstacksize(&attr, stacksize) != 0 &&
+           pthread_attr_init(&attr)) {
+               fprintf(stderr,"can't set log thread stack size\n");
+               exit(1);
+       }
+
        logq_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
        logev_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
        logev_cond = (pthread_cond_t *) malloc(sizeof(pthread_cond_t));
@@ -64,9 +85,6 @@ void log_thread_start (void)
        pthread_mutex_init(logev_lock, NULL);
        pthread_cond_init(logev_cond, NULL);
 
-       pthread_attr_init(&attr);
-       pthread_attr_setstacksize(&attr, 64 * 1024);
-
        if (log_init("multipathd", 0)) {
                fprintf(stderr,"can't initialize log buffer\n");
                exit(1);


This way we'll at least be notified if something goes wrong in
the future. We shouldn't make the same mistake again and
ignore error codes which don't happen to trigger now.

If agreed I'll post the full patch here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Benjamin Marzinski]

> Hmm. I don't quite agree. I run into the same problem, but having
> discovered that we're not checking any return values at all here
> we should rather do the prudent thing and check them once and for all.
>
> I've chosen this approach:
>
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 9e9aebe..c33480e 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -53,9 +53,30 @@ static void * log_thread (void * et)
> void log_thread_start (void)
> {
>        pthread_attr_t attr;
> +       size_t stacksize;
>
>        logdbg(stderr,"enter log_thread_start\n");
>
> +       if (pthread_attr_init(&attr)) {
> +               fprintf(stderr,"can't initialize log thread\n");
> +               exit(1);
> +       }
> +
> +       if (pthread_attr_getstacksize(&attr, &stacksize) != 0)
> +               stacksize = PTHREAD_STACK_MIN:
> +
> +       /* Check if the stacksize is large enough */
> +       if (stacksize < (64 * 1024))
> +               stacksize = 64 * 1024;
> +
> +       /* Set stacksize and try to reinitialize attr if failed */
> +       if (stacksize > PTHREAD_STACK_MIN &&
> +           pthread_attr_setstacksize(&attr, stacksize) != 0 &&
> +           pthread_attr_init(&attr)) {
> +               fprintf(stderr,"can't set log thread stack size\n");
> +               exit(1);
> +       }
> +
>        logq_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
>        logev_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
>        logev_cond = (pthread_cond_t *) malloc(sizeof(pthread_cond_t));
> @@ -64,9 +85,6 @@ void log_thread_start (void)
>        pthread_mutex_init(logev_lock, NULL);
>        pthread_cond_init(logev_cond, NULL);
>
> -       pthread_attr_init(&attr);
> -       pthread_attr_setstacksize(&attr, 64 * 1024);
> -
>        if (log_init("multipathd", 0)) {
>                fprintf(stderr,"can't initialize log buffer\n");
>                exit(1);
>
>
> This way we'll at least be notified if something goes wrong in
> the future. We shouldn't make the same mistake again and
> ignore error codes which don't happen to trigger now.
>
> If agreed I'll post the full patch here.

This approach doesn't doesn't actually fix the bug that I see. The
problem I was seeing is that setting the stacksize too small just causes
pthread_attr_setstacksize() to fail, leaving you with the default stack
size. On some architectures, the default stacksize is large, like 10Mb.
Since you start one waiter thread per multipath device, every 100
devices eats up 1Gb of memory. Your approach always uses the default
stack size, unless it's too small.  I've never seen problems with the
stack being too small. Only too large. Maybe your experience has been
different.

The other problem is that when I actually read the pthread_attr_init man
page (it can fail. who knew?), I saw that it can fail with ENOMEM. Also,
that it had a function to free it, and that the result of reinitializing
an attr that hadn't been freed was undefined.  Clearly, this function
wasn't intended to be called over and over without ever freeing the
attr, which is how we've been using it in multipathd. So, in the spirit
of writing code to the interface, instead of to how it appears to be
currently implemented, how about this.
---
 libmultipath/log_pthread.c |    9 +-------
 libmultipath/log_pthread.h |    4 ++-
 libmultipath/waiter.c      |   11 ++--------
 libmultipath/waiter.h      |    2 +
 multipathd/main.c          |   48 +++++++++++++++++++++++++++++++++++----------
 5 files changed, 48 insertions(+), 26 deletions(-)

Index: multipath-tools-090311/libmultipath/waiter.c
===================================================================
--- multipath-tools-090311.orig/libmultipath/waiter.c
+++ multipath-tools-090311/libmultipath/waiter.c
@@ -20,6 +20,8 @@
 #include "lock.h"
 #include "waiter.h"
 
+pthread_attr_t waiter_attr;
+
 struct event_thread *alloc_waiter (void)
 {
 
@@ -194,18 +196,11 @@ void *waitevent (void *et)
 
 int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 {
-	pthread_attr_t attr;
 	struct event_thread *wp;
 
 	if (!mpp)
 		return 0;
 
-	if (pthread_attr_init(&attr))
-		goto out;
-
-	pthread_attr_setstacksize(&attr, 32 * 1024);
-	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-
 	wp = alloc_waiter();
 
 	if (!wp)
@@ -216,7 +211,7 @@ int start_waiter_thread (struct multipat
 	wp->vecs = vecs;
 	wp->mpp = mpp;
 
-	if (pthread_create(&wp->thread, &attr, waitevent, wp)) {
+	if (pthread_create(&wp->thread, &waiter_attr, waitevent, wp)) {
 		condlog(0, "%s: cannot create event checker", wp->mapname);
 		goto out1;
 	}
Index: multipath-tools-090311/multipathd/main.c
===================================================================
--- multipath-tools-090311.orig/multipathd/main.c
+++ multipath-tools-090311/multipathd/main.c
@@ -14,6 +14,7 @@
 #include <errno.h>
 #include <sys/time.h>
 #include <sys/resource.h>
+#include <limits.h>
 
 /*
  * libcheckers
@@ -1264,17 +1265,47 @@ set_oom_adj (int val)
 	fclose(fp);
 }
 
+void
+setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
+{
+	if (pthread_attr_init(attr)) {
+		fprintf(stderr, "can't initialize thread attr: %s\n",
+			strerror(errno));
+		exit(1);
+	}
+	if (stacksize < PTHREAD_STACK_MIN)
+		stacksize = PTHREAD_STACK_MIN;
+
+	if (pthread_attr_setstacksize(attr, stacksize)) {
+		fprintf(stderr, "can't set thread stack size to %lu: %s\n",
+			(unsigned long)stacksize, strerror(errno));
+		exit(1);
+	}
+	if (detached && pthread_attr_setdetachstate(attr,
+						    PTHREAD_CREATE_DETACHED)) {
+		fprintf(stderr, "can't set thread to detached: %s\n",
+			strerror(errno));
+		exit(1);
+	}
+}
+
 static int
 child (void * param)
 {
 	pthread_t check_thr, uevent_thr, uxlsnr_thr;
-	pthread_attr_t attr;
+	pthread_attr_t log_attr, misc_attr;
 	struct vectors * vecs;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
-	if (logsink)
-		log_thread_start();
+	setup_thread_attr(&misc_attr, 64 * 1024, 1);
+	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
+
+	if (logsink) {
+		setup_thread_attr(&log_attr, 64 * 1024, 0);
+		log_thread_start(&log_attr);
+		pthread_attr_destroy(&log_attr);
+	}
 
 	condlog(2, "--------start up--------");
 	condlog(2, "read " DEFAULT_CONFIGFILE);
@@ -1346,13 +1377,10 @@ child (void * param)
 	/*
 	 * start threads
 	 */
-	pthread_attr_init(&attr);
-	pthread_attr_setstacksize(&attr, 64 * 1024);
-	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-
-	pthread_create(&check_thr, &attr, checkerloop, vecs);
-	pthread_create(&uevent_thr, &attr, ueventloop, vecs);
-	pthread_create(&uxlsnr_thr, &attr, uxlsnrloop, vecs);
+	pthread_create(&check_thr, &misc_attr, checkerloop, vecs);
+	pthread_create(&uevent_thr, &misc_attr, ueventloop, vecs);
+	pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
+	pthread_attr_destroy(&misc_attr);
 
 	pthread_cond_wait(&exit_cond, &exit_mutex);
 
Index: multipath-tools-090311/libmultipath/log_pthread.c
===================================================================
--- multipath-tools-090311.orig/libmultipath/log_pthread.c
+++ multipath-tools-090311/libmultipath/log_pthread.c
@@ -50,10 +50,8 @@ static void * log_thread (void * et)
 	}
 }
 
-void log_thread_start (void)
+void log_thread_start (pthread_attr_t *attr)
 {
-	pthread_attr_t attr;
-
 	logdbg(stderr,"enter log_thread_start\n");
 
 	logq_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
@@ -64,14 +62,11 @@ void log_thread_start (void)
 	pthread_mutex_init(logev_lock, NULL);
 	pthread_cond_init(logev_cond, NULL);
 
-	pthread_attr_init(&attr);
-	pthread_attr_setstacksize(&attr, 64 * 1024);
-
 	if (log_init("multipathd", 0)) {
 		fprintf(stderr,"can't initialize log buffer\n");
 		exit(1);
 	}
-	pthread_create(&log_thr, &attr, log_thread, NULL);
+	pthread_create(&log_thr, attr, log_thread, NULL);
 
 	return;
 }
Index: multipath-tools-090311/libmultipath/log_pthread.h
===================================================================
--- multipath-tools-090311.orig/libmultipath/log_pthread.h
+++ multipath-tools-090311/libmultipath/log_pthread.h
@@ -1,6 +1,8 @@
 #ifndef _LOG_PTHREAD_H
 #define _LOG_PTHREAD_H
 
+#include <pthread.h>
+
 pthread_t log_thr;
 
 pthread_mutex_t *logq_lock;
@@ -8,7 +10,7 @@ pthread_mutex_t *logev_lock;
 pthread_cond_t *logev_cond;
 
 void log_safe(int prio, const char * fmt, va_list ap);
-void log_thread_start(void);
+void log_thread_start(pthread_attr_t *attr);
 void log_thread_stop(void);
 
 #endif /* _LOG_PTHREAD_H */
Index: multipath-tools-090311/libmultipath/waiter.h
===================================================================
--- multipath-tools-090311.orig/libmultipath/waiter.h
+++ multipath-tools-090311/libmultipath/waiter.h
@@ -1,6 +1,8 @@
 #ifndef _WAITER_H
 #define _WAITER_H
 
+extern pthread_attr_t waiter_attr;
+
 struct event_thread {
 	struct dm_task *dmt;
 	pthread_t thread;

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Hannes Reinecke]

Benjamin Marzinski wrote:
[ .. ]
> 
> This approach doesn't doesn't actually fix the bug that I see. The
> problem I was seeing is that setting the stacksize too small just causes
> pthread_attr_setstacksize() to fail, leaving you with the default stack
> size. On some architectures, the default stacksize is large, like 10Mb.
> Since you start one waiter thread per multipath device, every 100
> devices eats up 1Gb of memory. Your approach always uses the default
> stack size, unless it's too small.  I've never seen problems with the
> stack being too small. Only too large. Maybe your experience has been
> different.
> 
Me neither. Makes me wonder if we _really_ need to set the stacksize.
After all, I'm not aware that we're having any excessive stack usage
somewhere. Maybe we can simplify it by removing the stack attribute
setting altogether?

I'll see if I can get the different stacksizes and just compare them
to the 'updated' setting. Maybe there's no big difference after all...

> The other problem is that when I actually read the pthread_attr_init man
> page (it can fail. who knew?), I saw that it can fail with ENOMEM. Also,
> that it had a function to free it, and that the result of reinitializing
> an attr that hadn't been freed was undefined.  Clearly, this function
> wasn't intended to be called over and over without ever freeing the
> attr, which is how we've been using it in multipathd. So, in the spirit
> of writing code to the interface, instead of to how it appears to be
> currently implemented, how about this.
Hmm. You're not freeing the attribute for all non-logging threads neither.
Oversight? 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Benjamin Marzinski]

> On Mon, Mar 16, 2009 at 05:12:57PM +0100, Hannes Reinecke wrote:
>> This approach doesn't doesn't actually fix the bug that I see. The
>> problem I was seeing is that setting the stacksize too small just causes
>> pthread_attr_setstacksize() to fail, leaving you with the default stack
>> size. On some architectures, the default stacksize is large, like 10Mb.
>> Since you start one waiter thread per multipath device, every 100
>> devices eats up 1Gb of memory. Your approach always uses the default
>> stack size, unless it's too small.  I've never seen problems with the
>> stack being too small. Only too large. Maybe your experience has been
>> different.
>>
> Me neither. Makes me wonder if we _really_ need to set the stacksize.
> After all, I'm not aware that we're having any excessive stack usage
> somewhere. Maybe we can simplify it by removing the stack attribute
> setting altogether?
>
> I'll see if I can get the different stacksizes and just compare them
> to the 'updated' setting. Maybe there's no big difference after all...

I definitely see a problem if we use the default stacksize on ia64
machines.  In RHEL5 at least, it's 10Mb per thread.  With one waiter
thread per multipath device, you get a gigabyte of memory wasted on
machines with over a hundred multipath devices.

>> The other problem is that when I actually read the pthread_attr_init man
>> page (it can fail. who knew?), I saw that it can fail with ENOMEM. Also,
>> that it had a function to free it, and that the result of reinitializing
>> an attr that hadn't been freed was undefined.  Clearly, this function
>> wasn't intended to be called over and over without ever freeing the
>> attr, which is how we've been using it in multipathd. So, in the spirit
>> of writing code to the interface, instead of to how it appears to be
>> currently implemented, how about this.
> Hmm. You're not freeing the attribute for all non-logging threads neither.

But I only allocate it once.

> Oversight?

Any time a new device is added, we need the waiter thread attribute.  I
suppose I could free it after each waiter thread is started, and then
reallocate it again, but it seems like sort of a waste since we want the
same values every time.

I don't explicitly deallocate it on shutdown, but no matter what the
implementation is, I expect it will be cleaned up when multipathd
ends.

Or am I missing something?

-Ben 

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Joe Thornber]

2009/3/16 Benjamin Marzinski <bmarzins@redhat.com>:
> I definitely see a problem if we use the default stacksize on ia64
> machines.  In RHEL5 at least, it's 10Mb per thread.  With one waiter
> thread per multipath device, you get a gigabyte of memory wasted on
> machines with over a hundred multipath devices.

You need to check whether this is 1G of physical memory, or just a 1G
chunk out of the address space.

Some threads need to have their stack reserved and locked into memory
before calls into the kernel.  This avoids deadlocks where the stack
gets paged out, but he vm can't page it back in until the thread
completes ...

It sounds like you have many more threads running these days than when
I last looked at LVM, it's not clear to me how many of these are ones
that need their stacks mem-locking.  Do you have an idea ?

If they don't need mem-locking then as long as you're not forcing the
stack to be physically allocated I wouldn't worry too much about
consuming address space.

Hope that ramble made sense,

- Joe

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Benjamin Marzinski]

On Tue, Mar 17, 2009 at 07:56:39AM +0000, Joe Thornber wrote:
> 2009/3/16 Benjamin Marzinski <bmarzins@redhat.com>:
> > I definitely see a problem if we use the default stacksize on ia64
> > machines.  In RHEL5 at least, it's 10Mb per thread.  With one waiter
> > thread per multipath device, you get a gigabyte of memory wasted on
> > machines with over a hundred multipath devices.
> 
> You need to check whether this is 1G of physical memory, or just a 1G
> chunk out of the address space.
> 
> Some threads need to have their stack reserved and locked into memory
> before calls into the kernel.  This avoids deadlocks where the stack
> gets paged out, but he vm can't page it back in until the thread
> completes ...
> 
> It sounds like you have many more threads running these days than when
> I last looked at LVM, it's not clear to me how many of these are ones
> that need their stacks mem-locking.  Do you have an idea ?
> 
> If they don't need mem-locking then as long as you're not forcing the
> stack to be physically allocated I wouldn't worry too much about
> consuming address space.
> 
> Hope that ramble made sense,

Yeah, sorry for missing your reply.

The issue is that the event threads occasionally need to hold mutexs
that the checker thread (the one that restores downed paths) needs. If
the system was low on memory because a number of devices had no paths to
them, and IO was queueing up, you could run into a problem where a event
thread was paged out while holding a mutex. With the mutex locked, the
checker thread could never restore the downed paths, letting the IO
complete, and freeing up the memory. 

Christophe, if people are O.k. with these patches now, could they get
in.

Thanks

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

Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Joe Thornber]

At least pull this logic out into a seperate function rather than
repeating it three times.  Also you may find min() useful.

- Joe

2009/3/12 Benjamin Marzinski <bmarzins@redhat.com>:
> Attempting to set the stacksize of a pthread to below
> PTHREAD_STACK_MIN causes pthread_attr_setstacksize() to fail, which
> means that the thread will use the default stack size.  This fix
> makes sure that multipathd never requests a stack size less than
> PTHREAD_STACK_MIN.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/log_pthread.c |    6 +++++-
>  libmultipath/waiter.c      |    5 ++++-
>  multipathd/main.c          |    5 ++++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index a1d4a10..5d2fe76 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -6,6 +6,7 @@
>  #include <stdarg.h>
>  #include <pthread.h>
>  #include <sys/mman.h>
> +#include <limits.h>
>
>  #include <memory.h>
>
> @@ -52,6 +53,7 @@ static void * log_thread (void * et)
>
>  void log_thread_start (void)
>  {
> +       size_t stacksize = 64 * 1024;
>        pthread_attr_t attr;
>
>        logdbg(stderr,"enter log_thread_start\n");
> @@ -65,7 +67,9 @@ void log_thread_start (void)
>        pthread_cond_init(logev_cond, NULL);
>
>        pthread_attr_init(&attr);
> -       pthread_attr_setstacksize(&attr, 64 * 1024);
> +       if (stacksize < PTHREAD_STACK_MIN)
> +               stacksize = PTHREAD_STACK_MIN;
> +       pthread_attr_setstacksize(&attr, stacksize);
>
>        if (log_init("multipathd", 0)) {
>                fprintf(stderr,"can't initialize log buffer\n");
> diff --git a/libmultipath/waiter.c b/libmultipath/waiter.c
> index 54cd19f..4d449e1 100644
> --- a/libmultipath/waiter.c
> +++ b/libmultipath/waiter.c
> @@ -194,6 +194,7 @@ void *waitevent (void *et)
>
>  int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
>  {
> +       size_t stacksize = 32 * 1024;
>        pthread_attr_t attr;
>        struct event_thread *wp;
>
> @@ -203,7 +204,9 @@ int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
>        if (pthread_attr_init(&attr))
>                goto out;
>
> -       pthread_attr_setstacksize(&attr, 32 * 1024);
> +       if (stacksize < PTHREAD_STACK_MIN)
> +               stacksize = PTHREAD_STACK_MIN;
> +       pthread_attr_setstacksize(&attr, stacksize);
>        pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
>
>        wp = alloc_waiter();
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 98153df..dae9152 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1267,6 +1267,7 @@ set_oom_adj (int val)
>  static int
>  child (void * param)
>  {
> +       size_t stacksize = 64 * 1024;
>        pthread_t check_thr, uevent_thr, uxlsnr_thr;
>        pthread_attr_t attr;
>        struct vectors * vecs;
> @@ -1347,7 +1348,9 @@ child (void * param)
>         * start threads
>         */
>        pthread_attr_init(&attr);
> -       pthread_attr_setstacksize(&attr, 64 * 1024);
> +       if (stacksize < PTHREAD_STACK_MIN)
> +               stacksize = PTHREAD_STACK_MIN;
> +       pthread_attr_setstacksize(&attr, stacksize);
>        pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
>
>        pthread_create(&check_thr, &attr, checkerloop, vecs);
> --
> 1.5.3.3
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>

[PATCH 3/3] Add options to multipathd to turn off queueing

[Benjamin Marzinski]

Even when the last path of a multipath device is deleted, it can't be
removed until all the queued IO is flushed. For devices that have
no_path_retry set to queue, this doesn't automatically happen.

This patch adds a "flush_on_last_del" config file option, that causes the
multipath device to automatically turn off queueing when the last path is
deleted.  It also adds the "disablequeueing" and "restorequeueing"
multipathd cli commands.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c      |    1 +
 libmultipath/config.h      |    3 +
 libmultipath/dict.c        |  117 ++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/propsel.c     |   32 ++++++++++++
 libmultipath/propsel.h     |    1 +
 libmultipath/structs.h     |    9 +++
 libmultipath/structs_vec.c |    1 +
 multipath.conf.annotated   |   30 +++++++++++
 multipath.conf.synthetic   |    1 +
 multipathd/cli.c           |    2 +
 multipathd/cli.h           |    4 ++
 multipathd/cli_handlers.c  |   90 +++++++++++++++++++++++++++++++++
 multipathd/cli_handlers.h  |    4 ++
 multipathd/main.c          |   12 +++++
 14 files changed, 307 insertions(+), 0 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 6d731d2..6393880 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -427,6 +427,7 @@ load_config (char * file)
 	conf->max_fds = 0;
 	conf->bindings_file = DEFAULT_BINDINGS_FILE;
 	conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR);
+	conf->flush_on_last_del = 0;
 
 	/*
 	 * preload default hwtable
diff --git a/libmultipath/config.h b/libmultipath/config.h
index fb917f4..4523aa8 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -28,6 +28,7 @@ struct hwentry {
 	int no_path_retry;
 	int minio;
 	int pg_timeout;
+	int flush_on_last_del;
 	char * bl_product;
 };
 
@@ -43,6 +44,7 @@ struct mpentry {
 	int no_path_retry;
 	int minio;
 	int pg_timeout;
+	int flush_on_last_del;
 };
 
 struct config {
@@ -64,6 +66,7 @@ struct config {
 	int pg_timeout;
 	int max_fds;
 	int force_reload;
+	int flush_on_last_del;
 
 	char * dev;
 	char * sysfs_dir;
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 2429a93..fc8516b 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -247,6 +247,28 @@ def_pg_timeout_handler(vector strvec)
 }
 
 static int
+def_flush_on_last_del_handler(vector strvec)
+{
+	char * buff;
+
+	buff = set_value(strvec);
+	if (!buff)
+		return 1;
+
+	if ((strlen(buff) == 2 && strcmp(buff, "no") == 0) ||
+	    (strlen(buff) == 1 && strcmp(buff, "0") == 0))
+		conf->flush_on_last_del = FLUSH_DISABLED;
+	if ((strlen(buff) == 3 && strcmp(buff, "yes") == 0) ||
+	    (strlen(buff) == 1 && strcmp(buff, "1") == 0))
+		conf->flush_on_last_del = FLUSH_ENABLED;
+	else
+		conf->flush_on_last_del = FLUSH_UNDEF;
+
+	FREE(buff);
+	return 0;
+}
+
+static int
 names_handler(vector strvec)
 {
 	char * buff;
@@ -724,6 +746,32 @@ hw_pg_timeout_handler(vector strvec)
 	return 0;
 }
 
+static int
+hw_flush_on_last_del_handler(vector strvec)
+{
+	struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable);
+	char * buff;
+
+	if (!hwe)
+		return 1;
+
+	buff = set_value(strvec);
+	if (!buff)
+		return 1;
+
+	if ((strlen(buff) == 2 && strcmp(buff, "no") == 0) ||
+	    (strlen(buff) == 1 && strcmp(buff, "0") == 0))
+		hwe->flush_on_last_del = FLUSH_DISABLED;
+	if ((strlen(buff) == 3 && strcmp(buff, "yes") == 0) ||
+	    (strlen(buff) == 1 && strcmp(buff, "1") == 0))
+		hwe->flush_on_last_del = FLUSH_ENABLED;
+	else
+		hwe->flush_on_last_del = FLUSH_UNDEF;
+
+	FREE(buff);
+	return 0;
+}
+
 /*
  * multipaths block handlers
  */
@@ -945,6 +993,32 @@ mp_pg_timeout_handler(vector strvec)
 	return 0;
 }
 
+static int
+mp_flush_on_last_del_handler(vector strvec)
+{
+	struct mpentry *mpe = VECTOR_LAST_SLOT(conf->mptable);
+	char * buff;
+
+	if (!mpe)
+		return 1;
+
+	buff = set_value(strvec);
+	if (!buff)
+		return 1;
+
+	if ((strlen(buff) == 2 && strcmp(buff, "no") == 0) ||
+	    (strlen(buff) == 1 && strcmp(buff, "0") == 0))
+		mpe->flush_on_last_del = FLUSH_DISABLED;
+	if ((strlen(buff) == 3 && strcmp(buff, "yes") == 0) ||
+	    (strlen(buff) == 1 && strcmp(buff, "1") == 0))
+		mpe->flush_on_last_del = FLUSH_ENABLED;
+	else
+		mpe->flush_on_last_del = FLUSH_UNDEF;
+
+	FREE(buff);
+	return 0;
+}
+
 /*
  * config file keywords printing
  */
@@ -1080,6 +1154,20 @@ snprint_mp_pg_timeout (char * buff, int len, void * data)
 }
 
 static int
+snprint_mp_flush_on_last_del (char * buff, int len, void * data)
+{
+	struct mpentry * mpe = (struct mpentry *)data;
+
+	switch (mpe->flush_on_last_del) {
+	case FLUSH_DISABLED:
+		return snprintf(buff, len, "no");
+	case FLUSH_ENABLED:
+		return snprintf(buff, len, "yes");
+	}
+	return 0;
+}
+
+static int
 snprint_hw_vendor (char * buff, int len, void * data)
 {
 	struct hwentry * hwe = (struct hwentry *)data;
@@ -1295,6 +1383,20 @@ snprint_hw_pg_timeout (char * buff, int len, void * data)
 }
 
 static int
+snprint_hw_flush_on_last_del (char * buff, int len, void * data)
+{
+	struct hwentry * hwe = (struct hwentry *)data;
+
+	switch (hwe->flush_on_last_del) {
+	case FLUSH_DISABLED:
+		return snprintf(buff, len, "no");
+	case FLUSH_ENABLED:
+		return snprintf(buff, len, "yes");
+	}
+	return 0;
+}
+
+static int
 snprint_hw_path_checker (char * buff, int len, void * data)
 {
 	struct hwentry * hwe = (struct hwentry *)data;
@@ -1509,6 +1611,18 @@ snprint_def_pg_timeout (char * buff, int len, void * data)
 }
 
 static int
+snprint_def_flush_on_last_del (char * buff, int len, void * data)
+{
+	switch (conf->flush_on_last_del) {
+	case FLUSH_DISABLED:
+		return snprintf(buff, len, "no");
+	case FLUSH_ENABLED:
+		return snprintf(buff, len, "yes");
+	}
+	return 0;
+}
+
+static int
 snprint_def_user_friendly_names (char * buff, int len, void * data)
 {
 	if (conf->user_friendly_names == DEFAULT_USER_FRIENDLY_NAMES)
@@ -1565,6 +1679,7 @@ init_keywords(void)
 	install_keyword("rr_weight", &def_weight_handler, &snprint_def_rr_weight);
 	install_keyword("no_path_retry", &def_no_path_retry_handler, &snprint_def_no_path_retry);
 	install_keyword("pg_timeout", &def_pg_timeout_handler, &snprint_def_pg_timeout);
+	install_keyword("flush_on_last_del", &def_flush_on_last_del_handler, &snprint_def_flush_on_last_del);
 	install_keyword("user_friendly_names", &names_handler, &snprint_def_user_friendly_names);
 	__deprecated install_keyword("default_selector", &def_selector_handler, NULL);
 	__deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL);
@@ -1619,6 +1734,7 @@ init_keywords(void)
 	install_keyword("no_path_retry", &hw_no_path_retry_handler, &snprint_hw_no_path_retry);
 	install_keyword("rr_min_io", &hw_minio_handler, &snprint_hw_rr_min_io);
 	install_keyword("pg_timeout", &hw_pg_timeout_handler, &snprint_hw_pg_timeout);
+	install_keyword("flush_on_last_del", &hw_flush_on_last_del_handler, &snprint_hw_flush_on_last_del);
 	install_sublevel_end();
 
 	install_keyword_root("multipaths", &multipaths_handler);
@@ -1633,5 +1749,6 @@ init_keywords(void)
 	install_keyword("no_path_retry", &mp_no_path_retry_handler, &snprint_mp_no_path_retry);
 	install_keyword("rr_min_io", &mp_minio_handler, &snprint_mp_rr_min_io);
 	install_keyword("pg_timeout", &mp_pg_timeout_handler, &snprint_mp_pg_timeout);
+	install_keyword("flush_on_last_del", &mp_flush_on_last_del_handler, &snprint_mp_flush_on_last_del);
 	install_sublevel_end();
 }
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 43611ff..c1bc591 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -279,6 +279,10 @@ select_prio (struct path * pp)
 extern int
 select_no_path_retry(struct multipath *mp)
 {
+	if (mp->flush_on_last_del == FLUSH_IN_PROGRESS) {
+		condlog(0, "flush_on_last_del in progress");
+		mp->no_path_retry = NO_PATH_RETRY_FAIL;
+	}
 	if (mp->mpe && mp->mpe->no_path_retry != NO_PATH_RETRY_UNDEF) {
 		mp->no_path_retry = mp->mpe->no_path_retry;
 		condlog(3, "%s: no_path_retry = %i (multipath setting)",
@@ -368,3 +372,31 @@ select_pg_timeout(struct multipath *mp)
 	condlog(3, "pg_timeout = NONE (internal default)");
 	return 0;
 }
+
+extern int
+select_flush_on_last_del(struct multipath *mp)
+{
+	if (mp->flush_on_last_del == FLUSH_IN_PROGRESS)
+		return 0;
+	if (mp->mpe && mp->mpe->flush_on_last_del != FLUSH_UNDEF) {
+		mp->flush_on_last_del = mp->mpe->flush_on_last_del;
+		condlog(3, "flush_on_last_del = %i (multipath setting)",
+				mp->flush_on_last_del);
+		return 0;
+	}
+	if (mp->hwe && mp->hwe->flush_on_last_del != FLUSH_UNDEF) {
+		mp->flush_on_last_del = mp->hwe->flush_on_last_del;
+		condlog(3, "flush_on_last_del = %i (controler setting)",
+				mp->flush_on_last_del);
+		return 0;
+	}
+	if (conf->flush_on_last_del != FLUSH_UNDEF) {
+		mp->flush_on_last_del = conf->flush_on_last_del;
+		condlog(3, "flush_on_last_del = %i (config file default)",
+				mp->flush_on_last_del);
+		return 0;
+	}
+	mp->flush_on_last_del = FLUSH_UNDEF;
+	condlog(3, "flush_on_last_del = DISABLED (internal default)");
+	return 0;
+}
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 62802f8..818060b 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -10,4 +10,5 @@ int select_getuid (struct path * pp);
 int select_prio (struct path * pp);
 int select_no_path_retry(struct multipath *mp);
 int select_pg_timeout(struct multipath *mp);
+int select_flush_on_last_del(struct multipath *mp);
 int select_minio(struct multipath *mp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index aeb4afd..eb21ab2 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -67,6 +67,14 @@ enum pgtimeouts {
 	PGTIMEOUT_NONE
 };
 
+
+enum flush_states {
+	FLUSH_UNDEF,
+	FLUSH_DISABLED,
+	FLUSH_ENABLED,
+	FLUSH_IN_PROGRESS,
+};
+
 struct scsi_idlun {
 	int dev_id;
 	int host_unique_id;
@@ -150,6 +158,7 @@ struct multipath {
 	int retry_tick;    /* remaining times for retries */
 	int minio;
 	int pg_timeout;
+	int flush_on_last_del;
 	unsigned long long size;
 	vector paths;
 	vector pg;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 785a766..0ff7273 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -321,6 +321,7 @@ retry:
 	select_pgfailback(mpp);
 	set_no_path_retry(mpp);
 	select_pg_timeout(mpp);
+	select_flush_on_last_del(mpp);
 
 	return 0;
 out:
diff --git a/multipath.conf.annotated b/multipath.conf.annotated
index bf15dc3..10aa5eb 100644
--- a/multipath.conf.annotated
+++ b/multipath.conf.annotated
@@ -99,6 +99,16 @@
 #	rr_min_io	100
 #
 #	#
+#	# name    : flush_on_last_del
+#	# scope   : multipathd
+#	# desc    : If set to "yes", multipathd will disable queueing when the
+#	#           last path to a device has been deleted.
+#	# values  : yes|no
+#	# default : no
+#	#
+#	flush_on_last_del       yes
+#
+#	#
 #	# name    : max_fds
 #	# scope   : multipathd
 #	# desc    : Sets the maximum number of open file descriptors for the
@@ -278,6 +288,16 @@
 #		#           to the next in the same path group
 #		#
 #		rr_min_io	100
+#
+#		#
+#		# name    : flush_on_last_del
+#		# scope   : multipathd
+#		# desc    : If set to "yes", multipathd will disable queueing
+#		#           when the last path to a device has been deleted.
+#		# values  : yes|no
+#		# default : no
+#		#
+#		flush_on_last_del       yes
 #	}
 #	multipath {
 #		wwid	1DEC_____321816758474
@@ -419,6 +439,16 @@
 #		rr_min_io	100
 #
 #		#
+#		# name    : flush_on_last_del
+#		# scope   : multipathd
+#		# desc    : If set to "yes", multipathd will disable queueing
+#		#           when the last path to a device has been deleted.
+#		# values  : yes|no
+#		# default : no
+#		#
+#		flush_on_last_del       yes
+#
+#		#
 #		# name    : product_blacklist
 #		# scope   : multipath & multipathd
 #		# desc    : product strings to blacklist for this vendor
diff --git a/multipath.conf.synthetic b/multipath.conf.synthetic
index a762016..bf94c04 100644
--- a/multipath.conf.synthetic
+++ b/multipath.conf.synthetic
@@ -11,6 +11,7 @@
 #	prio			const
 #	path_checker		directio
 #	rr_min_io		100
+#	flush_on_last_del	no
 #	max_fds			8192
 #	rr_weight		priorities
 #	failback		immediate
diff --git a/multipathd/cli.c b/multipathd/cli.c
index c93aa83..c1af9fb 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -155,6 +155,8 @@ load_keys (void)
 	r += add_key(keys, "resume", RESUME, 0);
 	r += add_key(keys, "reinstate", REINSTATE, 0);
 	r += add_key(keys, "fail", FAIL, 0);
+	r += add_key(keys, "disablequeueing", DISABLEQ, 0);
+	r += add_key(keys, "restorequeueing", RESTOREQ, 0);
 	r += add_key(keys, "paths", PATHS, 0);
 	r += add_key(keys, "maps", MAPS, 0);
 	r += add_key(keys, "multipaths", MAPS, 0);
diff --git a/multipathd/cli.h b/multipathd/cli.h
index d58a200..b33cd13 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -7,6 +7,8 @@ enum {
 	__RESUME,
 	__REINSTATE,
 	__FAIL,
+	__DISABLEQ,
+	__RESTOREQ,
 	__PATHS,
 	__MAPS,
 	__PATH,
@@ -32,6 +34,8 @@ enum {
 #define RESUME		(1 << __RESUME)
 #define REINSTATE	(1 << __REINSTATE)
 #define FAIL		(1 << __FAIL)
+#define DISABLEQ	(1 << __DISABLEQ)
+#define RESTOREQ	(1 << __RESTOREQ)
 #define PATHS		(1 << __PATHS)
 #define MAPS		(1 << __MAPS)
 #define PATH		(1 << __PATH)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 6cf232a..a75373b 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -420,6 +420,96 @@ cli_del_map (void * v, char ** reply, int * len, void * data)
 }
 
 int
+cli_restore_queueing(void *v, char **reply, int *len, void *data)
+{
+	struct vectors * vecs = (struct vectors *)data;
+	char * mapname = get_keyparam(v, MAP);
+	struct multipath *mpp;
+	int minor;
+
+	condlog(2, "%s: restore map queueing (operator)", mapname);
+	if (sscanf(mapname, "dm-%d", &minor) == 1)
+		mpp = find_mp_by_minor(vecs->mpvec, minor);
+	else
+		mpp = find_mp_by_alias(vecs->mpvec, mapname);
+
+	if (!mpp) {
+		condlog(0, "%s: invalid map name, cannot restore queueing", mapname);
+		return 1;
+	}
+
+	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
+			mpp->no_path_retry != NO_PATH_RETRY_FAIL) {
+		dm_queue_if_no_path(mpp->alias, 1);
+		if (mpp->nr_active > 0)
+			mpp->retry_tick = 0;
+		else
+			mpp->retry_tick = mpp->no_path_retry * conf->checkint;
+	}
+	return 0;
+}
+
+int
+cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
+{
+	struct vectors * vecs = (struct vectors *)data;
+	struct multipath *mpp;
+	int i;
+
+	condlog(2, "restore queueing (operator)");
+	vector_foreach_slot(vecs->mpvec, mpp, i) {
+		if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
+		    mpp->no_path_retry != NO_PATH_RETRY_FAIL) {
+			dm_queue_if_no_path(mpp->alias, 1);
+			if (mpp->nr_active > 0)
+				mpp->retry_tick = 0;
+			else
+				mpp->retry_tick = mpp->no_path_retry * conf->checkint;
+		}
+	}
+	return 0;
+}
+
+int
+cli_disable_queueing(void *v, char **reply, int *len, void *data)
+{
+	struct vectors * vecs = (struct vectors *)data;
+	char * mapname = get_keyparam(v, MAP);
+	struct multipath *mpp;
+	int minor;
+
+	condlog(2, "%s: disable map queueing (operator)", mapname);
+	if (sscanf(mapname, "dm-%d", &minor) == 1)
+		mpp = find_mp_by_minor(vecs->mpvec, minor);
+	else
+		mpp = find_mp_by_alias(vecs->mpvec, mapname);
+
+	if (!mpp) {
+		condlog(0, "%s: invalid map name, cannot disable queueing", mapname);
+		return 1;
+	}
+
+	mpp->retry_tick = 0;
+	dm_queue_if_no_path(mpp->alias, 0);
+	return 0;
+}
+
+int
+cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
+{
+	struct vectors * vecs = (struct vectors *)data;
+	struct multipath *mpp;
+	int i;
+
+	condlog(2, "disable queueing (operator)");
+	vector_foreach_slot(vecs->mpvec, mpp, i) {
+		mpp->retry_tick = 0;
+		dm_queue_if_no_path(mpp->alias, 0);
+	}
+	return 0;
+}
+
+int
 cli_switch_group(void * v, char ** reply, int * len, void * data)
 {
 	char * mapname = get_keyparam(v, MAP);
diff --git a/multipathd/cli_handlers.h b/multipathd/cli_handlers.h
index f57a160..10adaf9 100644
--- a/multipathd/cli_handlers.h
+++ b/multipathd/cli_handlers.h
@@ -17,6 +17,10 @@ int cli_add_map (void * v, char ** reply, int * len, void * data);
 int cli_del_map (void * v, char ** reply, int * len, void * data);
 int cli_switch_group(void * v, char ** reply, int * len, void * data);
 int cli_reconfigure(void * v, char ** reply, int * len, void * data);
+int cli_disable_queueing(void * v, char ** reply, int * len, void * data);
+int cli_disable_all_queueing(void * v, char ** reply, int * len, void * data);
+int cli_restore_queueing(void * v, char ** reply, int * len, void * data);
+int cli_restore_all_queueing(void * v, char ** reply, int * len, void * data);
 int cli_suspend(void * v, char ** reply, int * len, void * data);
 int cli_resume(void * v, char ** reply, int * len, void * data);
 int cli_reinstate(void * v, char ** reply, int * len, void * data);
diff --git a/multipathd/main.c b/multipathd/main.c
index dae9152..aedeeab 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -393,6 +393,7 @@ rescan:
 			return 1; /* leave path added to pathvec */
 
 		verify_paths(mpp, vecs, NULL);
+		mpp->flush_on_last_del = FLUSH_UNDEF;
 		mpp->action = ACT_RELOAD;
 	}
 	else {
@@ -503,6 +504,13 @@ ev_remove_path (char * devname, struct vectors * vecs)
 			 * flush_map will fail if the device is open
 			 */
 			strncpy(alias, mpp->alias, WWID_SIZE);
+			if (mpp->flush_on_last_del == FLUSH_ENABLED) {
+				condlog(2, "%s Last path deleted, disabling queueing", mpp->alias);
+				mpp->retry_tick = 0;
+				mpp->no_path_retry = NO_PATH_RETRY_FAIL;
+				mpp->flush_on_last_del = FLUSH_IN_PROGRESS;
+				dm_queue_if_no_path(mpp->alias, 0);
+			}
 			if (!flush_map(mpp, vecs)) {
 				condlog(2, "%s: removed map after"
 					" removing all paths",
@@ -725,6 +733,10 @@ uxlsnrloop (void * ap)
 	set_handler_callback(RESUME+MAP, cli_resume);
 	set_handler_callback(REINSTATE+PATH, cli_reinstate);
 	set_handler_callback(FAIL+PATH, cli_fail);
+	set_handler_callback(DISABLEQ+MAP, cli_disable_queueing);
+	set_handler_callback(RESTOREQ+MAP, cli_restore_queueing);
+	set_handler_callback(DISABLEQ+MAPS, cli_disable_all_queueing);
+	set_handler_callback(RESTOREQ+MAPS, cli_restore_all_queueing);
 	set_handler_callback(QUIT, cli_quit);
 
 	uxsock_listen(&uxsock_trigger, ap);
-- 
1.5.3.3

Re: [PATCH 3/3] Add options to multipathd to turn off queueing

[Xose Vazquez Perez]

On 03/12/2009 07:38 PM, Benjamin Marzinski wrote:
         ^^^^ Old patch.

> Even when the last path of a multipath device is deleted, it can't be
> removed until all the queued IO is flushed. For devices that have
> no_path_retry set to queue, this doesn't automatically happen.
> 
> This patch adds a "flush_on_last_del" config file option, that causes the
> multipath device to automatically turn off queueing when the last path is
> deleted.  It also adds the "disablequeueing" and "restorequeueing"
> multipathd cli commands.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> [...]
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 43611ff..c1bc591 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -279,6 +279,10 @@ select_prio (struct path * pp)
>  extern int
>  select_no_path_retry(struct multipath *mp)
>  {
> +	if (mp->flush_on_last_del == FLUSH_IN_PROGRESS) {
> +		condlog(0, "flush_on_last_del in progress");
                            ^^^^
If flush is done per LUN, it should be added a prefix(WWID or alias).

> +		mp->no_path_retry = NO_PATH_RETRY_FAIL;
> +	}
>  	if (mp->mpe && mp->mpe->no_path_retry != NO_PATH_RETRY_UNDEF) {
>  		mp->no_path_retry = mp->mpe->no_path_retry;
>  		condlog(3, "%s: no_path_retry = %i (multipath setting)",
> @@ -368,3 +372,31 @@ select_pg_timeout(struct multipath *mp)
>  	condlog(3, "pg_timeout = NONE (internal default)");
>  	return 0;
>  }
Thank you.

Re: [PATCH 3/3] Add options to multipathd to turn off queueing

[Benjamin Marzinski]

On Tue, Apr 25, 2017 at 03:38:38PM +0200, Xose Vazquez Perez wrote:
> On 03/12/2009 07:38 PM, Benjamin Marzinski wrote:
>          ^^^^ Old patch.
> 
> > Even when the last path of a multipath device is deleted, it can't be
> > removed until all the queued IO is flushed. For devices that have
> > no_path_retry set to queue, this doesn't automatically happen.
> > 
> > This patch adds a "flush_on_last_del" config file option, that causes the
> > multipath device to automatically turn off queueing when the last path is
> > deleted.  It also adds the "disablequeueing" and "restorequeueing"
> > multipathd cli commands.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > [...]
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 43611ff..c1bc591 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -279,6 +279,10 @@ select_prio (struct path * pp)
> >  extern int
> >  select_no_path_retry(struct multipath *mp)
> >  {
> > +	if (mp->flush_on_last_del == FLUSH_IN_PROGRESS) {
> > +		condlog(0, "flush_on_last_del in progress");
>                             ^^^^
> If flush is done per LUN, it should be added a prefix(WWID or alias).

Yep. I can send a patch for this.

Thanks.
-Ben

> 
> > +		mp->no_path_retry = NO_PATH_RETRY_FAIL;
> > +	}
> >  	if (mp->mpe && mp->mpe->no_path_retry != NO_PATH_RETRY_UNDEF) {
> >  		mp->no_path_retry = mp->mpe->no_path_retry;
> >  		condlog(3, "%s: no_path_retry = %i (multipath setting)",
> > @@ -368,3 +372,31 @@ select_pg_timeout(struct multipath *mp)
> >  	condlog(3, "pg_timeout = NONE (internal default)");
> >  	return 0;
> >  }
> Thank you.

Re: [PATCH 0/3] Some miscellaneous dm-multipath patches

[Christophe Varoqui]

Le Thu, 12 Mar 2009 13:38:10 -0500,
Benjamin Marzinski <bmarzins@redhat.com> a écrit :

> Here are a couple of my local multipath patches, that have been
> waiting to go upstream.
> 
> -Ben

I applied the set:
- including your reworked stack size patch
- adding the add_handler() for {disable,restore}queueing in cli.h

Thanks.