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