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

[Martin Wilck]

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?

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

Thanks,
Martin