[Cluster-devel] [PATCH v2 12/20] gfs2: Clean up gfs2_log_reserve

[Bob Peterson <rpeterso@redhat.com>]

Hi,

Comments inline.

----- Original Message -----
> Wake up log waiters in gfs2_log_release when log space has actually become
> available.  This is a much better place for the wakeup than gfs2_logd.
> 
> Check if enough log space is immeditely available before anything else.  If
> there isn't, use io_wait_event to wait instead of open-coding it.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/gfs2/log.c | 54 ++++++++++++++++++++++-----------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 95ad444bd3dc..7a65823ad1f3 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -420,6 +420,8 @@ void gfs2_log_release(struct gfs2_sbd *sdp, unsigned int
> blks)
>  	trace_gfs2_log_blocks(sdp, blks);
>  	gfs2_assert_withdraw(sdp, atomic_read(&sdp->sd_log_blks_free) <=
>  				  sdp->sd_jdesc->jd_blocks);
> +	if (atomic_read(&sdp->sd_log_blks_needed))
> +		wake_up(&sdp->sd_log_waitq);
>  }
>  
>  /**
> @@ -444,36 +446,33 @@ void gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned
> int blks)
>  {
>  	unsigned reserved_blks = 7 * (4096 / sdp->sd_vfs->s_blocksize);
>  	unsigned wanted = blks + reserved_blks;
> -	DEFINE_WAIT(wait);
> -	int did_wait = 0;
>  	unsigned int free_blocks;
>  
> -	atomic_add(blks, &sdp->sd_log_blks_needed);
> -retry:
>  	free_blocks = atomic_read(&sdp->sd_log_blks_free);
> -	if (unlikely(free_blocks <= wanted)) {
> -		do {
> -			prepare_to_wait_exclusive(&sdp->sd_log_waitq, &wait,
> -					TASK_UNINTERRUPTIBLE);
> +	while (free_blocks >= wanted) {
> +		if (atomic_try_cmpxchg(&sdp->sd_log_blks_free, &free_blocks,
> +				       free_blocks - blks))
> +			return;

This would be a good place to have cond_resched() or schedule() or something, no?

> +	}
> +
> +	atomic_add(blks, &sdp->sd_log_blks_needed);
> +	for (;;) {
> +		if (current != sdp->sd_logd_process)
>  			wake_up(&sdp->sd_logd_waitq);
> -			did_wait = 1;
> -			if (atomic_read(&sdp->sd_log_blks_free) <= wanted)
> -				io_schedule();
> -			free_blocks = atomic_read(&sdp->sd_log_blks_free);
> -		} while(free_blocks <= wanted);
> -		finish_wait(&sdp->sd_log_waitq, &wait);
> +		io_wait_event(sdp->sd_log_waitq,
> +			(free_blocks = atomic_read(&sdp->sd_log_blks_free),
> +			 free_blocks >= wanted));
> +		do {
> +			if (atomic_try_cmpxchg(&sdp->sd_log_blks_free,
> +					       &free_blocks,
> +					       free_blocks - blks))
> +				goto reserved;

Same here. We need to test these patches with a minimal number of cpus.

> +		} while (free_blocks >= wanted);
>  	}
> -	if (atomic_cmpxchg(&sdp->sd_log_blks_free, free_blocks,
> -				free_blocks - blks) != free_blocks)
> -		goto retry;
> -	atomic_sub(blks, &sdp->sd_log_blks_needed);
> -	trace_gfs2_log_blocks(sdp, -blks);
>  
> -	/*
> -	 * If we waited, then so might others, wake them up _after_ we get
> -	 * our share of the log.
> -	 */
> -	if (unlikely(did_wait))
> +reserved:
> +	trace_gfs2_log_blocks(sdp, -blks);
> +	if (atomic_sub_return(blks, &sdp->sd_log_blks_needed))
>  		wake_up(&sdp->sd_log_waitq);
>  }
>  
> @@ -1190,7 +1189,6 @@ int gfs2_logd(void *data)
>  	struct gfs2_sbd *sdp = data;
>  	unsigned long t = 1;
>  	DEFINE_WAIT(wait);
> -	bool did_flush;
>  
>  	while (!kthread_should_stop()) {
>  
> @@ -1209,12 +1207,10 @@ int gfs2_logd(void *data)
>  			continue;
>  		}
>  
> -		did_flush = false;
>  		if (gfs2_jrnl_flush_reqd(sdp) || t == 0) {
>  			gfs2_ail1_empty(sdp, 0);
>  			gfs2_log_flush(sdp, NULL, GFS2_LOG_HEAD_FLUSH_NORMAL |
>  				       GFS2_LFC_LOGD_JFLUSH_REQD);
> -			did_flush = true;
>  		}
>  
>  		if (gfs2_ail_flush_reqd(sdp)) {
> @@ -1223,12 +1219,8 @@ int gfs2_logd(void *data)
>  			gfs2_ail1_empty(sdp, 0);
>  			gfs2_log_flush(sdp, NULL, GFS2_LOG_HEAD_FLUSH_NORMAL |
>  				       GFS2_LFC_LOGD_AIL_FLUSH_REQD);
> -			did_flush = true;
>  		}
>  
> -		if (!gfs2_ail_flush_reqd(sdp) || did_flush)
> -			wake_up(&sdp->sd_log_waitq);
> -
>  		t = gfs2_tune_get(sdp, gt_logd_secs) * HZ;
>  
>  		try_to_freeze();
> --
> 2.26.2
> 
>