[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Bob Peterson]

Hi,

I amended my previous fsck.gfs2 performance patch to eliminate the
whitespace problem Andy pointed out, then I moved inline function
rgrp_contains_block to fsck.h in order to facilitate the following
additional patch.

So here's another fsck.gfs2 performance patch:

Before this patch, many functions in fsck.gfs2 checked for blocks
being valid. To check if a block is valid, you really need to check
that it's inside the area covered by a valid resource group. For
example, if a data block is pointing to a rgrp bitmap block, that's
invalid. To achieve this, it does a rgrp search, which traverses
the rgrp tree. This patch optimizes that by allowing functions to
pass in the ip pointer, so like before, we can check if that rgrp
covers the block in question and save ourselves a lot of work
traversing the tree.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
index 0632695..e39bfea 100644
--- a/gfs2/fsck/afterpass1_common.c
+++ b/gfs2/fsck/afterpass1_common.c
@@ -81,7 +81,7 @@ static int delete_block_if_notdup(struct gfs2_inode *ip, uint64_t block,
 	int q;
 	int removed_lastmeta;
 
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return meta_error;
 
 	q = bitmap_type(ip->i_sbd, block);
@@ -207,7 +207,7 @@ static int del_eattr_generic(struct gfs2_inode *ip, uint64_t block,
 	int was_free = 0;
 	int q;
 
-	if (valid_block(ip->i_sbd, block)) {
+	if (valid_block_ip(ip, block)) {
 		q = bitmap_type(ip->i_sbd, block);
 		if (q == GFS2_BLKST_FREE)
 			was_free = 1;
diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
index c4e6e3b..3e7f99f 100644
--- a/gfs2/fsck/fsck.h
+++ b/gfs2/fsck/fsck.h
@@ -173,4 +173,19 @@ static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t blk)
 	return 1;
 }
 
+static inline int valid_block_ip(struct gfs2_inode *ip, uint64_t blk)
+{
+	struct gfs2_sbd *sdp = ip->i_sbd;
+	struct rgrp_tree *rgd = ip->i_rgd;
+
+	if (blk > sdp->fssize)
+		return 0;
+	if (blk <= sdp->sb_addr)
+		return 0;
+	if (rgd == NULL || !rgrp_contains_block(rgd, blk))
+		rgd = gfs2_blk2rgrpd(sdp, blk);
+
+	return rgrp_contains_block(rgd, blk);
+}
+
 #endif /* _FSCK_H */
diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 7261422..c0cc2ab 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -539,7 +539,7 @@ int check_leaf(struct gfs2_inode *ip, int lindex, struct metawalk_fxns *pass,
 	int di_depth = ip->i_di.di_depth;
 
 	/* Make sure the block number is in range. */
-	if (!valid_block(ip->i_sbd, *leaf_no)) {
+	if (!valid_block_ip(ip, *leaf_no)) {
 		log_err( _("Leaf block #%llu (0x%llx) is out of range for "
 			   "directory #%llu (0x%llx) at index %d (0x%x).\n"),
 			 (unsigned long long)*leaf_no,
@@ -698,7 +698,7 @@ static void dir_leaf_reada(struct gfs2_inode *ip, uint64_t *tbl, unsigned hsize)
 
 	for (i = 0; i < hsize; i++) {
 		leaf_no = be64_to_cpu(tbl[i]);
-		if (valid_block(ip->i_sbd, leaf_no))
+		if (valid_block_ip(ip, leaf_no))
 			t[n++] = leaf_no * sdp->bsize;
 	}
 	qsort(t, n, sizeof(uint64_t), u64cmp);
@@ -760,7 +760,7 @@ int check_leaf_blks(struct gfs2_inode *ip, struct metawalk_fxns *pass)
 	first_ok_leaf = leaf_no = -1;
 	for (lindex = 0; lindex < hsize; lindex++) {
 		leaf_no = be64_to_cpu(tbl[lindex]);
-		if (valid_block(ip->i_sbd, leaf_no)) {
+		if (valid_block_ip(ip, leaf_no)) {
 			lbh = bread(sdp, leaf_no);
 			/* Make sure it's really a valid leaf block. */
 			if (gfs2_check_meta(lbh, GFS2_METATYPE_LF) == 0) {
@@ -1334,7 +1334,7 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
 						   (unsigned long long)block);
 					continue;
 				}
-				if (!valid_block(ip->i_sbd, block)) {
+				if (!valid_block_ip(ip, block)) {
 					log_debug( _("Skipping invalid block "
 						     "%lld (0x%llx)\n"),
 						   (unsigned long long)block,
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 3a1931d..6f04109 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -135,7 +135,7 @@ static int delete_block(struct gfs2_inode *ip, uint64_t block,
 			struct gfs2_buffer_head **bh, const char *btype,
 			void *private)
 {
-	if (valid_block(ip->i_sbd, block)) {
+	if (valid_block_ip(ip, block)) {
 		fsck_blockmap_set(ip, block, btype, GFS2_BLKST_FREE);
 		return 0;
 	}
@@ -233,7 +233,7 @@ static int resuscitate_metalist(struct gfs2_inode *ip, uint64_t block,
 	*is_valid = 1;
 	*was_duplicate = 0;
 	*bh = NULL;
-	if (!valid_block(ip->i_sbd, block)){ /* blk outside of FS */
+	if (!valid_block_ip(ip, block)){ /* blk outside of FS */
 		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
 				  _("itself"), GFS2_BLKST_UNLINKED);
 		log_err( _("Bad indirect block pointer (invalid or out of "
@@ -281,7 +281,7 @@ static int resuscitate_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent,
 		strncpy(tmp_name, filename, de->de_name_len);
 	else
 		strncpy(tmp_name, filename, sizeof(tmp_name) - 1);
-	if (!valid_block(sdp, block)) {
+	if (!valid_block_ip(ip, block)) {
 		log_err( _("Block # referenced by system directory entry %s "
 			   "in inode %lld (0x%llx) is invalid or out of range;"
 			   " ignored.\n"),
@@ -358,7 +358,7 @@ static int check_metalist(struct gfs2_inode *ip, uint64_t block,
 
 	*was_duplicate = 0;
 	*is_valid = 0;
-	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
+	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
 		/* The bad dinode should be invalidated later due to
 		   "unrecoverable" errors.  The inode itself should be
 		   set "free" and removed from the inodetree by
@@ -441,7 +441,7 @@ static int undo_reference(struct gfs2_inode *ip, uint64_t block, int meta,
 	int old_bitmap_state = 0;
 	struct rgrp_tree *rgd;
 
-	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
+	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
 		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
 				  _("bad block referencing"), GFS2_BLKST_FREE);
 		return 1;
@@ -549,7 +549,7 @@ static int check_data(struct gfs2_inode *ip, uint64_t metablock,
 	int q;
 	struct block_count *bc = (struct block_count *) private;
 
-	if (!valid_block(ip->i_sbd, block)) {
+	if (!valid_block_ip(ip, block)) {
 		log_err( _("inode %lld (0x%llx) has a bad data block pointer "
 			   "%lld (0x%llx) (invalid or out of range) "),
 			 (unsigned long long)ip->i_di.di_num.no_addr,
@@ -686,7 +686,7 @@ static int undo_eattr_indir_or_leaf(struct gfs2_inode *ip, uint64_t block,
 	int error;
 	struct block_count *bc = (struct block_count *) private;
 
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return meta_error;
 
 	/* Need to check block_type before undoing the reference, which can
@@ -735,7 +735,7 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
 
 	/* This inode contains an eattr - it may be invalid, but the
 	 * eattr attributes points to a non-zero block */
-	if (!valid_block(sdp, indirect)) {
+	if (!valid_block_ip(ip, indirect)) {
 		/* Doesn't help to mark this here - this gets checked
 		 * in pass1c */
 		return 1;
@@ -893,7 +893,7 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, int i,
 	struct gfs2_buffer_head *bh = NULL;
 	int error = 0;
 
-	if (!valid_block(sdp, el_blk)) {
+	if (!valid_block_ip(ip, el_blk)) {
 		log_err( _("Inode #%llu (0x%llx): Extended Attribute block "
 			   "%llu (0x%llx) has an extended leaf block #%llu "
 			   "(0x%llx) that is invalid or out of range.\n"),
@@ -943,9 +943,7 @@ static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
 			    uint64_t parent, struct gfs2_buffer_head **bh,
 			    void *private)
 {
-	struct gfs2_sbd *sdp = ip->i_sbd;
-
-	if (!valid_block(sdp, block)) {
+	if (!valid_block_ip(ip, block)) {
 		log_warn( _("Inode #%llu (0x%llx): Extended Attribute leaf "
 			    "block #%llu (0x%llx) is invalid or out of "
 			    "range.\n"),
@@ -1089,7 +1087,7 @@ static int mark_block_invalid(struct gfs2_inode *ip, uint64_t block,
 		*is_valid = 1;
 	if (was_duplicate)
 		*was_duplicate = 0;
-	if (!valid_block(ip->i_sbd, block)) {
+	if (!valid_block_ip(ip, block)) {
 		if (is_valid)
 			*is_valid = 0;
 		return meta_is_good;
@@ -1181,7 +1179,7 @@ static int rangecheck_block(struct gfs2_inode *ip, uint64_t block,
 	long *bad_pointers = (long *)private;
 	int q;
 
-	if (!valid_block(ip->i_sbd, block)) {
+	if (!valid_block_ip(ip, block)) {
 		(*bad_pointers)++;
 		log_info( _("Bad %s block pointer (invalid or out of range "
 			    "#%ld) found in inode %lld (0x%llx).\n"),
diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index d072634..f808cea 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -461,7 +461,7 @@ static int basic_dentry_checks(struct gfs2_inode *ip, struct gfs2_dirent *dent,
 	struct gfs2_inum inum = { 0 };
 
 	*isdir = 0;
-	if (!valid_block(ip->i_sbd, entry->no_addr)) {
+	if (!valid_block_ip(ip, entry->no_addr)) {
 		log_err( _("Block # referenced by directory entry %s in inode "
 			   "%lld (0x%llx) is invalid\n"),
 			 tmp_name, (unsigned long long)ip->i_di.di_num.no_addr,
@@ -1481,7 +1481,7 @@ static int check_hash_tbl_dups(struct gfs2_inode *ip, uint64_t *tbl,
 		   or the duplicate we found. */
 		memset(&leaf, 0, sizeof(leaf));
 		leaf_no = leafblk;
-		if (!valid_block(ip->i_sbd, leaf_no)) /* Checked later */
+		if (!valid_block_ip(ip, leaf_no)) /* Checked later */
 			continue;
 
 		lbh = bread(ip->i_sbd, leafblk);
@@ -1614,7 +1614,7 @@ static int check_hash_tbl(struct gfs2_inode *ip, uint64_t *tbl,
 		/* See if that leaf block is valid. If not, write a new one
 		   that falls on a proper boundary. If it doesn't naturally,
 		   we may need more. */
-		if (!valid_block(ip->i_sbd, leafblk)) {
+		if (!valid_block_ip(ip, leafblk)) {
 			uint64_t new_leafblk;
 
 			log_err(_("Dinode %llu (0x%llx) has bad leaf pointers "
@@ -1776,7 +1776,7 @@ static int check_data_qc(struct gfs2_inode *ip, uint64_t metablock,
 
 	/* At this point, basic data block checks have already been done,
 	   so we only need to make sure they're QC blocks. */
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return -1;
 
 	bh = bread(ip->i_sbd, block);
diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
index 8a8220b..1c3ed9c 100644
--- a/gfs2/fsck/util.c
+++ b/gfs2/fsck/util.c
@@ -335,7 +335,7 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
 	struct inode_with_dups *id;
 	struct duptree *dt;
 
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return meta_is_good;
 	/* If this is not the first reference (i.e. all calls from pass1) we
 	   need to create the duplicate reference. If this is pass1b, we want

[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Andrew Price]

On 02/06/16 15:00, Bob Peterson wrote:
> Hi,
>
> I amended my previous fsck.gfs2 performance patch to eliminate the
> whitespace problem Andy pointed out, then I moved inline function
> rgrp_contains_block to fsck.h in order to facilitate the following
> additional patch.
>
> So here's another fsck.gfs2 performance patch:
>
> Before this patch, many functions in fsck.gfs2 checked for blocks
> being valid. To check if a block is valid, you really need to check
> that it's inside the area covered by a valid resource group. For
> example, if a data block is pointing to a rgrp bitmap block, that's
> invalid. To achieve this, it does a rgrp search, which traverses
> the rgrp tree. This patch optimizes that by allowing functions to
> pass in the ip pointer, so like before, we can check if that rgrp
> covers the block in question and save ourselves a lot of work
> traversing the tree.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
> index 0632695..e39bfea 100644
> --- a/gfs2/fsck/afterpass1_common.c
> +++ b/gfs2/fsck/afterpass1_common.c
> @@ -81,7 +81,7 @@ static int delete_block_if_notdup(struct gfs2_inode *ip, uint64_t block,
>  	int q;
>  	int removed_lastmeta;
>
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return meta_error;
>
>  	q = bitmap_type(ip->i_sbd, block);
> @@ -207,7 +207,7 @@ static int del_eattr_generic(struct gfs2_inode *ip, uint64_t block,
>  	int was_free = 0;
>  	int q;
>
> -	if (valid_block(ip->i_sbd, block)) {
> +	if (valid_block_ip(ip, block)) {
>  		q = bitmap_type(ip->i_sbd, block);
>  		if (q == GFS2_BLKST_FREE)
>  			was_free = 1;
> diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
> index c4e6e3b..3e7f99f 100644
> --- a/gfs2/fsck/fsck.h
> +++ b/gfs2/fsck/fsck.h
> @@ -173,4 +173,19 @@ static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t blk)
>  	return 1;
>  }
>
> +static inline int valid_block_ip(struct gfs2_inode *ip, uint64_t blk)
> +{
> +	struct gfs2_sbd *sdp = ip->i_sbd;
> +	struct rgrp_tree *rgd = ip->i_rgd;
> +
> +	if (blk > sdp->fssize)
> +		return 0;
> +	if (blk <= sdp->sb_addr)
> +		return 0;
> +	if (rgd == NULL || !rgrp_contains_block(rgd, blk))
> +		rgd = gfs2_blk2rgrpd(sdp, blk);

Do we need another

	if (rgd == NULL)
		return 0;

here as gfs2_blk2rgrpd() can return NULL, e.g. if blk lies in the space 
between rgrps?

> +
> +	return rgrp_contains_block(rgd, blk);

I'm not sure this second call is needed once we know rgd is not NULL, 
since it either passed the first check or blk was matched to the rgrp in 
gfs2_blk2rgrpd().

Cheers,
Andy

> +}
> +
>  #endif /* _FSCK_H */
> diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
> index 7261422..c0cc2ab 100644
> --- a/gfs2/fsck/metawalk.c
> +++ b/gfs2/fsck/metawalk.c
> @@ -539,7 +539,7 @@ int check_leaf(struct gfs2_inode *ip, int lindex, struct metawalk_fxns *pass,
>  	int di_depth = ip->i_di.di_depth;
>
>  	/* Make sure the block number is in range. */
> -	if (!valid_block(ip->i_sbd, *leaf_no)) {
> +	if (!valid_block_ip(ip, *leaf_no)) {
>  		log_err( _("Leaf block #%llu (0x%llx) is out of range for "
>  			   "directory #%llu (0x%llx) at index %d (0x%x).\n"),
>  			 (unsigned long long)*leaf_no,
> @@ -698,7 +698,7 @@ static void dir_leaf_reada(struct gfs2_inode *ip, uint64_t *tbl, unsigned hsize)
>
>  	for (i = 0; i < hsize; i++) {
>  		leaf_no = be64_to_cpu(tbl[i]);
> -		if (valid_block(ip->i_sbd, leaf_no))
> +		if (valid_block_ip(ip, leaf_no))
>  			t[n++] = leaf_no * sdp->bsize;
>  	}
>  	qsort(t, n, sizeof(uint64_t), u64cmp);
> @@ -760,7 +760,7 @@ int check_leaf_blks(struct gfs2_inode *ip, struct metawalk_fxns *pass)
>  	first_ok_leaf = leaf_no = -1;
>  	for (lindex = 0; lindex < hsize; lindex++) {
>  		leaf_no = be64_to_cpu(tbl[lindex]);
> -		if (valid_block(ip->i_sbd, leaf_no)) {
> +		if (valid_block_ip(ip, leaf_no)) {
>  			lbh = bread(sdp, leaf_no);
>  			/* Make sure it's really a valid leaf block. */
>  			if (gfs2_check_meta(lbh, GFS2_METATYPE_LF) == 0) {
> @@ -1334,7 +1334,7 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
>  						   (unsigned long long)block);
>  					continue;
>  				}
> -				if (!valid_block(ip->i_sbd, block)) {
> +				if (!valid_block_ip(ip, block)) {
>  					log_debug( _("Skipping invalid block "
>  						     "%lld (0x%llx)\n"),
>  						   (unsigned long long)block,
> diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
> index 3a1931d..6f04109 100644
> --- a/gfs2/fsck/pass1.c
> +++ b/gfs2/fsck/pass1.c
> @@ -135,7 +135,7 @@ static int delete_block(struct gfs2_inode *ip, uint64_t block,
>  			struct gfs2_buffer_head **bh, const char *btype,
>  			void *private)
>  {
> -	if (valid_block(ip->i_sbd, block)) {
> +	if (valid_block_ip(ip, block)) {
>  		fsck_blockmap_set(ip, block, btype, GFS2_BLKST_FREE);
>  		return 0;
>  	}
> @@ -233,7 +233,7 @@ static int resuscitate_metalist(struct gfs2_inode *ip, uint64_t block,
>  	*is_valid = 1;
>  	*was_duplicate = 0;
>  	*bh = NULL;
> -	if (!valid_block(ip->i_sbd, block)){ /* blk outside of FS */
> +	if (!valid_block_ip(ip, block)){ /* blk outside of FS */
>  		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
>  				  _("itself"), GFS2_BLKST_UNLINKED);
>  		log_err( _("Bad indirect block pointer (invalid or out of "
> @@ -281,7 +281,7 @@ static int resuscitate_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent,
>  		strncpy(tmp_name, filename, de->de_name_len);
>  	else
>  		strncpy(tmp_name, filename, sizeof(tmp_name) - 1);
> -	if (!valid_block(sdp, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		log_err( _("Block # referenced by system directory entry %s "
>  			   "in inode %lld (0x%llx) is invalid or out of range;"
>  			   " ignored.\n"),
> @@ -358,7 +358,7 @@ static int check_metalist(struct gfs2_inode *ip, uint64_t block,
>
>  	*was_duplicate = 0;
>  	*is_valid = 0;
> -	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
> +	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
>  		/* The bad dinode should be invalidated later due to
>  		   "unrecoverable" errors.  The inode itself should be
>  		   set "free" and removed from the inodetree by
> @@ -441,7 +441,7 @@ static int undo_reference(struct gfs2_inode *ip, uint64_t block, int meta,
>  	int old_bitmap_state = 0;
>  	struct rgrp_tree *rgd;
>
> -	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
> +	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
>  		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
>  				  _("bad block referencing"), GFS2_BLKST_FREE);
>  		return 1;
> @@ -549,7 +549,7 @@ static int check_data(struct gfs2_inode *ip, uint64_t metablock,
>  	int q;
>  	struct block_count *bc = (struct block_count *) private;
>
> -	if (!valid_block(ip->i_sbd, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		log_err( _("inode %lld (0x%llx) has a bad data block pointer "
>  			   "%lld (0x%llx) (invalid or out of range) "),
>  			 (unsigned long long)ip->i_di.di_num.no_addr,
> @@ -686,7 +686,7 @@ static int undo_eattr_indir_or_leaf(struct gfs2_inode *ip, uint64_t block,
>  	int error;
>  	struct block_count *bc = (struct block_count *) private;
>
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return meta_error;
>
>  	/* Need to check block_type before undoing the reference, which can
> @@ -735,7 +735,7 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
>
>  	/* This inode contains an eattr - it may be invalid, but the
>  	 * eattr attributes points to a non-zero block */
> -	if (!valid_block(sdp, indirect)) {
> +	if (!valid_block_ip(ip, indirect)) {
>  		/* Doesn't help to mark this here - this gets checked
>  		 * in pass1c */
>  		return 1;
> @@ -893,7 +893,7 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, int i,
>  	struct gfs2_buffer_head *bh = NULL;
>  	int error = 0;
>
> -	if (!valid_block(sdp, el_blk)) {
> +	if (!valid_block_ip(ip, el_blk)) {
>  		log_err( _("Inode #%llu (0x%llx): Extended Attribute block "
>  			   "%llu (0x%llx) has an extended leaf block #%llu "
>  			   "(0x%llx) that is invalid or out of range.\n"),
> @@ -943,9 +943,7 @@ static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
>  			    uint64_t parent, struct gfs2_buffer_head **bh,
>  			    void *private)
>  {
> -	struct gfs2_sbd *sdp = ip->i_sbd;
> -
> -	if (!valid_block(sdp, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		log_warn( _("Inode #%llu (0x%llx): Extended Attribute leaf "
>  			    "block #%llu (0x%llx) is invalid or out of "
>  			    "range.\n"),
> @@ -1089,7 +1087,7 @@ static int mark_block_invalid(struct gfs2_inode *ip, uint64_t block,
>  		*is_valid = 1;
>  	if (was_duplicate)
>  		*was_duplicate = 0;
> -	if (!valid_block(ip->i_sbd, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		if (is_valid)
>  			*is_valid = 0;
>  		return meta_is_good;
> @@ -1181,7 +1179,7 @@ static int rangecheck_block(struct gfs2_inode *ip, uint64_t block,
>  	long *bad_pointers = (long *)private;
>  	int q;
>
> -	if (!valid_block(ip->i_sbd, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		(*bad_pointers)++;
>  		log_info( _("Bad %s block pointer (invalid or out of range "
>  			    "#%ld) found in inode %lld (0x%llx).\n"),
> diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
> index d072634..f808cea 100644
> --- a/gfs2/fsck/pass2.c
> +++ b/gfs2/fsck/pass2.c
> @@ -461,7 +461,7 @@ static int basic_dentry_checks(struct gfs2_inode *ip, struct gfs2_dirent *dent,
>  	struct gfs2_inum inum = { 0 };
>
>  	*isdir = 0;
> -	if (!valid_block(ip->i_sbd, entry->no_addr)) {
> +	if (!valid_block_ip(ip, entry->no_addr)) {
>  		log_err( _("Block # referenced by directory entry %s in inode "
>  			   "%lld (0x%llx) is invalid\n"),
>  			 tmp_name, (unsigned long long)ip->i_di.di_num.no_addr,
> @@ -1481,7 +1481,7 @@ static int check_hash_tbl_dups(struct gfs2_inode *ip, uint64_t *tbl,
>  		   or the duplicate we found. */
>  		memset(&leaf, 0, sizeof(leaf));
>  		leaf_no = leafblk;
> -		if (!valid_block(ip->i_sbd, leaf_no)) /* Checked later */
> +		if (!valid_block_ip(ip, leaf_no)) /* Checked later */
>  			continue;
>
>  		lbh = bread(ip->i_sbd, leafblk);
> @@ -1614,7 +1614,7 @@ static int check_hash_tbl(struct gfs2_inode *ip, uint64_t *tbl,
>  		/* See if that leaf block is valid. If not, write a new one
>  		   that falls on a proper boundary. If it doesn't naturally,
>  		   we may need more. */
> -		if (!valid_block(ip->i_sbd, leafblk)) {
> +		if (!valid_block_ip(ip, leafblk)) {
>  			uint64_t new_leafblk;
>
>  			log_err(_("Dinode %llu (0x%llx) has bad leaf pointers "
> @@ -1776,7 +1776,7 @@ static int check_data_qc(struct gfs2_inode *ip, uint64_t metablock,
>
>  	/* At this point, basic data block checks have already been done,
>  	   so we only need to make sure they're QC blocks. */
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return -1;
>
>  	bh = bread(ip->i_sbd, block);
> diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
> index 8a8220b..1c3ed9c 100644
> --- a/gfs2/fsck/util.c
> +++ b/gfs2/fsck/util.c
> @@ -335,7 +335,7 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
>  	struct inode_with_dups *id;
>  	struct duptree *dt;
>
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return meta_is_good;
>  	/* If this is not the first reference (i.e. all calls from pass1) we
>  	   need to create the duplicate reference. If this is pass1b, we want
>

[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Bob Peterson]

----- Original Message -----
| On 02/06/16 15:00, Bob Peterson wrote:
| > Hi,
| >
| > I amended my previous fsck.gfs2 performance patch to eliminate the
| > whitespace problem Andy pointed out, then I moved inline function
| > rgrp_contains_block to fsck.h in order to facilitate the following
| > additional patch.
| >
| > So here's another fsck.gfs2 performance patch:
| >
| > Before this patch, many functions in fsck.gfs2 checked for blocks
| > being valid. To check if a block is valid, you really need to check
| > that it's inside the area covered by a valid resource group. For
| > example, if a data block is pointing to a rgrp bitmap block, that's
| > invalid. To achieve this, it does a rgrp search, which traverses
| > the rgrp tree. This patch optimizes that by allowing functions to
| > pass in the ip pointer, so like before, we can check if that rgrp
| > covers the block in question and save ourselves a lot of work
| > traversing the tree.
| >
| > Signed-off-by: Bob Peterson <rpeterso@redhat.com>
| > ---
| > diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
| > index 0632695..e39bfea 100644
| > --- a/gfs2/fsck/afterpass1_common.c
| > +++ b/gfs2/fsck/afterpass1_common.c
| > @@ -81,7 +81,7 @@ static int delete_block_if_notdup(struct gfs2_inode *ip,
| > uint64_t block,
| >  	int q;
| >  	int removed_lastmeta;
| >
| > -	if (!valid_block(ip->i_sbd, block))
| > +	if (!valid_block_ip(ip, block))
| >  		return meta_error;
| >
| >  	q = bitmap_type(ip->i_sbd, block);
| > @@ -207,7 +207,7 @@ static int del_eattr_generic(struct gfs2_inode *ip,
| > uint64_t block,
| >  	int was_free = 0;
| >  	int q;
| >
| > -	if (valid_block(ip->i_sbd, block)) {
| > +	if (valid_block_ip(ip, block)) {
| >  		q = bitmap_type(ip->i_sbd, block);
| >  		if (q == GFS2_BLKST_FREE)
| >  			was_free = 1;
| > diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
| > index c4e6e3b..3e7f99f 100644
| > --- a/gfs2/fsck/fsck.h
| > +++ b/gfs2/fsck/fsck.h
| > @@ -173,4 +173,19 @@ static inline int rgrp_contains_block(struct rgrp_tree
| > *rgd, uint64_t blk)
| >  	return 1;
| >  }
| >
| > +static inline int valid_block_ip(struct gfs2_inode *ip, uint64_t blk)
| > +{
| > +	struct gfs2_sbd *sdp = ip->i_sbd;

| > +	struct rgrp_tree *rgd = ip->i_rgd;
| > +
| > +	if (blk > sdp->fssize)
| > +		return 0;
| > +	if (blk <= sdp->sb_addr)
| > +		return 0;
| > +	if (rgd == NULL || !rgrp_contains_block(rgd, blk))
| > +		rgd = gfs2_blk2rgrpd(sdp, blk);
| 
| Do we need another
| 
| 	if (rgd == NULL)
| 		return 0;
| 
| here as gfs2_blk2rgrpd() can return NULL, e.g. if blk lies in the space
| between rgrps?
| 
| > +
| > +	return rgrp_contains_block(rgd, blk);
| 
| I'm not sure this second call is needed once we know rgd is not NULL,
| since it either passed the first check or blk was matched to the rgrp in
| gfs2_blk2rgrpd().
| 
| Cheers,
| Andy

Hi Andy,

This coding was deliberate and intentional.

The idea here is: The ip->i_rgd is only a "hint" because in many cases,
the rgrp for an inode's block will often be the same as the inode's rgrp
itself. It's used for checking all the inode's blocks, not just the inode
itself, but sometimes they fall outside the inode's rgrp. So the concept is:

1. If rgrp is passed in NULL, the rgrp for the block needs to be found.
2. If the inode's block is NOT within the inode's rgrp, we need to find the
   proper rgrp for the block.
3. Once found, we need to make sure the block falls within it.

It's subtle, but it's important. I think those checks all need to be there.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Andrew Price]

On 02/06/16 15:44, Bob Peterson wrote:
> ----- Original Message -----
> | On 02/06/16 15:00, Bob Peterson wrote:
> | > Hi,
> | >
> | > I amended my previous fsck.gfs2 performance patch to eliminate the
> | > whitespace problem Andy pointed out, then I moved inline function
> | > rgrp_contains_block to fsck.h in order to facilitate the following
> | > additional patch.
> | >
> | > So here's another fsck.gfs2 performance patch:
> | >
> | > Before this patch, many functions in fsck.gfs2 checked for blocks
> | > being valid. To check if a block is valid, you really need to check
> | > that it's inside the area covered by a valid resource group. For
> | > example, if a data block is pointing to a rgrp bitmap block, that's
> | > invalid. To achieve this, it does a rgrp search, which traverses
> | > the rgrp tree. This patch optimizes that by allowing functions to
> | > pass in the ip pointer, so like before, we can check if that rgrp
> | > covers the block in question and save ourselves a lot of work
> | > traversing the tree.
> | >
> | > Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> | > ---
> | > diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
> | > index 0632695..e39bfea 100644
> | > --- a/gfs2/fsck/afterpass1_common.c
> | > +++ b/gfs2/fsck/afterpass1_common.c
> | > @@ -81,7 +81,7 @@ static int delete_block_if_notdup(struct gfs2_inode *ip,
> | > uint64_t block,
> | >  	int q;
> | >  	int removed_lastmeta;
> | >
> | > -	if (!valid_block(ip->i_sbd, block))
> | > +	if (!valid_block_ip(ip, block))
> | >  		return meta_error;
> | >
> | >  	q = bitmap_type(ip->i_sbd, block);
> | > @@ -207,7 +207,7 @@ static int del_eattr_generic(struct gfs2_inode *ip,
> | > uint64_t block,
> | >  	int was_free = 0;
> | >  	int q;
> | >
> | > -	if (valid_block(ip->i_sbd, block)) {
> | > +	if (valid_block_ip(ip, block)) {
> | >  		q = bitmap_type(ip->i_sbd, block);
> | >  		if (q == GFS2_BLKST_FREE)
> | >  			was_free = 1;
> | > diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
> | > index c4e6e3b..3e7f99f 100644
> | > --- a/gfs2/fsck/fsck.h
> | > +++ b/gfs2/fsck/fsck.h
> | > @@ -173,4 +173,19 @@ static inline int rgrp_contains_block(struct rgrp_tree
> | > *rgd, uint64_t blk)
> | >  	return 1;
> | >  }
> | >
> | > +static inline int valid_block_ip(struct gfs2_inode *ip, uint64_t blk)
> | > +{
> | > +	struct gfs2_sbd *sdp = ip->i_sbd;
>
> | > +	struct rgrp_tree *rgd = ip->i_rgd;
> | > +
> | > +	if (blk > sdp->fssize)
> | > +		return 0;
> | > +	if (blk <= sdp->sb_addr)
> | > +		return 0;
> | > +	if (rgd == NULL || !rgrp_contains_block(rgd, blk))
> | > +		rgd = gfs2_blk2rgrpd(sdp, blk);
> |
> | Do we need another
> |
> | 	if (rgd == NULL)
> | 		return 0;
> |
> | here as gfs2_blk2rgrpd() can return NULL, e.g. if blk lies in the space
> | between rgrps?
> |
> | > +
> | > +	return rgrp_contains_block(rgd, blk);
> |
> | I'm not sure this second call is needed once we know rgd is not NULL,
> | since it either passed the first check or blk was matched to the rgrp in
> | gfs2_blk2rgrpd().
> |
> | Cheers,
> | Andy
>
> Hi Andy,
>
> This coding was deliberate and intentional.
>
> The idea here is: The ip->i_rgd is only a "hint" because in many cases,
> the rgrp for an inode's block will often be the same as the inode's rgrp
> itself. It's used for checking all the inode's blocks, not just the inode
> itself, but sometimes they fall outside the inode's rgrp. So the concept is:
>
> 1. If rgrp is passed in NULL, the rgrp for the block needs to be found.
> 2. If the inode's block is NOT within the inode's rgrp, we need to find the
>    proper rgrp for the block.
> 3. Once found, we need to make sure the block falls within it.

Sure, I got that. I'm still worried about gfs2_blk2rgrpd() returning 
NULL. I wouldn't be surprised if static analysis threw a warning on that 
one.

> It's subtle, but it's important. I think those checks all need to be there.

Hm ok maybe I'm missing something. Given how gfs2_blk2rgrpd() works, in 
what case is the second rgrp_contains_block() call going to return 0?

Andy

[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Bob Peterson]

Hi Andy,

----- Original Message -----
| 
| Sure, I got that. I'm still worried about gfs2_blk2rgrpd() returning
| NULL. I wouldn't be surprised if static analysis threw a warning on that
| one.

The earlier checks in valid_block_ip should take care of this case
because they check if the block lies outside the fs boundaries, and
therefore there must be an rgrp that covers it. Still, it's probably safest
just to add the check, if only to avoid the coverity errors, etc.
At least when we had guaranteed uniform rgrps, it was the case. Now that
we're trying to align things differently, I suppose there might be holes.

See new patch version below.

| > It's subtle, but it's important. I think those checks all need to be there.
| 
| Hm ok maybe I'm missing something. Given how gfs2_blk2rgrpd() works, in
| what case is the second rgrp_contains_block() call going to return 0?
| 
| Andy

Again, it's subtle. It can happen if a block within an inode (a data block,
for example) is mistakenly pointing to either the rgrp block itself or one
of its bitmaps. In that case, it's still within the jurisdiction of that
particular rgrp, but it's still an invalid block because it's not inside
the "valid blocks" available for a dinode to use, for that rgrp.

For example, if a dinode was located at block 0x100, but pointed to a "data
block" of 0x12 in your typical average gfs2 file system, that's almost
guaranteed to be an invalid block because, while it's within the legal
boundaries of the device, and rgrp 0x11 would be "found" by blk2rgrpd, it's
pointing at a bitmap for the rgrp (assuming, of course, that rd_length > 1).

Regards,

Bob Peterson
Red Hat File Systems
---
fsck.gfs2: Further performance gains with function valid_block_ip

Before this patch, many functions in fsck.gfs2 checked for blocks
being valid. To check if a block is valid, you really need to check
that it's inside the area covered by a valid resource group. For
example, if a data block is pointing to a rgrp bitmap block, that's
invalid. To achieve this, it does a rgrp search, which traverses
the rgrp tree. This patch optimizes that by allowing functions to
pass in the ip pointer, so like before, we can check if that rgrp
covers the block in question and save ourselves a lot of work
traversing the tree.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>

diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
index 0632695..e39bfea 100644
--- a/gfs2/fsck/afterpass1_common.c
+++ b/gfs2/fsck/afterpass1_common.c
@@ -81,7 +81,7 @@ static int delete_block_if_notdup(struct gfs2_inode *ip, uint64_t block,
 	int q;
 	int removed_lastmeta;
 
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return meta_error;
 
 	q = bitmap_type(ip->i_sbd, block);
@@ -207,7 +207,7 @@ static int del_eattr_generic(struct gfs2_inode *ip, uint64_t block,
 	int was_free = 0;
 	int q;
 
-	if (valid_block(ip->i_sbd, block)) {
+	if (valid_block_ip(ip, block)) {
 		q = bitmap_type(ip->i_sbd, block);
 		if (q == GFS2_BLKST_FREE)
 			was_free = 1;
diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
index c4e6e3b..73cff4c 100644
--- a/gfs2/fsck/fsck.h
+++ b/gfs2/fsck/fsck.h
@@ -173,4 +173,22 @@ static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t blk)
 	return 1;
 }
 
+static inline int valid_block_ip(struct gfs2_inode *ip, uint64_t blk)
+{
+	struct gfs2_sbd *sdp = ip->i_sbd;
+	struct rgrp_tree *rgd = ip->i_rgd;
+
+	if (blk > sdp->fssize)
+		return 0;
+	if (blk <= sdp->sb_addr)
+		return 0;
+	if (rgd == NULL || !rgrp_contains_block(rgd, blk)) {
+		rgd = gfs2_blk2rgrpd(sdp, blk);
+		if (rgd == NULL)
+			return 0;
+	}
+
+	return rgrp_contains_block(rgd, blk);
+}
+
 #endif /* _FSCK_H */
diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 7261422..c0cc2ab 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -539,7 +539,7 @@ int check_leaf(struct gfs2_inode *ip, int lindex, struct metawalk_fxns *pass,
 	int di_depth = ip->i_di.di_depth;
 
 	/* Make sure the block number is in range. */
-	if (!valid_block(ip->i_sbd, *leaf_no)) {
+	if (!valid_block_ip(ip, *leaf_no)) {
 		log_err( _("Leaf block #%llu (0x%llx) is out of range for "
 			   "directory #%llu (0x%llx) at index %d (0x%x).\n"),
 			 (unsigned long long)*leaf_no,
@@ -698,7 +698,7 @@ static void dir_leaf_reada(struct gfs2_inode *ip, uint64_t *tbl, unsigned hsize)
 
 	for (i = 0; i < hsize; i++) {
 		leaf_no = be64_to_cpu(tbl[i]);
-		if (valid_block(ip->i_sbd, leaf_no))
+		if (valid_block_ip(ip, leaf_no))
 			t[n++] = leaf_no * sdp->bsize;
 	}
 	qsort(t, n, sizeof(uint64_t), u64cmp);
@@ -760,7 +760,7 @@ int check_leaf_blks(struct gfs2_inode *ip, struct metawalk_fxns *pass)
 	first_ok_leaf = leaf_no = -1;
 	for (lindex = 0; lindex < hsize; lindex++) {
 		leaf_no = be64_to_cpu(tbl[lindex]);
-		if (valid_block(ip->i_sbd, leaf_no)) {
+		if (valid_block_ip(ip, leaf_no)) {
 			lbh = bread(sdp, leaf_no);
 			/* Make sure it's really a valid leaf block. */
 			if (gfs2_check_meta(lbh, GFS2_METATYPE_LF) == 0) {
@@ -1334,7 +1334,7 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
 						   (unsigned long long)block);
 					continue;
 				}
-				if (!valid_block(ip->i_sbd, block)) {
+				if (!valid_block_ip(ip, block)) {
 					log_debug( _("Skipping invalid block "
 						     "%lld (0x%llx)\n"),
 						   (unsigned long long)block,
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 3a1931d..6f04109 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -135,7 +135,7 @@ static int delete_block(struct gfs2_inode *ip, uint64_t block,
 			struct gfs2_buffer_head **bh, const char *btype,
 			void *private)
 {
-	if (valid_block(ip->i_sbd, block)) {
+	if (valid_block_ip(ip, block)) {
 		fsck_blockmap_set(ip, block, btype, GFS2_BLKST_FREE);
 		return 0;
 	}
@@ -233,7 +233,7 @@ static int resuscitate_metalist(struct gfs2_inode *ip, uint64_t block,
 	*is_valid = 1;
 	*was_duplicate = 0;
 	*bh = NULL;
-	if (!valid_block(ip->i_sbd, block)){ /* blk outside of FS */
+	if (!valid_block_ip(ip, block)){ /* blk outside of FS */
 		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
 				  _("itself"), GFS2_BLKST_UNLINKED);
 		log_err( _("Bad indirect block pointer (invalid or out of "
@@ -281,7 +281,7 @@ static int resuscitate_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent,
 		strncpy(tmp_name, filename, de->de_name_len);
 	else
 		strncpy(tmp_name, filename, sizeof(tmp_name) - 1);
-	if (!valid_block(sdp, block)) {
+	if (!valid_block_ip(ip, block)) {
 		log_err( _("Block # referenced by system directory entry %s "
 			   "in inode %lld (0x%llx) is invalid or out of range;"
 			   " ignored.\n"),
@@ -358,7 +358,7 @@ static int check_metalist(struct gfs2_inode *ip, uint64_t block,
 
 	*was_duplicate = 0;
 	*is_valid = 0;
-	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
+	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
 		/* The bad dinode should be invalidated later due to
 		   "unrecoverable" errors.  The inode itself should be
 		   set "free" and removed from the inodetree by
@@ -441,7 +441,7 @@ static int undo_reference(struct gfs2_inode *ip, uint64_t block, int meta,
 	int old_bitmap_state = 0;
 	struct rgrp_tree *rgd;
 
-	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
+	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
 		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
 				  _("bad block referencing"), GFS2_BLKST_FREE);
 		return 1;
@@ -549,7 +549,7 @@ static int check_data(struct gfs2_inode *ip, uint64_t metablock,
 	int q;
 	struct block_count *bc = (struct block_count *) private;
 
-	if (!valid_block(ip->i_sbd, block)) {
+	if (!valid_block_ip(ip, block)) {
 		log_err( _("inode %lld (0x%llx) has a bad data block pointer "
 			   "%lld (0x%llx) (invalid or out of range) "),
 			 (unsigned long long)ip->i_di.di_num.no_addr,
@@ -686,7 +686,7 @@ static int undo_eattr_indir_or_leaf(struct gfs2_inode *ip, uint64_t block,
 	int error;
 	struct block_count *bc = (struct block_count *) private;
 
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return meta_error;
 
 	/* Need to check block_type before undoing the reference, which can
@@ -735,7 +735,7 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
 
 	/* This inode contains an eattr - it may be invalid, but the
 	 * eattr attributes points to a non-zero block */
-	if (!valid_block(sdp, indirect)) {
+	if (!valid_block_ip(ip, indirect)) {
 		/* Doesn't help to mark this here - this gets checked
 		 * in pass1c */
 		return 1;
@@ -893,7 +893,7 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, int i,
 	struct gfs2_buffer_head *bh = NULL;
 	int error = 0;
 
-	if (!valid_block(sdp, el_blk)) {
+	if (!valid_block_ip(ip, el_blk)) {
 		log_err( _("Inode #%llu (0x%llx): Extended Attribute block "
 			   "%llu (0x%llx) has an extended leaf block #%llu "
 			   "(0x%llx) that is invalid or out of range.\n"),
@@ -943,9 +943,7 @@ static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
 			    uint64_t parent, struct gfs2_buffer_head **bh,
 			    void *private)
 {
-	struct gfs2_sbd *sdp = ip->i_sbd;
-
-	if (!valid_block(sdp, block)) {
+	if (!valid_block_ip(ip, block)) {
 		log_warn( _("Inode #%llu (0x%llx): Extended Attribute leaf "
 			    "block #%llu (0x%llx) is invalid or out of "
 			    "range.\n"),
@@ -1089,7 +1087,7 @@ static int mark_block_invalid(struct gfs2_inode *ip, uint64_t block,
 		*is_valid = 1;
 	if (was_duplicate)
 		*was_duplicate = 0;
-	if (!valid_block(ip->i_sbd, block)) {
+	if (!valid_block_ip(ip, block)) {
 		if (is_valid)
 			*is_valid = 0;
 		return meta_is_good;
@@ -1181,7 +1179,7 @@ static int rangecheck_block(struct gfs2_inode *ip, uint64_t block,
 	long *bad_pointers = (long *)private;
 	int q;
 
-	if (!valid_block(ip->i_sbd, block)) {
+	if (!valid_block_ip(ip, block)) {
 		(*bad_pointers)++;
 		log_info( _("Bad %s block pointer (invalid or out of range "
 			    "#%ld) found in inode %lld (0x%llx).\n"),
diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index d072634..f808cea 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -461,7 +461,7 @@ static int basic_dentry_checks(struct gfs2_inode *ip, struct gfs2_dirent *dent,
 	struct gfs2_inum inum = { 0 };
 
 	*isdir = 0;
-	if (!valid_block(ip->i_sbd, entry->no_addr)) {
+	if (!valid_block_ip(ip, entry->no_addr)) {
 		log_err( _("Block # referenced by directory entry %s in inode "
 			   "%lld (0x%llx) is invalid\n"),
 			 tmp_name, (unsigned long long)ip->i_di.di_num.no_addr,
@@ -1481,7 +1481,7 @@ static int check_hash_tbl_dups(struct gfs2_inode *ip, uint64_t *tbl,
 		   or the duplicate we found. */
 		memset(&leaf, 0, sizeof(leaf));
 		leaf_no = leafblk;
-		if (!valid_block(ip->i_sbd, leaf_no)) /* Checked later */
+		if (!valid_block_ip(ip, leaf_no)) /* Checked later */
 			continue;
 
 		lbh = bread(ip->i_sbd, leafblk);
@@ -1614,7 +1614,7 @@ static int check_hash_tbl(struct gfs2_inode *ip, uint64_t *tbl,
 		/* See if that leaf block is valid. If not, write a new one
 		   that falls on a proper boundary. If it doesn't naturally,
 		   we may need more. */
-		if (!valid_block(ip->i_sbd, leafblk)) {
+		if (!valid_block_ip(ip, leafblk)) {
 			uint64_t new_leafblk;
 
 			log_err(_("Dinode %llu (0x%llx) has bad leaf pointers "
@@ -1776,7 +1776,7 @@ static int check_data_qc(struct gfs2_inode *ip, uint64_t metablock,
 
 	/* At this point, basic data block checks have already been done,
 	   so we only need to make sure they're QC blocks. */
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return -1;
 
 	bh = bread(ip->i_sbd, block);
diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
index 8a8220b..1c3ed9c 100644
--- a/gfs2/fsck/util.c
+++ b/gfs2/fsck/util.c
@@ -335,7 +335,7 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
 	struct inode_with_dups *id;
 	struct duptree *dt;
 
-	if (!valid_block(ip->i_sbd, block))
+	if (!valid_block_ip(ip, block))
 		return meta_is_good;
 	/* If this is not the first reference (i.e. all calls from pass1) we
 	   need to create the duplicate reference. If this is pass1b, we want

[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip

[Andrew Price]

On 02/06/16 16:35, Bob Peterson wrote:
> Hi Andy,
>
> ----- Original Message -----
> |
> | Sure, I got that. I'm still worried about gfs2_blk2rgrpd() returning
> | NULL. I wouldn't be surprised if static analysis threw a warning on that
> | one.
>
> The earlier checks in valid_block_ip should take care of this case
> because they check if the block lies outside the fs boundaries, and
> therefore there must be an rgrp that covers it. Still, it's probably safest
> just to add the check, if only to avoid the coverity errors, etc.
> At least when we had guaranteed uniform rgrps, it was the case. Now that
> we're trying to align things differently, I suppose there might be holes.
>
> See new patch version below.
>
> | > It's subtle, but it's important. I think those checks all need to be there.
> |
> | Hm ok maybe I'm missing something. Given how gfs2_blk2rgrpd() works, in
> | what case is the second rgrp_contains_block() call going to return 0?
> |
> | Andy
>
> Again, it's subtle. It can happen if a block within an inode (a data block,
> for example) is mistakenly pointing to either the rgrp block itself or one
> of its bitmaps. In that case, it's still within the jurisdiction of that
> particular rgrp, but it's still an invalid block because it's not inside
> the "valid blocks" available for a dinode to use, for that rgrp.
>
> For example, if a dinode was located at block 0x100, but pointed to a "data
> block" of 0x12 in your typical average gfs2 file system, that's almost
> guaranteed to be an invalid block because, while it's within the legal
> boundaries of the device, and rgrp 0x11 would be "found" by blk2rgrpd, it's
> pointing at a bitmap for the rgrp (assuming, of course, that rd_length > 1).

Ah ok. Sorry, I read the blk2rgrpd code as checking against ri_data0 but 
it actually checks against ri_addr. My bad.

ACK for the amended patch.

Thanks,
Andy

>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
> ---
> fsck.gfs2: Further performance gains with function valid_block_ip
>
> Before this patch, many functions in fsck.gfs2 checked for blocks
> being valid. To check if a block is valid, you really need to check
> that it's inside the area covered by a valid resource group. For
> example, if a data block is pointing to a rgrp bitmap block, that's
> invalid. To achieve this, it does a rgrp search, which traverses
> the rgrp tree. This patch optimizes that by allowing functions to
> pass in the ip pointer, so like before, we can check if that rgrp
> covers the block in question and save ourselves a lot of work
> traversing the tree.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
>
> diff --git a/gfs2/fsck/afterpass1_common.c b/gfs2/fsck/afterpass1_common.c
> index 0632695..e39bfea 100644
> --- a/gfs2/fsck/afterpass1_common.c
> +++ b/gfs2/fsck/afterpass1_common.c
> @@ -81,7 +81,7 @@ static int delete_block_if_notdup(struct gfs2_inode *ip, uint64_t block,
>  	int q;
>  	int removed_lastmeta;
>
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return meta_error;
>
>  	q = bitmap_type(ip->i_sbd, block);
> @@ -207,7 +207,7 @@ static int del_eattr_generic(struct gfs2_inode *ip, uint64_t block,
>  	int was_free = 0;
>  	int q;
>
> -	if (valid_block(ip->i_sbd, block)) {
> +	if (valid_block_ip(ip, block)) {
>  		q = bitmap_type(ip->i_sbd, block);
>  		if (q == GFS2_BLKST_FREE)
>  			was_free = 1;
> diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h
> index c4e6e3b..73cff4c 100644
> --- a/gfs2/fsck/fsck.h
> +++ b/gfs2/fsck/fsck.h
> @@ -173,4 +173,22 @@ static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t blk)
>  	return 1;
>  }
>
> +static inline int valid_block_ip(struct gfs2_inode *ip, uint64_t blk)
> +{
> +	struct gfs2_sbd *sdp = ip->i_sbd;
> +	struct rgrp_tree *rgd = ip->i_rgd;
> +
> +	if (blk > sdp->fssize)
> +		return 0;
> +	if (blk <= sdp->sb_addr)
> +		return 0;
> +	if (rgd == NULL || !rgrp_contains_block(rgd, blk)) {
> +		rgd = gfs2_blk2rgrpd(sdp, blk);
> +		if (rgd == NULL)
> +			return 0;
> +	}
> +
> +	return rgrp_contains_block(rgd, blk);
> +}
> +
>  #endif /* _FSCK_H */
> diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
> index 7261422..c0cc2ab 100644
> --- a/gfs2/fsck/metawalk.c
> +++ b/gfs2/fsck/metawalk.c
> @@ -539,7 +539,7 @@ int check_leaf(struct gfs2_inode *ip, int lindex, struct metawalk_fxns *pass,
>  	int di_depth = ip->i_di.di_depth;
>
>  	/* Make sure the block number is in range. */
> -	if (!valid_block(ip->i_sbd, *leaf_no)) {
> +	if (!valid_block_ip(ip, *leaf_no)) {
>  		log_err( _("Leaf block #%llu (0x%llx) is out of range for "
>  			   "directory #%llu (0x%llx) at index %d (0x%x).\n"),
>  			 (unsigned long long)*leaf_no,
> @@ -698,7 +698,7 @@ static void dir_leaf_reada(struct gfs2_inode *ip, uint64_t *tbl, unsigned hsize)
>
>  	for (i = 0; i < hsize; i++) {
>  		leaf_no = be64_to_cpu(tbl[i]);
> -		if (valid_block(ip->i_sbd, leaf_no))
> +		if (valid_block_ip(ip, leaf_no))
>  			t[n++] = leaf_no * sdp->bsize;
>  	}
>  	qsort(t, n, sizeof(uint64_t), u64cmp);
> @@ -760,7 +760,7 @@ int check_leaf_blks(struct gfs2_inode *ip, struct metawalk_fxns *pass)
>  	first_ok_leaf = leaf_no = -1;
>  	for (lindex = 0; lindex < hsize; lindex++) {
>  		leaf_no = be64_to_cpu(tbl[lindex]);
> -		if (valid_block(ip->i_sbd, leaf_no)) {
> +		if (valid_block_ip(ip, leaf_no)) {
>  			lbh = bread(sdp, leaf_no);
>  			/* Make sure it's really a valid leaf block. */
>  			if (gfs2_check_meta(lbh, GFS2_METATYPE_LF) == 0) {
> @@ -1334,7 +1334,7 @@ static int build_and_check_metalist(struct gfs2_inode *ip, osi_list_t *mlp,
>  						   (unsigned long long)block);
>  					continue;
>  				}
> -				if (!valid_block(ip->i_sbd, block)) {
> +				if (!valid_block_ip(ip, block)) {
>  					log_debug( _("Skipping invalid block "
>  						     "%lld (0x%llx)\n"),
>  						   (unsigned long long)block,
> diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
> index 3a1931d..6f04109 100644
> --- a/gfs2/fsck/pass1.c
> +++ b/gfs2/fsck/pass1.c
> @@ -135,7 +135,7 @@ static int delete_block(struct gfs2_inode *ip, uint64_t block,
>  			struct gfs2_buffer_head **bh, const char *btype,
>  			void *private)
>  {
> -	if (valid_block(ip->i_sbd, block)) {
> +	if (valid_block_ip(ip, block)) {
>  		fsck_blockmap_set(ip, block, btype, GFS2_BLKST_FREE);
>  		return 0;
>  	}
> @@ -233,7 +233,7 @@ static int resuscitate_metalist(struct gfs2_inode *ip, uint64_t block,
>  	*is_valid = 1;
>  	*was_duplicate = 0;
>  	*bh = NULL;
> -	if (!valid_block(ip->i_sbd, block)){ /* blk outside of FS */
> +	if (!valid_block_ip(ip, block)){ /* blk outside of FS */
>  		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
>  				  _("itself"), GFS2_BLKST_UNLINKED);
>  		log_err( _("Bad indirect block pointer (invalid or out of "
> @@ -281,7 +281,7 @@ static int resuscitate_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent,
>  		strncpy(tmp_name, filename, de->de_name_len);
>  	else
>  		strncpy(tmp_name, filename, sizeof(tmp_name) - 1);
> -	if (!valid_block(sdp, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		log_err( _("Block # referenced by system directory entry %s "
>  			   "in inode %lld (0x%llx) is invalid or out of range;"
>  			   " ignored.\n"),
> @@ -358,7 +358,7 @@ static int check_metalist(struct gfs2_inode *ip, uint64_t block,
>
>  	*was_duplicate = 0;
>  	*is_valid = 0;
> -	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
> +	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
>  		/* The bad dinode should be invalidated later due to
>  		   "unrecoverable" errors.  The inode itself should be
>  		   set "free" and removed from the inodetree by
> @@ -441,7 +441,7 @@ static int undo_reference(struct gfs2_inode *ip, uint64_t block, int meta,
>  	int old_bitmap_state = 0;
>  	struct rgrp_tree *rgd;
>
> -	if (!valid_block(ip->i_sbd, block)) { /* blk outside of FS */
> +	if (!valid_block_ip(ip, block)) { /* blk outside of FS */
>  		fsck_blockmap_set(ip, ip->i_di.di_num.no_addr,
>  				  _("bad block referencing"), GFS2_BLKST_FREE);
>  		return 1;
> @@ -549,7 +549,7 @@ static int check_data(struct gfs2_inode *ip, uint64_t metablock,
>  	int q;
>  	struct block_count *bc = (struct block_count *) private;
>
> -	if (!valid_block(ip->i_sbd, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		log_err( _("inode %lld (0x%llx) has a bad data block pointer "
>  			   "%lld (0x%llx) (invalid or out of range) "),
>  			 (unsigned long long)ip->i_di.di_num.no_addr,
> @@ -686,7 +686,7 @@ static int undo_eattr_indir_or_leaf(struct gfs2_inode *ip, uint64_t block,
>  	int error;
>  	struct block_count *bc = (struct block_count *) private;
>
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return meta_error;
>
>  	/* Need to check block_type before undoing the reference, which can
> @@ -735,7 +735,7 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
>
>  	/* This inode contains an eattr - it may be invalid, but the
>  	 * eattr attributes points to a non-zero block */
> -	if (!valid_block(sdp, indirect)) {
> +	if (!valid_block_ip(ip, indirect)) {
>  		/* Doesn't help to mark this here - this gets checked
>  		 * in pass1c */
>  		return 1;
> @@ -893,7 +893,7 @@ static int check_extended_leaf_eattr(struct gfs2_inode *ip, int i,
>  	struct gfs2_buffer_head *bh = NULL;
>  	int error = 0;
>
> -	if (!valid_block(sdp, el_blk)) {
> +	if (!valid_block_ip(ip, el_blk)) {
>  		log_err( _("Inode #%llu (0x%llx): Extended Attribute block "
>  			   "%llu (0x%llx) has an extended leaf block #%llu "
>  			   "(0x%llx) that is invalid or out of range.\n"),
> @@ -943,9 +943,7 @@ static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
>  			    uint64_t parent, struct gfs2_buffer_head **bh,
>  			    void *private)
>  {
> -	struct gfs2_sbd *sdp = ip->i_sbd;
> -
> -	if (!valid_block(sdp, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		log_warn( _("Inode #%llu (0x%llx): Extended Attribute leaf "
>  			    "block #%llu (0x%llx) is invalid or out of "
>  			    "range.\n"),
> @@ -1089,7 +1087,7 @@ static int mark_block_invalid(struct gfs2_inode *ip, uint64_t block,
>  		*is_valid = 1;
>  	if (was_duplicate)
>  		*was_duplicate = 0;
> -	if (!valid_block(ip->i_sbd, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		if (is_valid)
>  			*is_valid = 0;
>  		return meta_is_good;
> @@ -1181,7 +1179,7 @@ static int rangecheck_block(struct gfs2_inode *ip, uint64_t block,
>  	long *bad_pointers = (long *)private;
>  	int q;
>
> -	if (!valid_block(ip->i_sbd, block)) {
> +	if (!valid_block_ip(ip, block)) {
>  		(*bad_pointers)++;
>  		log_info( _("Bad %s block pointer (invalid or out of range "
>  			    "#%ld) found in inode %lld (0x%llx).\n"),
> diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
> index d072634..f808cea 100644
> --- a/gfs2/fsck/pass2.c
> +++ b/gfs2/fsck/pass2.c
> @@ -461,7 +461,7 @@ static int basic_dentry_checks(struct gfs2_inode *ip, struct gfs2_dirent *dent,
>  	struct gfs2_inum inum = { 0 };
>
>  	*isdir = 0;
> -	if (!valid_block(ip->i_sbd, entry->no_addr)) {
> +	if (!valid_block_ip(ip, entry->no_addr)) {
>  		log_err( _("Block # referenced by directory entry %s in inode "
>  			   "%lld (0x%llx) is invalid\n"),
>  			 tmp_name, (unsigned long long)ip->i_di.di_num.no_addr,
> @@ -1481,7 +1481,7 @@ static int check_hash_tbl_dups(struct gfs2_inode *ip, uint64_t *tbl,
>  		   or the duplicate we found. */
>  		memset(&leaf, 0, sizeof(leaf));
>  		leaf_no = leafblk;
> -		if (!valid_block(ip->i_sbd, leaf_no)) /* Checked later */
> +		if (!valid_block_ip(ip, leaf_no)) /* Checked later */
>  			continue;
>
>  		lbh = bread(ip->i_sbd, leafblk);
> @@ -1614,7 +1614,7 @@ static int check_hash_tbl(struct gfs2_inode *ip, uint64_t *tbl,
>  		/* See if that leaf block is valid. If not, write a new one
>  		   that falls on a proper boundary. If it doesn't naturally,
>  		   we may need more. */
> -		if (!valid_block(ip->i_sbd, leafblk)) {
> +		if (!valid_block_ip(ip, leafblk)) {
>  			uint64_t new_leafblk;
>
>  			log_err(_("Dinode %llu (0x%llx) has bad leaf pointers "
> @@ -1776,7 +1776,7 @@ static int check_data_qc(struct gfs2_inode *ip, uint64_t metablock,
>
>  	/* At this point, basic data block checks have already been done,
>  	   so we only need to make sure they're QC blocks. */
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return -1;
>
>  	bh = bread(ip->i_sbd, block);
> diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
> index 8a8220b..1c3ed9c 100644
> --- a/gfs2/fsck/util.c
> +++ b/gfs2/fsck/util.c
> @@ -335,7 +335,7 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
>  	struct inode_with_dups *id;
>  	struct duptree *dt;
>
> -	if (!valid_block(ip->i_sbd, block))
> +	if (!valid_block_ip(ip, block))
>  		return meta_is_good;
>  	/* If this is not the first reference (i.e. all calls from pass1) we
>  	   need to create the duplicate reference. If this is pass1b, we want
>