Re: xfs_repair: phase7.c:148: phase7: Assertion `no_modify || nrefs > 0' failed.

[Christoph Hellwig <hch@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

On Mon, Feb 20, 2012 at 04:00:18PM +0100, Arkadiusz Mi??kiewicz wrote:
> 
> xfs_repair from xfsprogs-dev git.kernel.org repository
> 
> on  http://routing.com.pl/xfs-metadump-2012-02-20.bz2 fails with:

The patch below fixes it:


[-- Attachment #2: repair-fix-nlink-counting --]
[-- Type: text/plain, Size: 16032 bytes --]

From: Christoph Hellwig <hch@lst.de>
Subject: repair: fix the variable-width nlink array

It looks like we currently never grow the variable-width nlink array
if only the on-disk nlink size overflows 8 bits.  This leads to a major
mess in nlink counting, and eventually an assert in phase7.

Replace the indirect all mess with a union that allows doing proper
array arithmetics while we're at it.

Signed-off-by: Christoph Hellwig <hch@lst.de>

diff --git a/repair/incore.h b/repair/incore.h
index fdb7742..8a578b5 100644
--- a/repair/incore.h
+++ b/repair/incore.h
@@ -268,11 +268,17 @@ typedef struct parent_list  {
 #endif
 } parent_list_t;
 
+union ino_nlink {
+	__uint8_t	*un8;
+	__uint16_t	*un16;
+	__uint32_t	*un32;
+};
+
 typedef struct ino_ex_data  {
 	__uint64_t		ino_reached;	/* bit == 1 if reached */
 	__uint64_t		ino_processed;	/* reference checked bit mask */
 	parent_list_t		*parents;
-	__uint8_t		*counted_nlinks;/* counted nlinks in P6 */
+	union ino_nlink		counted_nlinks;/* counted nlinks in P6 */
 } ino_ex_data_t;
 
 typedef struct ino_tree_node  {
@@ -281,8 +287,8 @@ typedef struct ino_tree_node  {
 	xfs_inofree_t		ir_free;	/* inode free bit mask */
 	__uint64_t		ino_confirmed;	/* confirmed bitmask */
 	__uint64_t		ino_isa_dir;	/* bit == 1 if a directory */
-	struct nlink_ops	*nlinkops;	/* pointer to current nlink ops */
-	__uint8_t		*disk_nlinks;	/* on-disk nlinks, set in P3 */
+	__uint8_t		nlink_size;
+	union ino_nlink		disk_nlinks;	/* on-disk nlinks, set in P3 */
 	union  {
 		ino_ex_data_t	*ex_data;	/* phases 6,7 */
 		parent_list_t	*plist;		/* phases 2-5 */
@@ -292,16 +298,6 @@ typedef struct ino_tree_node  {
 #define INOS_PER_IREC	(sizeof(__uint64_t) * NBBY)
 #define	IREC_MASK(i)	((__uint64_t)1 << (i))
 
-typedef struct nlink_ops {
-	const int	nlink_size;
-	void		(*disk_nlink_set)(ino_tree_node_t *, int, __uint32_t);
-	__uint32_t	(*disk_nlink_get)(ino_tree_node_t *, int);
-	__uint32_t	(*counted_nlink_get)(ino_tree_node_t *, int);
-	__uint32_t	(*counted_nlink_inc)(ino_tree_node_t *, int);
-	__uint32_t	(*counted_nlink_dec)(ino_tree_node_t *, int);
-} nlink_ops_t;
-
-
 void		add_ino_ex_data(xfs_mount_t *mp);
 
 /*
@@ -460,29 +456,12 @@ static inline int is_inode_free(struct ino_tree_node *irec, int offset)
  * detected and drop_inode_ref() is called every time a link to
  * an inode that we've counted is removed.
  */
+void add_inode_ref(struct ino_tree_node *irec, int offset);
+void drop_inode_ref(struct ino_tree_node *irec, int offset);
+__uint32_t num_inode_references(struct ino_tree_node *irec, int offset);
 
-static inline void add_inode_ref(struct ino_tree_node *irec, int offset)
-{
-	ASSERT(irec->ino_un.ex_data != NULL);
-
-	irec->nlinkops->counted_nlink_inc(irec, offset);
-}
-
-static inline void drop_inode_ref(struct ino_tree_node *irec, int offset)
-{
-	ASSERT(irec->ino_un.ex_data != NULL);
-
-	if (irec->nlinkops->counted_nlink_dec(irec, offset) == 0)
-		irec->ino_un.ex_data->ino_reached &= ~IREC_MASK(offset);
-}
-
-static inline __uint32_t num_inode_references(struct ino_tree_node *irec,
-		int offset)
-{
-	ASSERT(irec->ino_un.ex_data != NULL);
-
-	return irec->nlinkops->counted_nlink_get(irec, offset);
-}
+void set_inode_disk_nlinks(struct ino_tree_node *irec, int offset, __uint32_t nlinks);
+__uint32_t get_inode_disk_nlinks(struct ino_tree_node *irec, int offset);
 
 static inline int is_inode_reached(struct ino_tree_node *irec, int offset)
 {
@@ -496,18 +475,6 @@ static inline void add_inode_reached(struct ino_tree_node *irec, int offset)
 	irec->ino_un.ex_data->ino_reached |= IREC_MASK(offset);
 }
 
-static inline void set_inode_disk_nlinks(struct ino_tree_node *irec, int offset,
-		__uint32_t nlinks)
-{
-	irec->nlinkops->disk_nlink_set(irec, offset, nlinks);
-}
-
-static inline __uint32_t get_inode_disk_nlinks(struct ino_tree_node *irec,
-		int offset)
-{
-	return irec->nlinkops->disk_nlink_get(irec, offset);
-}
-
 /*
  * set/get inode number of parent -- works for directory inodes only
  */
diff --git a/repair/incore_ino.c b/repair/incore_ino.c
index b933765..2a40727 100644
--- a/repair/incore_ino.c
+++ b/repair/incore_ino.c
@@ -37,189 +37,176 @@ static avltree_desc_t	**inode_uncertain_tree_ptrs;
 
 /* memory optimised nlink counting for all inodes */
 
-static void nlink_grow_8_to_16(ino_tree_node_t *irec);
-static void nlink_grow_16_to_32(ino_tree_node_t *irec);
-
-static void
-disk_nlink_32_set(ino_tree_node_t *irec, int ino_offset, __uint32_t nlinks)
+static void *
+alloc_nlink_array(__uint8_t nlink_size)
 {
-	((__uint32_t*)irec->disk_nlinks)[ino_offset] = nlinks;
-}
+	void *ptr;
 
-static __uint32_t
-disk_nlink_32_get(ino_tree_node_t *irec, int ino_offset)
-{
-	return ((__uint32_t*)irec->disk_nlinks)[ino_offset];
+	ptr = calloc(XFS_INODES_PER_CHUNK, nlink_size);
+	if (!ptr)
+		do_error(_("could not allocate nlink array\n"));
+	return ptr;
 }
 
-static __uint32_t
-counted_nlink_32_get(ino_tree_node_t *irec, int ino_offset)
+static void
+nlink_grow_8_to_16(ino_tree_node_t *irec)
 {
-	return ((__uint32_t*)irec->ino_un.ex_data->counted_nlinks)[ino_offset];
-}
+	__uint16_t	*new_nlinks;
+	int		i;
 
-static __uint32_t
-counted_nlink_32_inc(ino_tree_node_t *irec, int ino_offset)
-{
-	return ++(((__uint32_t*)irec->ino_un.ex_data->counted_nlinks)[ino_offset]);
-}
+	irec->nlink_size = sizeof(__uint16_t);
 
-static __uint32_t
-counted_nlink_32_dec(ino_tree_node_t *irec, int ino_offset)
-{
-	__uint32_t *nlinks = (__uint32_t*)irec->ino_un.ex_data->counted_nlinks;
+	new_nlinks = alloc_nlink_array(irec->nlink_size);
+	for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
+		new_nlinks[i] = irec->disk_nlinks.un8[i];
+	free(irec->disk_nlinks.un8);
+	irec->disk_nlinks.un16 = new_nlinks;
 
-	ASSERT(nlinks[ino_offset] > 0);
-	return --(nlinks[ino_offset]);
+	if (full_ino_ex_data) {
+		new_nlinks = alloc_nlink_array(irec->nlink_size);
+		for (i = 0; i < XFS_INODES_PER_CHUNK; i++) {
+			new_nlinks[i] =
+				irec->ino_un.ex_data->counted_nlinks.un8[i];
+		}
+		free(irec->ino_un.ex_data->counted_nlinks.un8);
+		irec->ino_un.ex_data->counted_nlinks.un16 = new_nlinks;
+	}
 }
 
-
 static void
-disk_nlink_16_set(ino_tree_node_t *irec, int ino_offset, __uint32_t nlinks)
+nlink_grow_16_to_32(ino_tree_node_t *irec)
 {
-	if (nlinks >= 0x10000) {
-		nlink_grow_16_to_32(irec);
-		disk_nlink_32_set(irec, ino_offset, nlinks);
-	} else
-		((__uint16_t*)irec->disk_nlinks)[ino_offset] = nlinks;
-}
+	__uint32_t	*new_nlinks;
+	int		i;
 
-static __uint32_t
-disk_nlink_16_get(ino_tree_node_t *irec, int ino_offset)
-{
-	return ((__uint16_t*)irec->disk_nlinks)[ino_offset];
-}
+	irec->nlink_size = sizeof(__uint32_t);
 
-static __uint32_t
-counted_nlink_16_get(ino_tree_node_t *irec, int ino_offset)
-{
-	return ((__uint16_t*)irec->ino_un.ex_data->counted_nlinks)[ino_offset];
-}
+	new_nlinks = alloc_nlink_array(irec->nlink_size);
+	for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
+		new_nlinks[i] = irec->disk_nlinks.un16[i];
+	free(irec->disk_nlinks.un16);
+	irec->disk_nlinks.un32 = new_nlinks;
 
-static __uint32_t
-counted_nlink_16_inc(ino_tree_node_t *irec, int ino_offset)
-{
-	__uint16_t *nlinks = (__uint16_t*)irec->ino_un.ex_data->counted_nlinks;
+	if (full_ino_ex_data) {
+		new_nlinks = alloc_nlink_array(irec->nlink_size);
 
-	if (nlinks[ino_offset] == 0xffff) {
-		nlink_grow_16_to_32(irec);
-		return counted_nlink_32_inc(irec, ino_offset);
+		for (i = 0; i < XFS_INODES_PER_CHUNK; i++) {
+			new_nlinks[i] =
+				irec->ino_un.ex_data->counted_nlinks.un16[i];
+		}
+		free(irec->ino_un.ex_data->counted_nlinks.un16);
+		irec->ino_un.ex_data->counted_nlinks.un32 = new_nlinks;
 	}
-	return ++(nlinks[ino_offset]);
 }
 
-static __uint32_t
-counted_nlink_16_dec(ino_tree_node_t *irec, int ino_offset)
+void add_inode_ref(struct ino_tree_node *irec, int ino_offset)
 {
-	__uint16_t *nlinks = (__uint16_t*)irec->ino_un.ex_data->counted_nlinks;
-
-	ASSERT(nlinks[ino_offset] > 0);
-	return --(nlinks[ino_offset]);
-}
-
+	ASSERT(irec->ino_un.ex_data != NULL);
 
-static void
-disk_nlink_8_set(ino_tree_node_t *irec, int ino_offset, __uint32_t nlinks)
-{
-	if (nlinks >= 0x100) {
+	switch (irec->nlink_size) {
+	case sizeof(__uint8_t):
+		if (irec->ino_un.ex_data->counted_nlinks.un8[ino_offset] < 0xff) {
+			irec->ino_un.ex_data->counted_nlinks.un8[ino_offset]++;
+			break;
+		}
 		nlink_grow_8_to_16(irec);
-		disk_nlink_16_set(irec, ino_offset, nlinks);
-	} else
-		irec->disk_nlinks[ino_offset] = nlinks;
+		/*FALLTHRU*/
+	case sizeof(__uint16_t):
+		if (irec->ino_un.ex_data->counted_nlinks.un16[ino_offset] < 0xffff) {
+			irec->ino_un.ex_data->counted_nlinks.un16[ino_offset]++;
+			break;
+		}
+		nlink_grow_16_to_32(irec);
+		/*FALLTHRU*/
+	case sizeof(__uint32_t):
+		irec->ino_un.ex_data->counted_nlinks.un32[ino_offset]++;
+		break;
+	default:
+		ASSERT(0);
+	}
 }
 
-static __uint32_t
-disk_nlink_8_get(ino_tree_node_t *irec, int ino_offset)
+void drop_inode_ref(struct ino_tree_node *irec, int ino_offset)
 {
-	return irec->disk_nlinks[ino_offset];
-}
+	__uint32_t	refs = 0;
 
-static __uint32_t
-counted_nlink_8_get(ino_tree_node_t *irec, int ino_offset)
-{
-	return irec->ino_un.ex_data->counted_nlinks[ino_offset];
-}
+	ASSERT(irec->ino_un.ex_data != NULL);
 
-static __uint32_t
-counted_nlink_8_inc(ino_tree_node_t *irec, int ino_offset)
-{
-	if (irec->ino_un.ex_data->counted_nlinks[ino_offset] == 0xff) {
-		nlink_grow_8_to_16(irec);
-		return counted_nlink_16_inc(irec, ino_offset);
+	switch (irec->nlink_size) {
+	case sizeof(__uint8_t):
+		ASSERT(irec->ino_un.ex_data->counted_nlinks.un8[ino_offset] > 0);
+		refs = --irec->ino_un.ex_data->counted_nlinks.un8[ino_offset];
+		break;
+	case sizeof(__uint16_t):
+		ASSERT(irec->ino_un.ex_data->counted_nlinks.un16[ino_offset] > 0);
+		refs = --irec->ino_un.ex_data->counted_nlinks.un16[ino_offset];
+		break;
+	case sizeof(__uint32_t):
+		ASSERT(irec->ino_un.ex_data->counted_nlinks.un32[ino_offset] > 0);
+		refs = --irec->ino_un.ex_data->counted_nlinks.un32[ino_offset];
+		break;
+	default:
+		ASSERT(0);
 	}
-	return ++(irec->ino_un.ex_data->counted_nlinks[ino_offset]);
-}
 
-static __uint32_t
-counted_nlink_8_dec(ino_tree_node_t *irec, int ino_offset)
-{
-	ASSERT(irec->ino_un.ex_data->counted_nlinks[ino_offset] > 0);
-	return --(irec->ino_un.ex_data->counted_nlinks[ino_offset]);
+	if (refs == 0)
+		irec->ino_un.ex_data->ino_reached &= ~IREC_MASK(ino_offset);
 }
 
-
-static nlink_ops_t nlinkops[] = {
-	{sizeof(__uint8_t) * XFS_INODES_PER_CHUNK,
-		disk_nlink_8_set, disk_nlink_8_get,
-		counted_nlink_8_get, counted_nlink_8_inc, counted_nlink_8_dec},
-	{sizeof(__uint16_t) * XFS_INODES_PER_CHUNK,
-		disk_nlink_16_set, disk_nlink_16_get,
-		counted_nlink_16_get, counted_nlink_16_inc, counted_nlink_16_dec},
-	{sizeof(__uint32_t) * XFS_INODES_PER_CHUNK,
-		disk_nlink_32_set, disk_nlink_32_get,
-		counted_nlink_32_get, counted_nlink_32_inc, counted_nlink_32_dec},
-};
-
-static void
-nlink_grow_8_to_16(ino_tree_node_t *irec)
+__uint32_t num_inode_references(struct ino_tree_node *irec, int ino_offset)
 {
-	__uint16_t	*new_nlinks;
-	int		i;
+	ASSERT(irec->ino_un.ex_data != NULL);
 
-	new_nlinks = malloc(sizeof(__uint16_t) * XFS_INODES_PER_CHUNK);
-	if (new_nlinks == NULL)
-		do_error(_("could not allocate expanded nlink array\n"));
-	for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
-		new_nlinks[i] = irec->disk_nlinks[i];
-	free(irec->disk_nlinks);
-	irec->disk_nlinks = (__uint8_t*)new_nlinks;
-
-	if (full_ino_ex_data) {
-		new_nlinks = malloc(sizeof(__uint16_t) * XFS_INODES_PER_CHUNK);
-		if (new_nlinks == NULL)
-			do_error(_("could not allocate expanded nlink array\n"));
-		for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
-			new_nlinks[i] = irec->ino_un.ex_data->counted_nlinks[i];
-		free(irec->ino_un.ex_data->counted_nlinks);
-		irec->ino_un.ex_data->counted_nlinks = (__uint8_t*)new_nlinks;
+	switch (irec->nlink_size) {
+	case sizeof(__uint8_t):
+		return irec->ino_un.ex_data->counted_nlinks.un8[ino_offset];
+	case sizeof(__uint16_t):
+		return irec->ino_un.ex_data->counted_nlinks.un16[ino_offset];
+	case sizeof(__uint32_t):
+		return irec->ino_un.ex_data->counted_nlinks.un32[ino_offset];
+	default:
+		ASSERT(0);
 	}
-	irec->nlinkops = &nlinkops[1];
 }
 
-static void
-nlink_grow_16_to_32(ino_tree_node_t *irec)
+void set_inode_disk_nlinks(struct ino_tree_node *irec, int ino_offset,
+		__uint32_t nlinks)
 {
-	__uint32_t	*new_nlinks;
-	int		i;
-
-	new_nlinks = malloc(sizeof(__uint32_t) * XFS_INODES_PER_CHUNK);
-	if (new_nlinks == NULL)
-		do_error(_("could not allocate expanded nlink array\n"));
-	for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
-		new_nlinks[i] = ((__int16_t*)&irec->disk_nlinks)[i];
-	free(irec->disk_nlinks);
-	irec->disk_nlinks = (__uint8_t*)new_nlinks;
+	switch (irec->nlink_size) {
+	case sizeof(__uint8_t):
+		if (nlinks < 0xff) {
+			irec->disk_nlinks.un8[ino_offset] = nlinks;
+			break;
+		}
+		nlink_grow_8_to_16(irec);
+		/*FALLTHRU*/
+	case sizeof(__uint16_t):
+		if (nlinks < 0xffff) {
+			irec->disk_nlinks.un16[ino_offset] = nlinks;
+			break;
+		}
+		nlink_grow_16_to_32(irec);
+		/*FALLTHRU*/
+	case sizeof(__uint32_t):
+		irec->disk_nlinks.un32[ino_offset] = nlinks;
+		break;
+	default:
+		ASSERT(0);
+	}
+}
 
-	if (full_ino_ex_data) {
-		new_nlinks = malloc(sizeof(__uint32_t) * XFS_INODES_PER_CHUNK);
-		if (new_nlinks == NULL)
-			do_error(_("could not allocate expanded nlink array\n"));
-		for (i = 0; i < XFS_INODES_PER_CHUNK; i++)
-			new_nlinks[i] = ((__int16_t*)&irec->ino_un.ex_data->counted_nlinks)[i];
-		free(irec->ino_un.ex_data->counted_nlinks);
-		irec->ino_un.ex_data->counted_nlinks = (__uint8_t*)new_nlinks;
+__uint32_t get_inode_disk_nlinks(struct ino_tree_node *irec, int ino_offset)
+{
+	switch (irec->nlink_size) {
+	case sizeof(__uint8_t):
+		return irec->disk_nlinks.un8[ino_offset];
+	case sizeof(__uint16_t):
+		return irec->disk_nlinks.un16[ino_offset];
+	case sizeof(__uint32_t):
+		return irec->disk_nlinks.un32[ino_offset];
+	default:
+		ASSERT(0);
 	}
-	irec->nlinkops = &nlinkops[2];
 }
 
 /*
@@ -254,14 +241,30 @@ alloc_ino_node(
 	irec->ino_isa_dir = 0;
 	irec->ir_free = (xfs_inofree_t) - 1;
 	irec->ino_un.ex_data = NULL;
-	irec->nlinkops = &nlinkops[0];
-	irec->disk_nlinks = calloc(1, nlinkops[0].nlink_size);
-	if (!irec->disk_nlinks)
-		do_error(_("could not allocate nlink array\n"));
+	irec->nlink_size = sizeof(__uint8_t);
+	irec->disk_nlinks.un8 = alloc_nlink_array(irec->nlink_size);
 	return irec;
 }
 
 static void
+free_nlink_array(union ino_nlink nlinks, __uint8_t nlink_size)
+{
+	switch (nlink_size) {
+	case sizeof(__uint8_t):
+		free(nlinks.un8);
+		break;
+	case sizeof(__uint16_t):
+		free(nlinks.un16);
+		break;
+	case sizeof(__uint32_t):
+		free(nlinks.un32);
+		break;
+	default:
+		ASSERT(0);
+	}
+}
+
+static void
 free_ino_tree_node(
 	struct ino_tree_node	*irec)
 {
@@ -269,11 +272,12 @@ free_ino_tree_node(
 	irec->avl_node.avl_forw = NULL;
 	irec->avl_node.avl_back = NULL;
 
-	free(irec->disk_nlinks);
+	free_nlink_array(irec->disk_nlinks, irec->nlink_size);
 	if (irec->ino_un.ex_data != NULL)  {
 		if (full_ino_ex_data) {
 			free(irec->ino_un.ex_data->parents);
-			free(irec->ino_un.ex_data->counted_nlinks);
+			free_nlink_array(irec->ino_un.ex_data->counted_nlinks,
+					 irec->nlink_size);
 		}
 		free(irec->ino_un.ex_data);
 
@@ -707,10 +711,23 @@ alloc_ex_data(ino_tree_node_t *irec)
 		do_error(_("could not malloc inode extra data\n"));
 
 	irec->ino_un.ex_data->parents = ptbl;
-	irec->ino_un.ex_data->counted_nlinks = calloc(1, irec->nlinkops->nlink_size);
 
-	if (irec->ino_un.ex_data->counted_nlinks == NULL)
-		do_error(_("could not malloc inode extra data\n"));
+	switch (irec->nlink_size) {
+	case sizeof(__uint8_t):
+		irec->ino_un.ex_data->counted_nlinks.un8 =
+			alloc_nlink_array(irec->nlink_size);
+		break;
+	case sizeof(__uint16_t):
+		irec->ino_un.ex_data->counted_nlinks.un16 =
+			alloc_nlink_array(irec->nlink_size);
+		break;
+	case sizeof(__uint32_t):
+		irec->ino_un.ex_data->counted_nlinks.un32 =
+			alloc_nlink_array(irec->nlink_size);
+		break;
+	default:
+		ASSERT(0);
+	}
 }
 
 void

[-- Attachment #3: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs