Re: [PATCH 43/78] Fixup device-mapper 'cookie' handling

["Benjamin Marzinski" <bmarzins@redhat.com>]

On Wed, Mar 25, 2015 at 11:30:57AM -0500, Benjamin Marzinski wrote:
> On Mon, Mar 16, 2015 at 01:36:30PM +0100, Hannes Reinecke wrote:
> > device-mapper has a 'cookie', which is inserted with the ioctl
> > for modifying device-mapper devices.
> > It is used as a synchronization point between udev and any other
> > applications to notify the latter when udev has finished
> > processing the event.
> > Originally multipath would only use a single cookie for every
> > transaction, and wait for that cookie at the end of the program.
> > Which works well if you only have one transaction, but for several
> > (like calling 'multipath') it will actually overwrite the cookie
> > and fail to wait for earlier events.

Another thing that might be interesting to try for debugging purposes:

If you enable DM_UDEV_DISABLE_LIBRARY_FALLBACK for multipath as well as
multipathd, then multipath shouldn't failback to manual device node
creation.  Even if multipath isn't waiting on it, udev should create the
device node eventually.  If you are always getting all of your device
nodes when you do this, then it seems that there is a race going on.  If
nodes occasionally aren't getting created at all when
DM_UDEV_DISABLE_LIBRARY_FALLBACK is enabled, then it would seem to point
to an issue in udev.

just a thought.

