| Message ID | 20191012212703.12989-17-martin.wilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: cleanup and warning enablement | expand |
On Sat, Oct 12, 2019 at 09:27:57PM +0000, Martin Wilck wrote: > From: Martin Wilck <mwilck@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@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? 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. -Ben > if (path_discover(pathvec, conf, > udevice, flag) == PATHINFO_OK) > num_paths++; > - pthread_cleanup_pop(1); > } > - udev_device_unref(udevice); > + udevice = udev_device_unref(udevice); > } > ret = total_paths - num_paths; > -out: > - udev_enumerate_unref(udev_iter); > condlog(4, "Discovered %d/%d paths", num_paths, total_paths); > +out: > + pthread_cleanup_pop(1); > + pthread_cleanup_pop(1); > + pthread_cleanup_pop(1); > return ret; > } > > -- > 2.23.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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@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@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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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@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@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 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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); if (path_discover(pathvec, conf, udevice, flag) == PATHINFO_OK) num_paths++; - pthread_cleanup_pop(1); } - udev_device_unref(udevice); + udevice = udev_device_unref(udevice); } ret = total_paths - num_paths; -out: - udev_enumerate_unref(udev_iter); condlog(4, "Discovered %d/%d paths", num_paths, total_paths); +out: + pthread_cleanup_pop(1); + pthread_cleanup_pop(1); + pthread_cleanup_pop(1); return ret; }