[Cluster-devel] [GFS2 PATCH 1/3] GFS2: Eliminate a goto from function gfs2_inplace_reserve

[Cluster-devel] [GFS2 PATCH 1/3] GFS2: Eliminate a goto from function gfs2_inplace_reserve

[Bob Peterson]

This patch just refactors function gfs2_inplace_reserve in order to
eliminate a goto. This is mainly just preparation for a future
patch that will simplify the logic and improve performance.
---
 fs/gfs2/rgrp.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index e4f3362..8d5d583 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1899,19 +1899,17 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, u32 requested, u32 aflags)
 			rg_mblk_search(rs->rs_rbm.rgd, ip, requested);
 
 		/* Skip rgrps when we can't get a reservation on first pass */
-		if (!gfs2_rs_active(rs) && (loops < 1))
-			goto check_rgrp;
+		if (gfs2_rs_active(rs) || (loops >= 1)) {
+			/* If rgrp has enough free space, use it */
+			if (rs->rs_rbm.rgd->rd_free_clone >= requested) {
+				ip->i_rgd = rs->rs_rbm.rgd;
+				return 0;
+			}
 
-		/* If rgrp has enough free space, use it */
-		if (rs->rs_rbm.rgd->rd_free_clone >= requested) {
-			ip->i_rgd = rs->rs_rbm.rgd;
-			return 0;
+			/* Drop reservation if we couldn't use reserved rgrp */
+			if (gfs2_rs_active(rs))
+				gfs2_rs_deltree(rs);
 		}
-
-		/* Drop reservation, if we couldn't use reserved rgrp */
-		if (gfs2_rs_active(rs))
-			gfs2_rs_deltree(rs);
-check_rgrp:
 		/* Check for unlinked inodes which can be reclaimed */
 		if (rs->rs_rbm.rgd->rd_flags & GFS2_RDF_CHECK)
 			try_rgrp_unlink(rs->rs_rbm.rgd, &last_unlinked,
-- 
1.8.3.1

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

[Bob Peterson]

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

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

[Cluster-devel] [GFS2 PATCH 3/3] GFS2: Don't insert into reservations tree unless rgrp has enough blocks

[Bob Peterson]

The previous two patches simplified the logic in gfs2_inplace_reserve
and ensured that inadequate reservations are dropped before we enter
the main loop. This patch takes the next logical step and makes sure
that new reservations don't get inserted into the rgrp reservations
tree unless they are also adequate. So now we don't have to repeatly
insert a reservation, determine it's inadequate, then remove it from
the tree again. We can take a proactive approach that says: Only
insert it if it's adequate. That's makes the code a little faster.
---
 fs/gfs2/rgrp.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index ed57fdf..59637ab 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1455,7 +1455,7 @@ static void rg_mblk_search(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip,
 		return;
 
 	ret = gfs2_rbm_find(&rbm, GFS2_BLKST_FREE, extlen, ip, true);
-	if (ret == 0) {
+	if (ret == 0 && rgd->rd_free_clone >= requested) {
 		rs->rs_rbm = rbm;
 		rs->rs_free = extlen;
 		rs->rs_inum = ip->i_no_addr;
@@ -1936,16 +1936,11 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, u32 requested, u32 aflags)
 			rg_mblk_search(rs->rs_rbm.rgd, ip, requested);
 
 		/* Skip rgrps when we can't get a reservation on first pass */
-		if (gfs2_rs_active(rs) || (loops >= 1)) {
-			/* If rgrp has enough free space, use it */
-			if (rs->rs_rbm.rgd->rd_free_clone >= requested) {
-				ip->i_rgd = rs->rs_rbm.rgd;
-				return 0;
-			}
-
-			/* Drop reservation if we couldn't use reserved rgrp */
-			if (gfs2_rs_active(rs))
-				gfs2_rs_deltree(rs);
+		if (gfs2_rs_active(rs) ||
+		    ((loops >= 1) &&
+		     (rs->rs_rbm.rgd->rd_free_clone >= requested))) {
+			    ip->i_rgd = rs->rs_rbm.rgd;
+			    return 0;
 		}
 		/* Check for unlinked inodes which can be reclaimed */
 		if (rs->rs_rbm.rgd->rd_flags & GFS2_RDF_CHECK)
-- 
1.8.3.1

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

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

[Bob Peterson]

----- Original Message -----
| 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.

Hi,

Actually, unless I'm reading the code wrong, it already does this.
If this is the wrong thing to do, we need to change the logic.
In today's gfs2_inplace_reserve, there's a check:

		if (rs->rs_rbm.rgd->rd_free_clone >= requested) {
			ip->i_rgd = rs->rs_rbm.rgd;
			return 0;
		}
                (otherwise it gets rejected)

This similarly rejects any pre-existing reservation if it doesn't satisfy
the requested size. What I was trying to do here is preserve the existing
logic. If it's wrong and we need to change it, that's fine, but I have
doubts that it will pass all the testing; when I was working on the
block reservations code, I believe I tried that and got into some nasty
problems. Still, it may be worth trying again.

Regards,

Bob Peterson
Red Hat File Systems