[PATCH 0/5] metadump: discontiguous directory block support

[PATCH 0/5] metadump: discontiguous directory block support

[Dave Chinner]

Hi folks,

In making xfs_repair handle discontiguous directory blocks properly,
it uncovered the fact that xfs_metadump has never handled
discontiguous directory blocks properly. It doesn't handle
discontiguous block format directories, and there are a couple of
other cases where it is just says "too hard" and gives up, leading
to un-obfuscated, corrupt or missing directory blocks in the
metadump image. xfs/291 on CRC enabled filesystems was causing all
three of these conditions to occur.

This patchset fixes metadump to fully support all forms of
discontiguous directory blocks. It changes the obfuscation code from
reading and extent at a time and trying to slice and dice the
objects within it - which will never work for objects that need CRC
recalculation as a result of obfuscation - to dealing with
individual objects. This does affect IO patterns somewhat - single
large contiguous IOs turn into multiple smaller sequential IOs - but
it means that we can use the object verifiers to do CRC
recalculation correctly.

It also means we can walk the extent tree to gather discontiguous
extents into a single buffer to build an object fom multiple IOs.
This is what all the other directory block IO does, and we need to
do it here too.

The result is that the code is simpler and more obvious in what it
does - the "walk over a large extent" code is generic rather than
object specific, and the discontiguous block code is separated from
the single block object code. Hence both cases are clearer and
easier to understand.

And it works, unlike the old code.

FWIW, with this fixed and xfs/291 passing, the only remaining
outstanding work that is blocking a 3.2.0 release is to trap IO
verifier errors in repair so we repair/rebuild objects based on CRC
errors.

Comments, flames, thoughts all welcome.

Cheers,

Dave.

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

[PATCH 1/5] metadump: sanitise write_buf/index return values

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Write_buf/write_index use confusing boolean values for return,
meaning that it's hard to tell what the correct error return is
supposed to be.  Convert them to return zero on success or a
negative errno otherwise so that it's clear what the error case is.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/metadump.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 117dc42..4104fcb 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -145,6 +145,8 @@ print_progress(const char *fmt, ...)
  * even if the dump is exactly aligned, the last index will be full of
  * zeros. If the last index entry is non-zero, the dump is incomplete.
  * Correspondingly, the last chunk will have a count < num_indicies.
+ *
+ * Return 0 for success, -1 for failure.
  */
 
 static int
@@ -156,12 +158,12 @@ write_index(void)
 	metablock->mb_count = cpu_to_be16(cur_index);
 	if (fwrite(metablock, (cur_index + 1) << BBSHIFT, 1, outf) != 1) {
 		print_warning("error writing to file: %s", strerror(errno));
-		return 0;
+		return -errno;
 	}
 
 	memset(block_index, 0, num_indicies * sizeof(__be64));
 	cur_index = 0;
-	return 1;
+	return 0;
 }
 
 static int
@@ -171,6 +173,7 @@ write_buf(
 	char		*data;
 	__int64_t	off;
 	int		i;
+	int		ret;
 
 	/*
 	 * Run the write verifier to recalculate the buffer CRCs and check
@@ -184,7 +187,7 @@ write_buf(
 	_("%s: write verifer failed on bno 0x%llx/0x%x\n"),
 				__func__, (long long)buf->bp->b_bn,
 				buf->bp->b_bcount);
-			return buf->bp->b_error;
+			return -buf->bp->b_error;
 		}
 	}
 
@@ -194,11 +197,12 @@ write_buf(
 		block_index[cur_index] = cpu_to_be64(off);
 		memcpy(&block_buffer[cur_index << BBSHIFT], data, BBSIZE);
 		if (++cur_index == num_indicies) {
-			if (!write_index())
-				return 0;
+			ret = write_index();
+			if (ret)
+				return ret;
 		}
 	}
-	return !seenint();
+	return seenint() ? -EINTR : 0;
 }
 
 
@@ -227,7 +231,7 @@ scan_btree(
 		rval = !stop_on_read_error;
 		goto pop_out;
 	}
-	if (!write_buf(iocur_top))
+	if (write_buf(iocur_top))
 		goto pop_out;
 
 	if (!(*func)(iocur_top->data, agno, agbno, level - 1, btype, arg))
@@ -1439,7 +1443,7 @@ process_bmbt_reclist(
 
 				default: ;
 			    }
-			if (!write_buf(iocur_top)) {
+			if (write_buf(iocur_top)) {
 				pop_cur();
 				return 0;
 			}
@@ -1748,7 +1752,7 @@ copy_inode_chunk(
 		xfs_dinode_calc_crc(mp, dip);
 	}
 skip_processing:
-	if (!write_buf(iocur_top))
+	if (write_buf(iocur_top))
 		goto pop_out;
 
 	inodes_copied += XFS_INODES_PER_CHUNK;
@@ -1866,7 +1870,7 @@ scan_ag(
 		if (stop_on_read_error)
 			goto pop_out;
 	} else {
-		if (!write_buf(iocur_top))
+		if (write_buf(iocur_top))
 			goto pop_out;
 	}
 
@@ -1881,7 +1885,7 @@ scan_ag(
 		if (stop_on_read_error)
 			goto pop_out;
 	} else {
-		if (!write_buf(iocur_top))
+		if (write_buf(iocur_top))
 			goto pop_out;
 	}
 
@@ -1896,7 +1900,7 @@ scan_ag(
 		if (stop_on_read_error)
 			goto pop_out;
 	} else {
-		if (!write_buf(iocur_top))
+		if (write_buf(iocur_top))
 			goto pop_out;
 	}
 
@@ -1910,7 +1914,7 @@ scan_ag(
 		if (stop_on_read_error)
 			goto pop_out;
 	} else {
-		if (!write_buf(iocur_top))
+		if (write_buf(iocur_top))
 			goto pop_out;
 	}
 
@@ -2015,7 +2019,7 @@ copy_log(void)
 		print_warning("cannot read log");
 		return !stop_on_read_error;
 	}
-	return write_buf(iocur_top);
+	return !write_buf(iocur_top);
 }
 
 static int
@@ -2121,7 +2125,7 @@ metadump_f(
 
 	/* write the remaining index */
 	if (!exitcode)
-		exitcode = !write_index();
+		exitcode = write_index() < 0;
 
 	if (progress_since_warning)
 		fputc('\n', (outf == stdout) ? stderr : stdout);
-- 
1.8.4.rc3

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

[PATCH 2/5] metadump: support writing discontiguous io cursors

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

To handle discontiguous buffers, metadump needs to be able to handle
io cursrors that use discontiguous buffer mappings. Factor
write_buf() to extract the data copy routine and use that to
implement support for both flat and discontiguous buffer maps.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/metadump.c | 45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 4104fcb..a8bc297 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -166,12 +166,34 @@ write_index(void)
 	return 0;
 }
 
