From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Mon, 11 Jun 2012 09:29:37 +0100 Subject: [Cluster-devel] GFS2: Cache last hash bucket for glock seq_files In-Reply-To: <1339230731.6001.159.camel@edumazet-glaptop> References: <1339152726.2752.3.camel@menhir> <1339230731.6001.159.camel@edumazet-glaptop> Message-ID: <1339403377.2734.7.camel@menhir> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 > > 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 > > Signed-off-by: Steven Whitehouse > > > > 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.