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@xxxxxxx +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) -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
