[PATCH 0/6] multipath cookie handling rework

[PATCH 0/6] multipath cookie handling rework

[Hannes Reinecke]

Hi all,

here's now an updated version of the cookie handling rework
for multipathd. It's a split off version of the original
SLES resync patch series sent earlier.

With this version I've fixed the 'reload' issue reported
by Ben Marzinski.

As usual, comments and reviews are welcome.

Hannes Reinecke (6):
  libmultipath: pass in cookie as argument for dm_simplecmd()
  libmultipath: pass in 'cookie' as argument for dm_addmap()
  Remove 'udev_sync' argument from dm_simplecmd()
  libmultipath: Fixup 'DM_DEVICE_RELOAD' handling
  devmapper: do not flush I/O for DM_DEVICE_CREATE
  libmultipath: fixup dm_rename to complete cookie on failure

 libmultipath/configure.c |  16 ++----
 libmultipath/devmapper.c | 123 +++++++++++++++++++++++++++++++----------------
 libmultipath/devmapper.h |   2 +-
 3 files changed, 86 insertions(+), 55 deletions(-)

-- 
2.6.6

[PATCH 1/6] libmultipath: pass in cookie as argument for dm_simplecmd()

[Hannes Reinecke]

Instead of generating the cookie internally we should be passing
it in as an argument; that will allow for cookie reuse.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/devmapper.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index fb202e8..48e7d50 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -207,11 +207,12 @@ dm_prereq (void)
 #define do_deferred(x) ((x) == DEFERRED_REMOVE_ON || (x) == DEFERRED_REMOVE_IN_PROGRESS)
 
 static int
-dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t udev_flags, int deferred_remove) {
+dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
+	      uint16_t udev_flags, int deferred_remove, uint32_t *cookie)
+{
 	int r = 0;
 	int udev_wait_flag = (need_sync && (task == DM_DEVICE_RESUME ||
 					    task == DM_DEVICE_REMOVE));
-	uint32_t cookie = 0;
 	struct dm_task *dmt;
 
 	if (!(dmt = dm_task_create (task)))
@@ -231,18 +232,18 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
 		dm_task_deferred_remove(dmt);
 #endif
 	if (udev_wait_flag &&
-	    !dm_task_set_cookie(dmt, &cookie,
+	    !dm_task_set_cookie(dmt, cookie,
 				DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags)) {
-		dm_udev_complete(cookie);
+		dm_udev_complete(*cookie);
 		goto out;
 	}
 	r = dm_task_run (dmt);
 
 	if (udev_wait_flag) {
 		if (!r)
-			dm_udev_complete(cookie);
+			dm_udev_complete(*cookie);
 		else
-			dm_udev_wait(cookie);
+			dm_udev_wait(*cookie);
 	}
 	out:
 	dm_task_destroy (dmt);
@@ -251,18 +252,24 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
 
 extern int
 dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags) {
-	return dm_simplecmd(task, name, 0, 1, udev_flags, 0);
+	uint32_t cookie = 0;
+
+	return dm_simplecmd(task, name, 0, 1, udev_flags, 0, &cookie);
 }
 
 extern int
 dm_simplecmd_noflush (int task, const char *name, int needsync, uint16_t udev_flags) {
-	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0);
+	uint32_t cookie = 0;
+
+	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0, &cookie);
 }
 
 static int
 dm_device_remove (const char *name, int needsync, int deferred_remove) {
+	uint32_t cookie = 0;
+
 	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, needsync, 0,
-			    deferred_remove);
+			    deferred_remove, &cookie);
 }
 
 static int
-- 
2.6.6

[PATCH 2/6] libmultipath: pass in 'cookie' as argument for dm_addmap()

[Hannes Reinecke]

