From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Benjamin Marzinski" <bmarzins@redhat.com>
Subject: Re: [PATCH 20/57] multipathd: Do not update the paths
 vec when removing paths
Date: Mon, 2 May 2016 10:12:46 -0500
Message-ID: <20160502151246.GO26117@octiron.msp.redhat.com>
References: <1461755458-29225-1-git-send-email-hare@suse.de>
	<1461755458-29225-21-git-send-email-hare@suse.de>
	<20160429223916.GN26117@octiron.msp.redhat.com>
	<5726EA42.3020307@suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <dm-devel-bounces@redhat.com>
Content-Disposition: inline
In-Reply-To: <5726EA42.3020307@suse.de>
List-Unsubscribe: <https://www.redhat.com/mailman/options/dm-devel>,
	<mailto:dm-devel-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/dm-devel>
List-Post: <mailto:dm-devel@redhat.com>
List-Help: <mailto:dm-devel-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/dm-devel>,
	<mailto:dm-devel-request@redhat.com?subject=subscribe>
Sender: dm-devel-bounces@redhat.com
Errors-To: dm-devel-bounces@redhat.com
To: Hannes Reinecke <hare@suse.de>
Cc: dm-devel@redhat.com, Mike Snitzer <snitzer@redhat.com>, Christophe Varoqui <christophe.varoqui@gmail.com>
List-Id: dm-devel.ids

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@suse.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
> GF: F. Imend=F6rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG N=FCrnberg)