[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] [GFS2 PATCH] GFS2: Misc minor optimizations



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 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);
> 



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]