Re: [dm-devel] [PATCH 05/35] libmultipath: improve cleanup of uevent queues on exit

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, Sep 10, 2021 at 01:40:50PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> uevents listed on merge_node must be cleaned up, too. uevents
> cancelled while being serviced and temporary queues, likewise.
> The global uevq must be cleaned out in the uevent listener thread,
> because it might have added events after the dispatcher thread
> had already finished.
> 

There's nothing wrong with this, but for the global list, wouldn't it be
easier to just wait till after cleanup_child() calls cleanup_threads(),
and then call cleanup_global_uevq(). That way you know nothing else is
running.

-Ben

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/uevent.c | 49 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 4265904..082e891 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -91,16 +91,25 @@ struct uevent * alloc_uevent (void)
>  	return uev;
>  }
>  
> +static void uevq_cleanup(struct list_head *tmpq);
> +
> +static void cleanup_uev(void *arg)
> +{
> +	struct uevent *uev = arg;
> +
> +	uevq_cleanup(&uev->merge_node);
> +	if (uev->udev)
> +		udev_device_unref(uev->udev);
> +	FREE(uev);
> +}
> +
>  static void uevq_cleanup(struct list_head *tmpq)
>  {
>  	struct uevent *uev, *tmp;
>  
>  	list_for_each_entry_safe(uev, tmp, tmpq, node) {
>  		list_del_init(&uev->node);
> -
> -		if (uev->udev)
> -			udev_device_unref(uev->udev);
> -		FREE(uev);
> +		cleanup_uev(uev);
>  	}
>  }
>  
> @@ -384,14 +393,10 @@ service_uevq(struct list_head *tmpq)
>  	list_for_each_entry_safe(uev, tmp, tmpq, node) {
>  		list_del_init(&uev->node);
>  
> +		pthread_cleanup_push(cleanup_uev, uev);
>  		if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
>  			condlog(0, "uevent trigger error");
> -
> -		uevq_cleanup(&uev->merge_node);
> -
> -		if (uev->udev)
> -			udev_device_unref(uev->udev);
> -		FREE(uev);
> +		pthread_cleanup_pop(1);
>  	}
>  }
>  
> @@ -411,6 +416,18 @@ static void monitor_cleanup(void *arg)
>  	udev_monitor_unref(monitor);
>  }
>  
> +static void cleanup_uevq(void *arg)
> +{
> +	uevq_cleanup(arg);
> +}
> +
> +static void cleanup_global_uevq(void *arg __attribute__((unused)))
> +{
> +	pthread_mutex_lock(uevq_lockp);
> +	uevq_cleanup(&uevq);
> +	pthread_mutex_unlock(uevq_lockp);
> +}
> +
>  /*
>   * Service the uevent queue.
>   */
> @@ -425,6 +442,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
>  	while (1) {
>  		LIST_HEAD(uevq_tmp);
>  
> +		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
>  		pthread_mutex_lock(uevq_lockp);
>  		servicing_uev = 0;
>  		/*
> @@ -436,14 +454,17 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
>  		}
>  		servicing_uev = 1;
>  		list_splice_init(&uevq, &uevq_tmp);
> -		pthread_mutex_unlock(uevq_lockp);
> +		pthread_cleanup_pop(1);
> +
>  		if (!my_uev_trigger)
>  			break;
> +
> +		pthread_cleanup_push(cleanup_uevq, &uevq_tmp);
>  		merge_uevq(&uevq_tmp);
>  		service_uevq(&uevq_tmp);
> +		pthread_cleanup_pop(1);
>  	}
>  	condlog(3, "Terminating uev service queue");
> -	uevq_cleanup(&uevq);
>  	return 0;
>  }
>  
> @@ -600,6 +621,8 @@ int uevent_listen(struct udev *udev)
>  
>  	events = 0;
>  	gettimeofday(&start_time, NULL);
> +	pthread_cleanup_push(cleanup_global_uevq, NULL);
> +	pthread_cleanup_push(cleanup_uevq, &uevlisten_tmp);
>  	while (1) {
>  		struct uevent *uev;
>  		struct udev_device *dev;
> @@ -650,6 +673,8 @@ int uevent_listen(struct udev *udev)
>  		gettimeofday(&start_time, NULL);
>  		timeout = 30;
>  	}
> +	pthread_cleanup_pop(1);
> +	pthread_cleanup_pop(1);
>  out:
>  	pthread_cleanup_pop(1);
>  out_udev:
> -- 
> 2.33.0

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel