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

[Dave Chinner <dchinner@redhat.com>]

On Fri, Feb 05, 2010 at 11:11:48AM +0000, Steven Whitehouse wrote:
> 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@redhat.com>
> 
> This looks good, but a couple of comments:
>
> > 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.

Cool. I wondered about that - I was going to just ignore the
wrappers and put direct calls in, but I thought it might be better
to just start by following the existing convention. I'll respin this
without the wrappers.



> [snip]
> > @@ -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,

Ok, I wasn't sure how that bit was protected, so I left it with the
same protection as it had before. I'll kill the log lock from there
entirely.

Cheers,

Dave.
-- 
Dave Chinner
dchinner at redhat.com