+/*
+ * Return 0 for success, -errno for failure.
+ */
+static int
+write_buf_segment(
+	char		*data,
+	__int64_t	off,
+	int		len)
+{
+	int		i;
+	int		ret;
+
+	for (i = 0; i < len; i++, off++, data += BBSIZE) {
+		block_index[cur_index] = cpu_to_be64(off);
+		memcpy(&block_buffer[cur_index << BBSHIFT], data, BBSIZE);
+		if (++cur_index == num_indicies) {
+			ret = write_index();
+			if (ret)
+				return -EIO;
+		}
+	}
+	return 0;
+}
+
 static int
 write_buf(
 	iocur_t		*buf)
 {
-	char		*data;
-	__int64_t	off;
 	int		i;
 	int		ret;
 
@@ -191,15 +213,20 @@ write_buf(
 		}
 	}
 
-	for (i = 0, off = buf->bb, data = buf->data;
-			i < buf->blen;
-			i++, off++, data += BBSIZE) {
-		block_index[cur_index] = cpu_to_be64(off);
-		memcpy(&block_buffer[cur_index << BBSHIFT], data, BBSIZE);
-		if (++cur_index == num_indicies) {
-			ret = write_index();
+	/* handle discontiguous buffers */
+	if (!buf->bbmap) {
+		ret = write_buf_segment(buf->data, buf->bb, buf->blen);
+		if (ret)
+			return ret;
+	} else {
+		int	len = 0;
+		for (i = 0; i < buf->bbmap->nmaps; i++) {
+			ret = write_buf_segment(buf->data + BBTOB(len),
+						buf->bbmap->b[i].bm_bn,
+						buf->bbmap->b[i].bm_len);
 			if (ret)
 				return ret;
+			len += buf->bbmap->b[i].bm_len;
 		}
 	}
 	return seenint() ? -EINTR : 0;
-- 
1.8.4.rc3

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

[PATCH 3/5] metadump: separate single block objects from multiblock objects

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When trying to dump objects, we have to treat multi-block objects
differently to single block objects. Separate out the code paths for
single block vs multi-block objects so we can add a separate path
for multi-block objects.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/metadump.c | 119 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 82 insertions(+), 37 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index a8bc297..9fc08ed 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1361,9 +1361,79 @@ obfuscate_attr_blocks(
 	}
 }
 
-/* inode copy routines */
+static int
+process_single_fsb_objects(
+	xfs_dfiloff_t	o,
+	xfs_dfsbno_t	s,
+	xfs_dfilblks_t	c,
+	typnm_t		btype,
+	xfs_dfiloff_t	last)
+{
+	int		ret = 0;
+
+	push_cur();
+	set_cur(&typtab[btype], XFS_FSB_TO_DADDR(mp, s), c * blkbb,
+			DB_RING_IGN, NULL);
+
+	if (!iocur_top->data) {
+		xfs_agnumber_t	agno = XFS_FSB_TO_AGNO(mp, s);
+		xfs_agblock_t	agbno = XFS_FSB_TO_AGBNO(mp, s);
+
+		print_warning("cannot read %s block %u/%u (%llu)",
+				typtab[btype].name, agno, agbno, s);
+		if (stop_on_read_error)
+			ret = -EIO;
+		goto out_pop;
+
+	}
+
+	if (dont_obfuscate) {
+		ret = write_buf(iocur_top);
+		goto out_pop;
+	}
+
+	switch (btype) {
+	case TYP_DIR2:
+		if (o >= mp->m_dirleafblk)
+			break;
+
+		obfuscate_dir_data_blocks(iocur_top->data, o, c,
+					 last == mp->m_dirblkfsbs);
+		break;
+	case TYP_SYMLINK:
+		obfuscate_symlink_blocks(iocur_top->data, c);
+		break;
+	case TYP_ATTR:
+		obfuscate_attr_blocks(iocur_top->data, o, c);
+		break;
+	default:
+		break;
+	}
+	ret = write_buf(iocur_top);
+
+out_pop:
+	pop_cur();
+	return ret;
+}
 
 static int
+process_multi_fsb_objects(
+	xfs_dfiloff_t	o,
+	xfs_dfsbno_t	s,
+	xfs_dfilblks_t	c,
+	typnm_t		btype,
+	xfs_dfiloff_t	last)
+{
+	if (btype != TYP_DIR2) {
+		print_warning("bad type for multi-fsb object %d", btype);
+		return -EINVAL;
+	}
+
+	return process_single_fsb_objects(o, s, c, btype, last);
+}
+
+/* inode copy routines */
+static int
 process_bmbt_reclist(
 	xfs_bmbt_rec_t 		*rp,
 	int 			numrecs,
@@ -1377,6 +1447,7 @@ process_bmbt_reclist(
 	xfs_dfiloff_t		last;
 	xfs_agnumber_t		agno;
 	xfs_agblock_t		agbno;
+	int			error;
 
 	if (btype == TYP_DATA)
 		return 1;
@@ -1438,44 +1509,18 @@ process_bmbt_reclist(
 			break;
 		}
 
-		push_cur();
-		set_cur(&typtab[btype], XFS_FSB_TO_DADDR(mp, s), c * blkbb,
-				DB_RING_IGN, NULL);
-		if (iocur_top->data == NULL) {
-			print_warning("cannot read %s block %u/%u (%llu)",
-					typtab[btype].name, agno, agbno, s);
-			if (stop_on_read_error) {
-				pop_cur();
-				return 0;
-			}
-		} else {
-			if (!dont_obfuscate)
-			    switch (btype) {
-				case TYP_DIR2:
-					if (o < mp->m_dirleafblk)
-						obfuscate_dir_data_blocks(
-							iocur_top->data, o, c,
-							last == mp->m_dirblkfsbs);
-					break;
-
-				case TYP_SYMLINK:
-					obfuscate_symlink_blocks(
-						iocur_top->data, c);
-					break;
-
-				case TYP_ATTR:
-					obfuscate_attr_blocks(iocur_top->data,
-						o, c);
-					break;
-
-				default: ;
-			    }
-			if (write_buf(iocur_top)) {
-				pop_cur();
+		/* single filesystem block objects are trivial to handle */
+		if (btype != TYP_DIR2 || mp->m_dirblkfsbs == 1) {
+			error = process_single_fsb_objects(o, s, c, btype, last);
+			if (error)
 				return 0;
-			}
+			continue;
 		}
-		pop_cur();
+
+		/* multi-extent directory blocks */
+		error = process_multi_fsb_objects(o, s, c, btype, last);
+		if (error)
+			return 0;
 	}
 
 	return 1;
-- 
1.8.4.rc3

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

[PATCH 4/5] metadump: walk single fsb objects a block at a time

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

To be able to support arbitrary discontiguous extents in multi-block
objects, we need to be able to process a single object at a time
presented as a single flat buffer. Right now we pass an arbitrary
extent and have the individal object processing functions break it
up and keep track of inter-call state.

This greatly complicates the processing of directory objects, such
that certain formats are simply not handled at all. Instead, for
single block objects loop over the extent a block at a time, feeding
a whole object to the processing function and hence making the
extent walking generic instead of per object.

At thsi point multi-block directory objects still need to use the
existing code, so duplicate the old single block object code into it
so we can fix it up properly. This means directory block processing
can't be fully cleaned up yet.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/metadump.c | 180 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 107 insertions(+), 73 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index 9fc08ed..e973c5e 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1257,12 +1257,11 @@ obfuscate_dir_data_blocks(
 }
 
 static void
-obfuscate_symlink_blocks(
-	char			*block,
-	xfs_dfilblks_t		count)
+obfuscate_symlink_block(
+	char			*block)
 {
-	count <<= mp->m_sb.sb_blocklog;
-	obfuscate_path_components(block, count);
+	/* XXX: need to handle CRC headers */
+	obfuscate_path_components(block, mp->m_sb.sb_blocksize);
 }
 
 #define MAX_REMOTE_VALS		4095
@@ -1283,80 +1282,79 @@ add_remote_vals(
 		blockidx++;
 		length -= XFS_LBSIZE(mp);
 	}
+
+	if (attr_data.remote_val_count >= MAX_REMOTE_VALS) {
+		print_warning(
+"Overflowed attr obfuscation array. No longer obfuscating remote attrs.");
+	}
 }
 
 static void
-obfuscate_attr_blocks(
+obfuscate_attr_block(
 	char			*block,
-	xfs_dfiloff_t		offset,
-	xfs_dfilblks_t		count)
+	xfs_dfiloff_t		offset)
 {
 	xfs_attr_leafblock_t	*leaf;
-	int			c;
 	int			i;
 	int			nentries;
 	xfs_attr_leaf_entry_t 	*entry;
 	xfs_attr_leaf_name_local_t *local;
 	xfs_attr_leaf_name_remote_t *remote;
 
-	for (c = 0; c < count; c++, offset++, block += XFS_LBSIZE(mp)) {
+	leaf = (xfs_attr_leafblock_t *)block;
 
-		leaf = (xfs_attr_leafblock_t *)block;
-
-		if (be16_to_cpu(leaf->hdr.info.magic) != XFS_ATTR_LEAF_MAGIC) {
-			for (i = 0; i < attr_data.remote_val_count; i++) {
-				if (attr_data.remote_vals[i] == offset)
-					memset(block, 0, XFS_LBSIZE(mp));
-			}
-			continue;
+	if (be16_to_cpu(leaf->hdr.info.magic) != XFS_ATTR_LEAF_MAGIC) {
+		for (i = 0; i < attr_data.remote_val_count; i++) {
+			/* XXX: need to handle CRC headers */
+			if (attr_data.remote_vals[i] == offset)
+				memset(block, 0, XFS_LBSIZE(mp));
 		}
+		return;
+	}
+
+	nentries = be16_to_cpu(leaf->hdr.count);
+	if (nentries * sizeof(xfs_attr_leaf_entry_t) +
+			sizeof(xfs_attr_leaf_hdr_t) > XFS_LBSIZE(mp)) {
+		if (show_warnings)
+			print_warning("invalid attr count in inode %llu",
+					(long long)cur_ino);
+		return;
+	}
 
-		nentries = be16_to_cpu(leaf->hdr.count);
-		if (nentries * sizeof(xfs_attr_leaf_entry_t) +
-				sizeof(xfs_attr_leaf_hdr_t) > XFS_LBSIZE(mp)) {
+	for (i = 0, entry = &leaf->entries[0]; i < nentries; i++, entry++) {
+		if (be16_to_cpu(entry->nameidx) > XFS_LBSIZE(mp)) {
 			if (show_warnings)
-				print_warning("invalid attr count in inode %llu",
+				print_warning(
+				"invalid attr nameidx in inode %llu",
 						(long long)cur_ino);
-			continue;
+			break;
 		}
-
-		for (i = 0, entry = &leaf->entries[0]; i < nentries;
-				i++, entry++) {
-			if (be16_to_cpu(entry->nameidx) > XFS_LBSIZE(mp)) {
+		if (entry->flags & XFS_ATTR_LOCAL) {
+			local = xfs_attr3_leaf_name_local(leaf, i);
+			if (local->namelen == 0) {
 				if (show_warnings)
-					print_warning("invalid attr nameidx "
-							"in inode %llu",
-							(long long)cur_ino);
+					print_warning(
+				"zero length for attr name in inode %llu",
+						(long long)cur_ino);
 				break;
 			}
-			if (entry->flags & XFS_ATTR_LOCAL) {
-				local = xfs_attr3_leaf_name_local(leaf, i);
-				if (local->namelen == 0) {
-					if (show_warnings)
-						print_warning("zero length for "
-							"attr name in inode %llu",
-							(long long)cur_ino);
-					break;
-				}
-				generate_obfuscated_name(0, local->namelen,
-					&local->nameval[0]);
-				memset(&local->nameval[local->namelen], 0,
-					be16_to_cpu(local->valuelen));
-			} else {
-				remote = xfs_attr3_leaf_name_remote(leaf, i);
-				if (remote->namelen == 0 ||
-						remote->valueblk == 0) {
-					if (show_warnings)
-						print_warning("invalid attr "
-							"entry in inode %llu",
-							(long long)cur_ino);
-					break;
-				}
-				generate_obfuscated_name(0, remote->namelen,
-					&remote->name[0]);
-				add_remote_vals(be32_to_cpu(remote->valueblk),
-					be32_to_cpu(remote->valuelen));
+			generate_obfuscated_name(0, local->namelen,
+				&local->nameval[0]);
+			memset(&local->nameval[local->namelen], 0,
+				be16_to_cpu(local->valuelen));
+		} else {
+			remote = xfs_attr3_leaf_name_remote(leaf, i);
+			if (remote->namelen == 0 || remote->valueblk == 0) {
+				if (show_warnings)
+					print_warning(
+				"invalid attr entry in inode %llu",
+						(long long)cur_ino);
+				break;
 			}
+			generate_obfuscated_name(0, remote->namelen,
+						 &remote->name[0]);
+			add_remote_vals(be32_to_cpu(remote->valueblk),
+					be32_to_cpu(remote->valuelen));
 		}
 	}
 }
@@ -1369,7 +1367,9 @@ process_single_fsb_objects(
 	typnm_t		btype,
 	xfs_dfiloff_t	last)
 {
+	char		*dp;
 	int		ret = 0;
+	int		i;
 
 	push_cur();
 	set_cur(&typtab[btype], XFS_FSB_TO_DADDR(mp, s), c * blkbb,
@@ -1392,22 +1392,27 @@ process_single_fsb_objects(
 		goto out_pop;
 	}
 
-	switch (btype) {
-	case TYP_DIR2:
-		if (o >= mp->m_dirleafblk)
-			break;
+	dp = iocur_top->data;
+	for (i = 0; i < c; i++) {
+		switch (btype) {
+		case TYP_DIR2:
+			if (o >= mp->m_dirleafblk)
+				break;
 
-		obfuscate_dir_data_blocks(iocur_top->data, o, c,
-					 last == mp->m_dirblkfsbs);
-		break;
-	case TYP_SYMLINK:
-		obfuscate_symlink_blocks(iocur_top->data, c);
-		break;
-	case TYP_ATTR:
-		obfuscate_attr_blocks(iocur_top->data, o, c);
-		break;
-	default:
-		break;
+			obfuscate_dir_data_blocks(dp, o, 1,
+						  last == mp->m_dirblkfsbs);
+			break;
+		case TYP_SYMLINK:
+			obfuscate_symlink_block(dp);
+			break;
+		case TYP_ATTR:
+			obfuscate_attr_block(dp, o);
+			break;
+		default:
+			break;
+		}
+		o++;
+		dp += mp->m_sb.sb_blocksize;
 	}
 	ret = write_buf(iocur_top);
 
@@ -1424,12 +1429,41 @@ process_multi_fsb_objects(
 	typnm_t		btype,
 	xfs_dfiloff_t	last)
 {
+	int		ret = 0;
+
 	if (btype != TYP_DIR2) {
 		print_warning("bad type for multi-fsb object %d", btype);
 		return -EINVAL;
 	}
 
-	return process_single_fsb_objects(o, s, c, btype, last);
+	push_cur();
+	set_cur(&typtab[btype], XFS_FSB_TO_DADDR(mp, s), c * blkbb,
+			DB_RING_IGN, NULL);
+
+	if (!iocur_top->data) {
+		xfs_agnumber_t	agno = XFS_FSB_TO_AGNO(mp, s);
+		xfs_agblock_t	agbno = XFS_FSB_TO_AGBNO(mp, s);
+
+		print_warning("cannot read %s block %u/%u (%llu)",
+				typtab[btype].name, agno, agbno, s);
+		if (stop_on_read_error)
+			ret = -EIO;
+		goto out_pop;
+
+	}
+
+	if (dont_obfuscate || o >= mp->m_dirleafblk) {
+		ret = write_buf(iocur_top);
+		goto out_pop;
+	}
+
+	obfuscate_dir_data_blocks(iocur_top->data, o, c,
+				 last == mp->m_dirblkfsbs);
+	ret = write_buf(iocur_top);
+
+out_pop:
+	pop_cur();
+	return ret;
 }
 
 /* inode copy routines */
-- 
1.8.4.rc3

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

[PATCH 5/5] metadump: fully support discontiguous directory blocks

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Now that directory block obfuscation can handle single contiguous
directory blocks, we can make the multi-block code use discontiguous
buffers to read in an entire directory block at a time. This allows
us to pass a complete directory object to the processing function
and hence be able to process any sort of directory object regardless
of it's underlying layout.

With the, we can remove the multi-block loop from the directory
processing code and get rid of allt eh structures used to hold
inter-call state. This graeatly simplifies the code as well as
adding the additional functionality.

With this patch, a CRC enabled filesystem now passes xfs/291.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/metadump.c | 280 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 133 insertions(+), 147 deletions(-)

diff --git a/db/metadump.c b/db/metadump.c
index e973c5e..6ed392b 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -1111,24 +1111,11 @@ obfuscate_sf_attr(
 	}
 }
 
-/*
- * dir_data structure is used to track multi-fsblock dir2 blocks between extent
- * processing calls.
- */
-
-static struct dir_data_s {
-	int			end_of_data;
-	int			block_index;
-	int			offset_to_entry;
-	int			bad_block;
-} dir_data;
-
 static void
-obfuscate_dir_data_blocks(
-	char			*block,
-	xfs_dfiloff_t		offset,
-	xfs_dfilblks_t		count,
-	int			is_block_format)
+obfuscate_dir_data_block(
+	char		*block,
+	xfs_dfiloff_t	offset,
+	int		is_block_format)
 {
 	/*
 	 * we have to rely on the fileoffset and signature of the block to
@@ -1136,123 +1123,96 @@ obfuscate_dir_data_blocks(
 	 * for multi-fsblock dir blocks, if a name crosses an extent boundary,
 	 * ignore it and continue.
 	 */
-	int			c;
-	int			dir_offset;
-	char			*ptr;
-	char			*endptr;
-
-	if (is_block_format && count != mp->m_dirblkfsbs)
-		return; /* too complex to handle this rare case */
-
-	for (c = 0, endptr = block; c < count; c++) {
-
-		if (dir_data.block_index == 0) {
-			int		wantmagic;
-			struct xfs_dir2_data_hdr *datahdr;
-
-			datahdr = (struct xfs_dir2_data_hdr *)block;
-
-			if (offset % mp->m_dirblkfsbs != 0)
-				return;	/* corrupted, leave it alone */
-
-			dir_data.bad_block = 0;
-
-			if (is_block_format) {
-				xfs_dir2_leaf_entry_t	*blp;
-				xfs_dir2_block_tail_t	*btp;
-
-				btp = xfs_dir2_block_tail_p(mp, datahdr);
-				blp = xfs_dir2_block_leaf_p(btp);
-				if ((char *)blp > (char *)btp)
-					blp = (xfs_dir2_leaf_entry_t *)btp;
-
-				dir_data.end_of_data = (char *)blp - block;
-				wantmagic = XFS_DIR2_BLOCK_MAGIC;
-			} else { /* leaf/node format */
-				dir_data.end_of_data = mp->m_dirblkfsbs <<
-						mp->m_sb.sb_blocklog;
-				wantmagic = XFS_DIR2_DATA_MAGIC;
-			}
-			dir_data.offset_to_entry =
-					xfs_dir3_data_entry_offset(datahdr);
-
-			if (be32_to_cpu(datahdr->magic) != wantmagic) {
-				if (show_warnings)
-					print_warning("invalid magic in dir "
-						"inode %llu block %ld",
-						(long long)cur_ino,
-						(long)offset);
-				dir_data.bad_block = 1;
-			}
-		}
-		dir_data.block_index++;
-		if (dir_data.block_index == mp->m_dirblkfsbs)
-			dir_data.block_index = 0;
+	int		dir_offset;
+	char		*ptr;
+	char		*endptr;
+	int		end_of_data;
+	int		wantmagic;
+	struct xfs_dir2_data_hdr *datahdr;
+
+	datahdr = (struct xfs_dir2_data_hdr *)block;
+
+	if (offset % mp->m_dirblkfsbs != 0)
+		return;	/* corrupted, leave it alone */
+
+	if (is_block_format) {
+		xfs_dir2_leaf_entry_t	*blp;
+		xfs_dir2_block_tail_t	*btp;
+
+		btp = xfs_dir2_block_tail_p(mp, datahdr);
+		blp = xfs_dir2_block_leaf_p(btp);
+		if ((char *)blp > (char *)btp)
+			blp = (xfs_dir2_leaf_entry_t *)btp;
+
+		end_of_data = (char *)blp - block;
+		if (xfs_sb_version_hascrc(&mp->m_sb))
+			wantmagic = XFS_DIR3_BLOCK_MAGIC;
+		else
+			wantmagic = XFS_DIR2_BLOCK_MAGIC;
+	} else { /* leaf/node format */
+		end_of_data = mp->m_dirblkfsbs << mp->m_sb.sb_blocklog;
+		if (xfs_sb_version_hascrc(&mp->m_sb))
+			wantmagic = XFS_DIR3_DATA_MAGIC;
+		else
+			wantmagic = XFS_DIR2_DATA_MAGIC;
+	}
+
+	if (be32_to_cpu(datahdr->magic) != wantmagic) {
+		if (show_warnings)
+			print_warning(
+		"invalid magic in dir inode %llu block %ld",
+					(long long)cur_ino, (long)offset);
+		return;
+	}
 
-		if (dir_data.bad_block)
-			continue;
+	dir_offset = xfs_dir3_data_entry_offset(datahdr);
+	ptr = block + dir_offset;
+	endptr = block + mp->m_sb.sb_blocksize;
 
-		dir_offset = (dir_data.block_index << mp->m_sb.sb_blocklog) +
-				dir_data.offset_to_entry;
-
-		ptr = endptr + dir_data.offset_to_entry;
-		endptr += mp->m_sb.sb_blocksize;
-
-		while (ptr < endptr && dir_offset < dir_data.end_of_data) {
-			xfs_dir2_data_entry_t	*dep;
-			xfs_dir2_data_unused_t	*dup;
-			int			length;
-
-			dup = (xfs_dir2_data_unused_t *)ptr;
-
-			if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
-				int	length = be16_to_cpu(dup->length);
-				if (dir_offset + length > dir_data.end_of_data ||
-						length == 0 || (length &
-						 (XFS_DIR2_DATA_ALIGN - 1))) {
-					if (show_warnings)
-						print_warning("invalid length "
-							"for dir free space in "
-							"inode %llu",
-							(long long)cur_ino);
-					dir_data.bad_block = 1;
-					break;
-				}
-				if (be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)) !=
-						dir_offset) {
-					dir_data.bad_block = 1;
-					break;
-				}
-				dir_offset += length;
-				ptr += length;
-				if (dir_offset >= dir_data.end_of_data ||
-						ptr >= endptr)
-					break;
-			}
+	while (ptr < endptr && dir_offset < end_of_data) {
+		xfs_dir2_data_entry_t	*dep;
+		xfs_dir2_data_unused_t	*dup;
+		int			length;
 
-			dep = (xfs_dir2_data_entry_t *)ptr;
-			length = xfs_dir3_data_entsize(mp, dep->namelen);
+		dup = (xfs_dir2_data_unused_t *)ptr;
 
-			if (dir_offset + length > dir_data.end_of_data ||
-					ptr + length > endptr) {
+		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
+			int	length = be16_to_cpu(dup->length);
+			if (dir_offset + length > end_of_data ||
+			    !length || (length & (XFS_DIR2_DATA_ALIGN - 1))) {
 				if (show_warnings)
-					print_warning("invalid length for "
-						"dir entry name in inode %llu",
+					print_warning(
+			"invalid length for dir free space in inode %llu",
 						(long long)cur_ino);
-				break;
-			}
-			if (be16_to_cpu(*xfs_dir3_data_entry_tag_p(mp, dep)) !=
-					dir_offset) {
-				dir_data.bad_block = 1;
-				break;
+				return;
 			}
-			generate_obfuscated_name(be64_to_cpu(dep->inumber),
-					dep->namelen, &dep->name[0]);
+			if (be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)) !=
+					dir_offset)
+				return;
 			dir_offset += length;
 			ptr += length;
+			if (dir_offset >= end_of_data || ptr >= endptr)
+				return;
+		}
+
+		dep = (xfs_dir2_data_entry_t *)ptr;
+		length = xfs_dir3_data_entsize(mp, dep->namelen);
+
+		if (dir_offset + length > end_of_data ||
+		    ptr + length > endptr) {
+			if (show_warnings)
+				print_warning(
+			"invalid length for dir entry name in inode %llu",
+					(long long)cur_ino);
+			return;
 		}
-		dir_data.offset_to_entry = dir_offset &
-						(mp->m_sb.sb_blocksize - 1);
+		if (be16_to_cpu(*xfs_dir3_data_entry_tag_p(mp, dep)) !=
+				dir_offset)
+			return;
+		generate_obfuscated_name(be64_to_cpu(dep->inumber),
+					 dep->namelen, &dep->name[0]);
+		dir_offset += length;
+		ptr += length;
 	}
 }
 
@@ -1399,8 +1359,8 @@ process_single_fsb_objects(
 			if (o >= mp->m_dirleafblk)
 				break;
 
-			obfuscate_dir_data_blocks(dp, o, 1,
-						  last == mp->m_dirblkfsbs);
+			obfuscate_dir_data_block(dp, o,
+						 last == mp->m_dirblkfsbs);
 			break;
 		case TYP_SYMLINK:
 			obfuscate_symlink_block(dp);
@@ -1421,6 +1381,12 @@ out_pop:
 	return ret;
 }
 
+/*
+ * Static map to aggregate multiple extents into a single directory block.
+ */
+static struct bbmap mfsb_map;
+static int mfsb_length;
+
 static int
 process_multi_fsb_objects(
 	xfs_dfiloff_t	o,
@@ -1436,33 +1402,54 @@ process_multi_fsb_objects(
 		return -EINVAL;
 	}
 
-	push_cur();
-	set_cur(&typtab[btype], XFS_FSB_TO_DADDR(mp, s), c * blkbb,
-			DB_RING_IGN, NULL);
+	while (c > 0) {
+		unsigned int	bm_len;
 
-	if (!iocur_top->data) {
-		xfs_agnumber_t	agno = XFS_FSB_TO_AGNO(mp, s);
-		xfs_agblock_t	agbno = XFS_FSB_TO_AGBNO(mp, s);
+		if (mfsb_length + c >= mp->m_dirblkfsbs) {
+			bm_len = mp->m_dirblkfsbs - mfsb_length;
+			mfsb_length = 0;
+		} else {
+			mfsb_length += c;
+			bm_len = c;
+		}
 
-		print_warning("cannot read %s block %u/%u (%llu)",
-				typtab[btype].name, agno, agbno, s);
-		if (stop_on_read_error)
-			ret = -EIO;
-		goto out_pop;
+		mfsb_map.b[mfsb_map.nmaps].bm_bn = XFS_FSB_TO_DADDR(mp, s);
+		mfsb_map.b[mfsb_map.nmaps].bm_len = XFS_FSB_TO_BB(mp, bm_len);
+		mfsb_map.nmaps++;
 
-	}
+		if (mfsb_length == 0) {
+			push_cur();
+			set_cur(&typtab[btype], 0, 0, DB_RING_IGN, &mfsb_map);
+			if (!iocur_top->data) {
+				xfs_agnumber_t	agno = XFS_FSB_TO_AGNO(mp, s);
+				xfs_agblock_t	agbno = XFS_FSB_TO_AGBNO(mp, s);
 
-	if (dont_obfuscate || o >= mp->m_dirleafblk) {
-		ret = write_buf(iocur_top);
-		goto out_pop;
-	}
+				print_warning("cannot read %s block %u/%u (%llu)",
+						typtab[btype].name, agno, agbno, s);
+				if (stop_on_read_error)
+					ret = -1;
+				goto out_pop;
 
-	obfuscate_dir_data_blocks(iocur_top->data, o, c,
-				 last == mp->m_dirblkfsbs);
-	ret = write_buf(iocur_top);
+			}
 
+			if (dont_obfuscate || o >= mp->m_dirleafblk) {
+				ret = write_buf(iocur_top);
+				goto out_pop;
+			}
+
+			obfuscate_dir_data_block(iocur_top->data, o,
+						  last == mp->m_dirblkfsbs);
+			ret = write_buf(iocur_top);
 out_pop:
-	pop_cur();
+			pop_cur();
+			mfsb_map.nmaps = 0;
+			if (ret)
+				break;
+		}
+		c -= bm_len;
+		s += bm_len;
+	}
+
 	return ret;
 }
 
@@ -1750,7 +1737,6 @@ process_inode(
 	/* copy appropriate data fork metadata */
 	switch (be16_to_cpu(dip->di_mode) & S_IFMT) {
 		case S_IFDIR:
-			memset(&dir_data, 0, sizeof(dir_data));
 			success = process_inode_data(dip, TYP_DIR2);
 			break;
 		case S_IFLNK:
-- 
1.8.4.rc3

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

Re: [PATCH 0/5] metadump: discontiguous directory block support

[Dave Chinner]

ping?

On Thu, Jan 23, 2014 at 09:23:50PM +1100, Dave Chinner wrote:
> Hi folks,
> 
> In making xfs_repair handle discontiguous directory blocks properly,
> it uncovered the fact that xfs_metadump has never handled
> discontiguous directory blocks properly. It doesn't handle
> discontiguous block format directories, and there are a couple of
> other cases where it is just says "too hard" and gives up, leading
> to un-obfuscated, corrupt or missing directory blocks in the
> metadump image. xfs/291 on CRC enabled filesystems was causing all
> three of these conditions to occur.
> 
> This patchset fixes metadump to fully support all forms of
> discontiguous directory blocks. It changes the obfuscation code from
> reading and extent at a time and trying to slice and dice the
> objects within it - which will never work for objects that need CRC
> recalculation as a result of obfuscation - to dealing with
> individual objects. This does affect IO patterns somewhat - single
> large contiguous IOs turn into multiple smaller sequential IOs - but
> it means that we can use the object verifiers to do CRC
> recalculation correctly.
> 
> It also means we can walk the extent tree to gather discontiguous
> extents into a single buffer to build an object fom multiple IOs.
> This is what all the other directory block IO does, and we need to
> do it here too.
> 
> The result is that the code is simpler and more obvious in what it
> does - the "walk over a large extent" code is generic rather than
> object specific, and the discontiguous block code is separated from
> the single block object code. Hence both cases are clearer and
> easier to understand.
> 
> And it works, unlike the old code.
> 
> FWIW, with this fixed and xfs/291 passing, the only remaining
> outstanding work that is blocking a 3.2.0 release is to trap IO
> verifier errors in repair so we repair/rebuild objects based on CRC
> errors.
> 
> Comments, flames, thoughts all welcome.
> 
> Cheers,
> 
> Dave.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/5] metadump: sanitise write_buf/index return values

[Christoph Hellwig]

Looks good,

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

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

Re: [PATCH 2/5] metadump: support writing discontiguous io cursors

[Christoph Hellwig]

On Thu, Jan 23, 2014 at 09:23:52PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> To handle discontiguous buffers, metadump needs to be able to handle
> io cursrors that use discontiguous buffer mappings. Factor
> write_buf() to extract the data copy routine and use that to
> implement support for both flat and discontiguous buffer maps.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

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

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

Re: [PATCH 3/5] metadump: separate single block objects from multiblock objects

[Christoph Hellwig]

On Thu, Jan 23, 2014 at 09:23:53PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When trying to dump objects, we have to treat multi-block objects
> differently to single block objects. Separate out the code paths for
> single block vs multi-block objects so we can add a separate path
> for multi-block objects.

Looks good, but two minor style nitpicks below.

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

>  static int
> +process_multi_fsb_objects(
> +	xfs_dfiloff_t	o,
> +	xfs_dfsbno_t	s,
> +	xfs_dfilblks_t	c,
> +	typnm_t		btype,
> +	xfs_dfiloff_t	last)
> +{
> +	if (btype != TYP_DIR2) {
> +		print_warning("bad type for multi-fsb object %d", btype);
> +		return -EINVAL;
> +	}
> +
> +	return process_single_fsb_objects(o, s, c, btype, last);

I'd prefer a switch with a default statement for the unknown type here,
as that leads to nicer extensibility.

> +		/* single filesystem block objects are trivial to handle */
> +		if (btype != TYP_DIR2 || mp->m_dirblkfsbs == 1) {
> +			error = process_single_fsb_objects(o, s, c, btype, last);
> +			if (error)
>  				return 0;
> +			continue;
>  		}
> +
> +		/* multi-extent directory blocks */
> +		error = process_multi_fsb_objects(o, s, c, btype, last);
> +		if (error)
> +			return 0;

An if / else would look a little more obvious here, not that it really
matters all that much.

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

Re: [PATCH 4/5] metadump: walk single fsb objects a block at a time

[Christoph Hellwig]

Looks good,

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

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

Re: [PATCH 5/5] metadump: fully support discontiguous directory blocks

[Christoph Hellwig]

Looks good,

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

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

Re: [PATCH 3/5] metadump: separate single block objects from multiblock objects

[Dave Chinner]

On Mon, Feb 03, 2014 at 07:09:07AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 23, 2014 at 09:23:53PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When trying to dump objects, we have to treat multi-block objects
> > differently to single block objects. Separate out the code paths for
> > single block vs multi-block objects so we can add a separate path
> > for multi-block objects.
> 
> Looks good, but two minor style nitpicks below.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> >  static int
> > +process_multi_fsb_objects(
> > +	xfs_dfiloff_t	o,
> > +	xfs_dfsbno_t	s,
> > +	xfs_dfilblks_t	c,
> > +	typnm_t		btype,
> > +	xfs_dfiloff_t	last)
> > +{
> > +	if (btype != TYP_DIR2) {
> > +		print_warning("bad type for multi-fsb object %d", btype);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return process_single_fsb_objects(o, s, c, btype, last);
> 
> I'd prefer a switch with a default statement for the unknown type here,
> as that leads to nicer extensibility.

Ok, I'll fix that up.

> > +		/* single filesystem block objects are trivial to handle */
> > +		if (btype != TYP_DIR2 || mp->m_dirblkfsbs == 1) {
> > +			error = process_single_fsb_objects(o, s, c, btype, last);
> > +			if (error)
> >  				return 0;
> > +			continue;
> >  		}
> > +
> > +		/* multi-extent directory blocks */
> > +		error = process_multi_fsb_objects(o, s, c, btype, last);
> > +		if (error)
> > +			return 0;
> 
> An if / else would look a little more obvious here, not that it really
> matters all that much.

Easy enough to do.

Thanks for the reviews, Christoph.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/5] metadump: sanitise write_buf/index return values

[Mark Tinguely]

On 01/23/14 04:23, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Write_buf/write_index use confusing boolean values for return,
> meaning that it's hard to tell what the correct error return is
> supposed to be.  Convert them to return zero on success or a
> negative errno otherwise so that it's clear what the error case is.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---

Looks like this patch broke metadumps on some corrupted filesystems. 
This is a legacy filesystem that has zeroes overwriting the SB/AGF/AGI 
on AG 1/2/3:


# xfs_metadump -wgo /dev/sda8 myfile.metadata
xfs_agf_read_verify: XFS_CORRUPTION_ERROR
xfs_metadump: cannot init perag data (117). Continuing anyway.
Copied 64 of 64 inodes (0 of 4 AGs) 
xfs_agf_write_verify: XF
S_CORRUPTION_ERROR
write_buf: write verifer failed on bno 0x1100919/0x200
(no output)

commit dd9093de944cd802427bd42953ad5ccc1d5fb875 before it:

# xfs_metadump -wgo /dev/sda8 myfile.metadata
xfs_agf_read_verify: XFS_CORRUPTION_ERROR
xfs_metadump: cannot init perag data (117). Continuing anyway.
Copied 64 of 64 inodes (0 of 4 AGs) 
xfs_agf_write_verify: XF
S_CORRUPTION_ERROR
write_buf: write verifer failed on bno 0x1100919/0x200
Copying free space trees of AG 1
xfs_metadump: invalid block number (0) in bnobt root in agf 1
xfs_metadump: invalid block number (0) in cntbt root in agf 1
xfs_metadump: invalid block number (0) in inobt root in agi 1
Copying free space trees of AG 2
xfs_metadump: invalid block number (0) in bnobt root in agf 2
xfs_metadump: invalid block number (0) in cntbt root in agf 2
xfs_metadump: invalid block number (0) in inobt root in agi 2
Copying free space trees of AG 3
xfs_metadump: invalid block number (4294967295) in bnobt root in agf 3
xfs_metadump: invalid block number (4294967295) in cntbt root in agf 3
xfs_metadump: invalid block number (0) in inobt root in agi 3
Copying log

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

Re: [PATCH 1/5] metadump: sanitise write_buf/index return values

[Dave Chinner]

On Thu, Feb 13, 2014 at 01:30:00PM -0600, Mark Tinguely wrote:
> On 01/23/14 04:23, Dave Chinner wrote:
> >From: Dave Chinner<dchinner@redhat.com>
> >
> >Write_buf/write_index use confusing boolean values for return,
> >meaning that it's hard to tell what the correct error return is
> >supposed to be.  Convert them to return zero on success or a
> >negative errno otherwise so that it's clear what the error case is.
> >
> >Signed-off-by: Dave Chinner<dchinner@redhat.com>
> >---
> 
> Looks like this patch broke metadumps on some corrupted filesystems.
> This is a legacy filesystem that has zeroes overwriting the
> SB/AGF/AGI on AG 1/2/3:
> 
> 
> # xfs_metadump -wgo /dev/sda8 myfile.metadata
> xfs_agf_read_verify: XFS_CORRUPTION_ERROR
> xfs_metadump: cannot init perag data (117). Continuing anyway.
> Copied 64 of 64 inodes (0 of 4 AGs) xfs_agf_write_verify: XF
> S_CORRUPTION_ERROR
> write_buf: write verifer failed on bno 0x1100919/0x200
> (no output)

Where did it crash? Can you post the stack trace from gdb? Even
better, can you send a patch to fix the problem? ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 1/5] metadump: sanitise write_buf/index return values

[Mark Tinguely]

On 02/13/14 20:20, Dave Chinner wrote:
> On Thu, Feb 13, 2014 at 01:30:00PM -0600, Mark Tinguely wrote:
>> On 01/23/14 04:23, Dave Chinner wrote:
>>> From: Dave Chinner<dchinner@redhat.com>
>>>
>>> Write_buf/write_index use confusing boolean values for return,
>>> meaning that it's hard to tell what the correct error return is
>>> supposed to be.  Convert them to return zero on success or a
>>> negative errno otherwise so that it's clear what the error case is.
>>>
>>> Signed-off-by: Dave Chinner<dchinner@redhat.com>
>>> ---
>>
>> Looks like this patch broke metadumps on some corrupted filesystems.
>> This is a legacy filesystem that has zeroes overwriting the
>> SB/AGF/AGI on AG 1/2/3:
>>
>>
>> # xfs_metadump -wgo /dev/sda8 myfile.metadata
>> xfs_agf_read_verify: XFS_CORRUPTION_ERROR
>> xfs_metadump: cannot init perag data (117). Continuing anyway.
>> Copied 64 of 64 inodes (0 of 4 AGs) xfs_agf_write_verify: XF
>> S_CORRUPTION_ERROR
>> write_buf: write verifer failed on bno 0x1100919/0x200
>> (no output)
>
> Where did it crash? Can you post the stack trace from gdb? Even
> better, can you send a patch to fix the problem? ;)
>
> Cheers,
>
> Dave.

No crash, just exits without performing the dump.

--Mark.

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

Re: [PATCH 1/5] metadump: sanitise write_buf/index return values

[Mark Tinguely]

On 02/14/14 13:51, Mark Tinguely wrote:
> On 02/13/14 20:20, Dave Chinner wrote:
>> On Thu, Feb 13, 2014 at 01:30:00PM -0600, Mark Tinguely wrote:
>>> On 01/23/14 04:23, Dave Chinner wrote:
>>>> From: Dave Chinner<dchinner@redhat.com>
>>>>
>>>> Write_buf/write_index use confusing boolean values for return,
>>>> meaning that it's hard to tell what the correct error return is
>>>> supposed to be. Convert them to return zero on success or a
>>>> negative errno otherwise so that it's clear what the error case is.
>>>>
>>>> Signed-off-by: Dave Chinner<dchinner@redhat.com>
>>>> ---
>>>
>>> Looks like this patch broke metadumps on some corrupted filesystems.
>>> This is a legacy filesystem that has zeroes overwriting the
>>> SB/AGF/AGI on AG 1/2/3:
>>>
>>>
>>> # xfs_metadump -wgo /dev/sda8 myfile.metadata
>>> xfs_agf_read_verify: XFS_CORRUPTION_ERROR
>>> xfs_metadump: cannot init perag data (117). Continuing anyway.
>>> Copied 64 of 64 inodes (0 of 4 AGs) xfs_agf_write_verify: XF
>>> S_CORRUPTION_ERROR
>>> write_buf: write verifer failed on bno 0x1100919/0x200
>>> (no output)
>>
>> Where did it crash? Can you post the stack trace from gdb? Even
>> better, can you send a patch to fix the problem? ;)
>>
>> Cheers,
>>
>> Dave.
>
> No crash, just exits without performing the dump.
>
> --Mark.
>

The error from the verifier prevents the metadump from completing.
You will want to fix it another way...

--Mark.

---
  db/metadump.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/db/metadump.c
===================================================================
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -209,7 +209,7 @@ write_buf(
  	_("%s: write verifer failed on bno 0x%llx/0x%x\n"),
  				__func__, (long long)buf->bp->b_bn,
  				buf->bp->b_bcount);
-			return -buf->bp->b_error;
+			return 0;
  		}
  	}

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