[Cluster-devel] [gfs2-utils PATCH 0/7] fsck.gfs2 performance improvements

[Cluster-devel] [gfs2-utils PATCH 0/7] fsck.gfs2 performance improvements

[Bob Peterson]

My recent set of patches to fsck.gfs2 saved a lot of memory, thus
enabling us to run fsck.gfs2 on much larger file systems. However,
it slowed things down and our performance regressed. This is a set
of seven patches designed to improve performance again. There are
probably more improvements I can make, but I've been busy with kernel
work, so it's not my primary focus. If I come up with more patches,
I'll post them later.
---
Bob Peterson (7):
  fsck.gfs2: Don't bother to pass bl blockmap pointer
  fsck.gfs2: Remember the previous rgrp pointer for speed
  fsck.gfs2: Don't set gfs1rg pointer unless we need to
  fsck.gfs2: Make _fsck_bitmap_set not send a return code
  fsck.gfs2: convert fsck_bitmap_set to a macro
  fsck.gfs2: Speed up function bitmap_type
  fsck.gfs2: Make pass2 go by directory rbtree for performance

 gfs2/fsck/metawalk.c | 69 ++++++----------------------------------------------
 gfs2/fsck/metawalk.h | 65 +++++++++++++++++++++++++++++++++++++++++++------
 gfs2/fsck/pass1.c    | 47 +++++++++++++++++++++--------------
 gfs2/fsck/pass2.c    | 21 ++++++----------
 gfs2/fsck/util.h     |  8 +++++-
 5 files changed, 110 insertions(+), 100 deletions(-)

-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 1/7] fsck.gfs2: Don't bother to pass bl blockmap pointer

[Bob Peterson]

Since the blockmap pointer, bl, is a global variable of pass1, we
don't need to pass it around needlessly. Make this more efficient.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/pass1.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 6f04109..112b68e 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -90,17 +90,17 @@ static int delete_block(struct gfs2_inode *ip, uint64_t block,
 			struct gfs2_buffer_head **bh, const char *btype,
 			void *private);
 
-static int gfs2_blockmap_set(struct gfs2_bmap *bmap, uint64_t bblock, int mark)
+static int gfs2_blockmap_set(uint64_t bblock, int mark)
 {
 	static unsigned char *byte;
 	static uint64_t b;
 
-	if (!bmap)
+	if (bl == NULL)
 		return 0;
-	if (bblock > bmap->size)
+	if (bblock > bl->size)
 		return -1;
 
-	byte = bmap->map + BLOCKMAP_SIZE2(bblock);
+	byte = bl->map + BLOCKMAP_SIZE2(bblock);
 	b = BLOCKMAP_BYTE_OFFSET2(bblock);
 	*byte &= ~(BLOCKMAP_MASK2 << b);
 	*byte |= (mark & BLOCKMAP_MASK2) << b;
@@ -120,7 +120,7 @@ static int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t bblock,
 	if (error)
 		return error;
 
-	return gfs2_blockmap_set(bl, bblock, mark);
+	return gfs2_blockmap_set(bblock, mark);
 }
 
 #define fsck_blockmap_set(ip, b, bt, m) \
@@ -1336,7 +1336,7 @@ static int alloc_metalist(struct gfs2_inode *ip, uint64_t block,
 			    "%lld (0x%llx) is now marked as indirect.\n"),
 			  desc, (unsigned long long)block,
 			  (unsigned long long)block);
-		gfs2_blockmap_set(bl, block, ip->i_sbd->gfs1 ?
+		gfs2_blockmap_set(block, ip->i_sbd->gfs1 ?
 				  GFS2_BLKST_DINODE : GFS2_BLKST_USED);
 	}
 	return meta_is_good;
@@ -1358,7 +1358,7 @@ static int alloc_data(struct gfs2_inode *ip, uint64_t metablock,
 			    "%lld (0x%llx) is now marked as data.\n"),
 			  desc, (unsigned long long)block,
 			  (unsigned long long)block);
-		gfs2_blockmap_set(bl, block, GFS2_BLKST_USED);
+		gfs2_blockmap_set(block, GFS2_BLKST_USED);
 	}
 	return 0;
 }
