[dm-devel] [PATCH 36/57] libmultipath: pass in 'cookie' as argument for dm_addmap()
Benjamin Marzinski
bmarzins at redhat.com
Tue May 3 14:57:50 UTC 2016
On Tue, May 03, 2016 at 04:43:01PM +0200, Hannes Reinecke wrote:
> On 05/03/2016 04:39 PM, Benjamin Marzinski wrote:
> > On Tue, May 03, 2016 at 11:31:19AM +0200, Hannes Reinecke wrote:
> >> On 05/03/2016 12:23 AM, Benjamin Marzinski wrote:
> >>> On Wed, Apr 27, 2016 at 01:10:37PM +0200, Hannes Reinecke wrote:
> >>>> Instead of generating the cookie internally we should be
> >>>> passing in the cookie to dm_addmap().
> >>>
> >>> These look like they could actually cause problems. With
> >>> dm_addmap_create and dm_addmap_reload, you could be in a situation where
> >>> you get a cookie back on your first call to dm_task_set_cookie, wait on
> >>> it, so that cookie is no longer in use, and then use that same cookie
> >>> id again. It's possible that after you first waited on the cookie,
> >>> someone else could have been given that cookie id (without looking into
> >>> the dm cookie code more, I'm not sure how likely this is). If that is
> >>> so, the second call to dm_addmap would be adding its command to the list
> >>> of other commands for that cookie (since you are allowed to use a cookie
> >>> for multiple dm commands). Calling dm_udev_wait would make it wait of
> >>> all the commands.
> >>>
> >>> We can use a cookie multiple times and then wait on it once. But we
> >>> can't keep reusing the cookie after we've waited on it, because someone
> >>> else could be using the same cookie.
> >>>
> >>> -Ben
> >>>
> >>>>
> >>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
> >>>> ---
> >>>> libmultipath/devmapper.c | 23 ++++++++++++++---------
> >>>> 1 file changed, 14 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> >>>> index e6156f7..a96cc0e 100644
> >>>> --- a/libmultipath/devmapper.c
> >>>> +++ b/libmultipath/devmapper.c
> >>>> @@ -274,11 +274,10 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
> >>>>
> >>>> extern int
> >>>> dm_addmap (int task, const char *target, struct multipath *mpp,
> >>>> - char * params, int ro) {
> >>>> + char * params, int ro, uint32_t *cookie) {
> >>>> int r = 0;
> >>>> struct dm_task *dmt;
> >>>> char *prefixed_uuid = NULL;
> >>>> - uint32_t cookie = 0;
> >>>>
> >>>> if (!(dmt = dm_task_create (task)))
> >>>> return 0;
> >>>> @@ -319,18 +318,18 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
> >>>> dm_task_no_open_count(dmt);
> >>>>
> >>>> if (task == DM_DEVICE_CREATE &&
> >>>> - !dm_task_set_cookie(dmt, &cookie,
> >>>> + !dm_task_set_cookie(dmt, cookie,
> >>>> DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
> >>>> - dm_udev_complete(cookie);
> >>>> + dm_udev_complete(*cookie);
> >>>> goto freeout;
> >>>> }
> >>>> r = dm_task_run (dmt);
> >>>>
> >>>> if (task == DM_DEVICE_CREATE) {
> >>>> if (!r)
> >>>> - dm_udev_complete(cookie);
> >>>> + dm_udev_complete(*cookie);
> >>>> else
> >>>> - dm_udev_wait(cookie);
> >>>> + dm_udev_wait(*cookie);
> >>>> }
> >>>> freeout:
> >>>> if (prefixed_uuid)
> >>>> @@ -345,11 +344,13 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
> >>>> extern int
> >>>> dm_addmap_create (struct multipath *mpp, char * params) {
> >>>> int ro;
> >>>> + uint32_t cookie = 0;
> >>>>
> >>>> for (ro = 0; ro <= 1; ro++) {
> >>>> int err;
> >>>>
> >>>> - if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro))
> >>>> + if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH,
> >>>> + mpp, params, ro, &cookie))
> >>>> return 1;
> >>>> /*
> >>>> * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
> >>>> @@ -374,11 +375,15 @@ dm_addmap_create (struct multipath *mpp, char * params) {
> >>>>
> >>>> extern int
> >>>> dm_addmap_reload (struct multipath *mpp, char *params) {
> >>>> - if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW))
> >>>> + uint32_t cookie = 0;
> >>>> +
> >>>> + 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);
> >>>> + return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> >>>> + ADDMAP_RO, &cookie);
> >>>> }
> >>>>
> >>>> extern int
> >>>> --
> >>>> 2.6.6
> >> Looking closer, this entire section looks fishy.
> >> >From the lvm source code I couldn't figure out _how_ we could get
> >> into a situation where dm_task_run() would return 0 (ie no error
> >> occurred), but errno would be set to EROFS.
> >
> > dm_task_run returns a 0 on error, not success.
> >
> Yeah, after several hours of debugging I've finally fixed things up.
> Main reason why it worked was that multipathd disabled udev sync, so
> all cookies were disabled and it doesn't really matter if and how
> you called udevcomplete and stuff.
>
> So, question:
>
> Should be enable udev sync via
>
> dm_udev_set_sync_support(1);
>
> in multipathd or not?
Not unless you want to block every multipathd thread that grabs the vecs
lock after every sync-able dm command, until udev processes that event.
In short, No.
I looked at multiple ideas to deal with syncing, including a seperate
waiting thread. All of them had to potential to cause crippling
slowdowns. Then I realized that we don't need them. Multipathd itself
already listens to the udev events. If we want to know when udev has
finished processing the uevent, we just wait for multipathd to receive
it.
That's the crux of my solution to keeping multipath from reloading a
device before udev has finished initializing it (see commit
b833425029b59106e4e6de26470990ee39d614f5, multipathd: delay reloads
during creation). We know when udev has finished setting up the device,
because it sends us a uevent afterwards.
-Ben
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Teamlead Storage & Networking
> hare at 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)
More information about the dm-devel
mailing list