From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Price Date: Tue, 16 Jul 2013 17:52:34 +0100 Subject: [Cluster-devel] [gfs2-utils PATCH 7/7] fsck.gfs2: Detect and fix duplicate references in hash tables In-Reply-To: <67014263a531655f54e1b40b0058a93c42790e01.1373979296.git.rpeterso@redhat.com> References: <08b2ef646a21c19d0cdd1212fe8d4b4a8d7ee2b2.1373979296.git.rpeterso@redhat.com> <67014263a531655f54e1b40b0058a93c42790e01.1373979296.git.rpeterso@redhat.com> Message-ID: <51E57A52.8030103@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi Bob, A couple of comments below... On 16/07/13 13:56, Bob Peterson wrote: > This patch checks directory hash tables for multiple occurrences > of the same leaf block. When duplicates are encountered, it checks > to see which reference is appropriate for the hash table location > based on the leaf block's hash value. The appropriate reference is > kept and all duplicates are replaced by newly created leaf blocks. > > rhbz#984085 > --- > gfs2/fsck/pass2.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 108 insertions(+) > > diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c > index a40a4a7..9f3e5c1 100644 > --- a/gfs2/fsck/pass2.c > +++ b/gfs2/fsck/pass2.c > @@ -1278,6 +1278,110 @@ static int fix_hashtable(struct gfs2_inode *ip, uint64_t *tbl, unsigned hsize, > return changes; > } > > +/* check_hash_tbl_dups - check for the same leaf in multiple places */ > +static int check_hash_tbl_dups(struct gfs2_inode *ip, uint64_t *tbl, > + unsigned hsize, int lindex, int len) > +{ > + int l, len2; > + uint64_t leafblk, leaf_no; > + struct gfs2_buffer_head *lbh; > + struct gfs2_leaf leaf; > + struct gfs2_dirent dentry, *de; > + int hash_index; /* index into the hash table based on the hash */ > + > + leafblk = be64_to_cpu(tbl[lindex]); > + for (l = 0; l < hsize; l++) { > + if (l == lindex) { /* skip the valid reference */ > + l += len - 1; > + continue; > + } > + if (be64_to_cpu(tbl[l]) != leafblk) > + continue; > + > + for (len2 = 0; l + len2 < hsize; len2++) { > + if (l + len2 == lindex) > + break; > + if (be64_to_cpu(tbl[l + len2]) != leafblk) > + break; > + } > + log_err(_("Dinode %llu (0x%llx) has duplicate leaf pointers " > + "to block %llu (0x%llx) at offsets %u (0x%x) " > + "(for 0x%x) and %u (0x%x) (for 0x%x)\n"), > + (unsigned long long)ip->i_di.di_num.no_addr, > + (unsigned long long)ip->i_di.di_num.no_addr, > + (unsigned long long)leafblk, > + (unsigned long long)leafblk, lindex, lindex, len, > + l, l, len2); > + > + /* See which set of references is valid: the one passed in > + or the duplicate we found. */ > + memset(&leaf, 0, sizeof(leaf)); > + leaf_no = leafblk; > + if (!valid_block(ip->i_sbd, leaf_no)) /* Checked later */ > + continue; > + > + lbh = bread(ip->i_sbd, leafblk); > + if (gfs2_check_meta(lbh, GFS2_METATYPE_LF)) { /* Chked later */ > + brelse(lbh); > + continue; > + } > + > + memset(&dentry, 0, sizeof(struct gfs2_dirent)); > + de = (struct gfs2_dirent *)(lbh->b_data + > + sizeof(struct gfs2_leaf)); > + gfs2_dirent_in(&dentry, (char *)de); > + hash_index = hash_table_index(dentry.de_hash, ip); > + brelse(lbh); Unconditionally releasing lbh here ... > + /* check the duplicate ref first */ > + if (hash_index < l || hash_index > l + len2) { > + log_err(_("This leaf block has hash index %d, which " > + "is out of bounds for lindex (%d - %d)\n"), > + hash_index, l, l + len2); > + if (!query( _("Fix the hash table? (y/n) "))) { > + log_err(_("Hash table not fixed.\n")); > + brelse(lbh); so shouldn't need to here... > + return 0; > + } > + /* Adjust the ondisk block count. The original value > + may have been correct without the duplicates but > + pass1 would have counted them and adjusted the > + count to include them. So we must subtract them. */ > + ip->i_di.di_blocks--; > + bmodified(ip->i_bh); > + pad_with_leafblks(ip, tbl, l, len2); > + } else { > + log_debug(_("Hash index 0x%x is the proper " > + "reference to leaf 0x%llx.\n"), > + l, (unsigned long long)leafblk); > + } > + /* Check the original ref: both references might be bad. > + If both were bad, just return and if we encounter it > + again, we'll treat it as new. If the original ref is not > + bad, keep looking for (and fixing) other instances. */ > + if (hash_index < lindex || hash_index > lindex + len) { > + log_err(_("This leaf block has hash index %d, which " > + "is out of bounds for lindex (%d - %d).\n"), > + hash_index, lindex, lindex + len); > + if (!query( _("Fix the hash table? (y/n) "))) { > + log_err(_("Hash table not fixed.\n")); > + brelse(lbh); and here. The rest looks good to me. Cheers, Andy > + return 0; > + } > + ip->i_di.di_blocks--; > + bmodified(ip->i_bh); > + pad_with_leafblks(ip, tbl, lindex, len); > + /* At this point we know both copies are bad, so we > + return to start fresh */ > + return -EFAULT; > + } else { > + log_debug(_("Hash index 0x%x is the proper " > + "reference to leaf 0x%llx.\n"), > + lindex, (unsigned long long)leafblk); > + } > + } > + return 0; > +} > + > /* check_hash_tbl - check that the hash table is sane > * > * We've got to make sure the hash table is sane. Each leaf needs to > @@ -1372,6 +1476,10 @@ static int check_hash_tbl(struct gfs2_inode *ip, uint64_t *tbl, > lindex += proper_len; > continue; > } > + > + if (check_hash_tbl_dups(ip, tbl, hsize, lindex, len)) > + continue; > + > /* Make sure they call on proper leaf-split boundaries. This > is the calculation used by the kernel, and dir_split_leaf */ > proper_start = (lindex & ~(proper_len - 1)); >