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