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. -Ben > > Thanks for the clarification here. > > I'll see to come up with a replacement patch. > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > hare@xxxxxxx +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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
