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

[Bob Peterson <rpeterso@redhat.com>]

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