[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip
Andrew Price
anprice at redhat.com
Thu Jun 2 14:27:59 UTC 2016
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
> +}
> +
> #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
>
More information about the Cluster-devel
mailing list