Re: [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Dec 17, 2020 at 12:00:12PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> logq_lock protects internal data structures of log.c, and should
> be handled there. This patch doesn't change functionality, except
> improving cancel-safety somewhat.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log.c         | 34 ++++++++++++++++++++++++++++++++--
>  libmultipath/log_pthread.c |  9 ---------
>  2 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index debd36d..7f33787 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -9,13 +9,16 @@
>  #include <string.h>
>  #include <syslog.h>
>  #include <time.h>
> +#include <pthread.h>
>  
>  #include "memory.h"
>  #include "log.h"
> +#include "util.h"
>  
>  #define ALIGN(len, s) (((len)+(s)-1)/(s)*(s))
>  
>  struct logarea* la;
> +static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
>  
>  #if LOGDBG
>  static void dump_logarea (void)
> @@ -101,12 +104,17 @@ void log_close (void)
>  
>  void log_reset (char *program_name)
>  {
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +
>  	closelog();
>  	tzset();
>  	openlog(program_name, 0, LOG_DAEMON);
> +
> +	pthread_cleanup_pop(1);
>  }
>  
> -int log_enqueue (int prio, const char * fmt, va_list ap)
> +static int _log_enqueue(int prio, const char * fmt, va_list ap)
>  {
>  	int len, fwd;
>  	char buff[MAX_MSG_SIZE];
> @@ -165,7 +173,18 @@ int log_enqueue (int prio, const char * fmt, va_list ap)
>  	return 0;
>  }
>  
> -int log_dequeue (void * buff)
> +int log_enqueue(int prio, const char *fmt, va_list ap)
> +{
> +	int ret;
> +
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +	ret = _log_enqueue(prio, fmt, ap);
> +	pthread_cleanup_pop(1);
> +	return ret;
> +}
> +
> +static int _log_dequeue(void *buff)
>  {
>  	struct logmsg * src = (struct logmsg *)la->head;
>  	struct logmsg * dst = (struct logmsg *)buff;
> @@ -194,6 +213,17 @@ int log_dequeue (void * buff)
>  	return 0;
>  }
>  
> +int log_dequeue(void *buff)
> +{
> +	int ret;
> +
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +	ret = _log_dequeue(buff);
> +	pthread_cleanup_pop(1);
> +	return ret;
> +}
> +
>  /*
>   * this one can block under memory pressure
>   */
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 0d48c52..6599210 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -18,7 +18,6 @@
>  static pthread_t log_thr;
>  
>  /* logev_lock must not be taken with logq_lock held */
> -static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
>  static pthread_mutex_t logev_lock = PTHREAD_MUTEX_INITIALIZER;
>  static pthread_cond_t logev_cond = PTHREAD_COND_INITIALIZER;
>  
> @@ -41,10 +40,7 @@ void log_safe (int prio, const char * fmt, va_list ap)
>  	running = logq_running;
>  
>  	if (running) {
> -		pthread_mutex_lock(&logq_lock);
> -		pthread_cleanup_push(cleanup_mutex, &logq_lock);
>  		log_enqueue(prio, fmt, ap);
> -		pthread_cleanup_pop(1);
>  
>  		log_messages_pending = 1;
>  		pthread_cond_signal(&logev_cond);
> @@ -60,9 +56,7 @@ static void flush_logqueue (void)
>  	int empty;
>  
>  	do {
> -		pthread_mutex_lock(&logq_lock);
>  		empty = log_dequeue(la->buff);
> -		pthread_mutex_unlock(&logq_lock);
>  		if (!empty)
>  			log_syslog(la->buff);
>  	} while (empty == 0);
> @@ -138,10 +132,7 @@ void log_thread_start (pthread_attr_t *attr)
>  void log_thread_reset (void)
>  {
>  	logdbg(stderr,"resetting log\n");
> -
> -	pthread_mutex_lock(&logq_lock);
>  	log_reset("multipathd");
> -	pthread_mutex_unlock(&logq_lock);
>  }
>  
>  void log_thread_stop (void)
> -- 
> 2.29.0

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