[dm-devel] [PATCH 43/78] Fixup device-mapper 'cookie' handling
Benjamin Marzinski
bmarzins at redhat.com
Wed Mar 25 16:30:57 UTC 2015
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.
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 at 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
More information about the dm-devel
mailing list