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: > --- > 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.