[Cluster-devel] [gfs2-utils PATCH 00/20] fsck.gfs2: Fix bugs and eliminate pass1c

[Cluster-devel] [gfs2-utils PATCH 00/20] fsck.gfs2: Fix bugs and eliminate pass1c

[Bob Peterson]

Hi,

This original goal of this patch series was to eliminate pass1c, because
it is slow and takes lots of memory related to a linked list of all inodes
that have extended attributes. In thoroughly testing the patch, I uncovered
a bunch of fsck.gfs2 bugs, most of which have to do with extended attributes.
One patch spares the life of inodes that have bad extended attribute pointers:
rather than deleting the inode, it now removes its extended attributes.
Another patch adds a new feature to fsck.gfs2 whereby duplicated data block
pointers may be cloned to new data blocks, sparing the life of some inodes
that were previously deleted. There are a few cleanups thrown in as well.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
Bob Peterson (20):
  libgfs2: Check block range when inserting into rgrp tree
  libgfs2: Check rgd->bits before referencing it
  fsck.gfs2: Add check for gfs1 invalid inode refs in dentry
  fsck.gfs2: Make debug messages more succinct wrt extended attributes
  fsck.gfs2: Break up funtion handle_dup_blk
  fsck.gfs2: Only preserve the _first_ acceptable inode reference
  fsck.gfs2: Don't just assume the remaining EA reference is good
  fsck.gfs2: Don't delete inode for duplicate reference in EA
  fsck.gfs2: Don't traverse EAs that belong to another inode
  fsck.gfs2: Refactor function check_indirect_eattr
  fsck.gfs2: Once an indirect ea error is found, flag all that follow
  fsck.gfs2: Always restore saved value for di_eattr
  fsck.gfs2: Remove redundancy in add_duplicate_ref
  fsck.gfs2: Don't remove duplicate eattr blocks
  fsck.gfs2: Refactor check_eattr_entries and add error messages
  fsck.gfs2: remove bad EAs at the end, not as-you-go
  fsck.gfs2: Combine remove_inode_eattr with its only caller
  fsck.gfs2: Print debug message to dilineate metadata blocks
  fsck.gfs2: Remove pass1c in favor of processing in pass1
  fsck.gfs2: Clone duplicate data block pointers

 gfs2/fsck/Makefile.am  |   1 -
 gfs2/fsck/main.c       |   1 -
 gfs2/fsck/metawalk.c   | 185 ++++++++++++-----------
 gfs2/fsck/pass1.c      | 296 ++++++++++++++++++++++---------------
 gfs2/fsck/pass1b.c     | 392 +++++++++++++++++++++++++++++++++++--------------
 gfs2/fsck/pass1c.c     | 285 -----------------------------------
 gfs2/fsck/pass2.c      |  21 +++
 gfs2/fsck/util.c       |  15 +-
 gfs2/libgfs2/libgfs2.h |   2 -
 gfs2/libgfs2/rgrp.c    |   2 +-
 gfs2/libgfs2/super.c   |   6 +
 11 files changed, 592 insertions(+), 614 deletions(-)
 delete mode 100644 gfs2/fsck/pass1c.c

-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 01/20] libgfs2: Check block range when inserting into rgrp tree

[Bob Peterson]

This patch adds checks to function rindex_read to make sure the
rgrp starting address isn't grossly outside the file system.
It may be in the case of severely corrupt file systems from fsck.
If we added them to the rgrp tree, our calculations will get
screwed up, eventually causing a segfault.

rhbz#1257625
---
 gfs2/libgfs2/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gfs2/libgfs2/super.c b/gfs2/libgfs2/super.c
index b956366..73354ff 100644
--- a/gfs2/libgfs2/super.c
+++ b/gfs2/libgfs2/super.c
@@ -166,6 +166,12 @@ int rindex_read(struct gfs2_sbd *sdp, int fd, int *count1, int *sane)
 			return -1;
 
 		gfs2_rindex_in(&ri, (char *)&buf.bufgfs2);
+		if (gfs2_check_range(sdp, ri.ri_addr) != 0) {
+			*sane = 0;
+			if (prev_rgd == NULL)
+				return -1;
+			ri.ri_addr = prev_rgd->ri.ri_addr + prev_rgd->length;
+		}
 		rgd = rgrp_insert(&sdp->rgtree, ri.ri_addr);
 		memcpy(&rgd->ri, &ri, sizeof(struct gfs2_rindex));
 
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 02/20] libgfs2: Check rgd->bits before referencing it

[Bob Peterson]

This patch adds a check to function gfs2_rgrp_free to make sure
rgd->bits is non-zero before attempting to reference it.
This might be NULL because no buffers actually existed because
it was concocted in an attempt to repair damaged rgrps in fsck.

rhbz#1257625
---
 gfs2/libgfs2/rgrp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gfs2/libgfs2/rgrp.c b/gfs2/libgfs2/rgrp.c
index cf4385a..2a55523 100644
--- a/gfs2/libgfs2/rgrp.c
+++ b/gfs2/libgfs2/rgrp.c
@@ -244,7 +244,7 @@ void gfs2_rgrp_free(struct osi_root *rgrp_tree)
 	while ((n = osi_first(rgrp_tree))) {
 		rgd = (struct rgrp_tree *)n;
 
-		if (rgd->bits[0].bi_bh) { /* if a buffer exists */
+		if (rgd->bits && rgd->bits[0].bi_bh) { /* if a buffer exists */
 			rgs_since_sync++;
 			if (rgs_since_sync >= RG_SYNC_TOLERANCE) {
 				if (!sdp)
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 03/20] fsck.gfs2: Add check for gfs1 invalid inode refs in dentry

[Bob Peterson]

This patch adds a check to function basic_dentry_checks for a
special case where a GFS1 dirent points to a non-inode. Since
GFS1 marked all its metadata as "metadata" (not just dinodes as
GFS2 does), we need to verify that GFS1 dirents actually point
to GFS1 dinodes. If they point to a different kind of metadata
block, we need to treat it as an error.

rhbz#1257625
---
 gfs2/fsck/pass2.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index 900d4e1..11777eb 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -611,6 +611,27 @@ static int basic_dentry_checks(struct gfs2_inode *ip, struct gfs2_dirent *dent,
 			 (unsigned long long)ii->di_num.no_formal_ino);
 		return 1;
 	}
+	/* Check for a special case where a (bad) GFS1 dirent points to what
+	 * is not a known inode. It could be other GFS1 metadata, such as an
+	 * eattr or indirect block, but marked "dinode" in the bitmap because
+	 * gfs1 marked all gfs1 metadata that way. */
+	if (ii == NULL && sdp->gfs1) {
+		struct gfs2_buffer_head *tbh;
+
+		tbh = bread(sdp, entry->no_addr);
+		if (gfs2_check_meta(tbh, GFS2_METATYPE_DI)) { /* not dinode */
+			log_err( _("Directory entry '%s' pointing to block "
+				   "%llu (0x%llx) in directory %llu (0x%llx) "
+				   "is not really a GFS1 dinode.\n"), tmp_name,
+			 (unsigned long long)entry->no_addr,
+			 (unsigned long long)entry->no_addr,
+			 (unsigned long long)ip->i_di.di_num.no_addr,
+			 (unsigned long long)ip->i_di.di_num.no_addr);
+			brelse(tbh);
+			return 1;
+		}
+		brelse(tbh);
+	}
 	return 0;
 }
 
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 04/20] fsck.gfs2: Make debug messages more succinct wrt extended attributes

[Bob Peterson]

This patch changes function check_inode_eattr, which is called from
several passes, to remove the debug message that says basically
Extended attributes exist for inode X. That way the debug output is
not cluttered with those messages for every inode for every pass.
Instead, the existing message regarding the processing of EA leaf blocks
has been enhanced to add the inode number. Similarly, a new debug
message was added to function check_inode_eattr for indirect EA block
processing. This makes the output much more clear.

rhbz#1257625
---
 gfs2/fsck/metawalk.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 333bec6..cf3231f 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -955,9 +955,12 @@ static int check_leaf_eattr(struct gfs2_inode *ip, uint64_t block,
 	if (pass->check_eattr_leaf) {
 		int error = 0;
 
-		log_debug( _("Checking EA leaf block #%llu (0x%llx).\n"),
+		log_debug( _("Checking EA leaf block #%llu (0x%llx) for "
+			     "inode #%llu (0x%llx).\n"),
 			   (unsigned long long)block,
-			   (unsigned long long)block);
+			   (unsigned long long)block,
+			   (unsigned long long)ip->i_di.di_num.no_addr,
+			   (unsigned long long)ip->i_di.di_num.no_addr);
 
 		error = pass->check_eattr_leaf(ip, block, parent, &bh,
 					       pass->private);
@@ -1175,11 +1178,13 @@ int check_inode_eattr(struct gfs2_inode *ip, struct metawalk_fxns *pass)
 	if (!ip->i_di.di_eattr)
 		return 0;
 
-	log_debug( _("Extended attributes exist for inode #%llu (0x%llx).\n"),
-		  (unsigned long long)ip->i_di.di_num.no_addr,
-		  (unsigned long long)ip->i_di.di_num.no_addr);
-
 	if (ip->i_di.di_flags & GFS2_DIF_EA_INDIRECT){
+		log_debug( _("Checking EA indirect block #%llu (0x%llx) for "
+			     "inode #%llu (0x%llx)..\n"),
+			   (unsigned long long)ip->i_di.di_eattr,
+			   (unsigned long long)ip->i_di.di_eattr,
+			   (unsigned long long)ip->i_di.di_num.no_addr,
+			   (unsigned long long)ip->i_di.di_num.no_addr);
 		if ((error = check_indirect_eattr(ip, ip->i_di.di_eattr, pass)))
 			stack;
 	} else {
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 05/20] fsck.gfs2: Break up funtion handle_dup_blk

[Bob Peterson]

This patch just breaks function handle_dup_blk into a new function
resolve_last_reference to increase readability.

rhbz#1257625
---
 gfs2/fsck/pass1b.c | 131 +++++++++++++++++++++++++++--------------------------
 1 file changed, 67 insertions(+), 64 deletions(-)

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index c1598ff..845227e 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -364,6 +364,72 @@ static void clone_dup_ref_in_inode(struct gfs2_inode *ip, struct duptree *dt)
 	}
 }
 
+static void resolve_last_reference(struct gfs2_sbd *sdp, struct duptree *dt,
+				   enum dup_ref_type acceptable_ref)
+{
+	struct gfs2_inode *ip;
+	struct inode_with_dups *id;
+	osi_list_t *tmp;
+	uint8_t q;
+
+	log_notice( _("Block %llu (0x%llx) has only one remaining "
+		      "valid inode referencing it.\n"),
+		    (unsigned long long)dt->block,
+		    (unsigned long long)dt->block);
+	/* If we're down to a single reference (and not all references
+	   deleted, which may be the case of an inode that has only
+	   itself and a reference), we need to reset the block type
+	   from invalid to data or metadata. Start at the first one
+	   in the list, not the structure's place holder. */
+	tmp = dt->ref_inode_list.next;
+	id = osi_list_entry(tmp, struct inode_with_dups, list);
+	log_debug( _("----------------------------------------------\n"
+		     "Step 4. Set block type based on the remaining "
+		     "reference in inode %lld (0x%llx).\n"),
+		   (unsigned long long)id->block_no,
+		   (unsigned long long)id->block_no);
+	ip = fsck_load_inode(sdp, id->block_no);
+
+	if (dt->dup_flags & DUPFLAG_REF1_IS_DUPL)
+		clone_dup_ref_in_inode(ip, dt);
+
+	q = block_type(id->block_no);
+	if (q == GFS2_BLKST_UNLINKED) {
+		log_debug( _("The remaining reference inode %lld (0x%llx) is "
+			     "marked invalid: Marking the block as free.\n"),
+			   (unsigned long long)id->block_no,
+			   (unsigned long long)id->block_no);
+		fsck_blockmap_set(ip, dt->block, _("reference-repaired leaf"),
+				  GFS2_BLKST_FREE);
+	} else if (id->reftypecount[ref_is_inode]) {
+		set_ip_blockmap(ip, 0); /* 0=do not add to dirtree */
+	} else if (id->reftypecount[ref_as_data]) {
+		fsck_blockmap_set(ip, dt->block,  _("reference-repaired data"),
+				  GFS2_BLKST_USED);
+	} else if (id->reftypecount[ref_as_meta]) {
+		if (is_dir(&ip->i_di, sdp->gfs1))
+			fsck_blockmap_set(ip, dt->block,
+					  _("reference-repaired leaf"),
+					  sdp->gfs1 ? GFS2_BLKST_DINODE :
+					  GFS2_BLKST_USED);
+		else
+			fsck_blockmap_set(ip, dt->block,
+					  _("reference-repaired indirect"),
+					  sdp->gfs1 ? GFS2_BLKST_DINODE :
+					  GFS2_BLKST_USED);
+	} else {
+		fsck_blockmap_set(ip, dt->block,
+				  _("reference-repaired extended "
+				    "attribute"),
+				  sdp->gfs1 ? GFS2_BLKST_DINODE :
+				  GFS2_BLKST_USED);
+	}
+	fsck_inode_put(&ip); /* out, brelse, free */
+	log_debug(_("Done with duplicate reference to block 0x%llx\n"),
+		  (unsigned long long)dt->block);
+	dup_delete(dt);
+}
+
 /* handle_dup_blk - handle a duplicate block reference.
  *
  * This function should resolve and delete the duplicate block reference given,
@@ -372,8 +438,6 @@ static void clone_dup_ref_in_inode(struct gfs2_inode *ip, struct duptree *dt)
 static int handle_dup_blk(struct gfs2_sbd *sdp, struct duptree *dt)
 {
 	osi_list_t *tmp;
-	struct gfs2_inode *ip;
-	struct inode_with_dups *id;
 	struct dup_handler dh = {0};
 	struct gfs2_buffer_head *bh;
 	uint32_t cmagic, ctype;
@@ -484,68 +548,7 @@ static int handle_dup_blk(struct gfs2_sbd *sdp, struct duptree *dt)
 	   reference, use it to determine the correct block type for our
 	   blockmap and bitmap. */
 	if (dh.ref_inode_count == 1 && !osi_list_empty(&dt->ref_inode_list)) {
-		uint8_t q;
-
-		log_notice( _("Block %llu (0x%llx) has only one remaining "
-			      "valid inode referencing it.\n"),
-			    (unsigned long long)dup_blk,
-			    (unsigned long long)dup_blk);
-		/* If we're down to a single reference (and not all references
-		   deleted, which may be the case of an inode that has only
-		   itself and a reference), we need to reset the block type
-		   from invalid to data or metadata. Start at the first one
-		   in the list, not the structure's place holder. */
-		tmp = dt->ref_inode_list.next;
-		id = osi_list_entry(tmp, struct inode_with_dups, list);
-		log_debug( _("----------------------------------------------\n"
-			     "Step 4. Set block type based on the remaining "
-			     "reference in inode %lld (0x%llx).\n"),
-			   (unsigned long long)id->block_no,
-			   (unsigned long long)id->block_no);
-		ip = fsck_load_inode(sdp, id->block_no);
-
-		if (dt->dup_flags & DUPFLAG_REF1_IS_DUPL)
-			clone_dup_ref_in_inode(ip, dt);
-
-		q = block_type(id->block_no);
-		if (q == GFS2_BLKST_UNLINKED) {
-			log_debug( _("The remaining reference inode %lld "
-				     "(0x%llx) is marked invalid: Marking "
-				     "the block as free.\n"),
-				   (unsigned long long)id->block_no,
-				   (unsigned long long)id->block_no);
-			fsck_blockmap_set(ip, dt->block,
-					  _("reference-repaired leaf"),
-					  GFS2_BLKST_FREE);
-		} else if (id->reftypecount[ref_is_inode]) {
-			set_ip_blockmap(ip, 0); /* 0=do not add to dirtree */
-		} else if (id->reftypecount[ref_as_data]) {
-			fsck_blockmap_set(ip, dt->block,
-					  _("reference-repaired data"),
-					  GFS2_BLKST_USED);
-		} else if (id->reftypecount[ref_as_meta]) {
-			if (is_dir(&ip->i_di, sdp->gfs1))
-				fsck_blockmap_set(ip, dt->block,
-						  _("reference-repaired leaf"),
-						  sdp->gfs1 ?
-						  GFS2_BLKST_DINODE :
-						  GFS2_BLKST_USED);
-			else
-				fsck_blockmap_set(ip, dt->block,
-						  _("reference-repaired "
-						    "indirect"), sdp->gfs1 ?
-						  GFS2_BLKST_DINODE :
-						  GFS2_BLKST_USED);
-		} else
-			fsck_blockmap_set(ip, dt->block,
-					  _("reference-repaired extended "
-					    "attribute"),
-					  sdp->gfs1 ? GFS2_BLKST_DINODE :
-					  GFS2_BLKST_USED);
-		fsck_inode_put(&ip); /* out, brelse, free */
-		log_debug(_("Done with duplicate reference to block 0x%llx\n"),
-			  (unsigned long long)dt->block);
-		dup_delete(dt);
+		resolve_last_reference(sdp, dt, acceptable_ref);
 	} else {
 		/* They may have answered no and not fixed all references. */
 		log_debug( _("All duplicate references to block 0x%llx were "
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 06/20] fsck.gfs2: Only preserve the _first_ acceptable inode reference

[Bob Peterson]

Function resolve_dup_references starts out with a loop that traverses
the references for a given duplicate block. If it finds an acceptable
reference, it is supposed to preserve the first reference and resolve
the others. However, the logic allowed for multiple inodes to be
preserved, which can lead to fsck errors that need resolving with yet
another run of fsck.gfs2. This patch adds a check to see if we've
already preserved one, and if so, to not preserve others that might
happen to be acceptable.

rhbz#1257625
---
 gfs2/fsck/pass1b.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 845227e..83a506c 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -114,8 +114,7 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 		if (acceptable_ref != ref_types && /* If we're nuking all but
 						      an acceptable reference
 						      type and */
-		    this_ref == acceptable_ref && /* this ref is acceptable */
-		    !found_good_ref) { /* We haven't found a good reference */
+		    this_ref == acceptable_ref) { /* this ref is acceptable */
 			/* If this is an invalid inode, but not on the invalid
 			   list, it's better to delete it. */
 			if (q == GFS2_BLKST_DINODE) {
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 07/20] fsck.gfs2: Don't just assume the remaining EA reference is good

[Bob Peterson]

When pass1b is resolving duplicate block references, it pares them
down to the last reference, then sets the block type from that
last reference. However, the last reference might be bad too.
For example, consider a block referenced as data by two corrupt inodes
and referenced as an extended attribute by a third corrupt inode.
In pass1b, it might eliminate the two inodes that referenced it as
data, then set the block as extended attribute based on the remaining
reference. However, if the block is not really an extended attribute,
that would be an error that would only be caught on a subsequent run.
This patch adds a check to make sure the remaining reference as an
extended attribute is also valid. If not, the block is freed.

rhbz#1257625
---
 gfs2/fsck/pass1b.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 83a506c..4d16635 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -417,11 +417,40 @@ static void resolve_last_reference(struct gfs2_sbd *sdp, struct duptree *dt,
 					  sdp->gfs1 ? GFS2_BLKST_DINODE :
 					  GFS2_BLKST_USED);
 	} else {
-		fsck_blockmap_set(ip, dt->block,
-				  _("reference-repaired extended "
-				    "attribute"),
-				  sdp->gfs1 ? GFS2_BLKST_DINODE :
-				  GFS2_BLKST_USED);
+		if (acceptable_ref == ref_as_ea)
+			fsck_blockmap_set(ip, dt->block,
+					  _("reference-repaired extended "
+					    "attribute"),
+					  sdp->gfs1 ? GFS2_BLKST_DINODE :
+					  GFS2_BLKST_USED);
+		else {
+			log_err(_("Error: The remaining reference to block "
+				  " %lld (0x%llx) is as extended attribute, "
+				  "in inode %lld (0x%llx) but the block is "
+				  "not an EA.\n"),
+				(unsigned long long)dt->block,
+				(unsigned long long)dt->block,
+				(unsigned long long)id->block_no,
+				(unsigned long long)id->block_no);
+			if (query(_("Okay to remove the bad extended "
+				    "attribute from inode %lld (0x%llx)? "
+				    "(y/n) "),
+				  (unsigned long long)id->block_no,
+				  (unsigned long long)id->block_no)) {
+				ip->i_di.di_eattr = 0;
+				ip->i_di.di_flags &= ~GFS2_DIF_EA_INDIRECT;
+				ip->i_di.di_blocks--;
+				bmodified(ip->i_bh);
+				fsck_blockmap_set(ip, dt->block,
+						  _("reference-repaired EA"),
+						  GFS2_BLKST_FREE);
+				log_err(_("The bad extended attribute was "
+					  "removed.\n"));
+			} else {
+				log_err(_("The bad extended attribute was not "
+					  "removed.\n"));
+			}
+		}
 	}
 	fsck_inode_put(&ip); /* out, brelse, free */
 	log_debug(_("Done with duplicate reference to block 0x%llx\n"),
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 08/20] fsck.gfs2: Don't delete inode for duplicate reference in EA

[Bob Peterson]

Before this patch, when pass1b came across an extended attribute
referenced by multiple inodes, its solution was to delete one of
the inodes entirely. This is rather harsh and unnecessary treatment.
It's enough to just remove the extended attributes and keep the
rest of the file intact. This patch gives pass1b the added ability
to remove only the corrupt extended attribute.

rhbz#1257625
---
 gfs2/fsck/pass1b.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 4d16635..382d5e9 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -147,11 +147,28 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 			  (unsigned long long)dt->block,
 			  (unsigned long long)dt->block,
 			  reftypes[this_ref], reftypes[acceptable_ref]);
-		if (!(query( _("Okay to delete %s inode %lld (0x%llx)? "
-			       "(y/n) "),
-			     (inval ? _("invalidated") : ""),
-			     (unsigned long long)id->block_no,
-			     (unsigned long long)id->block_no))) {
+		if (this_ref == ref_as_ea) {
+			if (!(query( _("Okay to remove extended attributes "
+				       "from %s inode %lld (0x%llx)? (y/n) "),
+				     (inval ? _("invalidated") : ""),
+				     (unsigned long long)id->block_no,
+				     (unsigned long long)id->block_no))) {
+				log_warn( _("The bad EA reference was not "
+					    "cleared."));
+				/* delete the list entry so we don't leak
+				   memory but leave the reference count. If we
+				   decrement the ref count, we could get down
+				   to 1 and the dinode would be changed
+				   without a 'Yes' answer. */
+				/* (dh->ref_inode_count)--;*/
+				dup_listent_delete(dt, id);
+				continue;
+			}
+		} else if (!(query( _("Okay to delete %s inode %lld (0x%llx)? "
+				      "(y/n) "),
+				    (inval ? _("invalidated") : ""),
+				    (unsigned long long)id->block_no,
+				    (unsigned long long)id->block_no))) {
 			log_warn( _("The bad inode was not cleared."));
 			/* delete the list entry so we don't leak memory but
 			   leave the reference count. If we decrement the
@@ -166,6 +183,11 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 				    "deleted.\n"),
 				  (unsigned long long)id->block_no,
 				  (unsigned long long)id->block_no);
+		else if (this_ref == ref_as_ea)
+			log_warn(_("Pass1b is removing extended attributes "
+				   "from inode %lld (0x%llx).\n"),
+				 (unsigned long long)id->block_no,
+				 (unsigned long long)id->block_no);
 		else
 			log_warn(_("Pass1b is deleting inode %lld (0x%llx).\n"),
 				 (unsigned long long)id->block_no,
@@ -180,8 +202,9 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 			check_inode_eattr(ip, &pass1b_fxns_delete);
 			/* If the reference was as metadata or data, we've got
 			   a corrupt dinode that will be deleted. */
-			if (inval || id->reftypecount[ref_as_data] ||
-			    id->reftypecount[ref_as_meta]) {
+			if ((this_ref != ref_as_ea) &&
+			    (inval || id->reftypecount[ref_as_data] ||
+			     id->reftypecount[ref_as_meta])) {
 				/* Remove the inode from the inode tree */
 				ii = inodetree_find(ip->i_di.di_num.no_addr);
 				if (ii)
@@ -204,7 +227,7 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 				check_metatree(ip, &pass1b_fxns_delete);
 			}
 		}
-		/* Now we've got to go through an delete any other duplicate
+		/* Now we've got to go through and delete any other duplicate
 		   references from this dinode we're deleting. If we don't,
 		   pass1b will discover the other duplicate record, try to
 		   delete this dinode a second time, and this time its earlier
@@ -212,7 +235,8 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 		   (because they were eliminated earlier in pass1b). And so
 		   the blocks will be mistakenly freed, when, in fact, they're
 		   still being referenced by a valid dinode. */
-		delete_all_dups(ip);
+		if (this_ref != ref_as_ea)
+			delete_all_dups(ip);
 		fsck_inode_put(&ip); /* out, brelse, free */
 	}
 	return;
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 09/20] fsck.gfs2: Don't traverse EAs that belong to another inode

[Bob Peterson]

Before this patch, pass1b would find a duplicate reference to
extended attributes, which may be an entire tree structure. After
deciding which inode gets to keep the extended attributes, it
removes the extended attributes from other dinodes with another
pointer to it. However, if the EAs are big, and there's a whole
list of them as indirect blocks, we only want to traverse the
indirect list if that list is owned by the second (bad) reference.

If we do traverse the tree, we're basically freeing EA blocks
belonging to the only remaining valid reference, and that's wrong.

This patch adds a check to function resolve_dup_references that
avoids a whole EA tree traversal if the duplicate EA reference is
the root of the EAs (not an indirect list of blocks). Instead,
the reference to the block is just removed from the inode.

rhbz#1257625
---
 gfs2/fsck/pass1b.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 382d5e9..4c41fef 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -198,8 +198,27 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 		   it again. That could free blocks that used to be duplicate
 		   references that are now resolved (and gone). */
 		if (q != GFS2_BLKST_FREE) {
-			/* Clear the EAs for the inode first */
-			check_inode_eattr(ip, &pass1b_fxns_delete);
+			/* If the inode's eattr pointer is to the duplicate
+			   ref block, we don't want to call check_inode_eattr
+			   because that would traverse the structure, and it's
+			   not ours to do anymore; it rightly belongs to a
+			   different dinode. On the other hand, if the dup
+			   block is buried deep within the eattr structure
+			   of this dinode, we need to traverse the structure
+			   because it IS ours, and we need to remove all the
+			   eattr leaf blocks: they do belong to us (except for
+			   the duplicate referenced one, which is handled). */
+			if (ip->i_di.di_eattr == dt->block) {
+				ip->i_di.di_eattr = 0;
+				if (ip->i_di.di_blocks > 0)
+					ip->i_di.di_blocks--;
+				ip->i_di.di_flags &= ~GFS2_DIF_EA_INDIRECT;
+				bmodified(ip->i_bh);
+				dup_listent_delete(dt, id);
+			} else {
+				/* Clear the EAs for the inode first */
+				check_inode_eattr(ip, &pass1b_fxns_delete);
+			}
 			/* If the reference was as metadata or data, we've got
 			   a corrupt dinode that will be deleted. */
 			if ((this_ref != ref_as_ea) &&
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 10/20] fsck.gfs2: Refactor function check_indirect_eattr

[Bob Peterson]

Function check_indirect_eattr was messy because the if statements
were nested too deep and the function was too long. This patch moves
some of its function into the smaller calling function, function
check_inode_eattr. The functionality should remain unchanged.

rhbz#1257625
---
 gfs2/fsck/metawalk.c | 141 +++++++++++++++++++++++++--------------------------
 1 file changed, 68 insertions(+), 73 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index cf3231f..fc215e4 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1078,89 +1078,71 @@ static int delete_block_if_notdup(struct gfs2_inode *ip, uint64_t block,
  * Returns: 0 on success -1 on error
  */
 static int check_indirect_eattr(struct gfs2_inode *ip, uint64_t indirect,
+				struct gfs2_buffer_head *indirect_buf,
 				struct metawalk_fxns *pass)
 {
 	int error = 0;
 	uint64_t *ea_leaf_ptr, *end;
 	uint64_t block;
-	struct gfs2_buffer_head *indirect_buf = NULL;
 	struct gfs2_sbd *sdp = ip->i_sbd;
 	int first_ea_is_bad = 0;
 	uint64_t di_eattr_save = ip->i_di.di_eattr;
 	uint64_t offset = ip->i_sbd->gfs1 ? sizeof(struct gfs_indirect) : sizeof(struct gfs2_meta_header);
+	int leaf_pointers = 0, leaf_pointer_errors = 0;
 
-	log_debug( _("Checking EA indirect block #%llu (0x%llx).\n"),
-		  (unsigned long long)indirect,
-		  (unsigned long long)indirect);
+	ea_leaf_ptr = (uint64_t *)(indirect_buf->b_data + offset);
+	end = ea_leaf_ptr + ((sdp->sd_sb.sb_bsize - offset) / 8);
 
-	if (!pass->check_eattr_indir)
-		return 0;
-	error = pass->check_eattr_indir(ip, indirect, ip->i_di.di_num.no_addr,
-					&indirect_buf, pass->private);
-	if (!error) {
-		int leaf_pointers = 0, leaf_pointer_errors = 0;
-
-		ea_leaf_ptr = (uint64_t *)(indirect_buf->b_data + offset);
-		end = ea_leaf_ptr + ((sdp->sd_sb.sb_bsize - offset) / 8);
-
-		while (*ea_leaf_ptr && (ea_leaf_ptr < end)){
-			block = be64_to_cpu(*ea_leaf_ptr);
-			leaf_pointers++;
-			error = check_leaf_eattr(ip, block, indirect, pass);
-			if (error) {
-				leaf_pointer_errors++;
-				if (query( _("Fix the indirect "
-					     "block too? (y/n) ")))
-					*ea_leaf_ptr = 0;
-			}
-			/* If the first eattr lead is bad, we can't have
-			   a hole, so we have to treat this as an unrecoverable
-			   eattr error and delete all eattr info. Calling
-			   finish_eattr_indir here causes ip->i_di.di_eattr = 0
-			   and that ensures that subsequent calls to
-			   check_leaf_eattr result in the eattr
-			   check_leaf_block nuking them all "due to previous
-			   errors" */
-			if (leaf_pointers == 1 && leaf_pointer_errors == 1) {
-				first_ea_is_bad = 1;
-				if (pass->finish_eattr_indir)
-					pass->finish_eattr_indir(ip,
-							leaf_pointers,
-							leaf_pointer_errors,
-							pass->private);
-			} else if (leaf_pointer_errors) {
-				/* This is a bit tricky.  We can't have eattr
-				   holes. So if we have 4 good eattrs, 1 bad
-				   eattr and 5 more good ones: GGGGBGGGGG,
-				   we need to tell check_leaf_eattr to delete
-				   all eattrs after the bad one.  So we want:
-				   GGGG when we finish.  To do that, we set
-				   di_eattr to 0 temporarily. */
-				ip->i_di.di_eattr = 0;
-				bmodified(ip->i_bh);
-			}
-			ea_leaf_ptr++;
+	while (*ea_leaf_ptr && (ea_leaf_ptr < end)){
+		block = be64_to_cpu(*ea_leaf_ptr);
+		leaf_pointers++;
+		error = check_leaf_eattr(ip, block, indirect, pass);
+		if (error) {
+			leaf_pointer_errors++;
+			if (query( _("Fix the indirect block too? (y/n) ")))
+				*ea_leaf_ptr = 0;
 		}
-		if (pass->finish_eattr_indir) {
-			if (!first_ea_is_bad) {
-				/* If the first ea is good but subsequent ones
-				   were bad and deleted, we need to restore
-				   the saved di_eattr block. */
-				if (leaf_pointer_errors)
-					ip->i_di.di_eattr = di_eattr_save;
+		/* If the first eattr lead is bad, we can't have a hole, so we
+		   have to treat this as an unrecoverable eattr error and
+		   delete all eattr info. Calling finish_eattr_indir here
+		   causes ip->i_di.di_eattr = 0 and that ensures that
+		   subsequent calls to check_leaf_eattr result in the eattr
+		   check_leaf_block nuking them all "due to previous errors" */
+		if (leaf_pointers == 1 && leaf_pointer_errors == 1) {
+			first_ea_is_bad = 1;
+			if (pass->finish_eattr_indir)
 				pass->finish_eattr_indir(ip, leaf_pointers,
 							 leaf_pointer_errors,
 							 pass->private);
-			}
-			if (leaf_pointer_errors &&
-			    leaf_pointer_errors == leaf_pointers) {
-				delete_block(ip, indirect, NULL, "leaf", NULL);
-				error = 1;
-			}
+		} else if (leaf_pointer_errors) {
+			/* This is a bit tricky.  We can't have eattr holes.
+			   So if we have 4 good eattrs, 1 bad eattr and 5 more
+			   good ones: GGGGBGGGGG, we need to tell
+			   check_leaf_eattr to delete all eattrs after the bad
+			   one. So we want: GGGG when we finish. To do that,
+			   we set di_eattr to 0 temporarily. */
+			ip->i_di.di_eattr = 0;
+			bmodified(ip->i_bh);
+		}
+		ea_leaf_ptr++;
+	}
+	if (pass->finish_eattr_indir) {
+		if (!first_ea_is_bad) {
+			/* If the first ea is good but subsequent ones were
+			   bad and deleted, we need to restore the saved
+			   di_eattr block. */
+			if (leaf_pointer_errors)
+				ip->i_di.di_eattr = di_eattr_save;
+			pass->finish_eattr_indir(ip, leaf_pointers,
+						 leaf_pointer_errors,
+						 pass->private);
+		}
+		if (leaf_pointer_errors &&
+		    leaf_pointer_errors == leaf_pointers) {
+			delete_block(ip, indirect, NULL, "leaf", NULL);
+			error = 1;
 		}
 	}
-	if (indirect_buf)
-		brelse(indirect_buf);
 
 	return error;
 }
@@ -1174,25 +1156,38 @@ static int check_indirect_eattr(struct gfs2_inode *ip, uint64_t indirect,
 int check_inode_eattr(struct gfs2_inode *ip, struct metawalk_fxns *pass)
 {
 	int error = 0;
+	struct gfs2_buffer_head *indirect_buf = NULL;
 
 	if (!ip->i_di.di_eattr)
 		return 0;
 
 	if (ip->i_di.di_flags & GFS2_DIF_EA_INDIRECT){
+		if (!pass->check_eattr_indir)
+			return 0;
+
 		log_debug( _("Checking EA indirect block #%llu (0x%llx) for "
 			     "inode #%llu (0x%llx)..\n"),
 			   (unsigned long long)ip->i_di.di_eattr,
 			   (unsigned long long)ip->i_di.di_eattr,
 			   (unsigned long long)ip->i_di.di_num.no_addr,
 			   (unsigned long long)ip->i_di.di_num.no_addr);
-		if ((error = check_indirect_eattr(ip, ip->i_di.di_eattr, pass)))
-			stack;
-	} else {
-		error = check_leaf_eattr(ip, ip->i_di.di_eattr,
-					 ip->i_di.di_num.no_addr, pass);
-		if (error)
-			stack;
+		error = pass->check_eattr_indir(ip, ip->i_di.di_eattr,
+						ip->i_di.di_num.no_addr,
+						&indirect_buf, pass->private);
+		if (!error) {
+			error = check_indirect_eattr(ip, ip->i_di.di_eattr,
+						     indirect_buf, pass);
+			if (error)
+				stack;
+		}
+		if (indirect_buf)
+			brelse(indirect_buf);
+		return error;
 	}
+	error = check_leaf_eattr(ip, ip->i_di.di_eattr,
+				 ip->i_di.di_num.no_addr, pass);
+	if (error)
+		stack;
 
 	return error;
 }
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 11/20] fsck.gfs2: Once an indirect ea error is found, flag all that follow

[Bob Peterson]

Before this patch, function check_indirect_eattr could find an error
with an indirect extended attribute, but it would ignore subsequent
EA errors and return a good return code. That's bad. Once you find
an EA error in an indirect list, all the ones that follow should be
flagged as errors and subsequently cleared. The return code should
also reflect that an error was found during the processing. This
patch adds a second "err" variable to keep track of whether new errors
were found as opposed to whether _any_ errors were found in the list.
It also prints new error messages where the error was found and on all
pointers following the original error.

rhbz#1257625
---
 gfs2/fsck/metawalk.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index fc215e4..dfb6c1a 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1081,7 +1081,7 @@ static int check_indirect_eattr(struct gfs2_inode *ip, uint64_t indirect,
 				struct gfs2_buffer_head *indirect_buf,
 				struct metawalk_fxns *pass)
 {
-	int error = 0;
+	int error = 0, err;
 	uint64_t *ea_leaf_ptr, *end;
 	uint64_t block;
 	struct gfs2_sbd *sdp = ip->i_sbd;
@@ -1096,11 +1096,29 @@ static int check_indirect_eattr(struct gfs2_inode *ip, uint64_t indirect,
 	while (*ea_leaf_ptr && (ea_leaf_ptr < end)){
 		block = be64_to_cpu(*ea_leaf_ptr);
 		leaf_pointers++;
-		error = check_leaf_eattr(ip, block, indirect, pass);
-		if (error) {
+		err = check_leaf_eattr(ip, block, indirect, pass);
+		if (err) {
+			error = err;
+			log_err(_("Error detected in leaf block %lld (0x%llx) "
+				  "referenced by indirect block %lld (0x%llx)"
+				  ".\n"),
+				(unsigned long long)block,
+				(unsigned long long)block,
+				(unsigned long long)indirect,
+				(unsigned long long)indirect);
+			log_err(_("Subsequent leaf block pointers should be "
+				  "cleared.\n"));
+		}
+		if (error) { /* leaf blocks following an error must also be
+				treated as error blocks and cleared. */
 			leaf_pointer_errors++;
-			if (query( _("Fix the indirect block too? (y/n) ")))
-				*ea_leaf_ptr = 0;
+			log_err(_("Pointer to EA leaf block %lld (0x%llx) in "
+				  "indirect block %lld (0x%llx) should be "
+				  "cleared.\n"),
+				(unsigned long long)block,
+				(unsigned long long)block,
+				(unsigned long long)indirect,
+				(unsigned long long)indirect);
 		}
 		/* If the first eattr lead is bad, we can't have a hole, so we
 		   have to treat this as an unrecoverable eattr error and
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 12/20] fsck.gfs2: Always restore saved value for di_eattr

[Bob Peterson]

There are times when function check_indirect_eattr saves off the value
of di_eattr in order to properly process the extended attributes.
The saved value must always be restored, if it exists, otherwise
we can accidentally remove an inode's EA reference.

rhbz#1257625
---
 gfs2/fsck/metawalk.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index dfb6c1a..6fb5c56 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1144,13 +1144,12 @@ static int check_indirect_eattr(struct gfs2_inode *ip, uint64_t indirect,
 		}
 		ea_leaf_ptr++;
 	}
+	/* If we temporarily nuked the ea block to prevent checking past
+	   a corrupt ea leaf, we need to restore the saved di_eattr block. */
+	if (di_eattr_save != 0)
+		ip->i_di.di_eattr = di_eattr_save;
 	if (pass->finish_eattr_indir) {
 		if (!first_ea_is_bad) {
-			/* If the first ea is good but subsequent ones were
-			   bad and deleted, we need to restore the saved
-			   di_eattr block. */
-			if (leaf_pointer_errors)
-				ip->i_di.di_eattr = di_eattr_save;
 			pass->finish_eattr_indir(ip, leaf_pointers,
 						 leaf_pointer_errors,
 						 pass->private);
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 13/20] fsck.gfs2: Remove redundancy in add_duplicate_ref

[Bob Peterson]

Before this patch, when a duplicate reference was added to the list,
function add_duplicate_ref would print out an info message that said:
This brings the total to: X duplicate references.
I found this message to be redundant and confusing. It's much more
clear to simply say:
This brings the total to: X references

rhbz#1257625
---
 gfs2/fsck/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
index a6a5cdc..b22abc6 100644
--- a/gfs2/fsck/util.c
+++ b/gfs2/fsck/util.c
@@ -402,8 +402,8 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
 	if (first)
 		log_info( _("This is the original reference.\n"));
 	else
-		log_info( _("This brings the total to: %d duplicate "
-			    "references\n"), dt->refs);
+		log_info( _("This brings the total to: %d references\n"),
+			  dt->refs);
 	return meta_is_good;
 }
 
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 14/20] fsck.gfs2: Don't remove duplicate eattr blocks

[Bob Peterson]

Before this patch, if two dinodes referenced the same block, one of
them in the extended attributes, it would delete the extended
attributes. That's wrong because the other dinode referencing the
block might be a valid reference, and the block should not be
deleted out from under it. This patch changes the return code from
1 to 0 in this case, which causes pass1b to resolve whichever
reference is proper and delete or not delete the other reference
as appropriate. It also splits the clear_eas function into a
separate function to complain about the problem; that way the
duplicate errors above are still reported.

rhbz#1257625
---
 gfs2/fsck/pass1.c | 76 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index c0f2f1e..a554845 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -585,6 +585,24 @@ static int ask_remove_inode_eattr(struct gfs2_inode *ip,
 	return 0;
 }
 
+/* complain_eas - complain about extended attribute errors for an inode
+ *
+ * @ip       - in core inode pointer
+ * block     - the block that had the problem
+ * duplicate - if this is a duplicate block, don't set it "free"
+ * emsg      - what to tell the user about the eas being checked
+ * Returns: 1 if the EA is fixed, else 0 if it was not fixed.
+ */
+static void complain_eas(struct gfs2_inode *ip, uint64_t block,
+			 const char *emsg)
+{
+	log_err(_("Inode #%llu (0x%llx): %s"),
+		(unsigned long long)ip->i_di.di_num.no_addr,
+		(unsigned long long)ip->i_di.di_num.no_addr, emsg);
+	log_err(_(" at block #%lld (0x%llx).\n"),
+		 (unsigned long long)block, (unsigned long long)block);
+}
+
 /* clear_eas - clear the extended attributes for an inode
  *
  * @ip       - in core inode pointer
@@ -597,11 +615,7 @@ static int ask_remove_inode_eattr(struct gfs2_inode *ip,
 static int clear_eas(struct gfs2_inode *ip, struct block_count *bc,
 		     uint64_t block, int duplicate, const char *emsg)
 {
-	log_err( _("Inode #%llu (0x%llx): %s"),
-		(unsigned long long)ip->i_di.di_num.no_addr,
-		(unsigned long long)ip->i_di.di_num.no_addr, emsg);
-	log_err( _(" at block #%lld (0x%llx).\n"),
-		 (unsigned long long)block, (unsigned long long)block);
+	complain_eas(ip, block, emsg);
 	if (query( _("Clear the bad Extended Attribute? (y/n) "))) {
 		if (block == ip->i_di.di_eattr) {
 			remove_inode_eattr(ip, bc);
@@ -646,11 +660,15 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
 		if (q != GFS2_BLKST_FREE) { /* Duplicate? */
 			add_duplicate_ref(ip, indirect, ref_as_ea, 0,
 					  INODE_VALID);
-			if (!clear_eas(ip, bc, indirect, 1,
-				       _("Bad indirect Extended Attribute "
-					 "duplicate found")))
-				bc->ea_count++;
-			return 1;
+			complain_eas(ip, indirect,
+				     _("Bad indirect Extended Attribute "
+				       "duplicate found"));
+			bc->ea_count++;
+			/* Return 0 here because if all that's wrong is a
+			   duplicate block reference, we want pass1b to figure
+			   it out. We don't want to delete all the extended
+			   attributes as if they are in error. */
+			return 0;
 		}
 		clear_eas(ip, bc, indirect, 0,
 			  _("Extended Attribute indirect block has incorrect "
@@ -658,16 +676,11 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
 		return 1;
 	}
 	if (q != GFS2_BLKST_FREE) { /* Duplicate? */
-		log_err( _("Inode #%llu (0x%llx): Duplicate Extended "
-			   "Attribute indirect block found at #%llu "
-			   "(0x%llx).\n"),
-			 (unsigned long long)ip->i_di.di_num.no_addr,
-			 (unsigned long long)ip->i_di.di_num.no_addr,
-			 (unsigned long long)indirect,
-			 (unsigned long long)indirect);
 		add_duplicate_ref(ip, indirect, ref_as_ea, 0, INODE_VALID);
+		complain_eas(ip, indirect,
+			     _("Duplicate Extended Attribute indirect block"));
 		bc->ea_count++;
-		ret = 1;
+		ret = 0; /* For the same reason stated above. */
 	} else {
 		fsck_blockmap_set(ip, indirect,
 				  _("indirect Extended Attribute"), sdp->gfs1 ?
@@ -740,27 +753,34 @@ static int check_ealeaf_block(struct gfs2_inode *ip, uint64_t block, int btype,
 	   If it isn't, clear it but don't count it as a duplicate. */
 	leaf_bh = bread(sdp, block);
 	if (gfs2_check_meta(leaf_bh, btype)) {
+		bc->ea_count++;
 		if (q != GFS2_BLKST_FREE) { /* Duplicate? */
 			add_duplicate_ref(ip, block, ref_as_ea, 0,
 					  INODE_VALID);
-			clear_eas(ip, bc, block, 1,
-				  _("Bad Extended Attribute duplicate found"));
-		} else {
-			clear_eas(ip, bc, block, 0,
-				  _("Extended Attribute leaf block "
-				    "has incorrect type"));
+			complain_eas(ip, block, _("Extended attribute leaf "
+						  "duplicate found"));
+			/* Return 0 here because if all that's wrong is a
+			   duplicate block reference, we want pass1b to figure
+			   it out. We don't want to delete all the extended
+			   attributes as if they are in error. */
+			return 0;
 		}
+		clear_eas(ip, bc, block, 0, _("Extended Attribute leaf block "
+					      "has incorrect type"));
 		brelse(leaf_bh);
 		return 1;
 	}
 	if (q != GFS2_BLKST_FREE) { /* Duplicate? */
-		log_debug( _("Duplicate block found at #%lld (0x%llx).\n"),
-			   (unsigned long long)block,
-			   (unsigned long long)block);
+		complain_eas(ip, block, _("Extended Attribute leaf "
+					  "duplicate found"));
 		add_duplicate_ref(ip, block, ref_as_data, 0, INODE_VALID);
 		bc->ea_count++;
 		brelse(leaf_bh);
-		return 1;
+		/* Return 0 here because if all that's wrong is a duplicate
+		   block reference, we want pass1b to figure it out. We don't
+		   want to delete all the extended attributes as if they are
+		   in error. */
+		return 0;
 	}
 	if (ip->i_di.di_eattr == 0) {
 		/* Can only get in here if there were unrecoverable ea
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 15/20] fsck.gfs2: Refactor check_eattr_entries and add error messages

[Bob Peterson]

This patch adds a couple error messages to document when extended
attributes have errors in function check_eattr_entries. It also
refactors some of the code to make it more readable.

rhbz#1257625
---
 gfs2/fsck/pass1.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index a554845..aa31815 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -891,6 +891,8 @@ static int check_eattr_entries(struct gfs2_inode *ip,
 {
 	struct gfs2_sbd *sdp = ip->i_sbd;
 	char ea_name[256];
+	uint32_t avail_size;
+	int max_ptrs;
 
 	if (!ea_hdr->ea_name_len){
 		/* Skip this entry for now */
@@ -904,22 +906,26 @@ static int check_eattr_entries(struct gfs2_inode *ip,
 	if (!GFS2_EATYPE_VALID(ea_hdr->ea_type) &&
 	   ((ea_hdr_prev) || (!ea_hdr_prev && ea_hdr->ea_type))){
 		/* Skip invalid entry */
+		log_err(_("EA (%s) type is invalid (%d > %d).\n"),
+			ea_name, ea_hdr->ea_type, GFS2_EATYPE_LAST);
 		return 1;
 	}
 
-	if (ea_hdr->ea_num_ptrs){
-		uint32_t avail_size;
-		int max_ptrs;
+	if (!ea_hdr->ea_num_ptrs)
+		return 0;
 
-		avail_size = sdp->sd_sb.sb_bsize - sizeof(struct gfs2_meta_header);
-		max_ptrs = (be32_to_cpu(ea_hdr->ea_data_len)+avail_size-1)/avail_size;
+	avail_size = sdp->sd_sb.sb_bsize - sizeof(struct gfs2_meta_header);
+	max_ptrs = (be32_to_cpu(ea_hdr->ea_data_len)+avail_size-1)/avail_size;
 
-		if (max_ptrs > ea_hdr->ea_num_ptrs) {
-			return 1;
-		} else {
-			log_debug( _("  Pointers Required: %d\n  Pointers Reported: %d\n"),
-				  max_ptrs, ea_hdr->ea_num_ptrs);
-		}
+	if (max_ptrs > ea_hdr->ea_num_ptrs) {
+		log_err(_("EA (%s) has incorrect number of pointers.\n"),
+			ea_name);
+		log_err(_("  Required:  %d\n  Reported:  %d\n"),
+			max_ptrs, ea_hdr->ea_num_ptrs);
+		return 1;
+	} else {
+		log_debug( _("  Pointers Required: %d\n  Pointers Reported: %d\n"),
+			   max_ptrs, ea_hdr->ea_num_ptrs);
 	}
 	return 0;
 }
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 16/20] fsck.gfs2: remove bad EAs at the end, not as-you-go

[Bob Peterson]

Before this patch, pass1 would check extended attributes and if they
were found to be bad, it would delete them as it found them. This
is the wrong approach and only gets us into trouble. The proper
approach is to traverse the tree and see if there are problems, and
if so, delete them at the end. This patch:
1. Removes function clear_eas() which was delete-as-you-go.
2. Instead, the errors are complained about but the traversal goes on.
3. Adds a new tree traversal, eattr_undo_fxns, which is used to
   properly remove the corrupt extended attributes at the end of
   the previous tree traversal.

rhbz#1257625
---
 gfs2/fsck/pass1.c | 107 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 58 insertions(+), 49 deletions(-)

diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index aa31815..a34e9e4 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -102,7 +102,6 @@ struct metawalk_fxns pass1_fxns = {
 	.check_dentry = NULL,
 	.check_eattr_entry = check_eattr_entries,
 	.check_eattr_extentry = check_extended_leaf_eattr,
-	.finish_eattr_indir = finish_eattr_indir,
 	.big_file_msg = big_file_comfort,
 	.repair_leaf = pass1_repair_leaf,
 	.undo_check_meta = undo_check_metalist,
@@ -585,6 +584,36 @@ static int ask_remove_inode_eattr(struct gfs2_inode *ip,
 	return 0;
 }
 
+static int undo_eattr_indir_or_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;
+	uint8_t q;
+	int error;
+	struct block_count *bc = (struct block_count *) private;
+
+	if (!valid_block(ip->i_sbd, block))
+		return meta_error;
+
+	/* Need to check block_type before undoing the reference, which can
+	   set it to free, which would cause the test below to fail. */
+	q = block_type(block);
+
+	error = undo_reference(ip, block, 0, private);
+	if (error)
+		return error;
+
+	bc->ea_count--;
+
+	if (q != (sdp->gfs1 ? GFS2_BLKST_DINODE : GFS2_BLKST_USED))
+		return 1;
+
+	*bh = bread(sdp, block);
+	return 0;
+}
+
 /* complain_eas - complain about extended attribute errors for an inode
  *
  * @ip       - in core inode pointer
@@ -603,36 +632,6 @@ static void complain_eas(struct gfs2_inode *ip, uint64_t block,
 		 (unsigned long long)block, (unsigned long long)block);
 }
 
-/* clear_eas - clear the extended attributes for an inode
- *
- * @ip       - in core inode pointer
- * @bc       - pointer to a block count structure
- * block     - the block that had the problem
- * duplicate - if this is a duplicate block, don't set it "free"
- * emsg      - what to tell the user about the eas being checked
- * Returns: 1 if the EA is fixed, else 0 if it was not fixed.
- */
-static int clear_eas(struct gfs2_inode *ip, struct block_count *bc,
-		     uint64_t block, int duplicate, const char *emsg)
-{
-	complain_eas(ip, block, emsg);
-	if (query( _("Clear the bad Extended Attribute? (y/n) "))) {
-		if (block == ip->i_di.di_eattr) {
-			remove_inode_eattr(ip, bc);
-			log_err( _("The bad extended attribute was "
-				   "removed.\n"));
-		} else if (!duplicate) {
-			delete_block(ip, block, NULL,
-				     _("bad extended attribute"), NULL);
-		}
-		return 1;
-	} else {
-		log_err( _("The bad Extended Attribute was not fixed.\n"));
-		bc->ea_count++;
-		return 0;
-	}
-}
-
 static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
 			     uint64_t parent, struct gfs2_buffer_head **bh,
 			     void *private)
@@ -657,22 +656,22 @@ static int check_eattr_indir(struct gfs2_inode *ip, uint64_t indirect,
 	   count it as a duplicate. */
 	*bh = bread(sdp, indirect);
 	if (gfs2_check_meta(*bh, GFS2_METATYPE_IN)) {
+		bc->ea_count++;
 		if (q != GFS2_BLKST_FREE) { /* Duplicate? */
 			add_duplicate_ref(ip, indirect, ref_as_ea, 0,
 					  INODE_VALID);
 			complain_eas(ip, indirect,
 				     _("Bad indirect Extended Attribute "
 				       "duplicate found"));
-			bc->ea_count++;
 			/* Return 0 here because if all that's wrong is a
 			   duplicate block reference, we want pass1b to figure
 			   it out. We don't want to delete all the extended
 			   attributes as if they are in error. */
 			return 0;
 		}
-		clear_eas(ip, bc, indirect, 0,
-			  _("Extended Attribute indirect block has incorrect "
-			    "type"));
+		complain_eas(ip, indirect,
+			     _("Extended Attribute indirect block has "
+			       "incorrect type"));
 		return 1;
 	}
 	if (q != GFS2_BLKST_FREE) { /* Duplicate? */
@@ -765,8 +764,8 @@ static int check_ealeaf_block(struct gfs2_inode *ip, uint64_t block, int btype,
 			   attributes as if they are in error. */
 			return 0;
 		}
-		clear_eas(ip, bc, block, 0, _("Extended Attribute leaf block "
-					      "has incorrect type"));
+		complain_eas(ip, block, _("Extended Attribute leaf block has "
+					  "incorrect type"));
 		brelse(leaf_bh);
 		return 1;
 	}
@@ -782,16 +781,6 @@ static int check_ealeaf_block(struct gfs2_inode *ip, uint64_t block, int btype,
 		   in error. */
 		return 0;
 	}
-	if (ip->i_di.di_eattr == 0) {
-		/* Can only get in here if there were unrecoverable ea
-		   errors that caused clear_eas to be called.  What we
-		   need to do here is remove the subsequent ea blocks. */
-		clear_eas(ip, bc, block, 0,
-			  _("Extended Attribute block removed due to "
-			    "previous errors.\n"));
-		brelse(leaf_bh);
-		return 1;
-	}
 	/* Point of confusion: We've got to set the ea block itself to
 	   GFS2_BLKST_USED here.  Elsewhere we mark the inode with
 	   gfs2_eattr_block meaning it contains an eattr for pass1c. */
@@ -1129,6 +1118,13 @@ struct metawalk_fxns rangecheck_fxns = {
         .check_eattr_leaf = rangecheck_eattr_leaf,
 };
 
+struct metawalk_fxns eattr_undo_fxns = {
+	.private = NULL,
+	.check_eattr_indir = undo_eattr_indir_or_leaf,
+	.check_eattr_leaf = undo_eattr_indir_or_leaf,
+	.finish_eattr_indir = finish_eattr_indir,
+};
+
 /*
  * handle_ip - process an incore structure representing a dinode.
  */
@@ -1195,9 +1191,22 @@ static int handle_ip(struct gfs2_sbd *sdp, struct gfs2_inode *ip)
 	if (!error) {
 		error = check_inode_eattr(ip, &pass1_fxns);
 
-		if (error &&
-		    !(ip->i_di.di_flags & GFS2_DIF_EA_INDIRECT))
+		if (error) {
+			if (!query(_("Clear the bad Extended Attributes? "
+				    "(y/n) "))) {
+				log_err( _("The bad Extended Attributes were "
+					   "not fixed.\n"));
+				return 0;
+			}
+			log_err(_("Clearing the bad Extended Attributes in "
+				  "inode %lld (0x%llx).\n"),
+				(unsigned long long)ip->i_di.di_num.no_addr,
+				(unsigned long long)ip->i_di.di_num.no_addr);
+			eattr_undo_fxns.private = &bc;
+			check_inode_eattr(ip, &eattr_undo_fxns);
 			ask_remove_inode_eattr(ip, &bc);
+			return 1;
+		}
 	}
 
 	if (ip->i_di.di_blocks != 
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 17/20] fsck.gfs2: Combine remove_inode_eattr with its only caller

[Bob Peterson]

Since remove_inode_eattr() is now only called from one place,
it is combined with its caller to make the code more readable
and less confusing: function ask_remove_inode_eattr.

rhbz#1257625
---
 gfs2/fsck/pass1.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index a34e9e4..f95e7eb 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -553,17 +553,6 @@ static int check_data(struct gfs2_inode *ip, uint64_t metablock,
 	return 0;
 }
 
-static int remove_inode_eattr(struct gfs2_inode *ip, struct block_count *bc)
-{
-	undo_reference(ip, ip->i_di.di_eattr, 0, bc);
-	ip->i_di.di_eattr = 0;
-	bc->ea_count = 0;
-	ip->i_di.di_blocks = 1 + bc->indir_count + bc->data_count;
-	ip->i_di.di_flags &= ~GFS2_DIF_EA_INDIRECT;
-	bmodified(ip->i_bh);
-	return 0;
-}
-
 static int ask_remove_inode_eattr(struct gfs2_inode *ip,
 				  struct block_count *bc)
 {
@@ -573,11 +562,13 @@ static int ask_remove_inode_eattr(struct gfs2_inode *ip,
 		   "errors.\n"), (unsigned long long)ip->i_di.di_num.no_addr,
 		 (unsigned long long)ip->i_di.di_num.no_addr);
 	if (query( _("Clear all Extended Attributes from the inode? (y/n) "))){
-		if (!remove_inode_eattr(ip, bc))
-			log_err( _("Extended attributes were removed.\n"));
-		else
-			log_err( _("Unable to remove inode eattr pointer; "
-				   "the error remains.\n"));
+		undo_reference(ip, ip->i_di.di_eattr, 0, bc);
+		ip->i_di.di_eattr = 0;
+		bc->ea_count = 0;
+		ip->i_di.di_blocks = 1 + bc->indir_count + bc->data_count;
+		ip->i_di.di_flags &= ~GFS2_DIF_EA_INDIRECT;
+		bmodified(ip->i_bh);
+		log_err( _("Extended attributes were removed.\n"));
 	} else {
 		log_err( _("Extended attributes were not removed.\n"));
 	}
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 18/20] fsck.gfs2: Print debug message to dilineate metadata blocks

[Bob Peterson]

Before this patch, when the data blocks were checked for a huge
file, there was no way to distinguish which metadata block had the
reference to the data block. This patch adds a new debug message
to announce which metadata block is being processed for the next
set of data blocks being analyzed.

rhbz#1257625
---
 gfs2/fsck/metawalk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index 6fb5c56..5efbdd3 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1454,6 +1454,10 @@ static int check_data(struct gfs2_inode *ip, struct metawalk_fxns *pass,
 	uint64_t metablock = bh->b_blocknr;
 
 	/* If there isn't much pointer corruption check the pointers */
+	log_debug(_("\nProcessing data blocks for inode 0x%llx, metadata "
+		    "block 0x%llx.\n"),
+		  (unsigned long long)ip->i_di.di_num.no_addr,
+		  (unsigned long long)bh->b_blocknr);
 	for (ptr = ptr_start ; (char *)ptr < ptr_end && !fsck_abort; ptr++) {
 		if (!*ptr)
 			continue;
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 19/20] fsck.gfs2: Remove pass1c in favor of processing in pass1

[Bob Peterson]

This patch eliminates pass1c and moves its checks to pass1. In
pass1c, it used to delete bad extended attribute blocks, but in
pass1, we have to go about things a little differently. Since
pass1 is prior to resolving duplicate references (pass1b) it
must "undo" the references rather than delete them.

rhbz#1257625
---
 gfs2/fsck/Makefile.am  |   1 -
 gfs2/fsck/main.c       |   1 -
 gfs2/fsck/pass1.c      |  98 ++++++++++-------
 gfs2/fsck/pass1c.c     | 285 -------------------------------------------------
 gfs2/fsck/util.c       |   2 -
 gfs2/libgfs2/libgfs2.h |   2 -
 6 files changed, 62 insertions(+), 327 deletions(-)
 delete mode 100644 gfs2/fsck/pass1c.c

diff --git a/gfs2/fsck/Makefile.am b/gfs2/fsck/Makefile.am
index fb93110..73d957e 100644
--- a/gfs2/fsck/Makefile.am
+++ b/gfs2/fsck/Makefile.am
@@ -21,7 +21,6 @@ fsck_gfs2_SOURCES = \
 	metawalk.c \
 	pass1b.c \
 	pass1.c \
-	pass1c.c \
 	pass2.c \
 	pass3.c \
 	pass4.c \
diff --git a/gfs2/fsck/main.c b/gfs2/fsck/main.c
index 658cd17..08343ca 100644
--- a/gfs2/fsck/main.c
+++ b/gfs2/fsck/main.c
@@ -237,7 +237,6 @@ struct fsck_pass {
 static const struct fsck_pass passes[] = {
 	{ .name = "pass1",  .f = pass1 },
 	{ .name = "pass1b", .f = pass1b },
-	{ .name = "pass1c", .f = pass1c },
 	{ .name = "pass2",  .f = pass2 },
 	{ .name = "pass3",  .f = pass3 },
 	{ .name = "pass4",  .f = pass4 },
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index f95e7eb..a892b4b 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -684,8 +684,6 @@ static int finish_eattr_indir(struct gfs2_inode *ip, int leaf_pointers,
 			      int leaf_pointer_errors, void *private)
 {
 	struct block_count *bc = (struct block_count *) private;
-	osi_list_t *head;
-	struct special_blocks *b = NULL;
 
 	if (leaf_pointer_errors == leaf_pointers) /* All eas were bad */
 		return ask_remove_inode_eattr(ip, bc);
@@ -693,18 +691,6 @@ static int finish_eattr_indir(struct gfs2_inode *ip, int leaf_pointers,
 		     "attribute block\n"),
 		   (unsigned long long)ip->i_di.di_num.no_addr,
 		   (unsigned long long)ip->i_di.di_num.no_addr);
-	/* Mark the inode as having an eattr in the block map
-	   so pass1c can check it. We may have previously added this inode
-	   to the eattr_blocks list and if we did, it would be the first
-	   one on the list.  So check that one only (to save time) and
-	   if that one matches, no need to add it again. */
-	if (!osi_list_empty(&ip->i_sbd->eattr_blocks.list)) {
-		head = &ip->i_sbd->eattr_blocks.list;
-		b = osi_list_entry(head->next, struct special_blocks, list);
-	}
-	if (!b || b->block != ip->i_di.di_num.no_addr)
-		gfs2_special_add(&ip->i_sbd->eattr_blocks,
-				 ip->i_di.di_num.no_addr);
 	if (!leaf_pointer_errors)
 		return 0;
 	log_err( _("Inode %lld (0x%llx) has recoverable indirect "
@@ -774,7 +760,7 @@ static int check_ealeaf_block(struct gfs2_inode *ip, uint64_t block, int btype,
 	}
 	/* Point of confusion: We've got to set the ea block itself to
 	   GFS2_BLKST_USED here.  Elsewhere we mark the inode with
-	   gfs2_eattr_block meaning it contains an eattr for pass1c. */
+	   gfs2_eattr_block meaning it contains an eattr. */
 	fsck_blockmap_set(ip, block, _("Extended Attribute"),
 			  sdp->gfs1 ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
 	bc->ea_count++;
@@ -830,24 +816,7 @@ static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
 			    void *private)
 {
 	struct gfs2_sbd *sdp = ip->i_sbd;
-	osi_list_t *head;
-	struct special_blocks *b = NULL;
 
-	/* This inode contains an eattr - it may be invalid, but the
-	 * eattr attributes points to a non-zero block.
-	 * Clarification: If we're here we're checking a leaf block, and the
-	 * source dinode needs to be marked as having extended attributes.
-	 * That instructs pass1c to check the contents of the ea blocks. */
-	log_debug( _("Setting inode %lld (0x%llx) as having eattr "
-		     "block(s) attached.\n"),
-		   (unsigned long long)ip->i_di.di_num.no_addr,
-		   (unsigned long long)ip->i_di.di_num.no_addr);
-	if (!osi_list_empty(&ip->i_sbd->eattr_blocks.list)) {
-		head = &ip->i_sbd->eattr_blocks.list;
-		b = osi_list_entry(head->next, struct special_blocks, list);
-	}
-	if (!b || b->block != ip->i_di.di_num.no_addr)
-		gfs2_special_add(&sdp->eattr_blocks, ip->i_di.di_num.no_addr);
 	if (!valid_block(sdp, block)) {
 		log_warn( _("Inode #%llu (0x%llx): Extended Attribute leaf "
 			    "block #%llu (0x%llx) is invalid or out of "
@@ -863,6 +832,41 @@ static int check_eattr_leaf(struct gfs2_inode *ip, uint64_t block,
 	return check_ealeaf_block(ip, block, GFS2_METATYPE_EA, bh, private);
 }
 
+static int ask_remove_eattr_entry(struct gfs2_sbd *sdp,
+				  struct gfs2_buffer_head *leaf_bh,
+				  struct gfs2_ea_header *curr,
+				  struct gfs2_ea_header *prev,
+				  int fix_curr, int fix_curr_len)
+{
+	if (!query( _("Remove the bad Extended Attribute entry? (y/n) "))) {
+		log_err( _("Bad Extended Attribute not removed.\n"));
+		return 0;
+	}
+	if (fix_curr)
+		curr->ea_flags |= GFS2_EAFLAG_LAST;
+	if (fix_curr_len) {
+		uint32_t max_size = sdp->sd_sb.sb_bsize;
+		uint32_t offset = (uint32_t)(((unsigned long)curr) -
+					     ((unsigned long)leaf_bh->b_data));
+		curr->ea_rec_len = cpu_to_be32(max_size - offset);
+	}
+	if (!prev)
+		curr->ea_type = GFS2_EATYPE_UNUSED;
+	else {
+		uint32_t tmp32 = be32_to_cpu(curr->ea_rec_len) +
+			be32_to_cpu(prev->ea_rec_len);
+		prev->ea_rec_len = cpu_to_be32(tmp32);
+		if (curr->ea_flags & GFS2_EAFLAG_LAST)
+			prev->ea_flags |= GFS2_EAFLAG_LAST;	
+	}
+	log_err( _("Bad Extended Attribute at block #%llu"
+		   " (0x%llx) removed.\n"),
+		 (unsigned long long)leaf_bh->b_blocknr,
+		 (unsigned long long)leaf_bh->b_blocknr);
+	bmodified(leaf_bh);
+	return 1;
+}
+
 static int check_eattr_entries(struct gfs2_inode *ip,
 			       struct gfs2_buffer_head *leaf_bh,
 			       struct gfs2_ea_header *ea_hdr,
@@ -871,12 +875,32 @@ static int check_eattr_entries(struct gfs2_inode *ip,
 {
 	struct gfs2_sbd *sdp = ip->i_sbd;
 	char ea_name[256];
+	uint32_t offset = (uint32_t)(((unsigned long)ea_hdr) -
+				     ((unsigned long)leaf_bh->b_data));
+	uint32_t max_size = sdp->sd_sb.sb_bsize;
 	uint32_t avail_size;
 	int max_ptrs;
 
 	if (!ea_hdr->ea_name_len){
-		/* Skip this entry for now */
-		return 1;
+		log_err( _("EA has name length of zero\n"));
+		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
+					      ea_hdr_prev, 1, 1);
+	}
+	if (offset + be32_to_cpu(ea_hdr->ea_rec_len) > max_size){
+		log_err( _("EA rec length too long\n"));
+		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
+					      ea_hdr_prev, 1, 1);
+	}
+	if (offset + be32_to_cpu(ea_hdr->ea_rec_len) == max_size &&
+	   (ea_hdr->ea_flags & GFS2_EAFLAG_LAST) == 0){
+		log_err( _("last EA has no last entry flag\n"));
+		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
+					      ea_hdr_prev, 0, 0);
+	}
+	if (!ea_hdr->ea_name_len){
+		log_err( _("EA has name length of zero\n"));
+		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
+					      ea_hdr_prev, 0, 0);
 	}
 
 	memset(ea_name, 0, sizeof(ea_name));
@@ -888,7 +912,8 @@ static int check_eattr_entries(struct gfs2_inode *ip,
 		/* Skip invalid entry */
 		log_err(_("EA (%s) type is invalid (%d > %d).\n"),
 			ea_name, ea_hdr->ea_type, GFS2_EATYPE_LAST);
-		return 1;
+		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
+					      ea_hdr_prev, 0, 0);
 	}
 
 	if (!ea_hdr->ea_num_ptrs)
@@ -902,7 +927,8 @@ static int check_eattr_entries(struct gfs2_inode *ip,
 			ea_name);
 		log_err(_("  Required:  %d\n  Reported:  %d\n"),
 			max_ptrs, ea_hdr->ea_num_ptrs);
-		return 1;
+		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
+					      ea_hdr_prev, 0, 0);
 	} else {
 		log_debug( _("  Pointers Required: %d\n  Pointers Reported: %d\n"),
 			   max_ptrs, ea_hdr->ea_num_ptrs);
diff --git a/gfs2/fsck/pass1c.c b/gfs2/fsck/pass1c.c
deleted file mode 100644
index aa905da..0000000
--- a/gfs2/fsck/pass1c.c
+++ /dev/null
@@ -1,285 +0,0 @@
-#include "clusterautoconfig.h"
-
-#include <inttypes.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include <libintl.h>
-#define _(String) gettext(String)
-
-#include <logging.h>
-#include "libgfs2.h"
-#include "fsck.h"
-#include "util.h"
-#include "metawalk.h"
-
-struct metawalk_fxns pass1c_fxns_delete = {
-	.private = NULL,
-	.check_eattr_indir = delete_eattr_indir,
-	.check_eattr_leaf = delete_eattr_leaf,
-};
-
-static int remove_eattr_entry(struct gfs2_sbd *sdp,
-			      struct gfs2_buffer_head *leaf_bh,
-			      struct gfs2_ea_header *curr,
-			      struct gfs2_ea_header *prev)
-{
-	if (!prev)
-		curr->ea_type = GFS2_EATYPE_UNUSED;
-	else {
-		uint32_t tmp32 = be32_to_cpu(curr->ea_rec_len) +
-			be32_to_cpu(prev->ea_rec_len);
-		prev->ea_rec_len = cpu_to_be32(tmp32);
-		if (curr->ea_flags & GFS2_EAFLAG_LAST)
-			prev->ea_flags |= GFS2_EAFLAG_LAST;	
-	}
-	log_err( _("Bad Extended Attribute at block #%llu"
-		   " (0x%llx) removed.\n"),
-		 (unsigned long long)leaf_bh->b_blocknr,
-		 (unsigned long long)leaf_bh->b_blocknr);
-	bmodified(leaf_bh);
-	return 0;
-}
-
-static int ask_remove_eattr_entry(struct gfs2_sbd *sdp,
-				  struct gfs2_buffer_head *leaf_bh,
-				  struct gfs2_ea_header *curr,
-				  struct gfs2_ea_header *prev,
-				  int fix_curr, int fix_curr_len)
-{
-	if (query( _("Remove the bad Extended Attribute entry? (y/n) "))) {
-		if (fix_curr)
-			curr->ea_flags |= GFS2_EAFLAG_LAST;
-		if (fix_curr_len) {
-			uint32_t max_size = sdp->sd_sb.sb_bsize;
-			uint32_t offset = (uint32_t)(((unsigned long)curr) -
-					     ((unsigned long)leaf_bh->b_data));
-			curr->ea_rec_len = cpu_to_be32(max_size - offset);
-		}
-		if (remove_eattr_entry(sdp, leaf_bh, curr, prev)) {
-			stack;
-			return -1;
-		}
-	} else {
-		log_err( _("Bad Extended Attribute not removed.\n"));
-	}
-	return 1;
-}
-
-static int ask_remove_eattr(struct gfs2_inode *ip)
-{
-	if (query( _("Remove the bad Extended Attribute? (y/n) "))) {
-		check_inode_eattr(ip, &pass1c_fxns_delete);
-		bmodified(ip->i_bh);
-		log_err( _("Bad Extended Attribute removed.\n"));
-		return 1;
-	}
-	log_err( _("Bad Extended Attribute not removed.\n"));
-	return 0;
-}
-
-static int check_eattr_indir(struct gfs2_inode *ip, uint64_t block,
-		      uint64_t parent, struct gfs2_buffer_head **bh,
-		      void *private)
-{
-	struct gfs2_sbd *sdp = ip->i_sbd;
-	uint8_t q;
-	struct gfs2_buffer_head *indir_bh = NULL;
-
-	if (!valid_block(sdp, block)) {
-		log_err( _("Extended attributes indirect block #%llu"
-			" (0x%llx) for inode #%llu"
-			" (0x%llx) is invalid...removing\n"),
-			(unsigned long long)block,
-			(unsigned long long)block,
-			(unsigned long long)ip->i_di.di_num.no_addr,
-			(unsigned long long)ip->i_di.di_num.no_addr);
-		return ask_remove_eattr(ip);
-	}
-	q = block_type(block);
-	if (q != (sdp->gfs1 ? GFS2_BLKST_DINODE : GFS2_BLKST_USED)) {
-		log_err( _("Extended attributes indirect block #%llu"
-			" (0x%llx) for inode #%llu"
-			" (0x%llx) is invalid.\n"),
-			(unsigned long long)block,
-			(unsigned long long)block,
-			(unsigned long long)ip->i_di.di_num.no_addr,
-			(unsigned long long)ip->i_di.di_num.no_addr);
-		return ask_remove_eattr(ip);
-	}
-	else
-		indir_bh = bread(sdp, block);
-
-	*bh = indir_bh;
-	return 0;
-}
-
-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;
-	uint8_t q;
-
-	if (!valid_block(sdp, block)) {
-		log_err( _("Extended attributes block %lld (0x%llx) for "
-			   "inode #%llu (0x%llx) is invalid.\n"),
-			 (unsigned long long)block, (unsigned long long)block,
-			 (unsigned long long)ip->i_di.di_num.no_addr,
-			 (unsigned long long)ip->i_di.di_num.no_addr);
-		return ask_remove_eattr(ip);
-	}
-	q = block_type(block);
-	if (q != (sdp->gfs1 ? GFS2_BLKST_DINODE : GFS2_BLKST_USED)) {
-		log_err( _("Extended attributes block %lld (0x%llx) for "
-			   "inode #%llu (0x%llx) invalid.\n"),
-			 (unsigned long long)block, (unsigned long long)block,
-			 (unsigned long long)ip->i_di.di_num.no_addr,
-			 (unsigned long long)ip->i_di.di_num.no_addr);
-		return ask_remove_eattr(ip);
-	}
-	else 
-		*bh = bread(sdp, block);
-
-	return 0;
-}
-
-static int check_eattr_entry(struct gfs2_inode *ip,
-			     struct gfs2_buffer_head *leaf_bh,
-			     struct gfs2_ea_header *ea_hdr,
-			     struct gfs2_ea_header *ea_hdr_prev,
-			     void *private)
-{
-	struct gfs2_sbd *sdp = ip->i_sbd;
-	char ea_name[256];
-	uint32_t offset = (uint32_t)(((unsigned long)ea_hdr) -
-			                  ((unsigned long)leaf_bh->b_data));
-	uint32_t max_size = sdp->sd_sb.sb_bsize;
-
-	if (!ea_hdr->ea_name_len){
-		log_err( _("EA has name length of zero\n"));
-		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
-					      ea_hdr_prev, 1, 1);
-	}
-	if (offset + be32_to_cpu(ea_hdr->ea_rec_len) > max_size){
-		log_err( _("EA rec length too long\n"));
-		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
-					      ea_hdr_prev, 1, 1);
-	}
-	if (offset + be32_to_cpu(ea_hdr->ea_rec_len) == max_size &&
-	   (ea_hdr->ea_flags & GFS2_EAFLAG_LAST) == 0){
-		log_err( _("last EA has no last entry flag\n"));
-		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
-					      ea_hdr_prev, 0, 0);
-	}
-	if (!ea_hdr->ea_name_len){
-		log_err( _("EA has name length of zero\n"));
-		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
-					      ea_hdr_prev, 0, 0);
-	}
-
-	memset(ea_name, 0, sizeof(ea_name));
-	strncpy(ea_name, (char *)ea_hdr + sizeof(struct gfs2_ea_header),
-		ea_hdr->ea_name_len);
-
-	if (!GFS2_EATYPE_VALID(ea_hdr->ea_type) &&
-	   ((ea_hdr_prev) || (!ea_hdr_prev && ea_hdr->ea_type))){
-		log_err( _("EA (%s) type is invalid (%d > %d).\n"),
-			ea_name, ea_hdr->ea_type, GFS2_EATYPE_LAST);
-		return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
-					      ea_hdr_prev, 0, 0);
-	}
-
-	if (ea_hdr->ea_num_ptrs){
-		uint32_t avail_size;
-		int max_ptrs;
-
-		avail_size = sdp->sd_sb.sb_bsize - sizeof(struct gfs2_meta_header);
-		max_ptrs = (be32_to_cpu(ea_hdr->ea_data_len)+avail_size-1)/avail_size;
-
-		if (max_ptrs > ea_hdr->ea_num_ptrs){
-			log_err( _("EA (%s) has incorrect number of pointers.\n"), ea_name);
-			log_err( _("  Required:  %d\n  Reported:  %d\n"),
-				 max_ptrs, ea_hdr->ea_num_ptrs);
-			return ask_remove_eattr_entry(sdp, leaf_bh, ea_hdr,
-						      ea_hdr_prev, 0, 0);
-		} else {
-			log_debug( _(" Pointers Required: %d\n  Pointers Reported: %d\n"),
-					  max_ptrs, ea_hdr->ea_num_ptrs);
-		}
-	}
-	return 0;
-}
-
-static int check_eattr_extentry(struct gfs2_inode *ip, uint64_t *ea_ptr,
-			 struct gfs2_buffer_head *leaf_bh,
-			 struct gfs2_ea_header *ea_hdr,
-			 struct gfs2_ea_header *ea_hdr_prev, void *private)
-{
-	uint8_t q;
-	struct gfs2_sbd *sdp = ip->i_sbd;
-
-	q = block_type(be64_to_cpu(*ea_ptr));
-	if (q != (sdp->gfs1 ? GFS2_BLKST_DINODE : GFS2_BLKST_USED)) {
-		if (remove_eattr_entry(sdp, leaf_bh, ea_hdr, ea_hdr_prev)){
-			stack;
-			return -1;
-		}
-		return 1;
-	}
-	return 0;
-}
-
-/* Go over all inodes with extended attributes and verify the EAs are
- * valid */
-int pass1c(struct gfs2_sbd *sdp)
-{
-	uint64_t block_no = 0;
-	struct gfs2_buffer_head *bh;
-	struct gfs2_inode *ip = NULL;
-	struct metawalk_fxns pass1c_fxns = { 0 };
-	int error = 0;
-	osi_list_t *tmp, *x;
-	struct special_blocks *ea_block;
-
-	pass1c_fxns.check_eattr_indir = &check_eattr_indir;
-	pass1c_fxns.check_eattr_leaf = &check_eattr_leaf;
-	pass1c_fxns.check_eattr_entry = &check_eattr_entry;
-	pass1c_fxns.check_eattr_extentry = &check_eattr_extentry;
-	pass1c_fxns.private = NULL;
-
-	log_info( _("Looking for inodes containing ea blocks...\n"));
-	osi_list_foreach_safe(tmp, &sdp->eattr_blocks.list, x) {
-		ea_block = osi_list_entry(tmp, struct special_blocks, list);
-		block_no = ea_block->block;
-		warm_fuzzy_stuff(block_no);
-
-		if (skip_this_pass || fsck_abort) /* if asked to skip the rest */
-			return FSCK_OK;
-		bh = bread(sdp, block_no);
-		if (!gfs2_check_meta(bh, GFS2_METATYPE_DI)) { /* if a dinode */
-			log_info( _("EA in inode %llu (0x%llx)\n"),
-				 (unsigned long long)block_no,
-				 (unsigned long long)block_no);
-			gfs2_special_clear(&sdp->eattr_blocks, block_no);
-			ip = fsck_inode_get(sdp, bh);
-			ip->bh_owned = 1;
-
-			log_debug( _("Found eattr at %llu (0x%llx)\n"),
-				  (unsigned long long)ip->i_di.di_eattr,
-				  (unsigned long long)ip->i_di.di_eattr);
-			/* FIXME: Handle walking the eattr here */
-			error = check_inode_eattr(ip, &pass1c_fxns);
-			if (error < 0) {
-				stack;
-				brelse(bh);
-				return FSCK_ERROR;
-			}
-
-			fsck_inode_put(&ip); /* dinode_out, brelse, free */
-		} else {
-			brelse(bh);
-		}
-	}
-	return FSCK_OK;
-}
diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
index b22abc6..2745692 100644
--- a/gfs2/fsck/util.c
+++ b/gfs2/fsck/util.c
@@ -556,7 +556,6 @@ struct gfs2_bmap *gfs2_bmap_create(struct gfs2_sbd *sdp, uint64_t size,
 		free(il);
 		il = NULL;
 	}
-	osi_list_init(&sdp->eattr_blocks.list);
 	return il;
 }
 
@@ -584,7 +583,6 @@ void *gfs2_bmap_destroy(struct gfs2_sbd *sdp, struct gfs2_bmap *il)
 		free(il);
 		il = NULL;
 	}
-	gfs2_special_free(&sdp->eattr_blocks);
 	return il;
 }
 
diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index ccae721..12d57c7 100644
--- a/gfs2/libgfs2/libgfs2.h
+++ b/gfs2/libgfs2/libgfs2.h
@@ -304,8 +304,6 @@ struct gfs2_sbd {
 	struct gfs2_inode *master_dir;
 	struct master_dir md;
 
-	struct special_blocks eattr_blocks;
-
 	uint64_t rg_one_length;
 	uint64_t rg_length;
 	int gfs1;
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 20/20] fsck.gfs2: Clone duplicate data block pointers

[Bob Peterson]

Before this patch, pass1b would find duplicate data block pointers
in two different inodes, then arbitrarily keep one of them and
delete the other. That's harsh. This patch adds the ability to
clone the data block so each inode has its own copy.

rhbz#1257625
---
 gfs2/fsck/pass1b.c | 164 +++++++++++++++++++++++++++++++++++++++++------------
 gfs2/fsck/util.c   |  13 +++--
 2 files changed, 136 insertions(+), 41 deletions(-)

diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
index 4c41fef..16c0aec 100644
--- a/gfs2/fsck/pass1b.c
+++ b/gfs2/fsck/pass1b.c
@@ -33,6 +33,16 @@ struct clone_target {
 	int first;
 };
 
+struct meta_blk_ref {
+	uint64_t block; /* block to locate */
+	uint64_t metablock; /* returned metadata block addr containing ref */
+	int off; /* offset to the reference within the buffer */
+};
+
+static int clone_data(struct gfs2_inode *ip, uint64_t metablock,
+		      uint64_t block, void *private,
+		      struct gfs2_buffer_head *bh, uint64_t *ptr);
+
 static void log_inode_reference(struct duptree *dt, osi_list_t *tmp, int inval)
 {
 	char reftypestring[32];
@@ -57,6 +67,108 @@ static void log_inode_reference(struct duptree *dt, osi_list_t *tmp, int inval)
 		  (unsigned long long)dt->block, reftypestring);
 }
 
+static int findref_meta(struct gfs2_inode *ip, uint64_t block,
+			struct gfs2_buffer_head **bh, int h,
+			int *is_valid, int *was_duplicate, void *private)
+{
+	*is_valid = 1;
+	*was_duplicate = 0;
+	return meta_is_good;
+}
+
+static int findref_data(struct gfs2_inode *ip, uint64_t metablock,
+			uint64_t block, void *private,
+			struct gfs2_buffer_head *bh, uint64_t *ptr)
+{
+	struct meta_blk_ref *mbr = (struct meta_blk_ref *)private;
+
+	if (block == mbr->block) {
+		mbr->metablock = bh->b_blocknr;
+		mbr->off = (ptr - (uint64_t *)bh->b_data);
+		log_debug("Duplicate data reference located on metadata "
+			  "block 0x%llx, offset 0x%x\n",
+			  (unsigned long long)mbr->metablock, mbr->off);
+	}
+	return meta_is_good;
+}
+
+static void clone_data_block(struct gfs2_sbd *sdp, struct duptree *dt,
+			     struct inode_with_dups *id)
+{
+	struct meta_blk_ref metaref = { .block = dt->block, };
+	struct metawalk_fxns find1ref_fxns = {
+		.private = &metaref,
+		.check_metalist = findref_meta,
+		.check_data = findref_data,
+	};
+	struct clone_target clone = {.dup_block = dt->block,};
+	struct gfs2_inode *ip;
+	struct gfs2_buffer_head *bh;
+	uint64_t *ptr;
+
+	if (!(query(_("Okay to clone data block %lld (0x%llx) for inode "
+		      "%lld (0x%llx)? (y/n) "),
+		    (unsigned long long)dt->block,
+		    (unsigned long long)dt->block,
+		    (unsigned long long)id->block_no,
+		    (unsigned long long)id->block_no))) {
+		log_warn(_("The duplicate reference was not cloned.\n"));
+		return;
+	}
+	ip = fsck_load_inode(sdp, id->block_no);
+	check_metatree(ip, &find1ref_fxns);
+	if (metaref.metablock == 0) {
+		log_err(_("Unable to clone data block.\n"));
+	} else {
+		if (metaref.metablock != id->block_no)
+			bh = bread(sdp, metaref.metablock);
+		else
+			bh = ip->i_bh;
+		ptr = (uint64_t *)bh->b_data + metaref.off;
+		clone_data(ip, 0, dt->block, &clone, bh, ptr);
+		if (metaref.metablock != id->block_no)
+			brelse(bh);
+		else
+			bmodified(ip->i_bh);
+	}
+	fsck_inode_put(&ip); /* out, brelse, free */
+}
+
+/* revise_dup_handler - get current information about a duplicate reference
+ *
+ * Function resolve_dup_references can delete dinodes that reference blocks
+ * which may have duplicate references. Therefore, the duplicate tree is
+ * constantly being changed. This function revises the duplicate handler so
+ * that it accurately matches what's in the duplicate tree regarding this block
+ */
+static void revise_dup_handler(uint64_t dup_blk, struct dup_handler *dh)
+{
+	osi_list_t *tmp;
+	struct duptree *dt;
+	struct inode_with_dups *id;
+
+	dh->ref_inode_count = 0;
+	dh->ref_count = 0;
+	dh->dt = NULL;
+
+	dt = dupfind(dup_blk);
+	if (!dt)
+		return;
+
+	dh->dt = dt;
+	/* Count the duplicate references, both valid and invalid */
+	osi_list_foreach(tmp, &dt->ref_invinode_list) {
+		id = osi_list_entry(tmp, struct inode_with_dups, list);
+		dh->ref_inode_count++;
+		dh->ref_count += id->dup_count;
+	}
+	osi_list_foreach(tmp, &dt->ref_inode_list) {
+		id = osi_list_entry(tmp, struct inode_with_dups, list);
+		dh->ref_inode_count++;
+		dh->ref_count += id->dup_count;
+	}
+}
+
 /*
  * resolve_dup_references - resolve all but the last dinode that has a
  *                          duplicate reference to a given block.
@@ -164,6 +276,12 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 				dup_listent_delete(dt, id);
 				continue;
 			}
+		} else if (acceptable_ref == ref_types &&
+			   this_ref == ref_as_data) {
+			clone_data_block(sdp, dt, id);
+			dup_listent_delete(dt, id);
+			revise_dup_handler(dt->block, dh);
+			continue;
 		} else if (!(query( _("Okay to delete %s inode %lld (0x%llx)? "
 				      "(y/n) "),
 				    (inval ? _("invalidated") : ""),
@@ -215,9 +333,11 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 				ip->i_di.di_flags &= ~GFS2_DIF_EA_INDIRECT;
 				bmodified(ip->i_bh);
 				dup_listent_delete(dt, id);
+				(dh->ref_inode_count)--;
 			} else {
 				/* Clear the EAs for the inode first */
 				check_inode_eattr(ip, &pass1b_fxns_delete);
+				(dh->ref_inode_count)--;
 			}
 			/* If the reference was as metadata or data, we've got
 			   a corrupt dinode that will be deleted. */
@@ -261,41 +381,6 @@ static void resolve_dup_references(struct gfs2_sbd *sdp, struct duptree *dt,
 	return;
 }
 
-/* revise_dup_handler - get current information about a duplicate reference
- *
- * Function resolve_dup_references can delete dinodes that reference blocks
- * which may have duplicate references. Therefore, the duplicate tree is
- * constantly being changed. This function revises the duplicate handler so
- * that it accurately matches what's in the duplicate tree regarding this block
- */
-static void revise_dup_handler(uint64_t dup_blk, struct dup_handler *dh)
-{
-	osi_list_t *tmp;
-	struct duptree *dt;
-	struct inode_with_dups *id;
-
-	dh->ref_inode_count = 0;
-	dh->ref_count = 0;
-	dh->dt = NULL;
-
-	dt = dupfind(dup_blk);
-	if (!dt)
-		return;
-
-	dh->dt = dt;
-	/* Count the duplicate references, both valid and invalid */
-	osi_list_foreach(tmp, &dt->ref_invinode_list) {
-		id = osi_list_entry(tmp, struct inode_with_dups, list);
-		dh->ref_inode_count++;
-		dh->ref_count += id->dup_count;
-	}
-	osi_list_foreach(tmp, &dt->ref_inode_list) {
-		id = osi_list_entry(tmp, struct inode_with_dups, list);
-		dh->ref_inode_count++;
-		dh->ref_count += id->dup_count;
-	}
-}
-
 static int clone_check_meta(struct gfs2_inode *ip, uint64_t block,
 			    struct gfs2_buffer_head **bh, int h,
 			    int *is_valid, int *was_duplicate, void *private)
@@ -333,8 +418,8 @@ static int clone_data(struct gfs2_inode *ip, uint64_t metablock,
 		clonet->first = 0;
 		return 0;
 	}
-	log_err(_("Error: Inode %lld (0x%llx)'s subsequent reference to "
-		  "block %lld (0x%llx) is an error.\n"),
+	log_err(_("Error: Inode %lld (0x%llx)'s reference to block %lld "
+		  "(0x%llx) should be replaced with a clone.\n"),
 		(unsigned long long)ip->i_di.di_num.no_addr,
 		(unsigned long long)ip->i_di.di_num.no_addr,
 		(unsigned long long)block, (unsigned long long)block);
@@ -398,6 +483,11 @@ static void clone_dup_ref_in_inode(struct gfs2_inode *ip, struct duptree *dt)
 		.check_data = clone_data,
 	};
 
+	log_err(_("There are multiple references to block %lld (0x%llx) in "
+		  "inode %lld (0x%llx)\n"),
+		(unsigned long long)ip->i_di.di_num.no_addr,
+		(unsigned long long)ip->i_di.di_num.no_addr,
+		(unsigned long long)dt->block, (unsigned long long)dt->block);
 	error = check_metatree(ip, &pass1b_fxns_clone);
 	if (error) {
 		log_err(_("Error cloning duplicate reference(s) to block %lld "
diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
index 2745692..84402fc 100644
--- a/gfs2/fsck/util.c
+++ b/gfs2/fsck/util.c
@@ -18,7 +18,7 @@
 #include "util.h"
 
 const char *reftypes[ref_types + 1] = {"data", "metadata",
-				       "extended attribute", "itself",
+				       "an extended attribute", "an inode",
 				       "unimportant"};
 
 void big_file_comfort(struct gfs2_inode *ip, uint64_t blks_checked)
@@ -401,9 +401,14 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
 		  (unsigned long long)ip->i_di.di_num.no_addr);
 	if (first)
 		log_info( _("This is the original reference.\n"));
-	else
-		log_info( _("This brings the total to: %d references\n"),
-			  dt->refs);
+	else {
+		/* Check for duplicate refs to the same block in one inode. */
+		if (id->dup_count > 1)
+			dt->dup_flags |= DUPFLAG_REF1_FOUND;
+		log_info( _("This brings the total to: %d inode references, "
+			    "%d from this inode.\n"),
+			  dt->refs, id->dup_count);
+	}
 	return meta_is_good;
 }
 
-- 
2.4.3

[Cluster-devel] [gfs2-utils PATCH 00/20] fsck.gfs2: Fix bugs and eliminate pass1c

[Andrew Price]

Hi Bob,

On 28/09/15 15:45, Bob Peterson wrote:
> Hi,
>
> This original goal of this patch series was to eliminate pass1c, because
> it is slow and takes lots of memory related to a linked list of all inodes
> that have extended attributes. In thoroughly testing the patch, I uncovered
> a bunch of fsck.gfs2 bugs, most of which have to do with extended attributes.
> One patch spares the life of inodes that have bad extended attribute pointers:
> rather than deleting the inode, it now removes its extended attributes.
> Another patch adds a new feature to fsck.gfs2 whereby duplicated data block
> pointers may be cloned to new data blocks, sparing the life of some inodes
> that were previously deleted. There are a few cleanups thrown in as well.

The series looks good to me. ACK.

Thanks,
Andy

>
> Regards,
>
> Bob Peterson
> Red Hat File Systems
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
> Bob Peterson (20):
>    libgfs2: Check block range when inserting into rgrp tree
>    libgfs2: Check rgd->bits before referencing it
>    fsck.gfs2: Add check for gfs1 invalid inode refs in dentry
>    fsck.gfs2: Make debug messages more succinct wrt extended attributes
>    fsck.gfs2: Break up funtion handle_dup_blk
>    fsck.gfs2: Only preserve the _first_ acceptable inode reference
>    fsck.gfs2: Don't just assume the remaining EA reference is good
>    fsck.gfs2: Don't delete inode for duplicate reference in EA
>    fsck.gfs2: Don't traverse EAs that belong to another inode
>    fsck.gfs2: Refactor function check_indirect_eattr
>    fsck.gfs2: Once an indirect ea error is found, flag all that follow
>    fsck.gfs2: Always restore saved value for di_eattr
>    fsck.gfs2: Remove redundancy in add_duplicate_ref
>    fsck.gfs2: Don't remove duplicate eattr blocks
>    fsck.gfs2: Refactor check_eattr_entries and add error messages
>    fsck.gfs2: remove bad EAs at the end, not as-you-go
>    fsck.gfs2: Combine remove_inode_eattr with its only caller
>    fsck.gfs2: Print debug message to dilineate metadata blocks
>    fsck.gfs2: Remove pass1c in favor of processing in pass1
>    fsck.gfs2: Clone duplicate data block pointers
>
>   gfs2/fsck/Makefile.am  |   1 -
>   gfs2/fsck/main.c       |   1 -
>   gfs2/fsck/metawalk.c   | 185 ++++++++++++-----------
>   gfs2/fsck/pass1.c      | 296 ++++++++++++++++++++++---------------
>   gfs2/fsck/pass1b.c     | 392 +++++++++++++++++++++++++++++++++++--------------
>   gfs2/fsck/pass1c.c     | 285 -----------------------------------
>   gfs2/fsck/pass2.c      |  21 +++
>   gfs2/fsck/util.c       |  15 +-
>   gfs2/libgfs2/libgfs2.h |   2 -
>   gfs2/libgfs2/rgrp.c    |   2 +-
>   gfs2/libgfs2/super.c   |   6 +
>   11 files changed, 592 insertions(+), 614 deletions(-)
>   delete mode 100644 gfs2/fsck/pass1c.c
>