@@ -1407,8 +1407,7 @@ static int pass1_check_metatree(struct gfs2_inode *ip,
 
 	error = check_metatree(ip, pass);
 	if (error)
-		gfs2_blockmap_set(bl, ip->i_di.di_num.no_addr,
-				  GFS2_BLKST_FREE);		
+		gfs2_blockmap_set(ip->i_di.di_num.no_addr, GFS2_BLKST_FREE);
 	return error;
 }
 
@@ -1671,7 +1670,7 @@ static int check_system_inode(struct gfs2_sbd *sdp,
 				   "%llu (0x%llx)\n"),
 				 (unsigned long long)iblock,
 				 (unsigned long long)iblock);
-			gfs2_blockmap_set(bl, iblock, GFS2_BLKST_FREE);
+			gfs2_blockmap_set(iblock, GFS2_BLKST_FREE);
 			check_n_fix_bitmap(sdp, (*sysinode)->i_rgd, iblock, 0,
 					   GFS2_BLKST_FREE);
 			inode_put(sysinode);
@@ -1706,7 +1705,7 @@ static int check_system_inode(struct gfs2_sbd *sdp,
 				/* Set the blockmap (but not bitmap) back to
 				   'free' so that it gets checked like any
 				   normal dinode. */
-				gfs2_blockmap_set(bl, iblock, GFS2_BLKST_FREE);
+				gfs2_blockmap_set(iblock, GFS2_BLKST_FREE);
 				log_err( _("Removed system inode \"%s\".\n"),
 					 filename);
 			}
@@ -2044,7 +2043,7 @@ static int pass1_process_rgrp(struct gfs2_sbd *sdp, struct rgrp_tree *rgd)
 
 		n = lgfs2_bm_scan(rgd, k, ibuf, GFS2_BLKST_UNLINKED);
 		for (i = 0; i < n; i++) {
-			gfs2_blockmap_set(bl, ibuf[i], GFS2_BLKST_UNLINKED);
+			gfs2_blockmap_set(ibuf[i], GFS2_BLKST_UNLINKED);
 			if (fsck_abort)
 				goto out;
 		}
@@ -2200,7 +2199,7 @@ int pass1(struct gfs2_sbd *sdp)
 			log_debug( _("rgrp block %lld (0x%llx) "
 				     "is now marked as 'rgrp data'\n"),
 				   rgd->ri.ri_addr + i, rgd->ri.ri_addr + i);
-			if (gfs2_blockmap_set(bl, rgd->ri.ri_addr + i,
+			if (gfs2_blockmap_set(rgd->ri.ri_addr + i,
 					      GFS2_BLKST_USED)) {
 				stack;
 				gfs2_special_free(&gfs1_rindex_blks);
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 1/7] fsck.gfs2: Don't bother to pass bl blockmap pointer

[Andrew Price]

On 22/06/16 20:26, Bob Peterson wrote:
> Since the blockmap pointer, bl, is a global variable of pass1, we
> don't need to pass it around needlessly. Make this more efficient.

Is it measurably more efficient? I'd rather make bl's scope narrower 
instead of widening it from a maintainability standpoint, but it could 
also let the optimizer make better decisions with more limited scope, 
e.g. when inlining.

In general I don't think we need to optimize at this small scale at this 
point, especially without profiling to make sure the trade-offs are 
worthwhile.

Andy

> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  gfs2/fsck/pass1.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
> index 6f04109..112b68e 100644
> --- a/gfs2/fsck/pass1.c
> +++ b/gfs2/fsck/pass1.c
> @@ -90,17 +90,17 @@ static int delete_block(struct gfs2_inode *ip, uint64_t block,
>  			struct gfs2_buffer_head **bh, const char *btype,
>  			void *private);
>
> -static int gfs2_blockmap_set(struct gfs2_bmap *bmap, uint64_t bblock, int mark)
> +static int gfs2_blockmap_set(uint64_t bblock, int mark)
>  {
>  	static unsigned char *byte;
>  	static uint64_t b;
>
> -	if (!bmap)
> +	if (bl == NULL)
>  		return 0;
> -	if (bblock > bmap->size)
> +	if (bblock > bl->size)
>  		return -1;
>
> -	byte = bmap->map + BLOCKMAP_SIZE2(bblock);
> +	byte = bl->map + BLOCKMAP_SIZE2(bblock);
>  	b = BLOCKMAP_BYTE_OFFSET2(bblock);
>  	*byte &= ~(BLOCKMAP_MASK2 << b);
>  	*byte |= (mark & BLOCKMAP_MASK2) << b;
> @@ -120,7 +120,7 @@ static int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t bblock,
>  	if (error)
>  		return error;
>
> -	return gfs2_blockmap_set(bl, bblock, mark);
> +	return gfs2_blockmap_set(bblock, mark);
>  }
>
>  #define fsck_blockmap_set(ip, b, bt, m) \
> @@ -1336,7 +1336,7 @@ static int alloc_metalist(struct gfs2_inode *ip, uint64_t block,
>  			    "%lld (0x%llx) is now marked as indirect.\n"),
>  			  desc, (unsigned long long)block,
>  			  (unsigned long long)block);
> -		gfs2_blockmap_set(bl, block, ip->i_sbd->gfs1 ?
> +		gfs2_blockmap_set(block, ip->i_sbd->gfs1 ?
>  				  GFS2_BLKST_DINODE : GFS2_BLKST_USED);
>  	}
>  	return meta_is_good;
> @@ -1358,7 +1358,7 @@ static int alloc_data(struct gfs2_inode *ip, uint64_t metablock,
>  			    "%lld (0x%llx) is now marked as data.\n"),
>  			  desc, (unsigned long long)block,
>  			  (unsigned long long)block);
> -		gfs2_blockmap_set(bl, block, GFS2_BLKST_USED);
> +		gfs2_blockmap_set(block, GFS2_BLKST_USED);
>  	}
>  	return 0;
>  }
> @@ -1407,8 +1407,7 @@ static int pass1_check_metatree(struct gfs2_inode *ip,
>
>  	error = check_metatree(ip, pass);
>  	if (error)
> -		gfs2_blockmap_set(bl, ip->i_di.di_num.no_addr,
> -				  GFS2_BLKST_FREE);		
> +		gfs2_blockmap_set(ip->i_di.di_num.no_addr, GFS2_BLKST_FREE);
>  	return error;
>  }
>
> @@ -1671,7 +1670,7 @@ static int check_system_inode(struct gfs2_sbd *sdp,
>  				   "%llu (0x%llx)\n"),
>  				 (unsigned long long)iblock,
>  				 (unsigned long long)iblock);
> -			gfs2_blockmap_set(bl, iblock, GFS2_BLKST_FREE);
> +			gfs2_blockmap_set(iblock, GFS2_BLKST_FREE);
>  			check_n_fix_bitmap(sdp, (*sysinode)->i_rgd, iblock, 0,
>  					   GFS2_BLKST_FREE);
>  			inode_put(sysinode);
> @@ -1706,7 +1705,7 @@ static int check_system_inode(struct gfs2_sbd *sdp,
>  				/* Set the blockmap (but not bitmap) back to
>  				   'free' so that it gets checked like any
>  				   normal dinode. */
> -				gfs2_blockmap_set(bl, iblock, GFS2_BLKST_FREE);
> +				gfs2_blockmap_set(iblock, GFS2_BLKST_FREE);
>  				log_err( _("Removed system inode \"%s\".\n"),
>  					 filename);
>  			}
> @@ -2044,7 +2043,7 @@ static int pass1_process_rgrp(struct gfs2_sbd *sdp, struct rgrp_tree *rgd)
>
>  		n = lgfs2_bm_scan(rgd, k, ibuf, GFS2_BLKST_UNLINKED);
>  		for (i = 0; i < n; i++) {
> -			gfs2_blockmap_set(bl, ibuf[i], GFS2_BLKST_UNLINKED);
> +			gfs2_blockmap_set(ibuf[i], GFS2_BLKST_UNLINKED);
>  			if (fsck_abort)
>  				goto out;
>  		}
> @@ -2200,7 +2199,7 @@ int pass1(struct gfs2_sbd *sdp)
>  			log_debug( _("rgrp block %lld (0x%llx) "
>  				     "is now marked as 'rgrp data'\n"),
>  				   rgd->ri.ri_addr + i, rgd->ri.ri_addr + i);
> -			if (gfs2_blockmap_set(bl, rgd->ri.ri_addr + i,
> +			if (gfs2_blockmap_set(rgd->ri.ri_addr + i,
>  					      GFS2_BLKST_USED)) {
>  				stack;
>  				gfs2_special_free(&gfs1_rindex_blks);
>

[Cluster-devel] [gfs2-utils PATCH 2/7] fsck.gfs2: Remember the previous rgrp pointer for speed

[Bob Peterson]

This is a minor speedup: function check_n_fix_bitmap will now
remember the previous rgrp that it used. This should make it
faster.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/metawalk.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index c0cc2ab..cda59ac 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -40,9 +40,14 @@ int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 		{"free", "data", "unlinked", "inode", "reserved"},
 		/* gfs1 descriptions: */
 		{"free", "data", "free meta", "metadata", "reserved"}};
+	static struct rgrp_tree *prevrgd = NULL;
 
-	if (rgd == NULL || !rgrp_contains_block(rgd, blk))
+	if (prevrgd && rgrp_contains_block(prevrgd, blk)) {
+		rgd = prevrgd;
+	} else if (rgd == NULL || !rgrp_contains_block(rgd, blk)) {
 		rgd = gfs2_blk2rgrpd(sdp, blk);
+		prevrgd = rgd;
+	}
 
 	gfs1rg = (struct gfs_rgrp *)&rgd->rg;
 
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 3/7] fsck.gfs2: Don't set gfs1rg pointer unless we need to

[Bob Peterson]

This patch simply delays setting the gfs1rg pointer until later
in function check_n_fix_bitmap, making it a bit faster.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/metawalk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index cda59ac..651bd79 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -49,8 +49,6 @@ int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 		prevrgd = rgd;
 	}
 
