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 <dchin...@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 dchin...@redhat.com
