XFS_TRANS_DEBUG fixes

RFC PATCH In review Lachlan McIlroy 2 participants 2 messages Dec 5, 2008 – Dec 8, 2008 XFS filesystem

Share & tools

Lachlan McIlroy

author

2008-12-05 07:10 UTC #0 lore raw

RFC PATCH 1/1 XFS_TRANS_DEBUG fixes

XFS_TRANS_DEBUG is not enabled on a debug build and has suffered some bit rot. Below is a set of fixes to get it working again. Whether it's of any use I don't know but if we are going to carry the code we might as well make it work.

Some of the things I had to do to get it to work (and could be done some other way) are:
- all buffers that are logged need to be mapped into kernel space so the debugging code can make a copy of the buffer data and compare it later. The easiest way to do that is to make all buffers mapped in xfs_bug_get_flags() when XFS_TRANS_DEBUG is set.
- turning XFS_TRANS_DEBUG on does make the system run really slow (even slower than a normal debug build) so we might want to keep it optional.
- Some bit setting functions (btst()/bset()/bfset()) appear to be missing so I've coded up some trivial versions. There maybe some linux kernel functions that do the same thing.
- the transaction debugging code will detect if we have modified more data in a buffer than we have indicated to be logged. This picked up two places where we modify a inode cluster buffer without logging it - firstly when we create a new inode cluster we zero the entire buffer but only log the inode fields (ie data after the xfs_dicore_t finishes is modified) and secondly the bulkstat hack that zeroes the di_mode. I don't know if not logging these buffer changes is actually a bug or not.

Is this debugging code worth hanging onto or should we just ditch the whole lot?

fs/xfs/linux-2.6/xfs_buf.c +4 -0

