Re: [PATCH 20/57] multipathd: Do not update the paths vec when removing paths

[Hannes Reinecke <hare@suse.de>]

On 05/02/2016 05:12 PM, Benjamin Marzinski wrote:
> On Mon, May 02, 2016 at 07:48:50AM +0200, Hannes Reinecke wrote:
>> On 04/30/2016 12:39 AM, Benjamin Marzinski wrote:
>>> On Wed, Apr 27, 2016 at 01:10:21PM +0200, Hannes Reinecke wrote:
>>>> When we remove a path it's totally pointless to add it to
>>>> the path list first; it'll be removed on the next step anyway.
>>>> And we should be cleaning up the comments while we're at it.
>>>
>>> This one causes problems. The easiest way to see that is to run
>>> something like
>>>
>>> # multipathd reload map <map> ; multipathd del path <path-in-that-map>
>>>
>>> This really messes things up. The reason is that at the start of
>>> ev_remove_path, there is no guarantee that any of the paths will be in
>>> mpp->paths.  This is because when multipath runs the pgpolicyfn in
>>> setup_map(), all of policy functions free mpp->paths once they have set
>>> up the path groups.  I assume that this was done so that there is no
>>> chance that the list of paths in mpp->paths will get out of sync with
>>> the list of paths in the pathgroups.
>>>
>>> I can see why it someone might want to only keep mpp->pg as the
>>> definitive list of paths, and to use update_mpp_paths() to regenerate
>>> mpp->paths when necessary. But that's not what multipathd does. Instead,
>>> mpp->paths is almost always regenerated by calling setup_multipath()
>>> later in the same function that called setup_map(). However not every
>>> function will always do this.  ev_remove_path doesn't do this if domap()
>>> fails, and reload_map() never calls setup_multipath(). coalesce_paths()
>>> doesn't call setup_multipath() itself, but some if it's callers do. Even
>>> if mpp->paths isn't restored right away, it will be when check_path
>>> calls update_multipath_strings().
>>>
>>> So, if you call "cli reload map" and then call "cli del path" before the
>>> checker function restores mpp->paths, and multipath doesn't call
>>> update_mpp_pths() in ev_remove_path, you get into problems.
>>>
>>> The question is, what's the right thing to do?
>>>
>>> Option 1 is to never delete mpp->paths in the first place. Then we can
>>> probably do away with some more of the update_mpp_paths() calls. We just
>>> need to make certain that whenever we update mpp->pg, we are always
>>> either getting the paths from mpp->paths, or we call update_mpp_paths()
>>> afterwards to sync them.
>>>
>>> Option 2 is to say that we will alway regenerate mpp->paths whenever we
>>> need it. In that case, we should probably be freeing it after we're done
>>> with it.
>>>
>>> I don't really care either way, as long as we're consistent. Otherwise
>>> we'll get into bizzare situations like the one above.
>>>
>> Personally, I would opt for 1).
>> Regenerating mpp->paths has the very big risk of running into even
>> more race conditions, and I'd rather have it stable as far as possible.
> 
> I lean towards option 1 too. Alos, we should probably make sure that we
> call setup_multipath in all the code paths that call setup_map and domap
> for consistency.
> 
Actually, I've been looking at the code; and I think it might be easier
to make mpp->paths a local variable and remove it from struct mpp.
That way we can be sure no stale ->paths variable is left, and it will
always be computed correctly.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)