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@xxxxxxxx>
> > >
> > > The udev_enumerate and udev_device refs wouldn't be released
> > > if the thread was cancelled. Fix it.
> > >
> > > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > > ---
> > > 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
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
