| Message ID | 1462341450-5507-5-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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
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) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 *);
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(-)