Instead of generating the cookie internally we should be
passing in the cookie to dm_addmap().

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/devmapper.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 48e7d50..ab71fda 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -274,11 +274,10 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
 
 static int
 dm_addmap (int task, const char *target, struct multipath *mpp,
-	   char * params, int ro) {
+	   char * params, int ro, uint32_t *cookie) {
 	int r = 0;
 	struct dm_task *dmt;
 	char *prefixed_uuid = NULL;
-	uint32_t cookie = 0;
 
 	if (!(dmt = dm_task_create (task)))
 		return 0;
@@ -319,18 +318,18 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	dm_task_no_open_count(dmt);
 
 	if (task == DM_DEVICE_CREATE &&
-	    !dm_task_set_cookie(dmt, &cookie,
+	    !dm_task_set_cookie(dmt, cookie,
 				DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
-		dm_udev_complete(cookie);
+		dm_udev_complete(*cookie);
 		goto freeout;
 	}
 	r = dm_task_run (dmt);
 
 	if (task == DM_DEVICE_CREATE) {
 		if (!r)
-			dm_udev_complete(cookie);
+			dm_udev_complete(*cookie);
 		else
-			dm_udev_wait(cookie);
+			dm_udev_wait(*cookie);
 	}
 	freeout:
 	if (prefixed_uuid)
@@ -345,11 +344,13 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 extern int
 dm_addmap_create (struct multipath *mpp, char * params) {
 	int ro;
+	uint32_t cookie = 0;
 
 	for (ro = 0; ro <= 1; ro++) {
 		int err;
 
-		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro))
+		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH,
+			      mpp, params, ro, &cookie))
 			return 1;
 		/*
 		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
@@ -374,11 +375,15 @@ dm_addmap_create (struct multipath *mpp, char * params) {
 
 extern int
 dm_addmap_reload (struct multipath *mpp, char *params) {
-	if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW))
+	uint32_t cookie = 0;
+
+	if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
+		      ADDMAP_RW, &cookie))
 		return 1;
 	if (errno != EROFS)
 		return 0;
-	return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RO);
+	return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
+			 ADDMAP_RO, &cookie);
 }
 
 extern int
-- 
2.6.6

[PATCH 3/6] Remove 'udev_sync' argument from dm_simplecmd()

[Hannes Reinecke]

The 'udev_sync' attribute is pointless without a cookie, so we
can as well use the existence of the 'cookie' argument for
the same function.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/devmapper.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index ab71fda..9facbb9 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -207,12 +207,10 @@ dm_prereq (void)
 #define do_deferred(x) ((x) == DEFERRED_REMOVE_ON || (x) == DEFERRED_REMOVE_IN_PROGRESS)
 
 static int
-dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
+dm_simplecmd (int task, const char *name, int no_flush,
 	      uint16_t udev_flags, int deferred_remove, uint32_t *cookie)
 {
 	int r = 0;
-	int udev_wait_flag = (need_sync && (task == DM_DEVICE_RESUME ||
-					    task == DM_DEVICE_REMOVE));
 	struct dm_task *dmt;
 
 	if (!(dmt = dm_task_create (task)))
@@ -231,7 +229,7 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
 	if (do_deferred(deferred_remove))
 		dm_task_deferred_remove(dmt);
 #endif
-	if (udev_wait_flag &&
+	if (cookie &&
 	    !dm_task_set_cookie(dmt, cookie,
 				DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags)) {
 		dm_udev_complete(*cookie);
@@ -239,7 +237,7 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
 	}
 	r = dm_task_run (dmt);
 
-	if (udev_wait_flag) {
+	if (cookie) {
 		if (!r)
 			dm_udev_complete(*cookie);
 		else
@@ -254,22 +252,27 @@ extern int
 dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags) {
 	uint32_t cookie = 0;
 
-	return dm_simplecmd(task, name, 0, 1, udev_flags, 0, &cookie);
+	if (task == DM_DEVICE_RESUME ||
+	    task == DM_DEVICE_REMOVE)
+		return dm_simplecmd(task, name, 0, udev_flags, 0, &cookie);
+	else
+		return dm_simplecmd(task, name, 0, udev_flags, 0, NULL);
 }
 
 extern int
 dm_simplecmd_noflush (int task, const char *name, int needsync, uint16_t udev_flags) {
 	uint32_t cookie = 0;
 
-	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0, &cookie);
+	return dm_simplecmd(task, name, 1, udev_flags, 0,
+			    needsync ? &cookie : NULL);
 }
 
 static int
 dm_device_remove (const char *name, int needsync, int deferred_remove) {
 	uint32_t cookie = 0;
 
-	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, needsync, 0,
-			    deferred_remove, &cookie);
+	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, 0,
+			    deferred_remove, needsync ? &cookie : NULL);
 }
 
 static int
-- 
2.6.6

[PATCH 4/6] libmultipath: Fixup 'DM_DEVICE_RELOAD' handling

[Hannes Reinecke]

libdevmapper has the 'quirk' that DM_DEVICE_CREATE is translated
internally into a create/load/resume sequence, and the associated
cookie will wait for the last 'resume' to complete.
However, DM_DEVICE_RELOAD has no such translation, so if there
is a cookie assigned to it the caller _cannot_ wait for it,
as the cookie will only ever be completed upon the next
DM_DEVICE_RESUME.
multipathd already has some provisions for that (but even there
the cookie handling is dodgy), but 'multipath -r' doesn't know
about this.
So to avoid any future irritations this patch updates the
dm_addmad_reload() call to handle the cookie correctly,
and removes the special handling from multipathd.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/configure.c | 16 ++++------------
 libmultipath/devmapper.c | 41 ++++++++++++++++++++++++++++-------------
 libmultipath/devmapper.h |  2 +-
 3 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index affd1d5..ed6cf98 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -625,16 +625,11 @@ domap (struct multipath * mpp, char * params)
 		break;
 
 	case ACT_RELOAD:
-		r = dm_addmap_reload(mpp, params);
-		if (r)
-			r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
-						 0, MPATH_UDEV_RELOAD_FLAG);
+		r = dm_addmap_reload(mpp, params, 0);
 		break;
 
 	case ACT_RESIZE:
-		r = dm_addmap_reload(mpp, params);
-		if (r)
-			r = dm_simplecmd_flush(DM_DEVICE_RESUME, mpp->alias, 0);
+		r = dm_addmap_reload(mpp, params, 1);
 		break;
 
 	case ACT_RENAME:
@@ -643,11 +638,8 @@ domap (struct multipath * mpp, char * params)
 
 	case ACT_RENAME2:
 		r = dm_rename(mpp->alias_old, mpp->alias);
-		if (r) {
-			r = dm_addmap_reload(mpp, params);
-			if (r)
-				r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, 0, MPATH_UDEV_RELOAD_FLAG);
-		}
+		if (r)
+			r = dm_addmap_reload(mpp, params, 0);
 		break;
 
 	default:
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 9facbb9..04dcb72 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -315,12 +315,13 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	if (mpp->attribute_flags & (1 << ATTR_GID) &&
 	    !dm_task_set_gid(dmt, mpp->gid))
 		goto freeout;
-	condlog(4, "%s: addmap [0 %llu %s %s]", mpp->alias, mpp->size,
+	condlog(4, "%s: %s [0 %llu %s %s]", mpp->alias,
+		task == DM_DEVICE_RELOAD ? "reload" : "addmap", mpp->size,
 		target, params);
 
 	dm_task_no_open_count(dmt);
 
-	if (task == DM_DEVICE_CREATE &&
+	if (cookie &&
 	    !dm_task_set_cookie(dmt, cookie,
 				DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
 		dm_udev_complete(*cookie);
@@ -328,10 +329,11 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	}
 	r = dm_task_run (dmt);
 
-	if (task == DM_DEVICE_CREATE) {
-		if (!r)
+	if (cookie) {
+		if (!r || task != DM_DEVICE_CREATE) {
+			/* Do not wait on a cookie for DM_DEVICE_RELOAD */
 			dm_udev_complete(*cookie);
-		else
+		} else
 			dm_udev_wait(*cookie);
 	}
 	freeout:
