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