From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Thu, 09 Aug 2012 10:12:09 +0100 Subject: [Cluster-devel] [GFS2 PATCH] GFS2: Misc minor optimizations In-Reply-To: <1352170958.4074439.1344459144566.JavaMail.root@redhat.com> References: <1352170958.4074439.1344459144566.JavaMail.root@redhat.com> Message-ID: <1344503529.2710.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 Wed, 2012-08-08 at 16:52 -0400, Bob Peterson wrote: > Hi, > > This patch implements some minor optimizations of the glock and rgrp code: > In most cases, I just combined tiny one or two-line functions into their > only caller. Some of these optimizations may gain us little to no real > improvement, but having fewer functions also makes it easier to read and > understand. In order of appearance: > When there are a number of unrelated things it would be better to break into separate patches. It at least gives us a better chance if we need to bisect in the future. > 1. In function gfs2_direct_IO, the glock was dequeued as a group of 1 > glock. This was changed to a normal dequeue for readability. Looks good > 2. Two-line function __gfs2_glock_schedule_for_reclaim was coded into > its only caller, gfs2_glock_dq. Ok, although that is unlikely to make any real difference since the compiler will do that anyway. > 3. Function gfs2_glock_wait only called function wait_on_holder and > returned its return code, so they were combined. Looks good > 4. Function gfs2_glock_dq_wait called two-line function wait_on_demote, > so they were combined. As per #2, the compiler will do this for us anyway, so no real gain there. > 5. Function add_to_queue was checking may_grant for the passed-in > holder for every iteration of its gh2 loop. Now it only checks it > once at the beginning to see if a try lock is futile. Sounds like a good plan, but this should really be split out from the rest of this patch. > 6. Function gfs2_bitfit was checking for state > 3, but that's > impossible since it is only called from rgblk_search, which receives > only GFS2_BLKST_ constants. > Looks good - this has become redundant now that we can easily visually check the passed constants, Steve. > Regards, > > Bob Peterson > Red Hat File Systems > > Signed-off-by: Bob Peterson > --- > diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c > index 00eaa83..01c4975 100644 > --- a/fs/gfs2/aops.c > +++ b/fs/gfs2/aops.c > @@ -1024,7 +1024,7 @@ static ssize_t gfs2_direct_IO(int rw, struct kiocb *iocb, > offset, nr_segs, gfs2_get_block_direct, > NULL, NULL, 0); > out: > - gfs2_glock_dq_m(1, &gh); > + gfs2_glock_dq(&gh); > gfs2_holder_uninit(&gh); > return rv; > } > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c > index 1ed81f4..8146c6f 100644 > --- a/fs/gfs2/glock.c > +++ b/fs/gfs2/glock.c > @@ -186,20 +186,6 @@ static void gfs2_glock_remove_from_lru(struct gfs2_glock *gl) > } > > /** > - * __gfs2_glock_schedule_for_reclaim - Add a glock to the reclaim list > - * @gl: the glock > - * > - * If the glock is demotable, then we add it (or move it) to the end > - * of the glock LRU list. > - */ > - > -static void __gfs2_glock_schedule_for_reclaim(struct gfs2_glock *gl) > -{ > - if (demote_ok(gl)) > - gfs2_glock_add_to_lru(gl); > -} > - > -/** > * gfs2_glock_put_nolock() - Decrement reference count on glock > * @gl: The glock to put > * > @@ -883,7 +869,7 @@ static int gfs2_glock_demote_wait(void *word) > return 0; > } > > -static void wait_on_holder(struct gfs2_holder *gh) > +int gfs2_glock_wait(struct gfs2_holder *gh) > { > unsigned long time1 = jiffies; > > @@ -894,12 +880,7 @@ static void wait_on_holder(struct gfs2_holder *gh) > gh->gh_gl->gl_hold_time = min(gh->gh_gl->gl_hold_time + > GL_GLOCK_HOLD_INCR, > GL_GLOCK_MAX_HOLD); > -} > - > -static void wait_on_demote(struct gfs2_glock *gl) > -{ > - might_sleep(); > - wait_on_bit(&gl->gl_flags, GLF_DEMOTE, gfs2_glock_demote_wait, TASK_UNINTERRUPTIBLE); > + return gh->gh_error; > } > > /** > @@ -929,19 +910,6 @@ static void handle_callback(struct gfs2_glock *gl, unsigned int state, > trace_gfs2_demote_rq(gl); > } > > -/** > - * gfs2_glock_wait - wait on a glock acquisition > - * @gh: the glock holder > - * > - * Returns: 0 on success > - */ > - > -int gfs2_glock_wait(struct gfs2_holder *gh) > -{ > - wait_on_holder(gh); > - return gh->gh_error; > -} > - > void gfs2_print_dbg(struct seq_file *seq, const char *fmt, ...) > { > struct va_format vaf; > @@ -979,7 +947,7 @@ __acquires(&gl->gl_spin) > struct gfs2_sbd *sdp = gl->gl_sbd; > struct list_head *insert_pt = NULL; > struct gfs2_holder *gh2; > - int try_lock = 0; > + int try_futile = 0; > > BUG_ON(gh->gh_owner_pid == NULL); > if (test_and_set_bit(HIF_WAIT, &gh->gh_iflags)) > @@ -987,7 +955,7 @@ __acquires(&gl->gl_spin) > > if (gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)) { > if (test_bit(GLF_LOCK, &gl->gl_flags)) > - try_lock = 1; > + try_futile = !may_grant(gl, gh); > if (test_bit(GLF_INVALIDATE_IN_PROGRESS, &gl->gl_flags)) > goto fail; > } > @@ -996,9 +964,8 @@ __acquires(&gl->gl_spin) > if (unlikely(gh2->gh_owner_pid == gh->gh_owner_pid && > (gh->gh_gl->gl_ops->go_type != LM_TYPE_FLOCK))) > goto trap_recursive; > - if (try_lock && > - !(gh2->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)) && > - !may_grant(gl, gh)) { > + if (try_futile && > + !(gh2->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) { > fail: > gh->gh_error = GLR_TRYFAILED; > gfs2_holder_wake(gh); > @@ -1121,8 +1088,8 @@ void gfs2_glock_dq(struct gfs2_holder *gh) > !test_bit(GLF_DEMOTE, &gl->gl_flags)) > fast_path = 1; > } > - if (!test_bit(GLF_LFLUSH, &gl->gl_flags)) > - __gfs2_glock_schedule_for_reclaim(gl); > + if (!test_bit(GLF_LFLUSH, &gl->gl_flags) && demote_ok(gl)) > + gfs2_glock_add_to_lru(gl); > trace_gfs2_glock_queue(gh, 0); > spin_unlock(&gl->gl_spin); > if (likely(fast_path)) > @@ -1141,7 +1108,8 @@ void gfs2_glock_dq_wait(struct gfs2_holder *gh) > { > struct gfs2_glock *gl = gh->gh_gl; > gfs2_glock_dq(gh); > - wait_on_demote(gl); > + might_sleep(); > + wait_on_bit(&gl->gl_flags, GLF_DEMOTE, gfs2_glock_demote_wait, TASK_UNINTERRUPTIBLE); > } > > /** > diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c > index c267118..47d2346 100644 > --- a/fs/gfs2/rgrp.c > +++ b/fs/gfs2/rgrp.c > @@ -228,8 +228,6 @@ static u32 gfs2_bitfit(const u8 *buf, const unsigned int len, > u64 mask = 0x5555555555555555ULL; > u32 bit; > > - BUG_ON(state > 3); > - > /* Mask off bits we don't care about at the start of the search */ > mask <<= spoint; > tmp = gfs2_bit_search(ptr, mask, state); >