> 
> That shouldn't be happening.  Looking at the dm_task_set_cookie code
> 
>         if (*cookie) {
>                 if (!_get_cookie_sem(*cookie, &semid))
>                         goto_bad;
>         } else if (!_udev_notify_sem_create(cookie, &semid))
>                 goto_bad;
> 
> The first time we use that cookie, it should be zero, so a new semaphore
> will be created, and the id will be returned to us.  Subsequent calls to
> dm_task_set_cookie with the same cookie (which is now non-zero) should
> just be using the same semaphore, allowing you you wait just one time on
> all the actions linked to that cookie.  This should be more efficient
> than having to wait on each action (since udev can complete them in the
> background as we go on).
> 
> If there really are cookies that are not waited for, you should be able
> to see that by running
> 
> # dmsetup udevcookies
> 
> After running the multipath command.  Cookies won't go away until
> someone waits for them. If there are cookies listed there, then my guess
> would be that something is resetting conf->cookie to 0 after our first
> call to dm_task_set_cookie.  If there aren't, then it would appear that
> something else, instead of not waiting for the cookies, is causing
> device mapper to fail back to manual device node creation.
> 
> If you can verify that you are passing the cookie value that you get
> back from the first call to dm_task_set_cookie() into sebsequent calls,
> and you still are seeing non-watited for cookies with "dmsetup
> udevcookies" then that sounds to me like a problem in device-mapper, and
> we should check that out.
> 
> -Ben
> 
> > This causes libdevmapper to create the device nodes on its own,
> > and the device nodes not being handled by udev.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> >  kpartx/devmapper.c        | 53 +++++++++++++++++++++++++++++++++++------------
> >  kpartx/devmapper.h        |  4 ++--
> >  kpartx/kpartx.c           | 22 +++++++++-----------
> >  libmultipath/config.h     |  1 -
> >  libmultipath/configure.c  |  5 +++--
> >  libmultipath/devmapper.c  | 48 +++++++++++++++++++++++++++++++-----------
> >  libmultipath/devmapper.h  |  2 +-
> >  multipath/main.c          |  2 --
> >  multipathd/cli_handlers.c |  4 ++--
> >  9 files changed, 94 insertions(+), 47 deletions(-)
> > 
> > diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
> > index a3272d4..82be990 100644
> > --- a/kpartx/devmapper.c
> > +++ b/kpartx/devmapper.c
> > @@ -14,13 +14,6 @@
> >  #define MAX_PREFIX_LEN 8
> >  #define PARAMS_SIZE 1024
> >  
> > -#ifndef LIBDM_API_COOKIE
> > -static inline int dm_task_set_cookie(struct dm_task *dmt, uint32_t *c, int a)
> > -{
> > -	return 1;
> > -}
> > -#endif
> > -
> >  extern int
> >  dm_prereq (char * str, int x, int y, int z)
> >  {
> > @@ -60,10 +53,13 @@ dm_prereq (char * str, int x, int y, int z)
> >  }
> >  
> >  extern int
> > -dm_simplecmd (int task, const char *name, int no_flush, uint32_t *cookie, uint16_t udev_flags) {
> > +dm_simplecmd (int task, const char *name, int no_flush, uint16_t udev_flags) {
> >  	int r = 0;
> >  	int udev_wait_flag = (task == DM_DEVICE_RESUME ||
> >  			      task == DM_DEVICE_REMOVE);
> > +#ifdef LIBDM_API_COOKIE
> > +	uint32_t cookie = 0;
> > +#endif
> >  	struct dm_task *dmt;
> >  
> >  	if (!(dmt = dm_task_create(task)))
> > @@ -78,10 +74,23 @@ dm_simplecmd (int task, const char *name, int no_flush, uint32_t *cookie, uint16
> >  	if (no_flush)
> >  		dm_task_no_flush(dmt);
> >  
> > -	if (udev_wait_flag && !dm_task_set_cookie(dmt, cookie, ((udev_sync)? 0 : DM_UDEV_DISABLE_LIBRARY_FALLBACK) | udev_flags))
> > +#ifdef LIBDM_API_COOKIE
> > +	if (!udev_sync)
> > +		udev_flags |= DM_UDEV_DISABLE_LIBRARY_FALLBACK;
> > +	if (udev_wait_flag && !dm_task_set_cookie(dmt, &cookie, udev_flags)) {
> > +		dm_udev_complete(cookie);
> >  		goto out;
> > +	}
> > +#endif
> >  	r = dm_task_run(dmt);
> > -
> > +#ifdef LIBDM_API_COOKIE
> > +	if (udev_wait_flag) {
> > +		if (!r)
> > +			dm_udev_complete(cookie);
> > +		else
> > +			dm_udev_wait(cookie);
> > +	}
> > +#endif
> >  	out:
> >  	dm_task_destroy(dmt);
> >  	return r;
> > @@ -90,10 +99,14 @@ dm_simplecmd (int task, const char *name, int no_flush, uint32_t *cookie, uint16
> >  extern int
> >  dm_addmap (int task, const char *name, const char *target,
> >  	   const char *params, uint64_t size, int ro, const char *uuid, int part,
> > -	   mode_t mode, uid_t uid, gid_t gid, uint32_t *cookie) {
> > +	   mode_t mode, uid_t uid, gid_t gid) {
> >  	int r = 0;
> >  	struct dm_task *dmt;
> >  	char *prefixed_uuid = NULL;
> > +#ifdef LIBDM_API_COOKIE
> > +	uint32_t cookie = 0;
> > +	uint16_t udev_flags = 0;
> > +#endif
> >  
> >  	if (!(dmt = dm_task_create (task)))
> >  		return 0;
> > @@ -128,10 +141,24 @@ dm_addmap (int task, const char *name, const char *target,
> >  
> >  	dm_task_no_open_count(dmt);
> >  
> > -	if (task == DM_DEVICE_CREATE && !dm_task_set_cookie(dmt, cookie, (udev_sync)? 0 : DM_UDEV_DISABLE_LIBRARY_FALLBACK))
> > +#ifdef LIBDM_API_COOKIE
> > +	if (!udev_sync)
> > +		udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK;
> > +	if (task == DM_DEVICE_CREATE &&
> > +	    !dm_task_set_cookie(dmt, &cookie, udev_flags)) {
> > +		dm_udev_complete(cookie);
> >  		goto addout;
> > +	}
> > +#endif
> >  	r = dm_task_run (dmt);
> > -
> > +#ifdef LIBDM_API_COOKIE
> > +	if (task == DM_DEVICE_CREATE) {
> > +		if (!r)
> > +			dm_udev_complete(cookie);
> > +		else
> > +			dm_udev_wait(cookie);
> > +	}
> > +#endif
> >  addout:
> >  	dm_task_destroy (dmt);
> >  	free(prefixed_uuid);
> > diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
> > index 4b867df..ac1d5d9 100644
> > --- a/kpartx/devmapper.h
> > +++ b/kpartx/devmapper.h
> > @@ -10,9 +10,9 @@
> >  extern int udev_sync;
> >  
> >  int dm_prereq (char *, int, int, int);
> > -int dm_simplecmd (int, const char *, int, uint32_t *, uint16_t);
> > +int dm_simplecmd (int, const char *, int, uint16_t);
> >  int dm_addmap (int, const char *, const char *, const char *, uint64_t,
> > -	       int, const char *, int, mode_t, uid_t, gid_t, uint32_t *);
> > +	       int, const char *, int, mode_t, uid_t, gid_t);
> >  int dm_map_present (char *);
> >  char * dm_mapname(int major, int minor);
> >  dev_t dm_get_first_dep(char *devname);
> > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> > index 18c1d23..d69f9af 100644
> > --- a/kpartx/kpartx.c
> > +++ b/kpartx/kpartx.c
> > @@ -208,7 +208,6 @@ main(int argc, char **argv){
> >  	int hotplug = 0;
> >  	int loopcreated = 0;
> >  	struct stat buf;
> > -	uint32_t cookie = 0;
> >  
> >  	initpts();
> >  	init_crc32();
> > @@ -281,6 +280,8 @@ main(int argc, char **argv){
> >  #ifdef LIBDM_API_COOKIE
> >  	if (!udev_sync)
> >  		dm_udev_set_sync_support(0);
> > +	else
> > +		dm_udev_set_sync_support(1);
> >  #endif
> >  
> >  	if (dm_prereq(DM_TARGET, 0, 0, 0) && (what == ADD || what == DELETE || what == UPDATE)) {
> > @@ -437,7 +438,7 @@ main(int argc, char **argv){
> >  					continue;
> >  
> >  				if (!dm_simplecmd(DM_DEVICE_REMOVE, partname,
> > -						  0, &cookie, 0)) {
> > +						  0, 0)) {
> >  					r++;
> >  					continue;
> >  				}
> > @@ -488,18 +489,19 @@ main(int argc, char **argv){
> >  				if (!dm_addmap(op, partname, DM_TARGET, params,
> >  					       slices[j].size, ro, uuid, j+1,
> >  					       buf.st_mode & 0777, buf.st_uid,
> > -					       buf.st_gid, &cookie)) {
> > +					       buf.st_gid)) {
> >  					fprintf(stderr, "create/reload failed on %s\n",
> >  						partname);
> >  					r++;
> >  				}
> >  				if (op == DM_DEVICE_RELOAD &&
> >  				    !dm_simplecmd(DM_DEVICE_RESUME, partname,
> > -						  1, &cookie, MPATH_UDEV_RELOAD_FLAG)) {
> > +						  1, MPATH_UDEV_RELOAD_FLAG)) {
> >  					fprintf(stderr, "resume failed on %s\n",
> >  						partname);
> >  					r++;
> >  				}
> > +
> >  				dm_devn(partname, &slices[j].major,
> >  					&slices[j].minor);
> >  
> > @@ -551,14 +553,12 @@ main(int argc, char **argv){
> >  					dm_addmap(op, partname, DM_TARGET, params,
> >  						  slices[j].size, ro, uuid, j+1,
> >  						  buf.st_mode & 0777,
> > -						  buf.st_uid, buf.st_gid,
> > -						  &cookie);
> > +						  buf.st_uid, buf.st_gid);
> >  
> >  					if (op == DM_DEVICE_RELOAD)
> >  						dm_simplecmd(DM_DEVICE_RESUME,
> >  							     partname, 1,
> > -							     &cookie, MPATH_UDEV_RELOAD_FLAG);
> > -
> > +							     MPATH_UDEV_RELOAD_FLAG);
> >  					dm_devn(partname, &slices[j].major,
> >  						&slices[j].minor);
> >  
> > @@ -590,7 +590,7 @@ main(int argc, char **argv){
> >  					continue;
> >  
> >  				if (!dm_simplecmd(DM_DEVICE_REMOVE,
> > -						  partname, 1, &cookie, 0)) {
> > +						  partname, 1, 0)) {
> >  					r++;
> >  					continue;
> >  				}
> > @@ -616,9 +616,7 @@ main(int argc, char **argv){
> >  		}
> >  		printf("loop deleted : %s\n", device);
> >  	}
> > -#ifdef LIBDM_API_COOKIE
> > -	dm_udev_wait(cookie);
> > -#endif
> > +
> >  	dm_lib_release();
> >  	dm_lib_exit();
> >  
> > diff --git a/libmultipath/config.h b/libmultipath/config.h
> > index d304a6c..eff127e 100644
> > --- a/libmultipath/config.h
> > +++ b/libmultipath/config.h
> > @@ -127,7 +127,6 @@ struct config {
> >  	uid_t uid;
> >  	gid_t gid;
> >  	mode_t mode;
> > -	uint32_t cookie;
> >  	int reassign_maps;
> >  	int retain_hwhandler;
> >  	int detect_prio;
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 2465563..3c230a1 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -623,7 +623,8 @@ domap (struct multipath * mpp, char * params)
> >  	case ACT_RELOAD:
> >  		r = dm_addmap_reload(mpp, params);
> >  		if (r)
> > -			r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, MPATH_UDEV_RELOAD_FLAG);
> > +			r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> > +						 0, MPATH_UDEV_RELOAD_FLAG);
> >  		break;
> >  
> >  	case ACT_RESIZE:
> > @@ -641,7 +642,7 @@ domap (struct multipath * mpp, char * params)
> >  		if (r) {
> >  			r = dm_addmap_reload(mpp, params);
> >  			if (r)
> > -				r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, MPATH_UDEV_RELOAD_FLAG);
> > +				r = dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, 0, MPATH_UDEV_RELOAD_FLAG);
> >  		}
> >  		break;
> >  
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 1901052..f0b0da1 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -216,6 +216,7 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
> >  	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)))
> > @@ -234,10 +235,18 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
> >  	if (do_deferred(deferred_remove))
> >  		dm_task_deferred_remove(dmt);
> >  #endif
> > -	if (udev_wait_flag && !dm_task_set_cookie(dmt, &conf->cookie, ((conf->daemon)? DM_UDEV_DISABLE_LIBRARY_FALLBACK : 0) | udev_flags))
> > +	if (udev_wait_flag && !dm_task_set_cookie(dmt, &cookie, ((conf->daemon)? DM_UDEV_DISABLE_LIBRARY_FALLBACK : 0) | udev_flags)) {
> > +		dm_udev_complete(cookie);
> >  		goto out;
> > +	}
> >  	r = dm_task_run (dmt);
> >  
> > +	if (udev_wait_flag) {
> > +		if (!r)
> > +			dm_udev_complete(cookie);
> > +		else
> > +			udev_wait(cookie);
> > +	}
> >  	out:
> >  	dm_task_destroy (dmt);
> >  	return r;
> > @@ -249,8 +258,8 @@ dm_simplecmd_flush (int task, const char *name, int needsync, uint16_t udev_flag
> >  }
> >  
> >  extern int
> > -dm_simplecmd_noflush (int task, const char *name, uint16_t udev_flags) {
> > -	return dm_simplecmd(task, name, 1, 1, udev_flags, 0);
> > +dm_simplecmd_noflush (int task, const char *name, int needsync, uint16_t udev_flags) {
> > +	return dm_simplecmd(task, name, 1, needsync, udev_flags, 0);
> >  }
> >  
> >  static int
> > @@ -265,6 +274,7 @@ dm_addmap (int task, const char *target, struct multipath *mpp, char * params,
> >  	int r = 0;
> >  	struct dm_task *dmt;
> >  	char *prefixed_uuid = NULL;
> > +	uint32_t cookie = 0;
> >  
> >  	if (!(dmt = dm_task_create (task)))
> >  		return 0;
> > @@ -305,10 +315,18 @@ dm_addmap (int task, const char *target, struct multipath *mpp, char * params,
> >  	dm_task_no_open_count(dmt);
> >  
> >  	if (task == DM_DEVICE_CREATE &&
> > -	    !dm_task_set_cookie(dmt, &conf->cookie, (conf->daemon)? DM_UDEV_DISABLE_LIBRARY_FALLBACK : 0))
> > +	    !dm_task_set_cookie(dmt, &cookie, (conf->daemon)? DM_UDEV_DISABLE_LIBRARY_FALLBACK : 0)) {
> > +		dm_udev_complete(cookie);
> >  		goto freeout;
> > +	}
> >  	r = dm_task_run (dmt);
> >  
> > +	if (task == DM_DEVICE_CREATE) {
> > +		if (!r)
> > +			dm_udev_complete(cookie);
> > +		else
> > +			udev_wait(cookie);
> > +	}
> >  	freeout:
> >  	if (prefixed_uuid)
> >  		FREE(prefixed_uuid);
> > @@ -326,7 +344,8 @@ dm_addmap_create (struct multipath *mpp, char * params) {
> >  	for (ro = 0; ro <= 1; ro++) {
> >  		int err;
> >  
> > -		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, 1, ro))
> > +		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH,
> > +			      mpp, params, 1, ro))
> >  			return 1;
> >  		/*
> >  		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
> > @@ -755,14 +774,14 @@ dm_suspend_and_flush_map (const char * mapname)
> >  	if (s)
> >  		queue_if_no_path = 0;
> >  	else
> > -		s = dm_simplecmd_flush(DM_DEVICE_SUSPEND, mapname, 0, 0);
> > +		s = dm_simplecmd_flush(DM_DEVICE_SUSPEND, mapname, 1, 0);
> >  
> >  	if (!dm_flush_map(mapname)) {
> >  		condlog(4, "multipath map %s removed", mapname);
> >  		return 0;
> >  	}
> >  	condlog(2, "failed to remove multipath map %s", mapname);
> > -	dm_simplecmd_noflush(DM_DEVICE_RESUME, mapname, 0);
> > +	dm_simplecmd_noflush(DM_DEVICE_RESUME, mapname, 1, 0);
> >  	if (queue_if_no_path)
> >  		s = dm_queue_if_no_path((char *)mapname, 1);
> >  	return 1;
> > @@ -1312,6 +1331,7 @@ dm_rename (char * old, char * new)
> >  {
> >  	int r = 0;
> >  	struct dm_task *dmt;
> > +	uint32_t cookie;
> >  
> >  	if (dm_rename_partmaps(old, new))
> >  		return r;
> > @@ -1327,14 +1347,18 @@ dm_rename (char * old, char * new)
> >  
> >  	dm_task_no_open_count(dmt);
> >  
> > -	if (!dm_task_set_cookie(dmt, &conf->cookie, (conf->daemon)? DM_UDEV_DISABLE_LIBRARY_FALLBACK : 0))
> > -		goto out;
> > -	if (!dm_task_run(dmt))
> > +	if (!dm_task_set_cookie(dmt, &cookie, (conf->daemon)? DM_UDEV_DISABLE_LIBRARY_FALLBACK : 0))
> >  		goto out;
> > +	r = dm_task_run(dmt);
> > +
> > +	if (!r)
> > +		dm_udev_complete(cookie);
> > +	else
> > +		udev_wait(cookie);
> >  
> > -	r = 1;
> >  out:
> >  	dm_task_destroy(dmt);
> > +
> >  	return r;
> >  }
> >  
> > @@ -1399,7 +1423,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
> >  			condlog(3, "%s: failed to reassign targets", name);
> >  			goto out_reload;
> >  		}
> > -		dm_simplecmd_noflush(DM_DEVICE_RESUME, name, MPATH_UDEV_RELOAD_FLAG);
> > +		dm_simplecmd_noflush(DM_DEVICE_RESUME, name, 1, MPATH_UDEV_RELOAD_FLAG);
> >  	}
> >  	r = 1;
> >  
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index 5c8c50d..8188f48 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -16,7 +16,7 @@ void dm_init(void);
> >  int dm_prereq (void);
> >  int dm_drv_version (unsigned int * version, char * str);
> >  int dm_simplecmd_flush (int, const char *, int, uint16_t);
> > -int dm_simplecmd_noflush (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_map_present (const char *);
> > diff --git a/multipath/main.c b/multipath/main.c
> > index 1c1191a..c46a9f6 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -675,8 +675,6 @@ main (int argc, char *argv[])
> >  		condlog(3, "restart multipath configuration process");
> >  
> >  out:
> > -	udev_wait(conf->cookie);
> > -
> >  	dm_lib_release();
> >  	dm_lib_exit();
> >  
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 6abe72a..772531e 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -839,7 +839,7 @@ cli_suspend(void * v, char ** reply, int * len, void * data)
> >  {
> >  	struct vectors * vecs = (struct vectors *)data;
> >  	char * param = get_keyparam(v, MAP);
> > -	int r = dm_simplecmd_noflush(DM_DEVICE_SUSPEND, param, 0);
> > +	int r = dm_simplecmd_noflush(DM_DEVICE_SUSPEND, param, 0, 0);
> >  
> >  	param = convert_dev(param, 0);
> >  	condlog(2, "%s: suspend (operator)", param);
> > @@ -861,7 +861,7 @@ cli_resume(void * v, char ** reply, int * len, void * data)
> >  {
> >  	struct vectors * vecs = (struct vectors *)data;
> >  	char * param = get_keyparam(v, MAP);
> > -	int r = dm_simplecmd_noflush(DM_DEVICE_RESUME, param, 0);
> > +	int r = dm_simplecmd_noflush(DM_DEVICE_RESUME, param, 0, 0);
> >  
> >  	param = convert_dev(param, 0);
> >  	condlog(2, "%s: resume (operator)", param);
> > -- 
> > 1.8.4.5
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel