[PATCH 9/8] libxlog: fix log buffer alignment

[PATCH 9/8] libxlog: fix log buffer alignment

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The libxlog sync changed the way log buffers used in recovery are
aligned to sector sizes. The old code used to check for a zero log
sector size and not do any alignment - this check was removed from
the kernel code because there is always log sector size configured.
It turns out that userspace *never* sets the log sector size, so
userspace has always simply ignored the alignment in log replay
(oops!).

This problem manifested itself as detecting a freshly mkfs'd
filesystem as having a totally zeroed log and hence xfs_check was
emitting output and hence xfstests was refusing to run.

This patch correctly configures the callers that use the log
recovery code so that the log sector size is set appropriately and
ensures that log recovery operates as expected.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/sb.c                   |    5 +++++
 libxlog/xfs_log_recover.c |    5 ++++-
 logprint/logprint.c       |    6 ++++++
 repair/phase2.c           |    4 ++++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/db/sb.c b/db/sb.c
index d83db9c..4da1f6a 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -235,10 +235,15 @@ sb_logcheck(void)
 		x.logdev = x.ddev;
 	x.logBBsize = XFS_FSB_TO_BB(mp, mp->m_sb.sb_logblocks);
 	x.logBBstart = XFS_FSB_TO_DADDR(mp, mp->m_sb.sb_logstart);
+	x.lbsize = BBSIZE;
+	if (xfs_sb_version_hassector(&mp->m_sb))
+		x.lbsize <<= (mp->m_sb.sb_logsectlog - BBSHIFT);
+
 	log.l_dev = (mp->m_sb.sb_logstart == 0) ? x.logdev : x.ddev;
 	log.l_logsize = BBTOB(log.l_logBBsize);
 	log.l_logBBsize = x.logBBsize;
 	log.l_logBBstart = x.logBBstart;
