[Cluster-devel] GFS2: Cache last hash bucket for glock seq_files

[Steven Whitehouse]

Hi,

On Sat, 2012-06-09 at 10:32 +0200, Eric Dumazet wrote:
> On Fri, 2012-06-08 at 11:52 +0100, Steven Whitehouse wrote:
> > From ba1ddcb6ca0c46edd275790d1e4e2cfd6219ce19 Mon Sep 17 00:00:00 2001
> > From: Steven Whitehouse <swhiteho at redhat.com>
> > Date: Fri, 8 Jun 2012 11:16:22 +0100
> > Subject: [PATCH] GFS2: Cache last hash bucket for glock seq_files
> > 
> > For the glocks and glstats seq_files, which are exposed via debugfs
> > we should cache the most recent hash bucket, along with the offset
> > into that bucket. This allows us to restart from that point, rather
> > than having to begin at the beginning each time.
> > 
> > This is an idea from Eric Dumazet, however I've slightly extended it
> > so that if the position from which we are due to start is at any
> > point beyond the last cached point, we start from the last cached
> > point, plus whatever is the appropriate offset. I don't really expect
> > people to be lseeking around these files, but if they did so with only
> > positive offsets, then we'd still get some of the benefit of using a
> > cached offset.
> > 
> > With my simple test of around 200k entries in the file, I'm seeing
> > an approx 10x speed up.
> 
> Strange, a 10x speed up is not what I would expect...
> 
That is on top of the almost 8x from increasing the buffer size with a
patch that Al Viro had suggested as a response to our earlier
discussion:

http://git.kernel.org/?p=linux/kernel/git/steve/gfs2-3.0-nmw.git;a=commitdiff;h=df5d2f5560a9c33129391a136ed9f0ac26abe69b

Also, I suspect that I'd see a much larger effect if I used more glocks
(i.e. more entries in the file). It is not unusual to see a million or
more entries, but it does mean that tests take a long time, just to
create or cache all those entries. So I was testing with a smaller
number mainly to speed up the tests.

> > 
> > Cc: Eric Dumazet <eric.dumazet at gmail.com>
> > Signed-off-by: Steven Whitehouse <swhiteho at redhat.com>
> > 
> > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> > index 1c4cddf..3ad8cb3 100644
> > --- a/fs/gfs2/glock.c
> > +++ b/fs/gfs2/glock.c
> > @@ -46,10 +46,12 @@
> >  #include "trace_gfs2.h"
> >  
> >  struct gfs2_glock_iter {
> > -	int hash;			/* hash bucket index         */
> > -	struct gfs2_sbd *sdp;		/* incore superblock         */
> > -	struct gfs2_glock *gl;		/* current glock struct      */
> > -	char string[512];		/* scratch space             */
> > +	int hash;			/* hash bucket index           */
> > +	unsigned nhash;			/* Index within current bucket */
> > +	struct gfs2_sbd *sdp;		/* incore superblock           */
> > +	struct gfs2_glock *gl;		/* current glock struct        */
> > +	loff_t last_pos;		/* last position               */
> > +	char string[512];		/* scratch space               */
> >  };
> >  
> >  typedef void (*glock_examiner) (struct gfs2_glock * gl);
> > @@ -950,7 +952,7 @@ void gfs2_print_dbg(struct seq_file *seq, const char *fmt, ...)
> >  	if (seq) {
> >  		struct gfs2_glock_iter *gi = seq->private;
> >  		vsprintf(gi->string, fmt, args);
> > -		seq_printf(seq, gi->string);
> > +		seq_puts(seq, gi->string);
> 
> This looks like a bug fix on its own ?
> 
> Anyway, the vsprintf(gi->string, ...) sounds risky too, vsnprintf() is
> your friend.
> 
There are no strings (except fixed length ones) which will be printed.
So all the fields are of bounded length and the format is also fixed.

> I suggest you add seq_vprintf() interface to get rid of the string[512]
> scratch/kludge and remove one copy...
> 
That sounds like a good idea... this bit of code is quite old and
seq_vprintf was not available originally (neither was seq_puts or I
suspect I'd have used that at the time) so I'll do a follow up patch to
resolve that,

Steve.