[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] [gfs2-utils PATCH 7/7] fsck.gfs2: Detect and fix duplicate references in hash tables



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));



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]