[Cluster-devel] [GFS2 PATCH 2/3] GFS2: Drop initial reservation if too small

[Steven Whitehouse]

Hi,

On Mon, 2013-09-16 at 15:10 -0500, Bob Peterson wrote:
> Before this patch, function gfs2_inplace_reserve would grab a blocking
> lock on the rgrp glock. This is not necessary if we already know that
> rgrp doesn't have enough blocks to satisfy the request. This patch
> makes the function drop the inadequate reservation so that a more
> suitable rgrp may be found (with a try lock). This is not a very
> important thing to do, and it still requires a blocking lock to remove
> the reservation. However, it allows us to further simplify the code
> with a future patch because after this patch, we will know for sure
> that the reservation will not be in the rgrp's reservations tree
> unless it has enough blocks.
> ---
>  fs/gfs2/rgrp.c | 45 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 41 insertions(+), 4 deletions(-)
> 
This looks like the wrong thing to do. If we have a reservation, then we
should always use it until it is used up, otherwise we are adding more
fragmentation when we do not need to. The requested amount is supposed
to be the number of blocks that we'd like to have in an ideal world, but
provided we can supply the minimum number of blocks (currently 1) then
there is no need to drop the reservation.

Also, I don't want to reintroduce the try locks now that we've
eliminated them from this part of the code, since that brings back
another set of problems. Maybe I'm not understanding what the intent is
here though as despite the comment above, the patches don't appear to
actually bring the try lock back.

postmark only tends to write very small files, depending on what
settings it is being run with, so will likely not gain from the
reservations work at all. The main reason for using it is to ensure that
we don't get a regression rather than expecting it to show any
improvements.

Steve.


> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 8d5d583..ed57fdf 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1826,6 +1826,37 @@ static bool gfs2_select_rgrp(struct gfs2_rgrpd **pos, const struct gfs2_rgrpd *b
>  }
>  
>  /**
> + * drop_if_too_small - Drop a reservation if it's too small.
> + * @rs: reservation to check
> + * @requested: number of blocks requested
> + *
> + * Returns: 0 if good
> + */
> +static int drop_if_too_small(struct gfs2_blkreserv *rs, u32 requested)
> +{
> +	int rg_locked, error;
> +
> +	if (rs->rs_rbm.rgd->rd_free_clone >= requested)
> +		return 0;
> +
> +	rg_locked = 0;
> +	if (gfs2_glock_is_locked_by_me(rs->rs_rbm.rgd->rd_gl))
> +		rg_locked = 1;
> +
> +	if (!rg_locked) {
> +		error = gfs2_glock_nq_init(rs->rs_rbm.rgd->rd_gl,
> +					   LM_ST_EXCLUSIVE, 0, &rs->rs_rgd_gh);
> +		if (unlikely(error))
> +			return error;
> +	}
> +	gfs2_rs_deltree(rs);
> +	if (!rg_locked)
> +		gfs2_glock_dq_uninit(&rs->rs_rgd_gh);
> +
> +	return 0;
> +}
> +
> +/**
>   * gfs2_inplace_reserve - Reserve space in the filesystem
>   * @ip: the inode to reserve space for
>   * @requested: the number of blocks to be reserved
> @@ -1849,10 +1880,16 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, u32 requested, u32 aflags)
>  		return -EINVAL;
>  	if (gfs2_rs_active(rs)) {
>  		begin = rs->rs_rbm.rgd;
> -	} else if (ip->i_rgd && rgrp_contains_block(ip->i_rgd, ip->i_goal)) {
> -		rs->rs_rbm.rgd = begin = ip->i_rgd;
> -	} else {
> -		rs->rs_rbm.rgd = begin = gfs2_blk2rgrpd(sdp, ip->i_goal, 1);
> +		error = drop_if_too_small(rs, requested);
> +		if (unlikely(error))
> +			return error;
> +	}
> +	if (!gfs2_rs_active(rs)) {
> +		if (ip->i_rgd && rgrp_contains_block(ip->i_rgd, ip->i_goal)) {
> +			rs->rs_rbm.rgd = begin = ip->i_rgd;
> +		} else {
> +			rs->rs_rbm.rgd = begin = gfs2_blk2rgrpd(sdp, ip->i_goal, 1);
> +		}
>  	}
>  	if (S_ISDIR(ip->i_inode.i_mode) && (aflags & GFS2_AF_ORLOV))
>  		skip = gfs2_orlov_skip(ip);