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