@@ -377,16 +379,29 @@ dm_addmap_create (struct multipath *mpp, char * params) {
 #define ADDMAP_RO 1
 
 extern int
-dm_addmap_reload (struct multipath *mpp, char *params) {
+dm_addmap_reload (struct multipath *mpp, char *params, int flush) {
+	int r;
 	uint32_t cookie = 0;
+	uint16_t udev_flags = flush ? 0 : MPATH_UDEV_RELOAD_FLAG;
 
-	if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
-		      ADDMAP_RW, &cookie))
-		return 1;
-	if (errno != EROFS)
-		return 0;
-	return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
-			 ADDMAP_RO, &cookie);
+	/*
+	 * DM_DEVICE_RELOAD cannot wait on a cookie, as
+	 * the cookie will only ever be released after an
+	 * DM_DEVICE_RESUME. So call DM_DEVICE_RESUME
+	 * after each DM_DEVICE_RELOAD.
+	 */
+	r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
+		      ADDMAP_RW, &cookie);
+	if (!r) {
+		if (errno != EROFS)
+			return 0;
+		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
+			      ADDMAP_RO, &cookie);
+	}
+	if (r)
+		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, flush,
+				 udev_flags, 0, &cookie);
+	return r;
 }
 
 extern int
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 8dd0963..97f3742 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -18,7 +18,7 @@ int dm_drv_version (unsigned int * version, char * str);
 int dm_simplecmd_flush (int, const char *, uint16_t);
 int dm_simplecmd_noflush (int, const char *, int, uint16_t);
 int dm_addmap_create (struct multipath *mpp, char *params);
-int dm_addmap_reload (struct multipath *mpp, char *params);
+int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
 int dm_map_present (const char *);
 int dm_get_map(const char *, unsigned long long *, char *);
 int dm_get_status(char *, char *);
-- 
2.6.6

[PATCH 5/6] devmapper: do not flush I/O for DM_DEVICE_CREATE

[Hannes Reinecke]

DM_DEVICE_CREATE loads a new table, so there cannot be any
I/O pending. Hence we should be setting the 'no flush'
and 'skip lockfs' flag to avoid delays during creation.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 libmultipath/devmapper.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 04dcb72..0ae72fc 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -294,16 +294,23 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	if (ro)
 		dm_task_set_ro(dmt);
 
-	if ((task == DM_DEVICE_CREATE) && strlen(mpp->wwid) > 0){
-		prefixed_uuid = MALLOC(UUID_PREFIX_LEN + strlen(mpp->wwid) + 1);
-		if (!prefixed_uuid) {
-			condlog(0, "cannot create prefixed uuid : %s",
-				strerror(errno));
-			goto addout;
+	if (task == DM_DEVICE_CREATE) {
+		if (strlen(mpp->wwid) > 0) {
+			prefixed_uuid = MALLOC(UUID_PREFIX_LEN +
+					       strlen(mpp->wwid) + 1);
+			if (!prefixed_uuid) {
+				condlog(0, "cannot create prefixed uuid : %s",
+					strerror(errno));
+				goto addout;
+			}
+			sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
+			if (!dm_task_set_uuid(dmt, prefixed_uuid))
+				goto freeout;
 		}
-		sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
-		if (!dm_task_set_uuid(dmt, prefixed_uuid))
-			goto freeout;
+		dm_task_skip_lockfs(dmt);
+#ifdef LIBDM_API_FLUSH
+		dm_task_no_flush(dmt);
+#endif
 	}
 
 	if (mpp->attribute_flags & (1 << ATTR_MODE) &&
-- 
2.6.6

[PATCH 6/6] libmultipath: fixup dm_rename to complete cookie on failure

[Hannes Reinecke]

From my understanding we should be calling udev_complete() on
a cookie if dm_task_set_cookie() failed.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/devmapper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 0ae72fc..5396521 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1442,8 +1442,10 @@ dm_rename (const char * old, char * new)
 	dm_task_no_open_count(dmt);
 
 	if (!dm_task_set_cookie(dmt, &cookie,
-				DM_UDEV_DISABLE_LIBRARY_FALLBACK))
+				DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
+		dm_udev_complete(cookie);
 		goto out;
+	}
 	r = dm_task_run(dmt);
 
 	if (!r)
-- 
2.6.6

Re: [PATCH 1/6] libmultipath: pass in cookie as argument for dm_simplecmd()

[Benjamin Marzinski]

On Wed, May 04, 2016 at 07:57:25AM +0200, Hannes Reinecke wrote:
> Instead of generating the cookie internally we should be passing
> it in as an argument; that will allow for cookie reuse.

While this one doesn't break anything, it seems unhelpful. We are always
dealing with the cookies inside dm_simplecmd and dm_addmap. Once you
wait on a cookie (after more research, calling dm_udev_complete() looks
like it's always the wrong thing to do, but I'll get to that later). You
can't reuse it. You must have device-mapper give you an new
autogenerated cookie.  Because of this, there is no reason why anything
outside of dm_simplecmd or dm_addmap ever needs to know about cookies.
We are adding an extra parameter that needs to always be set to 0 (I
know that a later patch uses the cookie value to remove need_sync, and
I'll get to that later as well).

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/devmapper.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index fb202e8..48e7d50 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -207,11 +207,12 @@ dm_prereq (void)
>  #define do_deferred(x) ((x) == DEFERRED_REMOVE_ON || (x) == DEFERRED_REMOVE_IN_PROGRESS)
>  
>  static int
> -dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t udev_flags, int deferred_remove) {
> +dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
> +	      uint16_t udev_flags, int deferred_remove, uint32_t *cookie)
> +{
>  	int r = 0;
>  	int udev_wait_flag = (need_sync && (task == DM_DEVICE_RESUME ||
>  					    task == DM_DEVICE_REMOVE));
> -	uint32_t cookie = 0;
>  	struct dm_task *dmt;
>  
>  	if (!(dmt = dm_task_create (task)))
> @@ -231,18 +232,18 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
>  		dm_task_deferred_remove(dmt);
>  #endif
>  	if (udev_wait_flag &&
> -	    !dm_task_set_cookie(dmt, &cookie,
> +	    !dm_task_set_cookie(dmt, cookie,
>  				DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags)) {
> -		dm_udev_complete(cookie);
> +		dm_udev_complete(*cookie);
>  		goto out;
>  	}
>  	r = dm_task_run (dmt);
>  
>  	if (udev_wait_flag) {
>  		if (!r)
> -			dm_udev_complete(cookie);
> +			dm_udev_complete(*cookie);
>  		else
> -			dm_udev_wait(cookie);
> +			dm_udev_wait(*cookie);
>  	}
>  	out:
>  	dm_task_destroy (dmt);
> @@ -251,18 +252,24 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
>  
>  extern int
>  dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags) {
> -	return dm_simplecmd(task, name, 0, 1, udev_flags, 0);
> +	uint32_t cookie = 0;
> +
> +	return dm_simplecmd(task, name, 0, 1, udev_flags, 0, &cookie);
>  }
>  
>  extern int
>  dm_simplecmd_noflush (int task, const char *name, int needsync, uint16_t udev_flags) {
> -	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0);
> +	uint32_t cookie = 0;
> +
> +	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0, &cookie);
>  }
>  
>  static int
>  dm_device_remove (const char *name, int needsync, int deferred_remove) {
> +	uint32_t cookie = 0;
> +
>  	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, needsync, 0,
> -			    deferred_remove);
> +			    deferred_remove, &cookie);
>  }
>  
>  static int
> -- 
> 2.6.6

Re: [PATCH 2/6] libmultipath: pass in 'cookie' as argument for dm_addmap()

[Benjamin Marzinski]

On Wed, May 04, 2016 at 07:57:26AM +0200, Hannes Reinecke wrote:
> Instead of generating the cookie internally we should be
> passing in the cookie to dm_addmap().

Like I said in my comment to the last patch, you should never reuse a
cookie value once you've waited for it.  The only reason that this one
doesn't break is that multipath's cookie handling was already broken.
It got broken in this commit 4a2431aa944eb2e5b6f3ccd2d4fe1df67f9e5679
"Fixup device-mapper 'cookie' handling".

The thing that broke it was calling dm_udev_complete() instead of
dm_udev_wait() when dm_task_run() fails.  If dm_task_run() fails,
device-mapper will call dm_udev_complete() internally. However that
doesn't clean up the cookie.  The cookie won't be cleaned up until the
someone calls dm_udev_wait().  This won't actually wait for anything
since the cookie count has already been decremented, but it is necessary
for cleanup.

confusingly

# dmsetup udevcomplete_all

also cleans up the cookies, while

# dmsetup udevcomplete

doesn't.

You can easily see that multipath's udev cookie handling is currently
broken.  Just mount a path device, so that multipath will fail in
creating a multipath device with it, and then run

# multipath
# dmsetup udevcookies

and you will see a cooke with a value of 0, that is still sitting
around, because nobody has waited on it.  Because of that bug, this
patch doesn't break anything. The cookie you reuse is still around
because it hasn't been waited on, and you are able to increment it.
Once we replace the dm_udev_complete() calls with dm_udev_waits(), this
patch will start breaking things.

And again, while we could just reset the cookie to zero, and it wouldn't
break anything, we've just added a parameter that must always be set to
zero, and needlessly extended the cookie code outside of dm_addmap,
where it can all be contained.

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/devmapper.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 48e7d50..ab71fda 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -274,11 +274,10 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
>  
>  static int
>  dm_addmap (int task, const char *target, struct multipath *mpp,
> -	   char * params, int ro) {
> +	   char * params, int ro, uint32_t *cookie) {
>  	int r = 0;
>  	struct dm_task *dmt;
>  	char *prefixed_uuid = NULL;
> -	uint32_t cookie = 0;
>  
>  	if (!(dmt = dm_task_create (task)))
>  		return 0;
> @@ -319,18 +318,18 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	dm_task_no_open_count(dmt);
>  
>  	if (task == DM_DEVICE_CREATE &&
> -	    !dm_task_set_cookie(dmt, &cookie,
> +	    !dm_task_set_cookie(dmt, cookie,
>  				DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
> -		dm_udev_complete(cookie);
> +		dm_udev_complete(*cookie);
>  		goto freeout;
>  	}
>  	r = dm_task_run (dmt);
>  
>  	if (task == DM_DEVICE_CREATE) {
>  		if (!r)
> -			dm_udev_complete(cookie);
> +			dm_udev_complete(*cookie);
>  		else
> -			dm_udev_wait(cookie);
> +			dm_udev_wait(*cookie);
>  	}
>  	freeout:
>  	if (prefixed_uuid)
> @@ -345,11 +344,13 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  extern int
>  dm_addmap_create (struct multipath *mpp, char * params) {
>  	int ro;
> +	uint32_t cookie = 0;
>  
>  	for (ro = 0; ro <= 1; ro++) {
>  		int err;
>  
> -		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro))
> +		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH,
> +			      mpp, params, ro, &cookie))
>  			return 1;
>  		/*
>  		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
> @@ -374,11 +375,15 @@ dm_addmap_create (struct multipath *mpp, char * params) {
>  
>  extern int
>  dm_addmap_reload (struct multipath *mpp, char *params) {
> -	if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW))
> +	uint32_t cookie = 0;
> +
> +	if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> +		      ADDMAP_RW, &cookie))
>  		return 1;
>  	if (errno != EROFS)
>  		return 0;
> -	return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RO);
> +	return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> +			 ADDMAP_RO, &cookie);
>  }
>  
>  extern int
> -- 
> 2.6.6

Re: [PATCH 3/6] Remove 'udev_sync' argument from dm_simplecmd()

[Benjamin Marzinski]

On Wed, May 04, 2016 at 07:57:27AM +0200, Hannes Reinecke wrote:
> The 'udev_sync' attribute is pointless without a cookie, so we
> can as well use the existence of the 'cookie' argument for
> the same function.

Well, I've already said that we should leave the cookies inside
dm_simplecmd, and if we do that, we need the udev_sync code.

One reason to do this is that cookies aren't just used for
synchronization.  They are the mechanism by which udev flags are sent to
device-mapper.  Now it just so happens that we currently aren't sending
any udev_flags when we are not asking for synchronization, but that
doesn't mean it will always be the case.  So there could be a use
knowing whether or not we need synchronization, even in cases where we
know that we defintely need a cookie to pass flags to udev.

While again, we could do this by checking udev_flags as well and then
passing in a cookie and it wouldn't break anything, I just don't see the
point of passing in a pointer to a value that must be zero when we pass
it, and we're not allowed to use the result when we get it back.

-Ben 

> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  libmultipath/devmapper.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index ab71fda..9facbb9 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -207,12 +207,10 @@ dm_prereq (void)
>  #define do_deferred(x) ((x) == DEFERRED_REMOVE_ON || (x) == DEFERRED_REMOVE_IN_PROGRESS)
>  
>  static int
> -dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
> +dm_simplecmd (int task, const char *name, int no_flush,
>  	      uint16_t udev_flags, int deferred_remove, uint32_t *cookie)
>  {
>  	int r = 0;
> -	int udev_wait_flag = (need_sync && (task == DM_DEVICE_RESUME ||
> -					    task == DM_DEVICE_REMOVE));
>  	struct dm_task *dmt;
>  
>  	if (!(dmt = dm_task_create (task)))
> @@ -231,7 +229,7 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
>  	if (do_deferred(deferred_remove))
>  		dm_task_deferred_remove(dmt);
>  #endif
> -	if (udev_wait_flag &&
> +	if (cookie &&
>  	    !dm_task_set_cookie(dmt, cookie,
>  				DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags)) {
>  		dm_udev_complete(*cookie);
> @@ -239,7 +237,7 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
>  	}
>  	r = dm_task_run (dmt);
>  
> -	if (udev_wait_flag) {
> +	if (cookie) {
>  		if (!r)
>  			dm_udev_complete(*cookie);
>  		else
> @@ -254,22 +252,27 @@ extern int
>  dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags) {
>  	uint32_t cookie = 0;
>  
> -	return dm_simplecmd(task, name, 0, 1, udev_flags, 0, &cookie);
> +	if (task == DM_DEVICE_RESUME ||
> +	    task == DM_DEVICE_REMOVE)
> +		return dm_simplecmd(task, name, 0, udev_flags, 0, &cookie);
> +	else
> +		return dm_simplecmd(task, name, 0, udev_flags, 0, NULL);
>  }
>  
>  extern int
>  dm_simplecmd_noflush (int task, const char *name, int needsync, uint16_t udev_flags) {
>  	uint32_t cookie = 0;
>  
> -	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0, &cookie);
> +	return dm_simplecmd(task, name, 1, udev_flags, 0,
> +			    needsync ? &cookie : NULL);
>  }
>  
>  static int
>  dm_device_remove (const char *name, int needsync, int deferred_remove) {
>  	uint32_t cookie = 0;
>  
> -	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, needsync, 0,
> -			    deferred_remove, &cookie);
> +	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, 0,
> +			    deferred_remove, needsync ? &cookie : NULL);
>  }
>  
>  static int
> -- 
> 2.6.6

Re: [PATCH 4/6] libmultipath: Fixup 'DM_DEVICE_RELOAD' handling

[Benjamin Marzinski]

On Wed, May 04, 2016 at 07:57:28AM +0200, Hannes Reinecke wrote:
> libdevmapper has the 'quirk' that DM_DEVICE_CREATE is translated
> internally into a create/load/resume sequence, and the associated
> cookie will wait for the last 'resume' to complete.
> However, DM_DEVICE_RELOAD has no such translation, so if there
> is a cookie assigned to it the caller _cannot_ wait for it,
> as the cookie will only ever be completed upon the next
> DM_DEVICE_RESUME.
> multipathd already has some provisions for that (but even there
> the cookie handling is dodgy), but 'multipath -r' doesn't know
> about this.
> So to avoid any future irritations this patch updates the
> dm_addmad_reload() call to handle the cookie correctly,
> and removes the special handling from multipathd.

I don't see what's multipathd specific about any of the handling here.
The real answer is that device-mapper does nothing with cookies on
table reload. We should never be calling dm_task_set_cookie() for 
dm_addmap(DM_DEVICE_RELOAD, ...) calls in the first place. We end up
creating a cookie, decrementing the cookie, incrementing the cookie, and
finally waiting on it, when we could just be creating a cookie and then
waiting on it.

It's kind of hard to find an easy to show example of this breaking. You
would need to have the dm_addmap() command fail with some other error than
EROFS.  If that happens, the dm_simplecmd() call will never happen, and
there will be a cookie sitting around on the system.  If we never added
a cookie to the task in dm_addmap(DM_DEVICE_RELOAD, ...), then there
wouldn't be this cookie sitting around.

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/configure.c | 16 ++++------------
>  libmultipath/devmapper.c | 41 ++++++++++++++++++++++++++++-------------
>  libmultipath/devmapper.h |  2 +-
>  3 files changed, 33 insertions(+), 26 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index affd1d5..ed6cf98 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -625,16 +625,11 @@ domap (struct multipath * mpp, char * params)
>  		break;
>  
>  	case ACT_RELOAD:
> -		r = dm_addmap_reload(mpp, params);
> -		if (r)
> -			r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> -						 0, MPATH_UDEV_RELOAD_FLAG);
> +		r = dm_addmap_reload(mpp, params, 0);
>  		break;
>  
>  	case ACT_RESIZE:
> -		r = dm_addmap_reload(mpp, params);
> -		if (r)
> -			r = dm_simplecmd_flush(DM_DEVICE_RESUME, mpp->alias, 0);
> +		r = dm_addmap_reload(mpp, params, 1);
>  		break;
>  
>  	case ACT_RENAME:
> @@ -643,11 +638,8 @@ domap (struct multipath * mpp, char * params)
>  
>  	case ACT_RENAME2:
>  		r = dm_rename(mpp->alias_old, mpp->alias);
> -		if (r) {
> -			r = dm_addmap_reload(mpp, params);
> -			if (r)
> -				r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, 0, MPATH_UDEV_RELOAD_FLAG);
> -		}
> +		if (r)
> +			r = dm_addmap_reload(mpp, params, 0);
>  		break;
>  
>  	default:
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 9facbb9..04dcb72 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -315,12 +315,13 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	if (mpp->attribute_flags & (1 << ATTR_GID) &&
>  	    !dm_task_set_gid(dmt, mpp->gid))
>  		goto freeout;
> -	condlog(4, "%s: addmap [0 %llu %s %s]", mpp->alias, mpp->size,
> +	condlog(4, "%s: %s [0 %llu %s %s]", mpp->alias,
> +		task == DM_DEVICE_RELOAD ? "reload" : "addmap", mpp->size,
>  		target, params);
>  
>  	dm_task_no_open_count(dmt);
>  
> -	if (task == DM_DEVICE_CREATE &&
> +	if (cookie &&
>  	    !dm_task_set_cookie(dmt, cookie,
>  				DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
>  		dm_udev_complete(*cookie);
> @@ -328,10 +329,11 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	}
>  	r = dm_task_run (dmt);
>  
> -	if (task == DM_DEVICE_CREATE) {
> -		if (!r)
> +	if (cookie) {
> +		if (!r || task != DM_DEVICE_CREATE) {
> +			/* Do not wait on a cookie for DM_DEVICE_RELOAD */
>  			dm_udev_complete(*cookie);
> -		else
> +		} else
>  			dm_udev_wait(*cookie);
>  	}
>  	freeout:
> @@ -377,16 +379,29 @@ dm_addmap_create (struct multipath *mpp, char * params) {
>  #define ADDMAP_RO 1
>  
>  extern int
> -dm_addmap_reload (struct multipath *mpp, char *params) {
> +dm_addmap_reload (struct multipath *mpp, char *params, int flush) {
> +	int r;
>  	uint32_t cookie = 0;
> +	uint16_t udev_flags = flush ? 0 : MPATH_UDEV_RELOAD_FLAG;
>  
> -	if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> -		      ADDMAP_RW, &cookie))
> -		return 1;
> -	if (errno != EROFS)
> -		return 0;
> -	return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> -			 ADDMAP_RO, &cookie);
> +	/*
> +	 * DM_DEVICE_RELOAD cannot wait on a cookie, as
> +	 * the cookie will only ever be released after an
> +	 * DM_DEVICE_RESUME. So call DM_DEVICE_RESUME
> +	 * after each DM_DEVICE_RELOAD.
> +	 */
> +	r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> +		      ADDMAP_RW, &cookie);
> +	if (!r) {
> +		if (errno != EROFS)
> +			return 0;
> +		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> +			      ADDMAP_RO, &cookie);
> +	}
> +	if (r)
> +		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, flush,
> +				 udev_flags, 0, &cookie);
> +	return r;
>  }
>  
>  e



