[Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Further performance gains with function valid_block_ip
Bob Peterson
rpeterso at redhat.com
Thu Jun 2 15:35:46 UTC 2016
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).
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