[PATCH] xfs_logprint: fix the transcation type string for delaylog-enabled fs

[PATCH] xfs_logprint: fix the transcation type string for delaylog-enabled fs

[Hou Tao]

For delaylog-enabled fs, the only th_type is XFS_TRANS_CHECKPOINT,
but the value of XFS_TRANS_CHECKPOINT had been change from 42 to 40
by xfs commit 61e63ec (xfs: consolidate superblock logging functions),
so return trans_type[type] directly will be incorrect.
And there is no flag for delaylog testing, so the suboptimal solution
is to use super v5 flag instead. For pre-v5 fs used by kernel after
commit 61e63ec, the result of xlog_trans_type will still be incorrect.

before patch:
(1) v5 fs
TRAN:    type: SWAPEXT       tid: 321be024       num_items: 2
TRANS: tid:0x772d0805  type:SWAPEXT  #items:37  trans:0x772d0805  q:0x559104d71bc0

after patch:
(2) v5 fs
TRAN:    type: CHECKPOINT       tid: 321be024       num_items: 2
TRANS: tid:0x772d0805  type:SWAPEXT  #items:37  trans:0x772d0805  q:0x559104d71bc0

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 include/libxlog.h          |  5 ++---
 logprint/log_misc.c        | 34 ++++++++++++++++++++++++++++++----
 logprint/log_print_all.c   |  5 +++--
 logprint/log_print_trans.c |  9 ++++++---
 logprint/logprint.h        |  5 ++++-
 5 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/include/libxlog.h b/include/libxlog.h
index 0a11ec8..b0d2870 100644
--- a/include/libxlog.h
+++ b/include/libxlog.h
@@ -104,13 +104,12 @@ extern int	xlog_test_footer(struct xlog *log);
 extern int	xlog_recover(struct xlog *log, int readonly);
 extern void	xlog_recover_print_data(char *p, int len);
 extern void	xlog_recover_print_logitem(xlog_recover_item_t *item);
-extern void	xlog_recover_print_trans_head(xlog_recover_t *tr);
+extern void	xlog_recover_print_trans_head(xlog_recover_t *tr, int delaylog);
 extern int	xlog_print_find_oldest(struct xlog *log, xfs_daddr_t *last_blk);
 
 /* for transactional view */
-extern void	xlog_recover_print_trans_head(xlog_recover_t *tr);
 extern void	xlog_recover_print_trans(xlog_recover_t *trans,
-				struct list_head *itemq, int print);
+				struct list_head *itemq, int print, int delaylog);
 extern int	xlog_do_recovery_pass(struct xlog *log, xfs_daddr_t head_blk,
 				xfs_daddr_t tail_blk, int pass);
 extern int	xlog_recover_do_trans(struct xlog *log, xlog_recover_t *trans,
diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index 479fc14..066ac59 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -28,7 +28,7 @@
 #define NO_ERROR	(0)
 
 static int logBBsize;
-char *trans_type[] = {
+static char *trans_type[] = {
 	"",
 	"SETATTR",
 	"SETATTR_SIZE",
@@ -207,8 +207,30 @@ xlog_print_find_tid(xlog_tid_t tid, uint was_cont)
     return 1;
 }	/* xlog_print_find_tid */
 
+/*
+ * For delaylog-enabled fs, the only th_type is XFS_TRANS_CHECKPOINT,
+ * but the value of XFS_TRANS_CHECKPOINT had been change from 42 to 40
+ * by xfs commit 61e63ec (xfs: consolidate superblock logging functions),
+ * so return trans_type[type] directly will be incorrect.
+ *
+ * And there is no flag for delaylog testing, so the suboptimal solution
+ * is to use super v5 flag instead. For pre-v5 fs used by kernel after
+ * commit 61e63ec, the result of xlog_trans_type will still be incorrect.
+ */
+int
+xlog_delaylog_on(struct xfs_sb *sb)
+{
+	return (XFS_SB_VERSION_NUM(sb) == XFS_SB_VERSION_5);
+}
+
+const char *
+xlog_trans_type(uint type, int delaylog)
+{
+	return delaylog ? "CHECKPOINT" : trans_type[type];
+}
+
 int
-xlog_print_trans_header(char **ptr, int len)
+xlog_print_trans_header(char **ptr, int len, int delaylog)
 {
     xfs_trans_header_t  *h;
     char		*cptr = *ptr;
@@ -234,7 +256,8 @@ xlog_print_trans_header(char **ptr, int len)
     }
     h = (xfs_trans_header_t *)cptr;
     printf(_("    type: %s       tid: %x       num_items: %d\n"),
-	   trans_type[h->th_type], h->th_tid, h->th_num_items);
+			xlog_trans_type(h->th_type, delaylog),
+			h->th_tid, h->th_num_items);
     return 0;
 }	/* xlog_print_trans_header */
 
@@ -825,6 +848,7 @@ xlog_print_record(
     char		*buf, *ptr;
     int			read_len, skip, lost_context = 0;
     int			ret, n, i, j, k;
+    int         delaylog;
 
     if (print_no_print)
 	    return NO_ERROR;
@@ -917,6 +941,7 @@ xlog_print_record(
     }
 
     ptr = buf;
+    delaylog = xlog_delaylog_on(&log->l_mp->m_sb);
     for (i=0; i<num_ops; i++) {
 	int continued;
 
@@ -955,7 +980,8 @@ xlog_print_record(
 	if (be32_to_cpu(op_head->oh_len) != 0) {
 	    if (*(uint *)ptr == XFS_TRANS_HEADER_MAGIC) {
 		skip = xlog_print_trans_header(&ptr,
-					be32_to_cpu(op_head->oh_len));
+					be32_to_cpu(op_head->oh_len),
+					delaylog);
 	    } else {
 		switch (*(unsigned short *)ptr) {
 		    case XFS_LI_BUF: {
diff --git a/logprint/log_print_all.c b/logprint/log_print_all.c
index 0fe354b..ddc5475 100644
--- a/logprint/log_print_all.c
+++ b/logprint/log_print_all.c
@@ -482,7 +482,8 @@ void
 xlog_recover_print_trans(
 	xlog_recover_t		*trans,
 	struct list_head	*itemq,
-	int			print)
+	int			print,
+	int delaylog)
 {
 	xlog_recover_item_t	*item;
 
@@ -490,7 +491,7 @@ xlog_recover_print_trans(
 		return;
 
 	print_xlog_record_line();
-	xlog_recover_print_trans_head(trans);
+	xlog_recover_print_trans_head(trans, delaylog);
 	list_for_each_entry(item, itemq, ri_list)
 		xlog_recover_print_item(item);
 }
diff --git a/logprint/log_print_trans.c b/logprint/log_print_trans.c
index 9bf2b37..cf653ad 100644
--- a/logprint/log_print_trans.c
+++ b/logprint/log_print_trans.c
@@ -22,10 +22,12 @@
 
 void
 xlog_recover_print_trans_head(
-	xlog_recover_t	*tr)
+	xlog_recover_t	*tr,
+	int delaylog)
 {
 	printf(_("TRANS: tid:0x%x  type:%s  #items:%d  trans:0x%x  q:0x%lx\n"),
-	       tr->r_log_tid, trans_type[tr->r_theader.th_type],
+	       tr->r_log_tid,
+		   xlog_trans_type(tr->r_theader.th_type, delaylog),
 	       tr->r_theader.th_num_items,
 	       tr->r_theader.th_tid, (long)&tr->r_itemq);
 }
@@ -36,7 +38,8 @@ xlog_recover_do_trans(
 	xlog_recover_t	*trans,
 	int		pass)
 {
-	xlog_recover_print_trans(trans, &trans->r_itemq, 3);
+	xlog_recover_print_trans(trans, &trans->r_itemq, 3,
+							 xlog_delaylog_on(&log->l_mp->m_sb));
 	return 0;
 }
 
diff --git a/logprint/logprint.h b/logprint/logprint.h
index 0c03c08..95d13d5 100644
--- a/logprint/logprint.h
+++ b/logprint/logprint.h
@@ -18,6 +18,8 @@
 #ifndef LOGPRINT_H
 #define LOGPRINT_H
 
+struct xfs_sb;
+
 /* command line flags */
 extern int	print_data;
 extern int	print_only_data;
@@ -30,7 +32,8 @@ extern int	print_no_data;
 extern int	print_no_print;
 
 /* exports */
-extern char *trans_type[];
+extern int xlog_delaylog_on(struct xfs_sb *sb);
+extern const char *xlog_trans_type(uint type, int delaylog);
 
 extern void xlog_print_lseek(struct xlog *, int, xfs_daddr_t, int);
 
-- 
2.5.5

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

Re: [PATCH] xfs_logprint: fix the transcation type string for delaylog-enabled fs

[Dave Chinner]

On Fri, Sep 09, 2016 at 12:08:27PM +0800, Hou Tao wrote:
> For delaylog-enabled fs, the only th_type is XFS_TRANS_CHECKPOINT,
> but the value of XFS_TRANS_CHECKPOINT had been change from 42 to 40
> by xfs commit 61e63ec (xfs: consolidate superblock logging functions),
> so return trans_type[type] directly will be incorrect.
> And there is no flag for delaylog testing, so the suboptimal solution
> is to use super v5 flag instead. For pre-v5 fs used by kernel after
> commit 61e63ec, the result of xlog_trans_type will still be incorrect.

delaylog and v5 superblocks are completely unrelated and so this is
incorrect.

> before patch:
> (1) v5 fs
> TRAN:    type: SWAPEXT       tid: 321be024       num_items: 2
> TRANS: tid:0x772d0805  type:SWAPEXT  #items:37  trans:0x772d0805  q:0x559104d71bc0
> 
> after patch:
> (2) v5 fs
> TRAN:    type: CHECKPOINT       tid: 321be024       num_items: 2
> TRANS: tid:0x772d0805  type:SWAPEXT  #items:37  trans:0x772d0805  q:0x559104d71bc0

And so v4 filesystems are still incorrect.

Indeed, when delaylog is enabled, the only transaction type in the
log is "CHECKPOINT" - the whole "trans type" stuff has gone away
from the kernel and only exists as this in xfs_log_format.h:

/*
 * The only type valid for th_type in CIL-enabled file system logs:
 */
#define XFS_TRANS_CHECKPOINT    40

IOWs, most of the logprint code is for printing log information from
pre-delaylog kernels. IOWs, for the anyone using a 3.0+ kernel, the
"trans type" output from xfs_logprint is completely useless
information, so we should probably either put it behind a command
line option or remove it completely...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_logprint: fix the transcation type string for delaylog-enabled fs

[Hou Tao]

>> but the value of XFS_TRANS_CHECKPOINT had been change from 42 to 40
>> by xfs commit 61e63ec (xfs: consolidate superblock logging functions),
>> so return trans_type[type] directly will be incorrect.
>> And there is no flag for delaylog testing, so the suboptimal solution
>> is to use super v5 flag instead. For pre-v5 fs used by kernel after
>> commit 61e63ec, the result of xlog_trans_type will still be incorrect.
> 
> delaylog and v5 superblocks are completely unrelated and so this is
> incorrect.
I don't agree. As we can see from the commit log, v5 superblock was supported
after making delaylog as the only option, so v5 superblock implies delaylog.

Commit 93b8a5854f247138e401471a9c3b82ccb62ff608 makes the delaylog as the only
option, and its date is "Tue Dec 6 21:58:07 2011".

Commit 04a1e6c5b222b089c6960dfc5352002002a4355f adds the support of v5 superblock,
and its date is "Wed Apr 3 16:11:31 2013".

> And so v4 filesystems are still incorrect.
> 
Partial yes: For v4 filesystem before 3.19 (commit 61e63ec), the result
is still correct.

> IOWs, most of the logprint code is for printing log information from
> pre-delaylog kernels. IOWs, for the anyone using a 3.0+ kernel, the
> "trans type" output from xfs_logprint is completely useless
> information, so we should probably either put it behind a command
> line option or remove it completely...
I prefer the command line option over removing it. Pre-delaylog kernel
need it and some cases of xfstests rely on the transaction type string.

The command line will be used to tell whether or not it is a delaylog-enabled
xfs, and if it's a v5 superblock, it must be a delaylog-enabled xfs.

I will send a v2 patch.

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

Re: [PATCH] xfs_logprint: fix the transcation type string for delaylog-enabled fs

[Dave Chinner]

On Wed, Sep 14, 2016 at 09:02:54AM +0800, Hou Tao wrote:
> >> but the value of XFS_TRANS_CHECKPOINT had been change from 42 to 40
> >> by xfs commit 61e63ec (xfs: consolidate superblock logging functions),
> >> so return trans_type[type] directly will be incorrect.
> >> And there is no flag for delaylog testing, so the suboptimal solution
> >> is to use super v5 flag instead. For pre-v5 fs used by kernel after
> >> commit 61e63ec, the result of xlog_trans_type will still be incorrect.
> > 
> > delaylog and v5 superblocks are completely unrelated and so this is
> > incorrect.
> I don't agree.  As we can see from the commit log, v5 superblock was supported
> after making delaylog as the only option, so v5 superblock implies delaylog.

You can disagree, but that doesn't make your argument correct.

Superblock versions and features define changes to the on-disk
format, and delaylog was specifically crafted to avoid changing the
on-disk fomats. Tying detection of transaction type numbering to an
unrelated on-disk feature change is simply wrong. No ifs, not buts,
it's just wrong.

> Commit 93b8a5854f247138e401471a9c3b82ccb62ff608 makes the delaylog as the only
> option, and its date is "Tue Dec 6 21:58:07 2011".
> 
> Commit 04a1e6c5b222b089c6960dfc5352002002a4355f adds the support of v5 superblock,
> and its date is "Wed Apr 3 16:11:31 2013".

Sure, but that's irrelevant because these are unrelated features.

> > And so v4 filesystems are still incorrect.
> > 
> Partial yes: For v4 filesystem before 3.19 (commit 61e63ec), the result
> is still correct.

Actually, it was introduced in 3.20-rc1, aka 4.0. It was propagated
into xfsprogs 4.2.0-rc1. The trans type code in logprint is really
for legacy filesystems with kernels older than 3.0.
I don't see much point in trying to hack legacy support
into the TOT diagnostic tool, especailly as it is trivial to pull
a logprint from a previous release if such support is actually
necessary. e.g:

$ git checkout -b old_logprint v3.2.3
$ make

And now you have a logprint/xfs_logprint binary that you can use for
diagnostics of the pre-delaylog log format.

Removing functionality from the TOT code base does not mean that
functionality is lost - it's still in the revision contorl system
and a minute away from being usable if it's ever needed.

> > IOWs, most of the logprint code is for printing log information from
> > pre-delaylog kernels. IOWs, for the anyone using a 3.0+ kernel, the
> > "trans type" output from xfs_logprint is completely useless
> > information, so we should probably either put it behind a command
> > line option or remove it completely...
> I prefer the command line option over removing it. Pre-delaylog kernel
> need it and some cases of xfstests rely on the transaction type string.
> 
> The command line will be used to tell whether or not it is a delaylog-enabled
> xfs, and if it's a v5 superblock, it must be a delaylog-enabled xfs.

Users won't know this, because "delaylog" is something that has long
been deprecated and lots of users don't even know it exists.

Really, just remove the transaction type printing from logprint and
both developers and users can continue to ignore it like we have
been for the past few years....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs_logprint: fix the transcation type string for delaylog-enabled fs

[Hou Tao]

> Tying detection of transaction type numbering to an
> unrelated on-disk feature change is simply wrong. No ifs, not buts,
> it's just wrong.
Yes, I agree with you.

> Actually, it was introduced in 3.20-rc1, aka 4.0. It was propagated
> into xfsprogs 4.2.0-rc1. The trans type code in logprint is really
> for legacy filesystems with kernels older than 3.0.
> I don't see much point in trying to hack legacy support
> into the TOT diagnostic tool, especailly as it is trivial to pull
> a logprint from a previous release if such support is actually
> necessary. e.g:
> 
> $ git checkout -b old_logprint v3.2.3
> $ make
> 
> And now you have a logprint/xfs_logprint binary that you can use for
> diagnostics of the pre-delaylog log format.
> 
> Removing functionality from the TOT code base does not mean that
> functionality is lost - it's still in the revision contorl system
> and a minute away from being usable if it's ever needed.
I just think about the compatibility things too much.

> Users won't know this, because "delaylog" is something that has long
> been deprecated and lots of users don't even know it exists.
> 
> Really, just remove the transaction type printing from logprint and
> both developers and users can continue to ignore it like we have
> been for the past few years....
Thanks for your comment.
I will send a new patch to remove the transaction type printing.




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

[PATCH] xfs_logprint: remove the printing of transaction type

[Hou Tao]

th_type in struct xfs_trans_header was used to store
the transaction type for xfs without delaylog feature.
Now delaylog is a built-in feature (commit 93b8a58 "xfs:
remove the deprecated nodelaylog option" v3.3-rc1), so
remove the printing of transaction type.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 logprint/log_misc.c        | 50 ++--------------------------------------------
 logprint/log_print_trans.c |  4 ++--
 logprint/logprint.h        |  2 --
 3 files changed, 4 insertions(+), 52 deletions(-)

diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index 479fc14..e4af09b 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -28,52 +28,6 @@
 #define NO_ERROR	(0)
 
 static int logBBsize;
-char *trans_type[] = {
-	"",
-	"SETATTR",
-	"SETATTR_SIZE",
-	"INACTIVE",
-	"CREATE",
-	"CREATE_TRUNC",
-	"TRUNCATE_FILE",
-	"REMOVE",
-	"LINK",
-	"RENAME",
-	"MKDIR",
-	"RMDIR",
-	"SYMLINK",
-	"SET_DMATTRS",
-	"GROWFS",
-	"STRAT_WRITE",
-	"DIOSTRAT",
-	"WRITE_SYNC",
-	"WRITEID",
-	"ADDAFORK",
-	"ATTRINVAL",
-	"ATRUNCATE",
-	"ATTR_SET",
-	"ATTR_RM",
-	"ATTR_FLAG",
-	"CLEAR_AGI_BUCKET",
-	"QM_SBCHANGE",
-	"DUMMY1",
-	"DUMMY2",
-	"QM_QUOTAOFF",
-	"QM_DQALLOC",
-	"QM_SETQLIM",
-	"QM_DQCLUSTER",
-	"QM_QINOCREATE",
-	"QM_QUOTAOFF_END",
-	"SB_UNIT",
-	"FSYNC_TS",
-	"GROWFSRT_ALLOC",
-	"GROWFSRT_ZERO",
-	"GROWFSRT_FREE",
-	"SWAPEXT",
-	"SB_COUNT",
-	"CHECKPOINT",
-	"ICREATE",
-};
 
 typedef struct xlog_split_item {
 	struct xlog_split_item	*si_next;
@@ -233,8 +187,8 @@ xlog_print_trans_header(char **ptr, int len)
 	return 1;
     }
     h = (xfs_trans_header_t *)cptr;
-    printf(_("    type: %s       tid: %x       num_items: %d\n"),
-	   trans_type[h->th_type], h->th_tid, h->th_num_items);
+    printf(_("     tid: %x  num_items: %d\n"),
+	   h->th_tid, h->th_num_items);
     return 0;
 }	/* xlog_print_trans_header */
 
diff --git a/logprint/log_print_trans.c b/logprint/log_print_trans.c
index 9bf2b37..0fecbce 100644
--- a/logprint/log_print_trans.c
+++ b/logprint/log_print_trans.c
@@ -24,8 +24,8 @@ void
 xlog_recover_print_trans_head(
 	xlog_recover_t	*tr)
 {
-	printf(_("TRANS: tid:0x%x  type:%s  #items:%d  trans:0x%x  q:0x%lx\n"),
-	       tr->r_log_tid, trans_type[tr->r_theader.th_type],
+	printf(_("TRANS: tid:0x%x  #items:%d  trans:0x%x  q:0x%lx\n"),
+	       tr->r_log_tid,
 	       tr->r_theader.th_num_items,
 	       tr->r_theader.th_tid, (long)&tr->r_itemq);
 }
diff --git a/logprint/logprint.h b/logprint/logprint.h
index 0c03c08..bdd0ee1 100644
--- a/logprint/logprint.h
+++ b/logprint/logprint.h
@@ -30,8 +30,6 @@ extern int	print_no_data;
 extern int	print_no_print;
 
 /* exports */
-extern char *trans_type[];
-
 extern void xlog_print_lseek(struct xlog *, int, xfs_daddr_t, int);
 
 extern void xfs_log_copy(struct xlog *, int, char *);
-- 
2.5.5

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