xtern int
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index 8dd0963..97f3742 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -18,7 +18,7 @@ int dm_drv_version (unsigned int * version, char * str);
>  int dm_simplecmd_flush (int, const char *, uint16_t);
>  int dm_simplecmd_noflush (int, const char *, int, uint16_t);
>  int dm_addmap_create (struct multipath *mpp, char *params);
> -int dm_addmap_reload (struct multipath *mpp, char *params);
> +int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
>  int dm_map_present (const char *);
>  int dm_get_map(const char *, unsigned long long *, char *);
>  int dm_get_status(char *, char *);
> -- 
> 2.6.6

Re: [PATCH 5/6] devmapper: do not flush I/O for DM_DEVICE_CREATE

[Benjamin Marzinski]

On Wed, May 04, 2016 at 07:57:29AM +0200, Hannes Reinecke wrote:
> DM_DEVICE_CREATE loads a new table, so there cannot be any
> I/O pending. Hence we should be setting the 'no flush'
> and 'skip lockfs' flag to avoid delays during creation.

ACK

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  libmultipath/devmapper.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 04dcb72..0ae72fc 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -294,16 +294,23 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	if (ro)
>  		dm_task_set_ro(dmt);
>  
> -	if ((task == DM_DEVICE_CREATE) && strlen(mpp->wwid) > 0){
> -		prefixed_uuid = MALLOC(UUID_PREFIX_LEN + strlen(mpp->wwid) + 1);
> -		if (!prefixed_uuid) {
> -			condlog(0, "cannot create prefixed uuid : %s",
> -				strerror(errno));
> -			goto addout;
> +	if (task == DM_DEVICE_CREATE) {
> +		if (strlen(mpp->wwid) > 0) {
> +			prefixed_uuid = MALLOC(UUID_PREFIX_LEN +
> +					       strlen(mpp->wwid) + 1);
> +			if (!prefixed_uuid) {
> +				condlog(0, "cannot create prefixed uuid : %s",
> +					strerror(errno));
> +				goto addout;
> +			}
> +			sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
> +			if (!dm_task_set_uuid(dmt, prefixed_uuid))
> +				goto freeout;
>  		}
> -		sprintf(prefixed_uuid, UUID_PREFIX "%s", mpp->wwid);
> -		if (!dm_task_set_uuid(dmt, prefixed_uuid))
> -			goto freeout;
> +		dm_task_skip_lockfs(dmt);
> +#ifdef LIBDM_API_FLUSH
> +		dm_task_no_flush(dmt);
> +#endif
>  	}
>  
>  	if (mpp->attribute_flags & (1 << ATTR_MODE) &&
> -- 
> 2.6.6

Re: [PATCH 6/6] libmultipath: fixup dm_rename to complete cookie on failure

[Benjamin Marzinski]

On Wed, May 04, 2016 at 07:57:30AM +0200, Hannes Reinecke wrote:
> >From my understanding we should be calling udev_complete() on
> a cookie if dm_task_set_cookie() failed.

I was wrong when I said that this will sometimes be helpful to us. It is
true that it won't hurt things. But that is because it will never do
anything.  There does appear to be a bug in dm_task_set_cookie(), where
it can fail without cleaning up the cookie.  However, when it does that,
it will always zero out the cookie value before returning, so we will
never be able to clean up the cookie ourselves. Also, like I mentioned,
dm_udev_complete doesn't actually clean up the cookie completely. It
needs to be waited on for that.

I don't think that there is ever a time when we should be calling
dm_udev_complete(). None of the dmsetup cookie handling code does. If we
fail to set the cookie, we should just act like the cookie has been
cleaned up by device-mapper (and we need to make device-mapper actually
do that).  Once we call dm_task_run() after setting a cookie, we should
always call dm_udev_wait(), regardless of the whether or not
dm_task_run() fails. And we should never call dm_task_set_cookie()
cookie in the first place for tasks that will not generate a udev event.
That should make this all work correctly.

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  libmultipath/devmapper.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 0ae72fc..5396521 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -1442,8 +1442,10 @@ dm_rename (const char * old, char * new)
>  	dm_task_no_open_count(dmt);
>  
>  	if (!dm_task_set_cookie(dmt, &cookie,
> -				DM_UDEV_DISABLE_LIBRARY_FALLBACK))
> +				DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
> +		dm_udev_complete(cookie);
>  		goto out;
> +	}
>  	r = dm_task_run(dmt);
>  
>  	if (!r)
> -- 
> 2.6.6

Re: [PATCH 6/6] libmultipath: fixup dm_rename to complete cookie on failure

[Alasdair G Kergon]

The library functions and return states are supposed to be well-defined.

If you think you've found a cookie leak on an error path within a library
function, we can investigate that and fix the library if need be.

Alasdair

Re: [PATCH 6/6] libmultipath: fixup dm_rename to complete cookie on failure

[Benjamin Marzinski]

On Fri, May 06, 2016 at 10:59:01PM +0100, Alasdair G Kergon wrote:
> The library functions and return states are supposed to be well-defined.
> 
> If you think you've found a cookie leak on an error path within a library
> function, we can investigate that and fix the library if need be.

In _udev_notify_sem_create(), after dm creates the semaphore and sets
it's value, it calls

semctl(gen_semid, 0, GETVAL) to get the value.  I don't know how likely
this call is to ever fail, but if it does, _udev_notify_sem_create()
sets the cookie pointer to 0, so the caller has no access to the
semaphore, and returns an error, leaving the semaphore to just sit
there.

Conversely, if setting the value fails, dm destroys the semaphore before
returning, so that everything is the same as before the call happened.

There's a similar issue in _udev_notify_sem_inc, but in this case the
cookie is returned along with the error.  However, from the caller's
perspective, the cookie is still in an indeterminate state.

I'll open a bugzilla for it.

-Ben


> 
> Alasdair

Re: [PATCH 4/6] libmultipath: Fixup 'DM_DEVICE_RELOAD' handling

[Hannes Reinecke]

On 05/06/2016 10:12 PM, Benjamin Marzinski wrote:
> On Wed, May 04, 2016 at 07:57:28AM +0200, Hannes Reinecke wrote:
>> libdevmapper has the 'quirk' that DM_DEVICE_CREATE is translated
>> internally into a create/load/resume sequence, and the associated
>> cookie will wait for the last 'resume' to complete.
>> However, DM_DEVICE_RELOAD has no such translation, so if there
>> is a cookie assigned to it the caller _cannot_ wait for it,
>> as the cookie will only ever be completed upon the next
>> DM_DEVICE_RESUME.
>> multipathd already has some provisions for that (but even there
>> the cookie handling is dodgy), but 'multipath -r' doesn't know
>> about this.
>> So to avoid any future irritations this patch updates the
>> dm_addmad_reload() call to handle the cookie correctly,
>> and removes the special handling from multipathd.
> 
> I don't see what's multipathd specific about any of the handling here.
> The real answer is that device-mapper does nothing with cookies on
> table reload. We should never be calling dm_task_set_cookie() for 
> dm_addmap(DM_DEVICE_RELOAD, ...) calls in the first place. We end up
> creating a cookie, decrementing the cookie, incrementing the cookie, and
> finally waiting on it, when we could just be creating a cookie and then
> waiting on it.
> 
> It's kind of hard to find an easy to show example of this breaking. You
> would need to have the dm_addmap() command fail with some other error than
> EROFS.  If that happens, the dm_simplecmd() call will never happen, and
> there will be a cookie sitting around on the system.  If we never added
> a cookie to the task in dm_addmap(DM_DEVICE_RELOAD, ...), then there
> wouldn't be this cookie sitting around.
> 
But then ... how is this supposed to work?
Are we supposed to call DM_DEVICE_RESUME even after DM_DEVICE_RELOAD
failed?
None of the other callers do that ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 4/6] libmultipath: Fixup 'DM_DEVICE_RELOAD' handling

[Benjamin Marzinski]

On Mon, May 09, 2016 at 12:26:36PM +0200, Hannes Reinecke wrote:
> On 05/06/2016 10:12 PM, Benjamin Marzinski wrote:
> > On Wed, May 04, 2016 at 07:57:28AM +0200, Hannes Reinecke wrote:
> >> libdevmapper has the 'quirk' that DM_DEVICE_CREATE is translated
> >> internally into a create/load/resume sequence, and the associated
> >> cookie will wait for the last 'resume' to complete.
> >> However, DM_DEVICE_RELOAD has no such translation, so if there
> >> is a cookie assigned to it the caller _cannot_ wait for it,
> >> as the cookie will only ever be completed upon the next
> >> DM_DEVICE_RESUME.
> >> multipathd already has some provisions for that (but even there
> >> the cookie handling is dodgy), but 'multipath -r' doesn't know
> >> about this.
> >> So to avoid any future irritations this patch updates the
> >> dm_addmad_reload() call to handle the cookie correctly,
> >> and removes the special handling from multipathd.
> > 
> > I don't see what's multipathd specific about any of the handling here.
> > The real answer is that device-mapper does nothing with cookies on
> > table reload. We should never be calling dm_task_set_cookie() for 
> > dm_addmap(DM_DEVICE_RELOAD, ...) calls in the first place. We end up
> > creating a cookie, decrementing the cookie, incrementing the cookie, and
> > finally waiting on it, when we could just be creating a cookie and then
> > waiting on it.
> > 
> > It's kind of hard to find an easy to show example of this breaking. You
> > would need to have the dm_addmap() command fail with some other error than
> > EROFS.  If that happens, the dm_simplecmd() call will never happen, and
> > there will be a cookie sitting around on the system.  If we never added
> > a cookie to the task in dm_addmap(DM_DEVICE_RELOAD, ...), then there
> > wouldn't be this cookie sitting around.
> > 
> But then ... how is this supposed to work?
> Are we supposed to call DM_DEVICE_RESUME even after DM_DEVICE_RELOAD
> failed?
> None of the other callers do that ...

No. If we fail to load a new table, we do nothing, and the device
continues as it is.  If we don't ever call dm_task_set_cookie when we
reload the table, then there is no cookie to wait for. And if loading a
new table fails, then there is no new table to switch to, and no point
in calling resume.

-Ben

> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		   Teamlead Storage & Networking
> hare@suse.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)