[Cluster-devel] [GFS2 PATCH] GFS2: Misc minor optimizations
Steven Whitehouse
swhiteho at redhat.com
Thu Aug 9 09:12:09 UTC 2012
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 <rpeterso at redhat.com>
> ---
> 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);
>
More information about the Cluster-devel
mailing list