From: Steven Whitehouse <swhiteho@redhat.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [GFS2 PATCH 2/3] GFS2: Drop initial reservation if too small
Date: Tue, 17 Sep 2013 10:24:57 +0100 [thread overview]
Message-ID: <1379409897.2722.10.camel@menhir> (raw)
In-Reply-To: <4e9fc47e229352b8c2e2b7500df71fb9ac9a4a00.1379361813.git.rpeterso@redhat.com>
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);
next prev parent reply other threads:[~2013-09-17 9:24 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-16 20:10 [Cluster-devel] [GFS2 PATCH 1/3] GFS2: Eliminate a goto from function gfs2_inplace_reserve Bob Peterson
2013-09-16 20:10 ` [Cluster-devel] [GFS2 PATCH 2/3] GFS2: Drop initial reservation if too small Bob Peterson
2013-09-17 9:24 ` Steven Whitehouse [this message]
2013-09-17 17:33 ` Bob Peterson
2013-09-16 20:10 ` [Cluster-devel] [GFS2 PATCH 3/3] GFS2: Don't insert into reservations tree unless rgrp has enough blocks Bob Peterson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1379409897.2722.10.camel@menhir \
--to=swhiteho@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).