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 <[email protected]> > --- > 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 -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
