| Message ID | 1462341450-5507-7-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: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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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)