[dm-devel] [PATCH 36/57] libmultipath: pass in 'cookie' as argument for dm_addmap()

[Hannes Reinecke]

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?

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)