Re: [PATCH v3 02/19] libmultipath: fix tur checker double locking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2018-09-21 at 18:05 -0500, Benjamin Marzinski wrote:
> tur_devt() locks ct->lock. However, it is ocassionally called while
> ct->lock is already locked. In reality, there is no reason why we
> need
> to lock all the accesses to ct->devt. The tur checker only needs to
> write to this variable one time, when it first gets the file
> descripter
> that it is checking.  It also never uses ct->devt directly. Instead,
> it
> always graps the major and minor, and turns them into a string. This
> patch changes ct->devt into that string, and sets it in
> libcheck_init()
> when it is first initializing the checker context. After that, ct-
> >devt
> is only ever read.

I like the lock removal a lot, but not so much the conversion into a
string. Why not keep the dev_t? 

Or maybe even easier, the other way around: why don't we make it a
char* and simply set checker->dev_t = &pp->dev_t?

Regards,
Martin

> 
> Cc: Bart Van Assche <bart.vanassche@xxxxxxx>4
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
>  libmultipath/checkers/tur.c | 55 +++++++++++++--------------------
> ------------
>  1 file changed, 16 insertions(+), 39 deletions(-)
> 
> diff --git a/libmultipath/checkers/tur.c
> b/libmultipath/checkers/tur.c
> index 275541f..d173648 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -37,36 +37,24 @@
>  #define MSG_TUR_FAILED	"tur checker failed to initialize"
>  
>  struct tur_checker_context {
> -	dev_t devt;
> +	char devt[32];
>  	int state;
> -	int running;
> +	int running; /* uatomic access only */
>  	int fd;
>  	unsigned int timeout;
>  	time_t time;
>  	pthread_t thread;
>  	pthread_mutex_t lock;
>  	pthread_cond_t active;
> -	int holders;
> +	int holders; /* uatomic access only */
>  	char message[CHECKER_MSG_LEN];
>  };
>  
> -static const char *tur_devt(char *devt_buf, int size,
> -			    struct tur_checker_context *ct)
> -{
> -	dev_t devt;
> -
> -	pthread_mutex_lock(&ct->lock);
> -	devt = ct->devt;
> -	pthread_mutex_unlock(&ct->lock);
> -
> -	snprintf(devt_buf, size, "%d:%d", major(devt), minor(devt));
> -	return devt_buf;
> -}
> -
>  int libcheck_init (struct checker * c)
>  {
>  	struct tur_checker_context *ct;
>  	pthread_mutexattr_t attr;
> +	struct stat sb;
>  
>  	ct = malloc(sizeof(struct tur_checker_context));
>  	if (!ct)
> @@ -81,6 +69,9 @@ int libcheck_init (struct checker * c)
>  	pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
>  	pthread_mutex_init(&ct->lock, &attr);
>  	pthread_mutexattr_destroy(&attr);
> +	if (fstat(c->fd, &sb) == 0)
> +		snprintf(ct->devt, sizeof(ct->devt), "%d:%d",
> major(sb.st_rdev),
> +			 minor(sb.st_rdev));
>  	c->context = ct;
>  
>  	return 0;
> @@ -232,14 +223,12 @@ static void *tur_thread(void *ctx)
>  {
>  	struct tur_checker_context *ct = ctx;
>  	int state, running;
> -	char devt[32];
>  
>  	/* This thread can be canceled, so setup clean up */
>  	tur_thread_cleanup_push(ct);
>  	rcu_register_thread();
>  
> -	condlog(3, "%s: tur checker starting up",
> -		tur_devt(devt, sizeof(devt), ct));
> +	condlog(3, "%s: tur checker starting up", ct->devt);
>  
>  	/* TUR checker start up */
>  	pthread_mutex_lock(&ct->lock);
> @@ -256,8 +245,8 @@ static void *tur_thread(void *ctx)
>  	pthread_cond_signal(&ct->active);
>  	pthread_mutex_unlock(&ct->lock);
>  
> -	condlog(3, "%s: tur checker finished, state %s",
> -		tur_devt(devt, sizeof(devt), ct),
> checker_state_name(state));
> +	condlog(3, "%s: tur checker finished, state %s", ct->devt,
> +		checker_state_name(state));
>  
>  	running = uatomic_xchg(&ct->running, 0);
>  	if (!running)
> @@ -305,20 +294,12 @@ int libcheck_check(struct checker * c)
>  {
>  	struct tur_checker_context *ct = c->context;
>  	struct timespec tsp;
> -	struct stat sb;
>  	pthread_attr_t attr;
>  	int tur_status, r;
> -	char devt[32];
>  
>  	if (!ct)
>  		return PATH_UNCHECKED;
>  
> -	if (fstat(c->fd, &sb) == 0) {
> -		pthread_mutex_lock(&ct->lock);
> -		ct->devt = sb.st_rdev;
> -		pthread_mutex_unlock(&ct->lock);
> -	}
> -
>  	if (c->sync)
>  		return tur_check(c->fd, c->timeout,
> copy_msg_to_checker, c);
>  
> @@ -327,8 +308,7 @@ int libcheck_check(struct checker * c)
>  	 */
>  	r = pthread_mutex_lock(&ct->lock);
>  	if (r != 0) {
> -		condlog(2, "%s: tur mutex lock failed with %d",
> -			tur_devt(devt, sizeof(devt), ct), r);
> +		condlog(2, "%s: tur mutex lock failed with %d", ct-
> >devt, r);
>  		MSG(c, MSG_TUR_FAILED);
>  		return PATH_WILD;
>  	}
> @@ -338,14 +318,12 @@ int libcheck_check(struct checker * c)
>  			int running = uatomic_xchg(&ct->running, 0);
>  			if (running)
>  				pthread_cancel(ct->thread);
> -			condlog(3, "%s: tur checker timeout",
> -				tur_devt(devt, sizeof(devt), ct));
> +			condlog(3, "%s: tur checker timeout", ct-
> >devt);
>  			ct->thread = 0;
>  			MSG(c, MSG_TUR_TIMEOUT);
>  			tur_status = PATH_TIMEOUT;
>  		} else if (uatomic_read(&ct->running) != 0) {
> -			condlog(3, "%s: tur checker not finished",
> -					tur_devt(devt, sizeof(devt),
> ct));
> +			condlog(3, "%s: tur checker not finished", ct-
> >devt);
>  			tur_status = PATH_PENDING;
>  		} else {
>  			/* TUR checker done */
> @@ -361,7 +339,7 @@ int libcheck_check(struct checker * c)
>  			if (ct->state == PATH_PENDING) {
>  				pthread_mutex_unlock(&ct->lock);
>  				condlog(3, "%s: tur thread not
> responding",
> -					tur_devt(devt, sizeof(devt),
> ct));
> +					ct->devt);
>  				return PATH_TIMEOUT;
>  			}
>  		}
> @@ -381,7 +359,7 @@ int libcheck_check(struct checker * c)
>  			ct->thread = 0;
>  			pthread_mutex_unlock(&ct->lock);
>  			condlog(3, "%s: failed to start tur thread,
> using"
> -				" sync mode", tur_devt(devt,
> sizeof(devt), ct));
> +				" sync mode", ct->devt);
>  			return tur_check(c->fd, c->timeout,
>  					 copy_msg_to_checker, c);
>  		}
> @@ -392,8 +370,7 @@ int libcheck_check(struct checker * c)
>  		pthread_mutex_unlock(&ct->lock);
>  		if (uatomic_read(&ct->running) != 0 &&
>  		    (tur_status == PATH_PENDING || tur_status ==
> PATH_UNCHECKED)) {
> -			condlog(3, "%s: tur checker still running",
> -				tur_devt(devt, sizeof(devt), ct));
> +			condlog(3, "%s: tur checker still running", ct-
> >devt);
>  			tur_status = PATH_PENDING;
>  		} else {
>  			int running = uatomic_xchg(&ct->running, 0);

-- 
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


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




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux