Skip to contentSkip to conversation

xfs: patch queue for 3.8

PATCH In review Dave Chinner 5 participants 22 messages Nov 28, 2012 – Dec 3, 2012 XFS filesystem

lore
Share & tools
lore
#0 lore raw

cover xfs: patch queue for 3.8

Hi folks,

More bug fixes for 3.8. This series contains:

- the DIO transaction deadlock patch (unmodified),
- addressed Christoph's zero-range review points and fixes the ASSERT in the sub-block zeroing case. (updated)
- a dquot locking issue that has been causing me random hangs in xfstests 232 for months (new)
- fixes for the log CRC sparse endian warnings. (new)

Cheers,

Dave.

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

#1 lore raw

PATCH 1/4 xfs: fix direct IO nested transaction deadlock.

From: Dave Chinner <[email protected]>

The direct IO path can do a nested transaction reservation when writing past the EOF. The first transaction is the append transaction for setting the filesize at IO completion, but we can also need a transaction for allocation of blocks. If the log is low on space due to reservations and small log, the append transaction can be granted after wating for space as the only active transaction in the system. This then attempts a reservation for an allocation, which there isn't space in the log for, and the reservation sleeps. The result is that there is nothing left in the system to wake up all the processes waiting for log space to come free.

The stack trace that shows this deadlock is relatively innocuous:

xlog_grant_head_wait
xlog_grant_head_check
xfs_log_reserve
xfs_trans_reserve
xfs_iomap_write_direct
__xfs_get_blocks
xfs_get_blocks_direct
do_blockdev_direct_IO
__blockdev_direct_IO
xfs_vm_direct_IO
generic_file_direct_write
xfs_file_dio_aio_writ
xfs_file_aio_write
do_sync_write
vfs_write

This was discovered on a filesystem with a log of only 10MB, and a log stripe unit of 256k whih increased the base reservations by 512k. Hence a allocation transaction requires 1.2MB of log space to be available instead of only 260k, and so greatly increased the chance that there wouldn't be enough log space available for the nested transaction to succeed. The key to reproducing it is this mkfs command:

mkfs.xfs -f -d agcount=16,su=256k,sw=12 -l su=256k,size=2560b $SCRATCH_DEV

The test case was a 1000 fsstress processes running with random freeze and unfreezes every few seconds. Thanks to Eryu Guan ([email protected]) for writing the test that found this on a system with a somewhat unique default configuration....

---
 fs/xfs/xfs_aops.c |   81 +++++++++++++++++++----------------------------------
 fs/xfs/xfs_log.c  |    3 +-
 2 files changed, 31 insertions(+), 53 deletions(-)
fs/xfs/xfs_aops.c +29 -52
index 71361da..4111a40 100644
@@ -124,7 +124,7 @@ xfs_setfilesize_trans_alloc(
 	ioend->io_append_trans = tp;
 
 	/*
-	 * We will pass freeze protection with a transaction.  So tell lockdep
+	 * We may pass freeze protection with a transaction.  So tell lockdep
 	 * we released it.
 	 */
 	rwsem_release(&ioend->io_inode->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
@@ -149,11 +149,13 @@ xfs_setfilesize(
 	xfs_fsize_t		isize;
 
 	/*
-	 * The transaction was allocated in the I/O submission thread,
-	 * thus we need to mark ourselves as beeing in a transaction
-	 * manually.
+	 * The transaction may have been allocated in the I/O submission thread,
+	 * thus we need to mark ourselves as beeing in a transaction manually.
+	 * Similarly for freeze protection.
 	 */
 	current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
+	rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
+			   0, 1, _THIS_IP_);
 
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	isize = xfs_new_eof(ip, ioend->io_offset + ioend->io_size);
@@ -187,7 +189,8 @@ xfs_finish_ioend(
 
 		if (ioend->io_type == XFS_IO_UNWRITTEN)
 			queue_work(mp->m_unwritten_workqueue, &ioend->io_work);
-		else if (ioend->io_append_trans)
+		else if (ioend->io_append_trans ||
+			 (ioend->io_isdirect && xfs_ioend_is_append(ioend)))
 			queue_work(mp->m_data_workqueue, &ioend->io_work);
 		else
 			xfs_destroy_ioend(ioend);
@@ -205,15 +208,6 @@ xfs_end_io(
 	struct xfs_inode *ip = XFS_I(ioend->io_inode);
 	int		error = 0;
 
-	if (ioend->io_append_trans) {
-		/*
-		 * We've got freeze protection passed with the transaction.
-		 * Tell lockdep about it.
-		 */
-		rwsem_acquire_read(
-			&ioend->io_inode->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
-			0, 1, _THIS_IP_);
-	}
 	if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
 		ioend->io_error = -EIO;
 		goto done;
@@ -226,35 +220,31 @@ xfs_end_io(
 	 * range to normal written extens after the data I/O has finished.
 	 */
 	if (ioend->io_type == XFS_IO_UNWRITTEN) {
+		error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
+						  ioend->io_size);
+	} else if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {
 		/*
-		 * For buffered I/O we never preallocate a transaction when
-		 * doing the unwritten extent conversion, but for direct I/O
-		 * we do not know if we are converting an unwritten extent
-		 * or not at the point where we preallocate the transaction.
+		 * For direct I/O we do not know if we need to allocate blocks
+		 * or not so we can't preallocate an append transaction as that
+		 * results in nested reservations and log space deadlocks. Hence
+		 * allocate the transaction here. While this is sub-optimal and
+		 * can block IO completion for some time, we're stuck with doing
+		 * it this way until we can pass the ioend to the direct IO
+		 * allocation callbacks and avoid nesting that way.
 		 */
-		if (ioend->io_append_trans) {
-			ASSERT(ioend->io_isdirect);
-
-			current_set_flags_nested(
-				&ioend->io_append_trans->t_pflags, PF_FSTRANS);
-			xfs_trans_cancel(ioend->io_append_trans, 0);
-		}
-
-		error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
-						 ioend->io_size);
-		if (error) {
-			ioend->io_error = -error;
+		error = xfs_setfilesize_trans_alloc(ioend);
+		if (error)
 			goto done;
-		}
+		error = xfs_setfilesize(ioend);
 	} else if (ioend->io_append_trans) {
 		error = xfs_setfilesize(ioend);
-		if (error)
-			ioend->io_error = -error;
 	} else {
 		ASSERT(!xfs_ioend_is_append(ioend));
 	}
 
 done:
+	if (error)
+		ioend->io_error = -error;
 	xfs_destroy_ioend(ioend);
 }
 
@@ -1432,25 +1422,21 @@ xfs_vm_direct_IO(
 		size_t size = iov_length(iov, nr_segs);
 
 		/*
-		 * We need to preallocate a transaction for a size update
-		 * here.  In the case that this write both updates the size
-		 * and converts at least on unwritten extent we will cancel
-		 * the still clean transaction after the I/O has finished.
+		 * We cannot preallocate a size update transaction here as we
+		 * don't know whether allocation is necessary or not. Hence we
+		 * can only tell IO completion that one is necessary if we are
+		 * not doing unwritten extent conversion.
 		 */
 		iocb->private = ioend = xfs_alloc_ioend(inode, XFS_IO_DIRECT);
-		if (offset + size > XFS_I(inode)->i_d.di_size) {
-			ret = xfs_setfilesize_trans_alloc(ioend);
-			if (ret)
-				goto out_destroy_ioend;
+		if (offset + size > XFS_I(inode)->i_d.di_size)
 			ioend->io_isdirect = 1;
-		}
 
 		ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov,
 					    offset, nr_segs,
 					    xfs_get_blocks_direct,
 					    xfs_end_io_direct_write, NULL, 0);
 		if (ret != -EIOCBQUEUED && iocb->private)
-			goto out_trans_cancel;
+			goto out_destroy_ioend;
 	} else {
 		ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov,
 					    offset, nr_segs,
@@ -1460,15 +1446,6 @@ xfs_vm_direct_IO(
 
 	return ret;
 
-out_trans_cancel:
-	if (ioend->io_append_trans) {
-		current_set_flags_nested(&ioend->io_append_trans->t_pflags,
-					 PF_FSTRANS);
-		rwsem_acquire_read(
-			&inode->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
-			0, 1, _THIS_IP_);
-		xfs_trans_cancel(ioend->io_append_trans, 0);
-	}
 out_destroy_ioend:
 	xfs_destroy_ioend(ioend);
 	return ret;
fs/xfs/xfs_log.c +2 -1
index c6d6e13..c49e2c1 100644
@@ -460,7 +460,8 @@ xfs_log_reserve(
 	tic->t_trans_type = t_type;
 	*ticp = tic;
 
-	xlog_grant_push_ail(log, tic->t_unit_res * tic->t_cnt);
+	xlog_grant_push_ail(log, tic->t_cnt ? tic->t_unit_res * tic->t_cnt
+					    : tic->t_unit_res);
 
 	trace_xfs_log_reserve(log, tic);
 
Signature · 6 lines
1.7.10

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

PATCH 2/4 xfs: byte range granularity for XFS_IOC_ZERO_RANGE

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

XFS_IOC_ZERO_RANGE simply does not work properly for non page cache aligned ranges. Neither test 242 or 290 exercise this correctly, so the behaviour is completely busted even though the tests pass.

Fix it to support full byte range granularity as was originally intended for this ioctl.

---
 fs/xfs/xfs_file.c     |    2 +-
 fs/xfs/xfs_vnodeops.c |   95 ++++++++++++++++++++++++++++++++++++++-----------
 fs/xfs/xfs_vnodeops.h |    1 +
 3 files changed, 76 insertions(+), 22 deletions(-)
fs/xfs/xfs_file.c +1 -1
index 400b187..67284ed 100644
@@ -86,7 +86,7 @@ xfs_rw_ilock_demote(
  *	valid before the operation, it will be read from disk before
  *	being partially zeroed.
  */
-STATIC int
+int
 xfs_iozero(
 	struct xfs_inode	*ip,	/* inode			*/
 	loff_t			pos,	/* offset in file		*/
fs/xfs/xfs_vnodeops.c +74 -21

121 lines not on this page · raw message

fs/xfs/xfs_vnodeops.h +1 -0
index 91a03fa..5163022 100644
@@ -49,6 +49,7 @@ int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
 int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
 		int flags, struct attrlist_cursor_kern *cursor);
 
+int xfs_iozero(struct xfs_inode *, loff_t, size_t);
 int xfs_zero_eof(struct xfs_inode *, xfs_off_t, xfs_fsize_t);
 int xfs_free_eofblocks(struct xfs_mount *, struct xfs_inode *, bool);
 
Signature · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v1 2/4
#4 lore raw

Re: [PATCH 2/4] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

Quoted 10 lines · Dave Chinner

From: Dave Chinner <[email protected]>

XFS_IOC_ZERO_RANGE simply does not work properly for non page cache aligned ranges. Neither test 242 or 290 exercise this correctly, so the behaviour is completely busted even though the tests pass.

Fix it to support full byte range granularity as was originally intended for this ioctl.

Look good,

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

#5 lore raw

Re: [PATCH 2/4] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

Actually this seems to trip up an assert in xfstests 290 for me:

290 0s ...[ 45.997814] XFS: Assertion failed: offset + len <= start_boundary || offset == start_boundary, file: fs/xfs/xfs_vnodeops.c, line: 2153

[   46.000534] ------------[ cut here ]------------
[   46.001523] kernel BUG at fs/xfs/xfs_message.c:100!
[   46.002517] invalid opcode: 0000 [#1] SMP 
[   46.003614] Modules linked in:
[   46.003797] CPU 0 
[   46.003797] Pid: 4026, comm: xfs_io Not tainted 3.7.0-rc1+ #513 Bochs Bochs
[   46.003797] RIP: 0010:[<ffffffff8145f5fd>]  [<ffffffff8145f5fd>] assfail+0x1d/0x20
[   46.003797] RSP: 0018:ffff88005961dc28  EFLAGS: 00010292
[   46.003797] RAX: 000000000000007a RBX: 0000000000000fff RCX: 0000000000000006
[   46.003797] RDX: 0000000000000006 RSI: ffff88005658e710 RDI: ffff88005658e080
[   46.003797] RBP: ffff88005961dc28 R08: 0000000000000000 R09: 0000000000000000
[   46.003797] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000001000
[   46.003797] R13: 0000000000001000 R14: ffff88005b3de640 R15: 0000000000001fff
[   46.003797] FS:  0000000000000000(0000) GS:ffff88005d800000(0063) knlGS:00000000f76198d0
[   46.003797] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
[   46.003797] CR2: 00000000f7780000 CR3: 0000000059f61000 CR4: 00000000000006f0
[   46.003797] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   46.003797] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[   46.003797] Process xfs_io (pid: 4026, threadinfo ffff88005961c000, task ffff88005658e080)
[   46.003797] Stack:
[   46.003797]  ffff88005961dc88 ffffffff81467bcb ffff88005964b800 0000000000000000
[   46.003797]  0000000000001000 0000000000000000 ffffffff811a2a33 ffff88005b3de640
[   46.003797]  0000000000000000 0000000000000fff 0000000000002000 ffff880059f14800
[   46.003797] Call Trace:
[   46.003797]  [<ffffffff81467bcb>] xfs_zero_file_space+0xdb/0x1c0
[   46.003797]  [<ffffffff811a2a33>] ? mnt_want_write_file+0x23/0x50
[   46.003797]  [<ffffffff81467e11>] xfs_change_file_space+0x161/0x390
[   46.003797]  [<ffffffff811a1aad>] ? mnt_clone_write+0xd/0x40
[   46.003797]  [<ffffffff811a29fc>] ? __mnt_want_write_file+0x3c/0x50
[   46.003797]  [<ffffffff8145914c>] xfs_ioc_space+0xbc/0x120
[   46.003797]  [<ffffffff8115ec6e>] ? might_fault+0x4e/0xa0
[   46.003797]  [<ffffffff814dd85c>] xfs_file_compat_ioctl+0x4dc/0x6d0
[   46.003797]  [<ffffffff8119e6ca>] ? fget_light+0x3da/0x4d0
[   46.003797]  [<ffffffff811d72c7>] compat_sys_ioctl+0x97/0x14c0
[   46.003797]  [<ffffffff8115ec6e>] ? might_fault+0x4e/0xa0
[   46.003797]  [<ffffffff8115ecb7>] ? might_fault+0x97/0xa0
[   46.003797]  [<ffffffff8115ec6e>] ? might_fault+0x4e/0xa0
[   46.003797]  [<ffffffff816f661d>] ? trace_hardirqs_off_thunk+0x3a/0x3c
[   46.003797]  [<ffffffff81b883e1>] sysenter_dispatch+0x7/0x23
[   46.003797]  [<ffffffff816f65de>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[   46.003797] Code: ff ff 0f 0b 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 f1 41 89 d0 48 89 e5 48 89 fa 48 c7 c6 78 2f f4 81 31 ff 31 c0 e8 a3 fc ff ff <0f> 0b 90 55 48 63 f6 49 89 f9 48 89 e5 48 83 ec 10 41 b8 01 00 
[   46.003797] RIP  [<ffffffff8145f5fd>] assfail+0x1d/0x20
[   46.003797]  RSP <ffff88005961dc28>
[   46.080287] ---[ end trace 512c2106b365194f ]---

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

#6 lore raw

Re: [PATCH 2/4] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

Quoted 3 lines · Christoph Hellwig

Actually this seems to trip up an assert in xfstests 290 for me:

290 0s ...[ 45.997814] XFS: Assertion failed: offset + len <= start_boundary || offset == start_boundary, file: fs/xfs/xfs_vnodeops.c, line: 2153

Yes, i got that in one of my overnight QA runs. Haven't had a chance to debug it yet.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

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

Re: [PATCH 2/4] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

Quoted 7 lines · Dave Chinner
Quoted 3 lines · Christoph Hellwig

Actually this seems to trip up an assert in xfstests 290 for me:

290 0s ...[ 45.997814] XFS: Assertion failed: offset + len <= start_boundary || offset == start_boundary, file: fs/xfs/xfs_vnodeops.c, line: 2153

Yes, i got that in one of my overnight QA runs. Haven't had a chance to debug it yet.

Ok, I've decided that the ASSERT is simply bogus. It was there because I initially thought that this breanch woul dbe taken for sub-page zeroing. In fact, it's not just sub-page zeroing - the region can span two sub-pages. i.e.:

	0           4096	  8192
	+------o------+-------------+
	       |----len------|

In this case, offset is 4095, len = 4096, resulting in start/end = 4096. Hence we trigger the pure zeroing branch , and off+len is clearly greater then start or end....

This is why I put the ASSERT in there, anyway - to validate that my assumptions that lead to having that branch were correct. Turns out the assumption was wrong but the code is correct for both cases, so I'm just going to remove the ASSERT now.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

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

PATCH v2 2/4 xfs: byte range granularity for XFS_IOC_ZERO_RANGE

In reply to Dave Chinner

xfs: byte range granularity for XFS_IOC_ZERO_RANGE

From: Dave Chinner <[email protected]>

XFS_IOC_ZERO_RANGE simply does not work properly for non page cache aligned ranges. Neither test 242 or 290 exercise this correctly, so the behaviour is completely busted even though the tests pass.

Fix it to support full byte range granularity as was originally intended for this ioctl.

---

V2: remove bogus sub-granularity range assert.

 fs/xfs/xfs_file.c     |    2 +-
 fs/xfs/xfs_vnodeops.c |   96 ++++++++++++++++++++++++++++++++++++++-----------
 fs/xfs/xfs_vnodeops.h |    1 +
 3 files changed, 77 insertions(+), 22 deletions(-)
fs/xfs/xfs_file.c +1 -1
index 400b187..67284ed 100644
@@ -86,7 +86,7 @@ xfs_rw_ilock_demote(
  *	valid before the operation, it will be read from disk before
  *	being partially zeroed.
  */
-STATIC int
+int
 xfs_iozero(
 	struct xfs_inode	*ip,	/* inode			*/
 	loff_t			pos,	/* offset in file		*/
fs/xfs/xfs_vnodeops.c +75 -21

122 lines not on this page · raw message

fs/xfs/xfs_vnodeops.h +1 -0
index 91a03fa..5163022 100644
@@ -49,6 +49,7 @@ int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
 int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
 		int flags, struct attrlist_cursor_kern *cursor);
 
+int xfs_iozero(struct xfs_inode *, loff_t, size_t);
 int xfs_zero_eof(struct xfs_inode *, xfs_off_t, xfs_fsize_t);
 int xfs_free_eofblocks(struct xfs_mount *, struct xfs_inode *, bool);
 

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

Reviewed-by on v2 2/4
#9 lore raw

Re: [PATCH 2/4 V2] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

Quoted 4 lines · Dave Chinner

xfs: byte range granularity for XFS_IOC_ZERO_RANGE

From: Dave Chinner <[email protected]>

Quoted 8 lines

XFS_IOC_ZERO_RANGE simply does not work properly for non page cache aligned ranges. Neither test 242 or 290 exercise this correctly, so the behaviour is completely busted even though the tests pass.

Fix it to support full byte range granularity as was originally intended for this ioctl.

...

Looks good!

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

Reviewed-by on v2 2/4
#10 lore raw

Re: [PATCH 2/4 V2] xfs: byte range granularity for XFS_IOC_ZERO_RANGE

In reply to Dave Chinner
Quoted 4 lines · Dave Chinner

xfs: byte range granularity for XFS_IOC_ZERO_RANGE

From: Dave Chinner <[email protected]>

Quoted 8 lines

XFS_IOC_ZERO_RANGE simply does not work properly for non page cache aligned ranges. Neither test 242 or 290 exercise this correctly, so the behaviour is completely busted even though the tests pass.

Fix it to support full byte range granularity as was originally intended for this ioctl.

Looks good,

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

#11 lore raw

PATCH 3/4 xfs: fix stray dquot unlock when reclaiming dquots

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

When we fail to get a dquot lock during reclaim, we jump to an error handler that unlocks the dquot. This is wrong as we didn't lock the dquot, and unlocking it means who-ever is holding the lock has had it silently taken away, and hence it results in a lock imbalance.

Found by inspection while modifying the code for the numa-lru patchset. This fixes a random hang I've been seeing on xfstest 232 for the past several months.

---
 fs/xfs/xfs_qm.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
fs/xfs/xfs_qm.c +7 -8
index e6a0af0..60eff47 100644
@@ -1456,7 +1456,7 @@ xfs_qm_dqreclaim_one(
 	int			error;
 
 	if (!xfs_dqlock_nowait(dqp))
-		goto out_busy;
+		goto out_move_tail;
 
 	/*
 	 * This dquot has acquired a reference in the meantime remove it from
@@ -1479,7 +1479,7 @@ xfs_qm_dqreclaim_one(
 	 * getting flushed to disk, we don't want to reclaim it.
 	 */
 	if (!xfs_dqflock_nowait(dqp))
-		goto out_busy;
+		goto out_unlock_move_tail;
 
 	if (XFS_DQ_IS_DIRTY(dqp)) {
 		struct xfs_buf	*bp = NULL;
@@ -1490,7 +1490,7 @@ xfs_qm_dqreclaim_one(
 		if (error) {
 			xfs_warn(mp, "%s: dquot %p flush failed",
 				 __func__, dqp);
-			goto out_busy;
+			goto out_unlock_move_tail;
 		}
 
 		xfs_buf_delwri_queue(bp, buffer_list);
@@ -1499,7 +1499,7 @@ xfs_qm_dqreclaim_one(
 		 * Give the dquot another try on the freelist, as the
 		 * flushing will take some time.
 		 */
-		goto out_busy;
+		goto out_unlock_move_tail;
 	}
 	xfs_dqfunlock(dqp);
 
@@ -1518,14 +1518,13 @@ xfs_qm_dqreclaim_one(
 	XFS_STATS_INC(xs_qm_dqreclaims);
 	return;
 
-out_busy:
-	xfs_dqunlock(dqp);
-
 	/*
 	 * Move the dquot to the tail of the list so that we don't spin on it.
 	 */
+out_unlock_move_tail:
+	xfs_dqunlock(dqp);
+out_move_tail:
 	list_move_tail(&dqp->q_lru, &qi->qi_lru_list);
-
 	trace_xfs_dqreclaim_busy(dqp);
 	XFS_STATS_INC(xs_qm_dqreclaim_misses);
 }
Signature · 6 lines
1.7.10

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v1 3/4
#12 lore raw

Re: [PATCH 3/4] xfs: fix stray dquot unlock when reclaiming dquots

Quoted 10 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When we fail to get a dquot lock during reclaim, we jump to an error handler that unlocks the dquot. This is wrong as we didn't lock the dquot, and unlocking it means who-ever is holding the lock has had it silently taken away, and hence it results in a lock imbalance.

Found by inspection while modifying the code for the numa-lru patchset. This fixes a random hang I've been seeing on xfstest 232 for the past several months.

Ooops. I'm kinda surprise the mutex code never cought the double unlock. Beeing able to detect this was on of the sellings points for it vs the old semaphores.

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

#13 lore raw

PATCH 4/4 xfs: fix sparse reported log CRC endian issue

In reply to Dave Chinner

From: Dave Chinner <[email protected]>

Not a bug as such, just warning noise from the xlog_cksum() returning a __be32 type when it shoul dbe returning a __le32 type.

---
 fs/xfs/xfs_log.c         |    2 +-
 fs/xfs/xfs_log_priv.h    |    2 +-
 fs/xfs/xfs_log_recover.c |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)
fs/xfs/xfs_log.c +1 -1
index c49e2c1..46bd9d5 100644
@@ -1538,7 +1538,7 @@ xlog_pack_data(
  * This is a little more complicated than it should be because the various
  * headers and the actual data are non-contiguous.
  */
-__be32
+__le32
 xlog_cksum(
 	struct xlog		*log,
 	struct xlog_rec_header	*rhead,
fs/xfs/xfs_log_priv.h +1 -1
index dc3498b..16d8d12 100644
@@ -555,7 +555,7 @@ extern int
 xlog_recover_finish(
 	struct xlog		*log);
 
-extern __be32	 xlog_cksum(struct xlog *log, struct xlog_rec_header *rhead,
+extern __le32	 xlog_cksum(struct xlog *log, struct xlog_rec_header *rhead,
 			    char *dp, int size);
 
 extern kmem_zone_t *xfs_log_ticket_zone;
fs/xfs/xfs_log_recover.c +3 -3
index 9c3651c..96fcbb8 100644
@@ -3233,15 +3233,15 @@ xlog_unpack_data_crc(
 	xfs_caddr_t		dp,
 	struct xlog		*log)
 {
-	__be32			crc;
+	__le32			crc;
 
 	crc = xlog_cksum(log, rhead, dp, be32_to_cpu(rhead->h_len));
 	if (crc != rhead->h_crc) {
 		if (rhead->h_crc || xfs_sb_version_hascrc(&log->l_mp->m_sb)) {
 			xfs_alert(log->l_mp,
 		"log record CRC mismatch: found 0x%x, expected 0x%x.\n",
-					be32_to_cpu(rhead->h_crc),
-					be32_to_cpu(crc));
+					le32_to_cpu(rhead->h_crc),
+					le32_to_cpu(crc));
 			xfs_hex_dump(dp, 32);
 		}
 
Signature · 6 lines
1.7.10

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

Re: [PATCH 4/4] xfs: fix sparse reported log CRC endian issue

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Not a bug as such, just warning noise from the xlog_cksum() returning a __be32 type when it shoul dbe returning a __le32 type.

The patch look okay.

But why are we storing the crc field little endian while all other on disk formats are big endian? (And yes I realize it might as well have been me who did that back in the idea, but I still have no idea why)

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

#15 lore raw

Re: [PATCH 4/4] xfs: fix sparse reported log CRC endian issue

Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Not a bug as such, just warning noise from the xlog_cksum() returning a __be32 type when it shoul dbe returning a __le32 type.

Quoted 7 lines
Quoted 1 line

The patch look okay.

But why are we storing the crc field little endian while all other on disk formats are big endian? (And yes I realize it might as well have been me who did that back in the idea, but I still have no idea why)

Because the CRC always returns the calcuation LE format, even on BE systems. So rather than always having to byte swap it everywhere and have all the force casts and anootations for sparse, it seems simpler to just make it a __le32 everywhere....

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by
#16 lore raw

Re: [PATCH 4/4] xfs: fix sparse reported log CRC endian issue

Quoted 11 lines · Dave Chinner
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Not a bug as such, just warning noise from the xlog_cksum() returning a __be32 type when it shoul dbe returning a __le32 type.

The patch look okay.

Quoted 8 lines
Quoted 3 lines

But why are we storing the crc field little endian while all other on disk formats are big endian? (And yes I realize it might as well have been me who did that back in the idea, but I still have no idea why)

Because the CRC always returns the calcuation LE format, even on BE systems. So rather than always having to byte swap it everywhere and have all the force casts and anootations for sparse, it seems simpler to just make it a __le32 everywhere....

This seems reasonable to me, and the patch looks fine. Christoph, do have any further objection?

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

#18 lore raw

Re: [PATCH 4/4] xfs: fix sparse reported log CRC endian issue

Hey Christoph,

Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Ben Myers

This seems reasonable to me, and the patch looks fine. Christoph, do have any further objection?

No, although adding the explanation as a comment would be useful.

Cool. I'll add it in then.

Thanks,
Ben

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

#19 lore raw

Re: [PATCH 4/4] xfs: fix sparse reported log CRC endian issue

Quoted 4 lines · Ben Myers

Hey Christoph,

Quoted 1 line · Christoph Hellwig

On Thu, Nov 29, 2012 at 02:32:50PM -0600, Ben Myers wrote:

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

This seems reasonable to me, and the patch looks fine. Christoph, do have any further objection?

No, although adding the explanation as a comment would be useful.

Cool. I'll add it in then.

committed to git://oss.sgi.com/xfs/xfs.git, master and for-next branches.

-Ben

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

Reviewed-by on v1 4/4
#20 lore raw

Re: [PATCH 4/4] xfs: fix sparse reported log CRC endian issue

In reply to Dave Chinner
Quoted 7 lines · Dave Chinner

From: Dave Chinner<[email protected]>

Not a bug as such, just warning noise from the xlog_cksum() returning a __be32 type when it shoul dbe returning a __le32 type.

---

Looks good.

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

#21 lore raw
In reply to Dave Chinner
Quoted 8 lines · Dave Chinner

More bug fixes for 3.8. This series contains:

- the DIO transaction deadlock patch (unmodified),
- addressed Christoph's zero-range review points and fixes the ASSERT in the sub-block zeroing case. (updated)
- a dquot locking issue that has been causing me random hangs in xfstests 232 for months (new)
- fixes for the log CRC sparse endian warnings. (new)

committed 1-3. We'll see what Christoph says about #4.

Regards,
Ben

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