[dm-devel] [PATCH 16/72] libmultipath: make path_discovery() pthread_cancel()-safe

[Benjamin Marzinski]

On Mon, Nov 04, 2019 at 08:29:21AM +0000, Martin Wilck wrote:
> Hi Ben,
> 
> thanks for looking into this.
> 
> On Wed, 2019-10-30 at 09:53 -0500, Benjamin Marzinski wrote:
> > On Sat, Oct 12, 2019 at 09:27:57PM +0000, Martin Wilck wrote:
> > > From: Martin Wilck <mwilck at suse.com>
> > > 
> > > The udev_enumerate and udev_device refs wouldn't be released
> > > if the thread was cancelled. Fix it.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > > ---
> > >  libmultipath/discovery.c | 51 +++++++++++++++++++++++++++++++-----
> > > ----
> > >  1 file changed, 40 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > > index e68b0e9f..d217ca92 100644
> > > --- a/libmultipath/discovery.c
> > > +++ b/libmultipath/discovery.c
> > > @@ -140,19 +140,47 @@ path_discover (vector pathvec, struct config
> > > * conf,
> > >  	return pathinfo(pp, conf, flag);
> > >  }
> > >  
> > > +static void cleanup_udev_enumerate_ptr(void *arg)
> > > +{
> > > +	struct udev_enumerate *ue;
> > > +
> > > +	if (!arg)
> > > +		return;
> > > +	ue = *((struct udev_enumerate**) arg);
> > > +	if (ue)
> > > +		(void)udev_enumerate_unref(ue);
> > > +}
> > > +
> > > +static void cleanup_udev_device_ptr(void *arg)
> > > +{
> > > +	struct udev_device *ud;
> > > +
> > > +	if (!arg)
> > > +		return;
> > > +	ud = *((struct udev_device**) arg);
> > > +	if (ud)
> > > +		(void)udev_device_unref(ud);
> > > +}
> > > +
> > >  int
> > >  path_discovery (vector pathvec, int flag)
> > >  {
> > > -	struct udev_enumerate *udev_iter;
> > > +	struct udev_enumerate *udev_iter = NULL;
> > >  	struct udev_list_entry *entry;
> > > -	struct udev_device *udevice;
> > > +	struct udev_device *udevice = NULL;
> > >  	struct config *conf;
> > > -	const char *devpath;
> > >  	int num_paths = 0, total_paths = 0, ret;
> > >  
> > > +	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_iter);
> > > +	pthread_cleanup_push(cleanup_udev_device_ptr, &udevice);
> > > +	conf = get_multipath_config();
> > > +	pthread_cleanup_push(put_multipath_config, conf);
> > > +
> > >  	udev_iter = udev_enumerate_new(udev);
> > > -	if (!udev_iter)
> > > -		return -ENOMEM;
> > > +	if (!udev_iter) {
> > > +		ret = -ENOMEM;
> > > +		goto out;
> > > +	}
> > >  
> > >  	if (udev_enumerate_add_match_subsystem(udev_iter, "block") < 0
> > > ||
> > >  	    udev_enumerate_add_match_is_initialized(udev_iter) < 0 ||
> > > @@ -165,6 +193,8 @@ path_discovery (vector pathvec, int flag)
> > >  	udev_list_entry_foreach(entry,
> > >  				udev_enumerate_get_list_entry(udev_iter
> > > )) {
> > >  		const char *devtype;
> > > +		const char *devpath;
> > > +
> > >  		devpath = udev_list_entry_get_name(entry);
> > >  		condlog(4, "Discover device %s", devpath);
> > >  		udevice = udev_device_new_from_syspath(udev, devpath);
> > > @@ -175,19 +205,18 @@ path_discovery (vector pathvec, int flag)
> > >  		devtype = udev_device_get_devtype(udevice);
> > >  		if(devtype && !strncmp(devtype, "disk", 4)) {
> > >  			total_paths++;
> > > -			conf = get_multipath_config();
> > > -			pthread_cleanup_push(put_multipath_config,
> > > conf);
> > 
> > Why move grabbing the config RCU lock out of the loop? 
> 
> Yes, that was the idea.
> 
> > All things being
> > equal, it seems like we'd rather hold this for less time, and
> > rcu_read_lock() is designed to be lightweight, so calling it more
> > times
> > shouldn't be an issue. 
> 
> It's not the execution time of rcu_read_lock() that I'm concerned
> about. 
> 
> In this particular loop, my estimate is that >90% of time is spent in
> path_discover()/pathinfo(), so time-during-which-lock-is-held-wise, we
> gain little by taking and releasing the RCU lock in every iteration. 
> 
> Right, we might catch a configuration change _earlier_ if we release
> the lock between pathinfo() invocations. But - do we actually want
> that? This lock protects us against corruption of the multipathd
> configuration, basically against someone calling "multipathd
> reconfigure" while our code is running. But if the configuration ins
> really changed, what we're currently doing is vain anyway - once the
> configure() call is finished, we will go through yet another full
> reconfigure cycle. IOW: Do we seriously want to call pathinfo() for the
> different paths in the system with  different configuration, once with
> and once without "user_friendly_names", for example?
> 
> Given that the code we're talking about is only called from
> reconfigure(), multipath_conf having just been reassigned, IMO it's an
> improvement to hold the lock through the entire loop. It might even be
> good to hold the lock for the complete invocation of configure(), but I
> haven't thought about that in detail yet.
> 
> Does this make sense?

Sure. ACK

-Ben
 
> Besides, to my taste at least, it improves readability of the code to
> move get_multipath_config() out of certain loops.
> 
> Thanks,
> Martin
>