[Cluster-devel] [GFS2 PATCH] GFS2: Limit number of transaction blocks requested for truncates

[Cluster-devel] [GFS2 PATCH] GFS2: Limit number of transaction blocks requested for truncates

[Bob Peterson]

Hi,

This patch limits the number of transaction blocks requested during
file truncates. If we have very large multi-terabyte files, and want
to delete or truncate them, they might span so many resource groups
that we overflow the journal blocks, and cause an assert failure.
By limiting the number of blocks in the transaction, we prevent this
overflow and give other running processes time to do transactions.

Note that this is a temporary solution. A long-term solution would
be to implement the non-recursive truncate that processes one
resource group at a time, and therefore, the transactions need
much fewer blocks.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 645721f..77f2ab1 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -720,6 +720,7 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct gfs2_rgrp_list rlist;
+	struct gfs2_trans *tr;
 	u64 bn, bstart;
 	u32 blen, btotal;
 	__be64 *p;
@@ -728,6 +729,7 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
 	unsigned int revokes = 0;
 	int x;
 	int error;
+	int jblocks_rqsted;
 
 	error = gfs2_rindex_update(sdp);
 	if (error)
@@ -791,12 +793,17 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
 	if (gfs2_rs_active(&ip->i_res)) /* needs to be done with the rgrp glock held */
 		gfs2_rs_deltree(&ip->i_res);
 
-	error = gfs2_trans_begin(sdp, rg_blocks + RES_DINODE +
-				 RES_INDIRECT + RES_STATFS + RES_QUOTA,
-				 revokes);
+restart:
+	jblocks_rqsted = rg_blocks + RES_DINODE +
+		RES_INDIRECT + RES_STATFS + RES_QUOTA +
+		gfs2_struct2blk(sdp, revokes, sizeof(u64));
+	if (jblocks_rqsted > RES_MAX)
+		jblocks_rqsted = RES_MAX;
+	error = gfs2_trans_begin(sdp, jblocks_rqsted, revokes);
 	if (error)
 		goto out_rg_gunlock;
 
+	tr = current->journal_info;
 	down_write(&ip->i_rw_mutex);
 
 	gfs2_trans_add_meta(ip->i_gl, dibh);
@@ -810,6 +817,15 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
 		if (!*p)
 			continue;
 
+		/* check for max reasonable journal transaction blocks */
+		if (tr->tr_num_buf_new + RES_STATFS + RES_QUOTA >= RES_MAX) {
+			if (rg_blocks >= tr->tr_num_buf_new)
+				rg_blocks -= tr->tr_num_buf_new;
+			else
+				rg_blocks = 0;
+			break;
+		}
+
 		bn = be64_to_cpu(*p);
 
 		if (bstart + blen == bn)
@@ -827,6 +843,9 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
 		*p = 0;
 		gfs2_add_inode_blocks(&ip->i_inode, -1);
 	}
+	if (p == bottom)
+		rg_blocks = 0;
+
 	if (bstart) {
 		__gfs2_free_blocks(ip, bstart, blen, metadata);
 		btotal += blen;
@@ -844,6 +863,9 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
 
 	gfs2_trans_end(sdp);
 
+	if (rg_blocks)
+		goto restart;
+
 out_rg_gunlock:
 	gfs2_glock_dq_m(rlist.rl_rgrps, rlist.rl_ghs);
 out_rlist:
diff --git a/fs/gfs2/trans.h b/fs/gfs2/trans.h
index 1e6e7da..c212d16 100644
--- a/fs/gfs2/trans.h
+++ b/fs/gfs2/trans.h
@@ -25,6 +25,7 @@ struct gfs2_glock;
 #define RES_EATTR	1
 #define RES_STATFS	1
 #define RES_QUOTA	2
+#define RES_MAX		256 /* Must not exceed 8192 (min journal is 32MB) */
 
 /* reserve either the number of blocks to be allocated plus the rg header
  * block, or all of the blocks in the rg, whichever is smaller */

[Cluster-devel] [GFS2 PATCH] GFS2: Limit number of transaction blocks requested for truncates

[Steven Whitehouse]

Hi,

On 16/12/16 14:16, Bob Peterson wrote:
> Hi,
>
> This patch limits the number of transaction blocks requested during
> file truncates. If we have very large multi-terabyte files, and want
> to delete or truncate them, they might span so many resource groups
> that we overflow the journal blocks, and cause an assert failure.
> By limiting the number of blocks in the transaction, we prevent this
> overflow and give other running processes time to do transactions.
>
> Note that this is a temporary solution. A long-term solution would
> be to implement the non-recursive truncate that processes one
> resource group at a time, and therefore, the transactions need
> much fewer blocks.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index 645721f..77f2ab1 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -720,6 +720,7 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
>   {
>   	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>   	struct gfs2_rgrp_list rlist;
> +	struct gfs2_trans *tr;
>   	u64 bn, bstart;
>   	u32 blen, btotal;
>   	__be64 *p;
> @@ -728,6 +729,7 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
>   	unsigned int revokes = 0;
>   	int x;
>   	int error;
> +	int jblocks_rqsted;
>   
>   	error = gfs2_rindex_update(sdp);
>   	if (error)
> @@ -791,12 +793,17 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
>   	if (gfs2_rs_active(&ip->i_res)) /* needs to be done with the rgrp glock held */
>   		gfs2_rs_deltree(&ip->i_res);
>   
> -	error = gfs2_trans_begin(sdp, rg_blocks + RES_DINODE +
> -				 RES_INDIRECT + RES_STATFS + RES_QUOTA,
> -				 revokes);
> +restart:
> +	jblocks_rqsted = rg_blocks + RES_DINODE +
> +		RES_INDIRECT + RES_STATFS + RES_QUOTA +
> +		gfs2_struct2blk(sdp, revokes, sizeof(u64));
> +	if (jblocks_rqsted > RES_MAX)
> +		jblocks_rqsted = RES_MAX;
> +	error = gfs2_trans_begin(sdp, jblocks_rqsted, revokes);
>   	if (error)
>   		goto out_rg_gunlock;
>   
> +	tr = current->journal_info;
>   	down_write(&ip->i_rw_mutex);
>   
>   	gfs2_trans_add_meta(ip->i_gl, dibh);
> @@ -810,6 +817,15 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
>   		if (!*p)
>   			continue;
>   
> +		/* check for max reasonable journal transaction blocks */
> +		if (tr->tr_num_buf_new + RES_STATFS + RES_QUOTA >= RES_MAX) {
> +			if (rg_blocks >= tr->tr_num_buf_new)
> +				rg_blocks -= tr->tr_num_buf_new;
> +			else
> +				rg_blocks = 0;
> +			break;
> +		}
> +
>   		bn = be64_to_cpu(*p);
>   
>   		if (bstart + blen == bn)
> @@ -827,6 +843,9 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
>   		*p = 0;
>   		gfs2_add_inode_blocks(&ip->i_inode, -1);
>   	}
> +	if (p == bottom)
> +		rg_blocks = 0;
> +
>   	if (bstart) {
>   		__gfs2_free_blocks(ip, bstart, blen, metadata);
>   		btotal += blen;
> @@ -844,6 +863,9 @@ static int do_strip(struct gfs2_inode *ip, struct buffer_head *dibh,
>   
>   	gfs2_trans_end(sdp);
>   
> +	if (rg_blocks)
> +		goto restart;
> +
>   out_rg_gunlock:
>   	gfs2_glock_dq_m(rlist.rl_rgrps, rlist.rl_ghs);
>   out_rlist:
> diff --git a/fs/gfs2/trans.h b/fs/gfs2/trans.h
> index 1e6e7da..c212d16 100644
> --- a/fs/gfs2/trans.h
> +++ b/fs/gfs2/trans.h
> @@ -25,6 +25,7 @@ struct gfs2_glock;
>   #define RES_EATTR	1
>   #define RES_STATFS	1
>   #define RES_QUOTA	2
> +#define RES_MAX		256 /* Must not exceed 8192 (min journal is 32MB) */
>   
>   /* reserve either the number of blocks to be allocated plus the rg header
>    * block, or all of the blocks in the rg, whichever is smaller */
>
If it can't exceed 8192, then why is it only 256, could there not be a 
larger number? It should probably scale with journal size to avoid 
causing issues for those with larger journals. The approach looks good 
though for a temporary fix,

Steve.

[Cluster-devel] [GFS2 PATCH] GFS2: Limit number of transaction blocks requested for truncates

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| If it can't exceed 8192, then why is it only 256, could there not be a
| larger number? It should probably scale with journal size to avoid
| causing issues for those with larger journals. The approach looks good
| though for a temporary fix,
| 
| Steve.

Hi,

Yeah, I toyed with the idea of making it scale with the journal, but
anything I chose just seemed so...arbitrary. I originally coded it as the
number of blocks in the journal - some slop for log headers and such:

+       if (jblocks_needed + RES_SLOP > sdp->sd_jdesc->jd_blocks) {
+               jblocks_needed = sdp->sd_jdesc->jd_blocks - RES_SLOP;

At that time I started defined it as:

+#define RES_SLOP       16

I also toyed with the idea of using half the journal blocks:

jblocks_needed = sdp->sd_jdesc->jd_blocks > 1;

Or maybe 3/4 of the journal:

jblocks_needed = sdp->sd_jdesc->jd_blocks > 1 +
                 sdp->sd_jdesc->jd_blocks > 2;

...but I didn't want to waste a bunch of CPU time doing calculations
either: deletes are already slow. Not that that would slow us down much.

In the end, I decided an arbitrary 256 was simple and enough for the
vast majority of deletes.

I'm perfectly happy to use whatever value makes the most sense.
Suggestions?

Regards,

Bob Peterson
Red Hat File Systems