-	gfs1rg = (struct gfs_rgrp *)&rgd->rg;
-
 	old_state = lgfs2_get_bitmap(sdp, blk, rgd);
 	if (old_state < 0) {
 		log_err( _("Block %llu (0x%llx) is not represented in the "
@@ -93,6 +91,8 @@ int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 		rgd->rg.rg_free--;
 		rewrite_rgrp = 1;
 	}
+	gfs1rg = (struct gfs_rgrp *)&rgd->rg;
+
 	/* If we're freeing a dinode, get rid of the data structs for it. */
 	if (old_state == GFS2_BLKST_DINODE ||
 	    old_state == GFS2_BLKST_UNLINKED) {
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 4/7] fsck.gfs2: Make _fsck_bitmap_set not send a return code

[Bob Peterson]

Most places in fsck.gfs2 set the bitmap and don't bother to check
the return code from function fsck_bitmap_set. Only the call from
pass1 needs the return code. So sending back the return code is a
waste of time. We marginally increase the function efficiency by
eliminating the return value, but more importantly, this paves the
way for future patches to make fsck.gfs2 faster.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/metawalk.c | 50 ++++-------------------------------------------
 gfs2/fsck/metawalk.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++-----
 gfs2/fsck/pass1.c    |  9 +++++++--
 3 files changed, 61 insertions(+), 53 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 651bd79..6259a66 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -166,59 +166,17 @@ int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 /*
  * _fsck_bitmap_set - Mark a block in the bitmap, and adjust free space.
  */
-int _fsck_bitmap_set(struct gfs2_inode *ip, uint64_t bblock,
-		     const char *btype, int mark,
-		     int error_on_dinode, const char *caller, int fline)
+void _fsck_bitmap_set(struct gfs2_inode *ip, uint64_t bblock,
+		      const char *btype, int mark,
+		      int error_on_dinode, const char *caller, int fline)
 {
 	int error;
-	static int prev_ino_addr = 0;
-	static int prev_mark = 0;
-	static int prevcount = 0;
-	static const char *prev_caller = NULL;
-
-	if (print_level >= MSG_DEBUG) {
-		if ((ip->i_di.di_num.no_addr == prev_ino_addr) &&
-		    (mark == prev_mark) && caller == prev_caller) {
-			log_info("(0x%llx) ", (unsigned long long)bblock);
-			prevcount++;
-			if (prevcount > 10) {
-				log_info("\n");
-				prevcount = 0;
-			}
-		/* I'm circumventing the log levels here on purpose to make the
-		   output easier to debug. */
-		} else if (ip->i_di.di_num.no_addr == bblock) {
-			if (prevcount) {
-				log_info("\n");
-				prevcount = 0;
-			}
-			printf( _("(%s:%d) %s inode found at block "
-				  "(0x%llx): marking as '%s'\n"), caller, fline,
-			       btype,
-			       (unsigned long long)ip->i_di.di_num.no_addr,
-			       block_type_string(mark));
 
-		} else {
-			if (prevcount) {
-				log_info("\n");
-				prevcount = 0;
-			}
-			printf( _("(%s:%d) inode (0x%llx) references %s block"
-				  " (0x%llx): marking as '%s'\n"),
-			       caller, fline,
-			       (unsigned long long)ip->i_di.di_num.no_addr,
-			       btype, (unsigned long long)bblock,
-			       block_type_string(mark));
-		}
-		prev_ino_addr = ip->i_di.di_num.no_addr;
-		prev_mark = mark;
-		prev_caller = caller;
-	}
+	_fsck_bitmap_set_dbg(ip, bblock, btype, mark, caller, fline);
 	error = check_n_fix_bitmap(ip->i_sbd, ip->i_rgd, bblock,
 				   error_on_dinode, mark);
 	if (error < 0)
 		log_err(_("This block is not represented in the bitmap.\n"));
-	return error;
 }
 
 struct duptree *dupfind(uint64_t block)
diff --git a/gfs2/fsck/metawalk.h b/gfs2/fsck/metawalk.h
index 119efee..5410775 100644
--- a/gfs2/fsck/metawalk.h
+++ b/gfs2/fsck/metawalk.h
@@ -19,9 +19,9 @@ extern int check_linear_dir(struct gfs2_inode *ip, struct gfs2_buffer_head *bh,
 extern int check_leaf(struct gfs2_inode *ip, int lindex,
 		      struct metawalk_fxns *pass, uint64_t *leaf_no,
 		      struct gfs2_leaf *leaf, int *ref_count);
-extern int _fsck_bitmap_set(struct gfs2_inode *ip, uint64_t bblock,
-			    const char *btype, int mark, int error_on_dinode,
-			    const char *caller, int line);
+extern void _fsck_bitmap_set(struct gfs2_inode *ip, uint64_t bblock,
+			     const char *btype, int mark, int error_on_dinode,
+			     const char *caller, int line);
 extern int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 			      uint64_t blk, int error_on_dinode,
 			      int new_state);
@@ -29,12 +29,57 @@ extern struct duptree *dupfind(uint64_t block);
 extern struct gfs2_inode *fsck_system_inode(struct gfs2_sbd *sdp,
 					    uint64_t block);
 
+static inline void _fsck_bitmap_set_dbg(struct gfs2_inode *ip, uint64_t bblock,
+					const char *btype, int mark,
+					const char *caller, int fline)
+{
+	static int prev_ino_addr = 0;
+	static int prev_mark = 0;
+	static int prevcount = 0;
+	static const char *prev_caller = NULL;
+
+	if (print_level < MSG_DEBUG)
+		return;
+
+	if ((ip->i_di.di_num.no_addr == prev_ino_addr) &&
+	    (mark == prev_mark) && caller == prev_caller) {
+		log_info("(0x%llx) ", (unsigned long long)bblock);
+		prevcount++;
+		if (prevcount > 10) {
+			log_info("\n");
+			prevcount = 0;
+		}
+		/* I'm circumventing the log levels here on purpose to make the
+		   output easier to debug. */
+	} else if (ip->i_di.di_num.no_addr == bblock) {
+		if (prevcount) {
+			log_info("\n");
+			prevcount = 0;
+		}
+		printf( _("(%s:%d) %s inode found at block (0x%llx): "
+			  "marking as '%s'\n"), caller, fline, btype,
+			(unsigned long long)ip->i_di.di_num.no_addr,
+			block_type_string(mark));
+	} else {
+		if (prevcount) {
+			log_info("\n");
+			prevcount = 0;
+		}
+		printf( _("(%s:%d) inode (0x%llx) references %s block"
+			  " (0x%llx): marking as '%s'\n"), caller, fline,
+			(unsigned long long)ip->i_di.di_num.no_addr,
+			btype, (unsigned long long)bblock,
+			block_type_string(mark));
+	}
+	prev_ino_addr = ip->i_di.di_num.no_addr;
+	prev_mark = mark;
+	prev_caller = caller;
+}
+
 #define is_duplicate(dblock) ((dupfind(dblock)) ? 1 : 0)
 
 #define fsck_bitmap_set(ip, b, bt, m) \
 	_fsck_bitmap_set(ip, b, bt, m, 0, __FUNCTION__, __LINE__)
-#define fsck_bitmap_set_noino(ip, b, bt, m) \
-	_fsck_bitmap_set(ip, b, bt, m, 1, __FUNCTION__, __LINE__)
 enum meta_check_rc {
 	meta_error = -1,
 	meta_is_good = 0,
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 112b68e..2a3fbde 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -115,8 +115,13 @@ static int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t bblock,
 			      const char *btype, int mark, int error_on_dinode,
 			      const char *caller, int fline)
 {
-	int error = _fsck_bitmap_set(ip, bblock, btype, mark, error_on_dinode,
-				     caller, fline);
+	int error;
+
+	_fsck_bitmap_set_dbg(ip, bblock, btype, mark, caller, fline);
+	error = check_n_fix_bitmap(ip->i_sbd, ip->i_rgd, bblock,
+				   error_on_dinode, mark);
+	if (error < 0)
+		log_err(_("This block is not represented in the bitmap.\n"));
 	if (error)
 		return error;
 
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 5/7] fsck.gfs2: convert fsck_bitmap_set to a macro

[Bob Peterson]

Before this patch fsck_bitmap_set was a function that was passed
in values for function and line number, but it only used them
if it was in verbose mode. This patch reorganizes it into a macro
that only passes things it needs to.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/metawalk.c | 16 ----------------
 gfs2/fsck/metawalk.h | 22 ++++++++++++++--------
 gfs2/fsck/pass1.c    | 15 +++++++++++----
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 6259a66..5adb849 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -163,22 +163,6 @@ int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 	return 0;
 }
 
-/*
- * _fsck_bitmap_set - Mark a block in the bitmap, and adjust free space.
- */
-void _fsck_bitmap_set(struct gfs2_inode *ip, uint64_t bblock,
-		      const char *btype, int mark,
-		      int error_on_dinode, const char *caller, int fline)
-{
-	int error;
-
-	_fsck_bitmap_set_dbg(ip, bblock, btype, mark, caller, fline);
-	error = check_n_fix_bitmap(ip->i_sbd, ip->i_rgd, bblock,
-				   error_on_dinode, mark);
-	if (error < 0)
-		log_err(_("This block is not represented in the bitmap.\n"));
-}
-
 struct duptree *dupfind(uint64_t block)
 {
 	struct osi_node *node = dup_blocks.osi_node;
diff --git a/gfs2/fsck/metawalk.h b/gfs2/fsck/metawalk.h
index 5410775..60059cf 100644
--- a/gfs2/fsck/metawalk.h
+++ b/gfs2/fsck/metawalk.h
@@ -19,9 +19,6 @@ extern int check_linear_dir(struct gfs2_inode *ip, struct gfs2_buffer_head *bh,
 extern int check_leaf(struct gfs2_inode *ip, int lindex,
 		      struct metawalk_fxns *pass, uint64_t *leaf_no,
 		      struct gfs2_leaf *leaf, int *ref_count);
-extern void _fsck_bitmap_set(struct gfs2_inode *ip, uint64_t bblock,
-			     const char *btype, int mark, int error_on_dinode,
-			     const char *caller, int line);
 extern int check_n_fix_bitmap(struct gfs2_sbd *sdp, struct rgrp_tree *rgd,
 			      uint64_t blk, int error_on_dinode,
 			      int new_state);
@@ -38,9 +35,6 @@ static inline void _fsck_bitmap_set_dbg(struct gfs2_inode *ip, uint64_t bblock,
 	static int prevcount = 0;
 	static const char *prev_caller = NULL;
 
-	if (print_level < MSG_DEBUG)
-		return;
-
 	if ((ip->i_di.di_num.no_addr == prev_ino_addr) &&
 	    (mark == prev_mark) && caller == prev_caller) {
 		log_info("(0x%llx) ", (unsigned long long)bblock);
@@ -78,8 +72,20 @@ static inline void _fsck_bitmap_set_dbg(struct gfs2_inode *ip, uint64_t bblock,
 
 #define is_duplicate(dblock) ((dupfind(dblock)) ? 1 : 0)
 
-#define fsck_bitmap_set(ip, b, bt, m) \
-	_fsck_bitmap_set(ip, b, bt, m, 0, __FUNCTION__, __LINE__)
+#define fsck_bitmap_set(ip, bblock, btype, mark)			\
+	do {								\
+		int ret;						\
+									\
+		if (print_level >= MSG_DEBUG)				\
+			_fsck_bitmap_set_dbg(ip, bblock, btype, mark,	\
+					     __FUNCTION__, __LINE__);	\
+		ret = check_n_fix_bitmap(ip->i_sbd, ip->i_rgd, bblock, \
+					 0, mark);			\
+		if (ret < 0)						\
+			log_err(_("This block is not represented in "	\
+				  "the bitmap.\n"));			\
+	} while (0)
+
 enum meta_check_rc {
 	meta_error = -1,
 	meta_is_good = 0,
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 2a3fbde..2cb5a44 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -117,7 +117,8 @@ static int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t bblock,
 {
 	int error;
 
-	_fsck_bitmap_set_dbg(ip, bblock, btype, mark, caller, fline);
+	if (print_level >= MSG_DEBUG)
+		_fsck_bitmap_set_dbg(ip, bblock, btype, mark, caller, fline);
 	error = check_n_fix_bitmap(ip->i_sbd, ip->i_rgd, bblock,
 				   error_on_dinode, mark);
 	if (error < 0)
@@ -130,8 +131,6 @@ static int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t bblock,
 
 #define fsck_blockmap_set(ip, b, bt, m) \
 	_fsck_blockmap_set(ip, b, bt, m, 0, __FUNCTION__, __LINE__)
-#define fsck_blkmap_set_noino(ip, b, bt, m) \
-	_fsck_blockmap_set(ip, b, bt, m, 1, __FUNCTION__, __LINE__)
 
 /**
  * delete_block - delete a block associated with an inode
@@ -516,7 +515,15 @@ static int blockmap_set_as_data(struct gfs2_inode *ip, uint64_t block)
 	struct gfs2_buffer_head *bh;
 	struct gfs2_dinode *di;
 
-	error = fsck_blkmap_set_noino(ip, block, _("data"), GFS2_BLKST_USED);
+	if (print_level >= MSG_DEBUG)
+		_fsck_bitmap_set_dbg(ip, block, _("data"), GFS2_BLKST_USED,
+				     __FUNCTION__, __LINE__);
+	error = check_n_fix_bitmap(ip->i_sbd, ip->i_rgd, block, 1,
+				   GFS2_BLKST_USED);
+	if (error < 0)
+		log_err(_("This block is not represented in the bitmap.\n"));
+	if (!error)
+		error = gfs2_blockmap_set(block, GFS2_BLKST_USED);
 	if (!error)
 		return 0;
 
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 6/7] fsck.gfs2: Speed up function bitmap_type

[Bob Peterson]

This patch speeds up fsck.gfs2 by allowing function bitmap_type to
remember the last rgrp it used for the next lookup.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/util.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gfs2/fsck/util.h b/gfs2/fsck/util.h
index d93b65d..0649fec 100644
--- a/gfs2/fsck/util.h
+++ b/gfs2/fsck/util.h
@@ -70,8 +70,14 @@ static inline void link1_destroy(struct gfs2_bmap *bmap)
 static inline int bitmap_type(struct gfs2_sbd *sdp, uint64_t bblock)
 {
 	struct rgrp_tree *rgd;
+	static struct rgrp_tree *prevrgd = NULL;
 
-	rgd = gfs2_blk2rgrpd(sdp, bblock);
+	if (prevrgd && rgrp_contains_block(prevrgd, bblock))
+		rgd = prevrgd;
+	else {
+		rgd = gfs2_blk2rgrpd(sdp, bblock);
+		prevrgd = rgd;
+	}
 	return lgfs2_get_bitmap(sdp, bblock, rgd);
 }
 
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 7/7] fsck.gfs2: Make pass2 go by directory rbtree for performance

[Bob Peterson]

This patch speeds up pass2 of fsck.gfs2 for large file systems.
Before, pass2 was checking the bitmap for every block in the file
system. If the file system is 15TB, that means looping 15 trillion
times. With this patch, it merely traverses the inode tree, since
it needs that information anyway, and already ignores any blocks
that don't have it. Traversing an empty 15TB file system goes from
13.5 minutes to 0.00 seconds for pass2.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 gfs2/fsck/pass2.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index f808cea..bc6828a 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -2088,8 +2088,11 @@ static int pass2_check_dir(struct gfs2_sbd *sdp, struct gfs2_inode *ip)
  */
 int pass2(struct gfs2_sbd *sdp)
 {
+	struct osi_node *tmp, *next = NULL;
+	struct gfs2_inode *ip;
+	struct dir_info *dt;
 	uint64_t dirblk;
-	int q;
+	int error;
 
 	/* Check all the system directory inodes. */
 	if (!sdp->gfs1 &&
@@ -2121,11 +2124,11 @@ int pass2(struct gfs2_sbd *sdp)
 		return FSCK_OK;
 	log_info( _("Checking directory inodes.\n"));
 	/* Grab each directory inode, and run checks on it */
-	for (dirblk = 0; dirblk < last_fs_block; dirblk++) {
-		struct gfs2_inode *ip;
-		struct dir_info *dt;
-		int error;
+	for (tmp = osi_first(&dirtree); tmp; tmp = next) {
+		next = osi_next(tmp);
 
+		dt = (struct dir_info *)tmp;
+		dirblk = dt->dinode.no_addr;
 		warm_fuzzy_stuff(dirblk);
 		if (skip_this_pass || fsck_abort) /* if asked to skip the rest */
 			return FSCK_OK;
@@ -2134,14 +2137,6 @@ int pass2(struct gfs2_sbd *sdp)
 		if (is_system_dir(sdp, dirblk))
 			continue;
 
-		q = bitmap_type(sdp, dirblk);
-		if (q != GFS2_BLKST_DINODE)
-			continue;
-
-		dt = dirtree_find(dirblk);
-		if (dt == NULL)
-			continue;
-
 		/* If we created lost+found, its links should have been
 		   properly adjusted, so don't check it. */
 		if (lf_was_created &&
-- 
2.5.5

[Cluster-devel] [gfs2-utils PATCH 0/7] fsck.gfs2 performance improvements

[Steven Whitehouse]

Hi,

I think it would be good to separate out the performance patches from 
the others. I think there are probably only a few patches that will make 
any performance difference. Patch 7 appears to be the most important 
one, and looks like a really good speed up. That is definitely the kind 
of thing we need to find and fix. Using global variables to avoid 
passing an extra parameter is not a good idea - if that does make a 
difference then I'd look very carefully at the loop in which that code 
resides and see what the real problem is there.

Likewise, I'm not at all keen on having large functions in headers 
marked as inline functions or macros which appear to be mostly code that 
is only touched on a debug or error path. This should not be performance 
critical code and most of it at least should ok in a normal function. 
Again if this makes a performance difference, then it is probably a sign 
that something else is wrong,

Steve.

On 22/06/16 20:26, Bob Peterson wrote:
> My recent set of patches to fsck.gfs2 saved a lot of memory, thus
> enabling us to run fsck.gfs2 on much larger file systems. However,
> it slowed things down and our performance regressed. This is a set
> of seven patches designed to improve performance again. There are
> probably more improvements I can make, but I've been busy with kernel
> work, so it's not my primary focus. If I come up with more patches,
> I'll post them later.
> ---
> Bob Peterson (7):
>    fsck.gfs2: Don't bother to pass bl blockmap pointer
>    fsck.gfs2: Remember the previous rgrp pointer for speed
>    fsck.gfs2: Don't set gfs1rg pointer unless we need to
>    fsck.gfs2: Make _fsck_bitmap_set not send a return code
>    fsck.gfs2: convert fsck_bitmap_set to a macro
>    fsck.gfs2: Speed up function bitmap_type
>    fsck.gfs2: Make pass2 go by directory rbtree for performance
>
>   gfs2/fsck/metawalk.c | 69 ++++++----------------------------------------------
>   gfs2/fsck/metawalk.h | 65 +++++++++++++++++++++++++++++++++++++++++++------
>   gfs2/fsck/pass1.c    | 47 +++++++++++++++++++++--------------
>   gfs2/fsck/pass2.c    | 21 ++++++----------
>   gfs2/fsck/util.h     |  8 +++++-
>   5 files changed, 110 insertions(+), 100 deletions(-)
>