+	log.l_sectBBsize  = BTOBB(x.lbsize);
 	log.l_mp = mp;
 
 	if (xlog_find_tail(&log, &head_blk, &tail_blk)) {
diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
index ad53e86..028d1e3 100644
--- a/libxlog/xfs_log_recover.c
+++ b/libxlog/xfs_log_recover.c
@@ -21,7 +21,10 @@
 #define xfs_readonly_buftarg(buftarg)			(0)
 
 /* avoid set-but-unused var warning. gcc is not very bright. */
-#define xlog_clear_stale_blocks(log, taillsn)		((taillsn) = (taillsn))
+#define xlog_clear_stale_blocks(log, taillsn)		({ \
+	(taillsn) = (taillsn); \
+	(0); \
+})
 
 
 /*
diff --git a/logprint/logprint.c b/logprint/logprint.c
index 813424a..3fbcdba 100644
--- a/logprint/logprint.c
+++ b/logprint/logprint.c
@@ -94,6 +94,10 @@ logstat(xfs_mount_t *mp)
 
 		x.logBBsize = XFS_FSB_TO_BB(mp, sb->sb_logblocks);
 		x.logBBstart = XFS_FSB_TO_DADDR(mp, sb->sb_logstart);
+		x.lbsize = BBSIZE;
+		if (xfs_sb_version_hassector(sb))
+			x.lbsize <<= (sb->sb_logsectlog - BBSHIFT);
+
 		if (!x.logname && sb->sb_logstart == 0) {
 			fprintf(stderr, _("    external log device not specified\n\n"));
 			usage();
@@ -105,6 +109,7 @@ logstat(xfs_mount_t *mp)
 		stat(x.dname, &s);
 		x.logBBsize = s.st_size >> 9;
 		x.logBBstart = 0;
+		x.lbsize = BBSIZE;
 	}
 
 
@@ -235,6 +240,7 @@ main(int argc, char **argv)
 	log.l_logsize     = BBTOB(x.logBBsize);
 	log.l_logBBstart  = x.logBBstart;
 	log.l_logBBsize   = x.logBBsize;
+	log.l_sectBBsize  = BTOBB(x.lbsize);
 	log.l_mp          = &mount;
 
 	switch (print_operation) {
diff --git a/repair/phase2.c b/repair/phase2.c
index 23b457a..382cd7b 100644
--- a/repair/phase2.c
+++ b/repair/phase2.c
@@ -47,11 +47,15 @@ zero_log(xfs_mount_t *mp)
 		x.logdev = x.ddev;
 	x.logBBsize = XFS_FSB_TO_BB(mp, mp->m_sb.sb_logblocks);
 	x.logBBstart = XFS_FSB_TO_DADDR(mp, mp->m_sb.sb_logstart);
+	x.lbsize = BBSIZE;
+	if (xfs_sb_version_hassector(&mp->m_sb))
+		x.lbsize <<= (mp->m_sb.sb_logsectlog - BBSHIFT);
 
 	log.l_dev = logdev;
 	log.l_logsize = BBTOB(x.logBBsize);
 	log.l_logBBsize = x.logBBsize;
 	log.l_logBBstart = x.logBBstart;
+	log.l_sectBBsize  = BTOBB(x.lbsize);
 	log.l_mp = mp;
 	if (xfs_sb_version_hassector(&mp->m_sb)) {
 		log.l_sectbb_log = mp->m_sb.sb_logsectlog - BBSHIFT;

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

Re: [PATCH 9/8] libxlog: fix log buffer alignment

[Mark Tinguely]

On 04/30/13 19:17, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> The libxlog sync changed the way log buffers used in recovery are
> aligned to sector sizes. The old code used to check for a zero log
> sector size and not do any alignment - this check was removed from
> the kernel code because there is always log sector size configured.
> It turns out that userspace *never* sets the log sector size, so
> userspace has always simply ignored the alignment in log replay
> (oops!).
>
> This problem manifested itself as detecting a freshly mkfs'd
> filesystem as having a totally zeroed log and hence xfs_check was
> emitting output and hence xfstests was refusing to run.
>
> This patch correctly configures the callers that use the log
> recovery code so that the log sector size is set appropriately and
> ensures that log recovery operates as expected.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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

[PATCH 10/8] xfsdb: re-instate DA btree node headers

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When removing the dirv1 code, it wasn't immediately obvious that the
dir v2 code used a small chunk of the dirv1 field definitions. i.e.
those for the DA btree node headers. Hence bits of xfs_db didn't
work as expected, and some tests failed in non-obvious ways. e.g
test 073 failed with this additional line of output:

Use of uninitialized value $logstart in numeric gt (>) at /home/dave/src/xfstests-dev/src/fill2fs line 84

which was the result of the command:

	xfs_db -r -c sb -c $TEST_DEV

giving an unexpectedly incorrect output.

Re-instate the needed field definitions and rename them from "DIR"
to "DA" so it is obvious they are for decoding DA Btree format
blocks.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 db/dir2.c  |   30 +++++++++++++++++++++++++++---
 db/dir2.h  |    4 ++++
 db/field.c |    7 +++++++
 db/field.h |   15 +++------------
 4 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/db/dir2.c b/db/dir2.c
index a13fa25..a539f2d 100644
--- a/db/dir2.c
+++ b/db/dir2.c
@@ -84,9 +84,9 @@ const field_t	dir2_flds[] = {
 	  FLD_ARRAY|FLD_COUNT, TYP_NONE },
 	{ "ltail", FLDT_DIR2_LEAF_TAIL, dir2_leaf_tail_offset,
 	  dir2_leaf_tail_count, FLD_OFFSET|FLD_COUNT, TYP_NONE },
-	{ "nhdr", FLDT_DIR_NODE_HDR, OI(NOFF(hdr)), dir2_node_hdr_count,
+	{ "nhdr", FLDT_DA_NODE_HDR, OI(NOFF(hdr)), dir2_node_hdr_count,
 	  FLD_COUNT, TYP_NONE },
-	{ "nbtree", FLDT_DIR_NODE_ENTRY, OI(NOFF(btree)), dir2_node_btree_count,
+	{ "nbtree", FLDT_DA_NODE_ENTRY, OI(NOFF(btree)), dir2_node_btree_count,
 	  FLD_ARRAY|FLD_COUNT, TYP_NONE },
 	{ "fhdr", FLDT_DIR2_FREE_HDR, OI(FOFF(hdr)), dir2_free_hdr_count,
 	  FLD_COUNT, TYP_NONE },
@@ -144,7 +144,7 @@ const field_t	dir2_leaf_entry_flds[] = {
 
 #define	LHOFF(f)	bitize(offsetof(xfs_dir2_leaf_hdr_t, f))
 const field_t	dir2_leaf_hdr_flds[] = {
-	{ "info", FLDT_DIR_BLKINFO, OI(LHOFF(info)), C1, 0, TYP_NONE },
+	{ "info", FLDT_DA_BLKINFO, OI(LHOFF(info)), C1, 0, TYP_NONE },
 	{ "count", FLDT_UINT16D, OI(LHOFF(count)), C1, 0, TYP_NONE },
 	{ "stale", FLDT_UINT16D, OI(LHOFF(stale)), C1, 0, TYP_NONE },
 	{ NULL }
@@ -165,6 +165,30 @@ const field_t	dir2_free_hdr_flds[] = {
 	{ NULL }
 };
 
+#define	DBOFF(f)	bitize(offsetof(xfs_da_blkinfo_t, f))
+const field_t	da_blkinfo_flds[] = {
+	{ "forw", FLDT_DIRBLOCK, OI(DBOFF(forw)), C1, 0, TYP_INODATA },
+	{ "back", FLDT_DIRBLOCK, OI(DBOFF(back)), C1, 0, TYP_INODATA },
+	{ "magic", FLDT_UINT16X, OI(DBOFF(magic)), C1, 0, TYP_NONE },
+	{ "pad", FLDT_UINT16X, OI(DBOFF(pad)), C1, FLD_SKIPALL, TYP_NONE },
+	{ NULL }
+};
+
+#define	EOFF(f)	bitize(offsetof(xfs_da_node_entry_t, f))
+const field_t	da_node_entry_flds[] = {
+	{ "hashval", FLDT_UINT32X, OI(EOFF(hashval)), C1, 0, TYP_NONE },
+	{ "before", FLDT_DIRBLOCK, OI(EOFF(before)), C1, 0, TYP_INODATA },
+	{ NULL }
+};
+
+#define	HOFF(f)	bitize(offsetof(xfs_da_node_hdr_t, f))
+const field_t	da_node_hdr_flds[] = {
+	{ "info", FLDT_DA_BLKINFO, OI(HOFF(info)), C1, 0, TYP_NONE },
+	{ "count", FLDT_UINT16D, OI(HOFF(count)), C1, 0, TYP_NONE },
+	{ "level", FLDT_UINT16D, OI(HOFF(level)), C1, 0, TYP_NONE },
+	{ NULL }
+};
+
 /*ARGSUSED*/
 static int
 dir2_block_hdr_count(
diff --git a/db/dir2.h b/db/dir2.h
index 5e117ac..a5f0bec 100644
--- a/db/dir2.h
+++ b/db/dir2.h
@@ -27,6 +27,10 @@ extern const field_t	dir2_leaf_entry_flds[];
 extern const field_t	dir2_leaf_hdr_flds[];
 extern const field_t	dir2_leaf_tail_flds[];
 
+extern const field_t	da_blkinfo_flds[];
+extern const field_t	da_node_entry_flds[];
+extern const field_t	da_node_hdr_flds[];
+
 /*
  * generic dir2 structures used by xfs_db
  */
diff --git a/db/field.c b/db/field.c
index 002c5ae..dc72563 100644
--- a/db/field.c
+++ b/db/field.c
@@ -195,6 +195,13 @@ const ftattr_t	ftattrtab[] = {
 	  SI(bitsz(xfs_dir2_sf_off_t)), 0, NULL, NULL },
 	{ FLDT_DIR2SF, "dir2sf", NULL, (char *)dir2sf_flds, dir2sf_size,
 	  FTARG_SIZE, NULL, dir2sf_flds },
+	{ FLDT_DA_BLKINFO, "dir_blkinfo", NULL, (char *)da_blkinfo_flds,
+	  SI(bitsz(struct xfs_da_blkinfo)), 0, NULL, da_blkinfo_flds },
+	{ FLDT_DA_NODE_ENTRY, "dir_node_entry", fp_sarray,
+	  (char *)da_node_entry_flds, SI(bitsz(struct xfs_da_node_entry)), 0,
+	  NULL, da_node_entry_flds },
+	{ FLDT_DA_NODE_HDR, "dir_node_hdr", NULL, (char *)da_node_hdr_flds,
+	  SI(bitsz(struct xfs_da_node_hdr)), 0, NULL, da_node_hdr_flds },
 	{ FLDT_DIRBLOCK, "dirblock", fp_num, "%u", SI(bitsz(__uint32_t)), 0,
 	  fa_dirblock, NULL },
 	{ FLDT_DISK_DQUOT, "disk_dquot", NULL, (char *)disk_dquot_flds,
diff --git a/db/field.h b/db/field.h
index 6962d69..72c225b 100644
--- a/db/field.h
+++ b/db/field.h
@@ -75,7 +75,6 @@ typedef enum fldt	{
 	FLDT_DINODE_CORE,
 	FLDT_DINODE_FMT,
 	FLDT_DINODE_U,
-	FLDT_DIR,
 	FLDT_DIR2,
 	FLDT_DIR2_BLOCK_TAIL,
 	FLDT_DIR2_DATA_FREE,
@@ -94,18 +93,10 @@ typedef enum fldt	{
 	FLDT_DIR2_SF_HDR,
 	FLDT_DIR2_SF_OFF,
 	FLDT_DIR2SF,
-	FLDT_DIR_BLKINFO,
-	FLDT_DIR_INO,
-	FLDT_DIR_LEAF_ENTRY,
-	FLDT_DIR_LEAF_HDR,
-	FLDT_DIR_LEAF_MAP,
-	FLDT_DIR_LEAF_NAME,
-	FLDT_DIR_NODE_ENTRY,
-	FLDT_DIR_NODE_HDR,
-	FLDT_DIR_SF_ENTRY,
-	FLDT_DIR_SF_HDR,
+	FLDT_DA_BLKINFO,
+	FLDT_DA_NODE_ENTRY,
+	FLDT_DA_NODE_HDR,
 	FLDT_DIRBLOCK,
-	FLDT_DIRSHORT,
 	FLDT_DISK_DQUOT,
 	FLDT_DQBLK,
 	FLDT_DQID,

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

Re: [PATCH 10/8] xfsdb: re-instate DA btree node headers

[Mark Tinguely]

On 05/01/13 01:30, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> When removing the dirv1 code, it wasn't immediately obvious that the
> dir v2 code used a small chunk of the dirv1 field definitions. i.e.
> those for the DA btree node headers. Hence bits of xfs_db didn't
> work as expected, and some tests failed in non-obvious ways. e.g
> test 073 failed with this additional line of output:
>
> Use of uninitialized value $logstart in numeric gt (>) at /home/dave/src/xfstests-dev/src/fill2fs line 84
>
> which was the result of the command:
>
> 	xfs_db -r -c sb -c $TEST_DEV
>
> giving an unexpectedly incorrect output.
>
> Re-instate the needed field definitions and rename them from "DIR"
> to "DA" so it is obvious they are for decoding DA Btree format
> blocks.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---

This no longer cleanly applies.

--Mark.

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

Re: [PATCH 10/8] xfsdb: re-instate DA btree node headers

[Mark Tinguely]

On 05/09/13 10:23, Mark Tinguely wrote:
> On 05/01/13 01:30, Dave Chinner wrote:
>> From: Dave Chinner<dchinner@redhat.com>
>>
>> When removing the dirv1 code, it wasn't immediately obvious that the
>> dir v2 code used a small chunk of the dirv1 field definitions. i.e.
>> those for the DA btree node headers. Hence bits of xfs_db didn't
>> work as expected, and some tests failed in non-obvious ways. e.g
>> test 073 failed with this additional line of output:
>>
>> Use of uninitialized value $logstart in numeric gt (>) at
>> /home/dave/src/xfstests-dev/src/fill2fs line 84
>>
>> which was the result of the command:
>>
>> xfs_db -r -c sb -c $TEST_DEV
>>
>> giving an unexpectedly incorrect output.
>>
>> Re-instate the needed field definitions and rename them from "DIR"
>> to "DA" so it is obvious they are for decoding DA Btree format
>> blocks.
>>
>> Signed-off-by: Dave Chinner<dchinner@redhat.com>
>> ---
>
> This no longer cleanly applies.
>
> --Mark.

Oops, I forgot that patch 9/8 and 10/8 was folded back into the version 
2 of the tarball. 11/8 (xfs_logprint: fix continuation transactions) is 
not in the tarball.

Sorry for the noise.

--Mark.

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

[PATCH 11/8] xfs_logprint: fix continuation transactions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

As demonstrated by xfs/295, continuation transactions cause of
problems for xfs_logprint. The failure demonstrated by the test is
that the buffer log format structures are variable sized on disk -
the dirty bitmap is sized according to the buffer length, not fixed
to the length of the maximum supported buffer size.

xfs_logprint assumes that the buf log format reocrds are of fixed
size, and so when a short buffer is found it fails to handle it
properly and treats it like a continuation record.  This causses the
opheader pointer to be incremented incorrectly and then logprint
wanders off into a dark corner and gets eaten by a grue.

While fixing this, make the xlog_print_record code that does the
transaction opheader walking a little easier to read and stop it
from outputting binary data direct to the console by converting the
no-data-print case to use a hex dumping loop.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 logprint/log_misc.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index 30b7ea6..d08f900 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -268,7 +268,13 @@ xlog_print_trans_buffer(xfs_caddr_t *ptr, int len, int *i, int num_ops)
     blen = f->blf_len;
     map_size = f->blf_map_size;
     flags = f->blf_flags;
-    struct_size = sizeof(xfs_buf_log_format_t);
+
+    /*
+     * size of the format header is dependent on the size of the bitmap, not
+     * the size of the in-memory structure. Hence the slightly obtuse
+     * calculation.
+     */
+    struct_size = offsetof(struct xfs_buf_log_format, blf_map_size) + map_size;
 
     if (len >= struct_size) {
 	ASSERT((len - sizeof(struct_size)) % sizeof(int) == 0);
@@ -933,22 +939,28 @@ xlog_print_record(int			  fd,
 	continued = ((op_head->oh_flags & XLOG_WAS_CONT_TRANS) ||
 		     (op_head->oh_flags & XLOG_CONTINUE_TRANS));
 
-	/* print transaction data */
-	if (print_no_data ||
-	    (continued && be32_to_cpu(op_head->oh_len) == 0)) {
+	if (continued && be32_to_cpu(op_head->oh_len) == 0)
+		continue;
+
+	if (print_no_data) {
 	    for (n = 0; n < be32_to_cpu(op_head->oh_len); n++) {
-		printf("%c", *ptr);
+		printf("0x%02x ", (unsigned int)*ptr);
+		if (n % 16 == 15)
+			printf("\n");
 		ptr++;
 	    }
 	    printf("\n");
 	    continue;
 	}
+
+	/* print transaction data */
 	if (xlog_print_find_tid(be32_to_cpu(op_head->oh_tid),
 				op_head->oh_flags & XLOG_WAS_CONT_TRANS)) {
 	    printf(_("Left over region from split log item\n"));
 	    ptr += be32_to_cpu(op_head->oh_len);
 	    continue;
 	}
+
 	if (be32_to_cpu(op_head->oh_len) != 0) {
 	    if (*(uint *)ptr == XFS_TRANS_HEADER_MAGIC) {
 		skip = xlog_print_trans_header(&ptr,

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

Re: [PATCH 11/8] xfs_logprint: fix continuation transactions

[Mark Tinguely]

On 05/01/13 01:31, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> As demonstrated by xfs/295, continuation transactions cause of
> problems for xfs_logprint. The failure demonstrated by the test is
> that the buffer log format structures are variable sized on disk -
> the dirty bitmap is sized according to the buffer length, not fixed
> to the length of the maximum supported buffer size.
>
> xfs_logprint assumes that the buf log format reocrds are of fixed
> size, and so when a short buffer is found it fails to handle it
> properly and treats it like a continuation record.  This causses the
> opheader pointer to be incremented incorrectly and then logprint
> wanders off into a dark corner and gets eaten by a grue.
>
> While fixing this, make the xlog_print_record code that does the
> transaction opheader walking a little easier to read and stop it
> from outputting binary data direct to the console by converting the
> no-data-print case to use a hex dumping loop.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---
>   logprint/log_misc.c |   22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)


Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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