[Cluster-devel] [GFS2 PATCH 2/6] GFS2: Check gfs2_rgrp_used_recently inside gfs2_rgrp_congested

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,


On 10/01/18 20:42, Bob Peterson wrote:
> Function gfs2_rgrp_congested is called in two places: once before
> we acquire the rgrp glock, and once after. The first time, it checks
> to see if the rgrp was used recently because the usage statistics
> are only valid in short-term. For the second call, that check was
> not done, but should have been, for the same reason.
>
> This patch simply moves function gfs2_rgrp_used_recently before
> function gfs2_rgrp_congested, and it's now called from there so
> it applies equally to both checks.
I don't think this makes sense. The purpose of the 
gfs2_rgrp_used_recently test is that when we don't hold the glock we 
might not have up to date stats to make decisions from, so we check that 
we've held the lock recently enough that we can use historical 
statistics. Once we hold the lock however, then we can be sure that the 
stats are up to date, as the process of acquiring the lock will bring 
them up to date, so why repeat the test when we already have the lock?

Steve.

> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/rgrp.c | 36 ++++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index ea95d7c70e23..960fbc2d39d7 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1819,6 +1819,21 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
>   	return;
>   }
>   
> +/**
> + * gfs2_rgrp_used_recently - check if a rgrp has been used recently
> + * @rgd: The rgrp to test
> + * @msecs: The time limit in milliseconds
> + *
> + * Returns: True if the rgrp glock has been used within the time limit
> + */
> +static inline bool gfs2_rgrp_used_recently(const struct gfs2_rgrpd *rgd,
> +					   u64 msecs)
> +{
> +	return (ktime_before(ktime_get_real(),
> +			     ktime_add(rgd->rd_gl->gl_dstamp,
> +				       ms_to_ktime(msecs))));
> +}
> +
>   /**
>    * gfs2_rgrp_congested - Use stats to figure out whether an rgrp is congested
>    * @rgd: The rgrp in question
> @@ -1858,6 +1873,11 @@ static bool gfs2_rgrp_congested(const struct gfs2_rgrpd *rgd, int loops)
>   	u64 var;
>   	int cpu, nonzero = 0;
>   
> +	/* If it hasn't been used recently we can't judge the statistics, so
> +	   assume it's not congested. */
> +	if (!gfs2_rgrp_used_recently(rgd, HZ))
> +		return false;
> +
>   	preempt_disable();
>   	for_each_present_cpu(cpu) {
>   		st = &per_cpu_ptr(sdp->sd_lkstats, cpu)->lkstats[LM_TYPE_RGRP];
> @@ -1892,21 +1912,6 @@ static bool gfs2_rgrp_congested(const struct gfs2_rgrpd *rgd, int loops)
>   	return ((srttb_diff < 0) && (sqr_diff > var));
>   }
>   
> -/**
> - * gfs2_rgrp_used_recently - check if a rgrp has been used recently
> - * @rgd: The rgrp to test
> - * @msecs: The time limit in milliseconds
> - *
> - * Returns: True if the rgrp glock has been used within the time limit
> - */
> -static inline bool gfs2_rgrp_used_recently(const struct gfs2_rgrpd *rgd,
> -					   u64 msecs)
> -{
> -	return (ktime_before(ktime_get_real(),
> -			     ktime_add(rgd->rd_gl->gl_dstamp,
> -				       ms_to_ktime(msecs))));
> -}
> -
>   static u32 gfs2_orlov_skip(const struct gfs2_inode *ip)
>   {
>   	const struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
> @@ -2004,7 +2009,6 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap)
>   				    !fast_to_acquire(rs->rs_rbm.rgd))
>   					goto next_rgrp;
>   				if ((loops < 2) &&
> -				    gfs2_rgrp_used_recently(rs->rs_rbm.rgd, HZ) &&
>   				    gfs2_rgrp_congested(rs->rs_rbm.rgd, loops))
>   					goto next_rgrp;
>   			}