[Cluster-devel] [PATCH 4/4] gfs2: introduce AIL lock

[Steven Whitehouse]

Hi,

On Fri, 2010-02-05 at 16:45 +1100, Dave Chinner wrote:
> THe log lock is currently used to protect the AIL lists and
> the movements of buffers into and out of them. The lists
> are self contained and no log specific items outside the
> lists are accessed when starting or emptying the AIL lists.
> 
> Hence the operation of the AIL does not require the protection
> of the log lock so split them out into a new AIL specific lock
> to reduce the amount of traffic on the log lock. This will
> also reduce the amount of serialisation that occurs when
> the gfs2_logd pushes on the AIL to move it forward.
> 
> This reduces the impact of log pushing on sequential write
> throughput. On no-op scheduler on a disk that can do 85MB/s,
> this increases the write rate from 65MB/s with the ordering
> fixes to 75MB/s.
> 
> Signed-off-by: Dave Chinner <dchinner at redhat.com>

This looks good, but a couple of comments:

> ---
>  fs/gfs2/glops.c      |   10 ++++++++--
>  fs/gfs2/incore.h     |    1 +
>  fs/gfs2/log.c        |   32 +++++++++++++++++---------------
>  fs/gfs2/log.h        |   22 ++++++++++++++++++++++
>  fs/gfs2/lops.c       |    5 ++++-
>  fs/gfs2/ops_fstype.c |    1 +
>  6 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 78554ac..65048f9 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -57,20 +57,26 @@ static void gfs2_ail_empty_gl(struct gfs2_glock *gl)
>  	BUG_ON(current->journal_info);
>  	current->journal_info = &tr;
>  
> -	gfs2_log_lock(sdp);
> +	gfs2_ail_lock(sdp);
          ^^^^ this abstraction of a spinlock is left over from the old
gfs1 code. I'd prefer when adding new locks just to use spinlock(&....)
directly, rather than abstracting it out like this. That way we don't
have to think about what kind of lock it is.

[snip]
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 0fe2f3c..342d65e 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -80,7 +80,7 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct buffer_head *bh,
>  	mark_buffer_dirty(bh);
>  	clear_buffer_pinned(bh);
>  
> -	gfs2_log_lock(sdp);
> +	gfs2_ail_lock(sdp);
>  	if (bd->bd_ail) {
>  		list_del(&bd->bd_ail_st_list);
>  		brelse(bh);
> @@ -91,6 +91,9 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct buffer_head *bh,
>  	}
>  	bd->bd_ail = ai;
>  	list_add(&bd->bd_ail_st_list, &ai->ai_ail1_list);
> +	gfs2_ail_unlock(sdp);
> +
> +	gfs2_log_lock(sdp);
>  	clear_bit(GLF_LFLUSH, &bd->bd_gl->gl_flags);
>  	trace_gfs2_pin(bd, 0);
>  	gfs2_log_unlock(sdp);
I don't think the gfs2_log_lock() is actually required at this point.
the LFLUSH bit is protected by the sd_log_flush_lock rwsem
and the tracing doesn't need the log lock either,

Steve.