[dm-devel] [PATCH 73/78] multipathd: push down lock in checkerloop()

[Benjamin Marzinski]

On Mon, Mar 16, 2015 at 01:37:00PM +0100, Hannes Reinecke wrote:
> Instead of grabbing the lock at the start of the checkerloop
> and releasing it at the end we should be holding it only
> during the time when we actually need it.

the vecs lock is designed to protect the vecs vectors (and conf), so
checking vecs->pathvec and vecs->mpvec outside of the vecs lock doesn't
guarantee us that they will still be there once we've grabbed the lock.
I don't think this could actually cause a memory issue, since all the
vecs code already checks if the vector is NULL, and the check before
locking will probably be right most of the time.  But perhaps we should
check again after we do the locking, or at least add a comment saying
that we know that the pre-lock check isn't guaranteed to be correct,
so as not to confuse people.

-Ben


> 
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
>  multipathd/main.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index f876258..9e7bf4f 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1399,32 +1399,40 @@ checkerloop (void *ap)
>  
>  		if (gettimeofday(&start_time, NULL) != 0)
>  			start_time.tv_sec = 0;
> -		pthread_cleanup_push(cleanup_lock, &vecs->lock);
> -		lock(vecs->lock);
> -		pthread_testcancel();
>  		condlog(4, "tick");
>  #ifdef USE_SYSTEMD
>  		if (use_watchdog)
>  			sd_notify(0, "WATCHDOG=1");
>  #endif
>  		if (vecs->pathvec) {
> +			pthread_cleanup_push(cleanup_lock, &vecs->lock);
> +			lock(vecs->lock);
> +			pthread_testcancel();
>  			vector_foreach_slot (vecs->pathvec, pp, i) {
>  				num_paths += check_path(vecs, pp);
>  			}
> +			lock_cleanup_pop(vecs->lock);
>  		}
>  		if (vecs->mpvec) {
> +			pthread_cleanup_push(cleanup_lock, &vecs->lock);
> +			lock(vecs->lock);
> +			pthread_testcancel();
>  			defered_failback_tick(vecs->mpvec);
>  			retry_count_tick(vecs->mpvec);
> +			lock_cleanup_pop(vecs->lock);
>  		}
>  		if (count)
>  			count--;
>  		else {
> +			pthread_cleanup_push(cleanup_lock, &vecs->lock);
> +			lock(vecs->lock);
> +			pthread_testcancel();
>  			condlog(4, "map garbage collection");
>  			mpvec_garbage_collector(vecs);
>  			count = MAPGCINT;
> +			lock_cleanup_pop(vecs->lock);
>  		}
>  
> -		lock_cleanup_pop(vecs->lock);
>  		if (start_time.tv_sec &&
>  		    gettimeofday(&end_time, NULL) == 0 &&
>  		    num_paths) {
> -- 
> 1.8.4.5