index cd89c56..4ee182c 100644
@@ -584,6 +584,10 @@ xfs_buf_get_flags(
  	xfs_buf_t		*bp, *new_bp;
  	int			error = 0, i;
 
+#ifdef XFS_TRANS_DEBUG
+	flags |= XBF_MAPPED;
+#endif
+
  	new_bp = xfs_buf_allocate(flags);
  	if (unlikely(!new_bp))
  		return NULL;

fs/xfs/xfs.h +1 -0

index 17254b5..3df4a54 100644
@@ -38,6 +38,7 @@
  #define XFS_RW_TRACE 1
  #define XFS_BUF_TRACE 1
  #define XFS_INODE_TRACE 1
+#define XFS_TRANS_DEBUG 1
  #define XFS_FILESTREAMS_TRACE 1
  #endif

fs/xfs/xfs_buf_item.c +32 -1

index d245d04..bb019a0 100644
@@ -35,6 +35,37 @@ kmem_zone_t	*xfs_buf_item_zone;
 
  #ifdef XFS_TRANS_DEBUG
  /*
+ * Test bit b in bitmap bp
+ */
+int
+btst(char *bp, int b)
+{
+	return (*(bp + (b >> 3)) & (0x80 >> (b & 0x7)));
+}
+
+/*
+ * Set bit b in bitmap bp
+ */
+void
+bset(char *bp, int b)
+{
+	*(bp + (b >> 3)) |= (0x80 >> (b & 0x7));
+}
+
+/*
+ * Set a bit field of length len in bitmap bp starting at b
+ */
+void
+bfset(char *bp, int b, int len)
+{
+	while (len) {
+		bset(bp, b);
+		len--;
+		b++;
+	}
+}
+
+/*
   * This function uses an alternate strategy for tracking the bytes
   * that the user requests to be logged.  This can then be used
   * in conjunction with the bli_orig array in the buf log item to

@@ -126,7 +157,7 @@ xfs_buf_item_log_check(
  	for (x = 0; x < XFS_BUF_COUNT(bp); x++) {
  		if (orig[x] != buffer[x] && !btst(bip->bli_logged, x))
  			cmn_err(CE_PANIC,
-	"xfs_buf_item_log_check bip %x buffer %x orig %x index %d",
+	"xfs_buf_item_log_check bip %p buffer %p orig %p index %d",
  				bip, bp, orig, x);
  	}
  }

fs/xfs/xfs_ialloc.c +2 -0

index e6ebbae..681d622 100644
@@ -353,6 +353,8 @@ xfs_ialloc_ag_alloc(
  		 *      transactions causing a lot of log traffic.
  		 */
  		xfs_biozero(fbuf, 0, ninodes << args.mp->m_sb.sb_inodelog);
+		xfs_trans_log_buf(tp, fbuf, 0,
+				(ninodes << args.mp->m_sb.sb_inodelog) - 1);
  		for (i = 0; i < ninodes; i++) {
  			int	ioffset = i << args.mp->m_sb.sb_inodelog;
  			uint	isize = sizeof(struct xfs_dinode);

fs/xfs/xfs_inode.c +4 -6

index 063da34..9d5df3a 100644
@@ -2121,6 +2121,7 @@ xfs_ifree(
  {
  	int			error;
  	int			delete;
+	int			offset;
  	xfs_ino_t		first_ino;
  	xfs_dinode_t    	*dip;
  	xfs_buf_t       	*ibp;

@@ -2179,6 +2180,8 @@ xfs_ifree(
  	* in the future.
  	*/
  	dip->di_mode = 0;
+	offset = ip->i_imap.im_boffset + offsetof(xfs_dinode_t, di_mode);
+	xfs_trans_log_buf(tp, ibp, offset, (offset + sizeof(__be16) - 1));
 
  	if (delete) {
  		xfs_ifree_cluster(ip, tp, first_ino);

@@ -2598,9 +2601,7 @@ xfs_iflush_fork(
  	char			*cp;
  	xfs_ifork_t		*ifp;
  	xfs_mount_t		*mp;
-#ifdef XFS_TRANS_DEBUG
-	int			first;
-#endif
+
  	static const short	brootflag[2] =
  		{ XFS_ILOG_DBROOT, XFS_ILOG_ABROOT };
  	static const short	dataflag[2] =

@@ -3004,9 +3005,6 @@ xfs_iflush_int(
  	xfs_inode_log_item_t	*iip;
  	xfs_dinode_t		*dip;
  	xfs_mount_t		*mp;
-#ifdef XFS_TRANS_DEBUG
-	int			first;
-#endif
 
  	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
  	ASSERT(!completion_done(&ip->i_flush));

fs/xfs/xfsidbg.c +2 -2

@@ -3231,7 +3208,7 @@ xfs_buf_item_print(xfs_buf_log_item_t *b
  	kdb_printf("blf flags: ");
  	printflags((uint)blip->bli_format.blf_flags, blf_flags, NULL);
  #ifdef XFS_TRANS_DEBUG
-	kdb_printf("orig 0x%x logged 0x%x",
+	kdb_printf("orig 0x%p logged 0x%p",
  		blip->bli_orig, blip->bli_logged);
  #endif
  	kdb_printf("\n");

@@ -3599,7 +3576,7 @@ xfs_inode_item_print(xfs_inode_log_item_
  		ilip->ili_ilock_recur, ilip->ili_iolock_recur,
  		ilip->ili_extents_buf);
  #ifdef XFS_TRANS_DEBUG
-	kdb_printf("root bytes %d root orig 0x%x\n",
+	kdb_printf("root bytes %d root orig 0x%p\n",
  		ilip->ili_root_size, ilip->ili_orig_root);
  #endif
  	kdb_printf("size %d ", ilip->ili_format.ilf_size);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Christoph Hellwig

2008-12-08 22:54 UTC #1 lore raw

Quoted 6 lines · Lachlan McIlroy

Some of the things I had to do to get it to work (and could be done some other way) are:
- all buffers that are logged need to be mapped into kernel space so the debugging code can make a copy of the buffer data and compare it later. The easiest way to do that is to make all buffers mapped in xfs_bug_get_flags() when XFS_TRANS_DEBUG is set.

Need a comment similar to this next to the place that sets the flag.

Quoted 3 lines

- Some bit setting functions (btst()/bset()/bfset()) appear to be missing so I've coded up some trivial versions. There maybe some linux kernel functions that do the same thing.

Just use the Linu set_bit / clear_bit / test_bit / test_and_set_bit / test_and_clear_bit helpers, or the __ prefixed versions if you don't need atomicy. Note that they want to have pointers to unsigned long passed.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs