review: don't hold ilock when calling vn_iowait
Regression introduced by recent freezing fixes - we should not hold the ilock while waiting for I/O completion.
Cheers,
Dave.
Signature · 99 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/xfs_vfsops.c | 73 +++++++++++++++++++---------------------------------
1 file changed, 28 insertions(+), 45 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/xfs_vfsops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_vfsops.c 2007-04-19 17:51:09.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_vfsops.c 2007-04-20 17:38:39.946453274 +1000
@@ -1169,58 +1169,41 @@ xfs_sync_inodes(
* in the inode list.
*/
- if ((flags & SYNC_CLOSE) && (vp != NULL)) {
- /*
- * This is the shutdown case. We just need to
- * flush and invalidate all the pages associated
- * with the inode. Drop the inode lock since
- * we can't hold it across calls to the buffer
- * cache.
- *
- * We don't set the VREMAPPING bit in the vnode
- * here, because we don't hold the vnode lock
- * exclusively. It doesn't really matter, though,
- * because we only come here when we're shutting
- * down anyway.
- */
- xfs_iunlock(ip, XFS_ILOCK_SHARED);
-
- if (XFS_FORCED_SHUTDOWN(mp)) {
- bhv_vop_toss_pages(vp, 0, -1, FI_REMAPF);
- } else {
- error = bhv_vop_flushinval_pages(vp, 0, -1, FI_REMAPF);
+ /*
+ * If we have to flush data or wait for I/O completion
+ * we need to drop the ilock that we currently hold.
+ * If we need to drop the lock, insert a marker if we
+ * have not already done so.
+ */
+ if ((flags & (SYNC_CLOSE|SYNC_IOWAIT)) ||
+ ((flags & SYNC_DELWRI) && VN_DIRTY(vp))) {
+ if (mount_locked) {
+ IPOINTER_INSERT(ip, mp);
}
+ xfs_iunlock(ip, XFS_ILOCK_SHARED);
- xfs_ilock(ip, XFS_ILOCK_SHARED);
-
- } else if ((flags & SYNC_DELWRI) && (vp != NULL)) {
- if (VN_DIRTY(vp)) {
- /* We need to have dropped the lock here,
- * so insert a marker if we have not already
- * done so.
- */
- if (mount_locked) {
- IPOINTER_INSERT(ip, mp);
- }
-
- /*
- * Drop the inode lock since we can't hold it
- * across calls to the buffer cache.
- */
- xfs_iunlock(ip, XFS_ILOCK_SHARED);
+ if (flags & SYNC_CLOSE) {
+ /* Shutdown case. Flush and invalidate. */
+ if (XFS_FORCED_SHUTDOWN(mp))
+ bhv_vop_toss_pages(vp, 0, -1, FI_REMAPF);
+ else
+ error = bhv_vop_flushinval_pages(vp, 0,
+ -1, FI_REMAPF);
+ } else if ((flags & SYNC_DELWRI) && VN_DIRTY(vp)) {
error = bhv_vop_flush_pages(vp, (xfs_off_t)0,
-1, fflag, FI_NONE);
- xfs_ilock(ip, XFS_ILOCK_SHARED);
}
+ /*
+ * When freezing, we need to wait ensure all I/O (including direct
+ * I/O) is complete to ensure no further data modification can take
+ * place after this point
+ */
+ if (flags & SYNC_IOWAIT)
+ vn_iowait(vp);
+
+ xfs_ilock(ip, XFS_ILOCK_SHARED);
}
- /*
- * When freezing, we need to wait ensure all I/O (including direct
- * I/O) is complete to ensure no further data modification can take
- * place after this point
- */
- if (flags & SYNC_IOWAIT)
- vn_iowait(vp);
if (flags & SYNC_BDFLUSH) {
if ((flags & SYNC_ATTR) &&Quoted 3 lines · David Chinner
Regression introduced by recent freezing fixes - we should not hold the ilock while waiting for I/O completion.
Looks good, and actually simplies the twisted maze the xfs_sync_inodes is a little bit. And the missing IPOINTER_INSERT in the SYNC_CLOSE case looks like an actual bugfix.
Of course in the end I'd still like to see all pagecache-writeout to be driven by sync_sb_inodes() instead of the fs code, but it'll probably take a little longer until that is done.
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · David Chinner
Regression introduced by recent freezing fixes - we should not hold the ilock while waiting for I/O completion.
Looks good, and actually simplies the twisted maze the
xfs_sync_inodesis a little bit. And the missingIPOINTER_INSERTin theSYNC_CLOSEcase looks like an actual bugfix.
I had to look closely at that IPOINTER_INSERT case with SYNC_CLOSE; it was actaully working properly because you'd always end up in the SYNC_CLOSE case having inserted a pointer earlier on in the flow of the function. It certainly wasn't obvious that it was doing the right thing, though.
Quoted 3 lines
Of course in the end I'd still like to see all pagecache-writeout to be driven by
sync_sb_inodes()instead of the fs code, but it'll probably take a little longer until that is done.
Agreed on both counts.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 10 lines · --On 24 April 2007 9:17:06 AM +1000 David Chinner <[email protected]> wrote:
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · David Chinner
Regression introduced by recent freezing fixes - we should not hold the ilock while waiting for I/O completion.
Looks good, and actually simplies the twisted maze the
xfs_sync_inodesis a little bit. And the missingIPOINTER_INSERTin theSYNC_CLOSEcase looks like an actual bugfix.
Quoted 6 lines
I had to look closely at that
IPOINTER_INSERTcase withSYNC_CLOSE; it was actaully working properly because you'd always end up in theSYNC_CLOSEcase having inserted a pointer earlier on in the flow of the function. It certainly wasn't obvious that it was doing the right thing, though.
I find this existing code and the use of marker pointer macros a bit hard to follow. Can you explain where "earlier on in the flow of the function" we've inserted the marker pointer (and unlocked the inode-list lock).
Obviously whenever we release the inode-list lock, we have to insert the marker first (which is what IPOINTER_INSERT does).
But in what cases do we need to release the inode-list lock (m_ilock).
Having a stab in looking at the code:
* before xfs_finish_reclaim
* before VN_RELE which can call xfs_inactive
* in the vnode reference case, prior to locking inode ????
* prior to unlocking the inode and calling one of the flush or toss pages routines.
* prior to unlocking the inode and reading in its buffer
* Prior to flushing the inode (xfs_iflush)
* Every so often if we loop a lot in the code (preempt variable and mask)
We don't seem to remove the marker afterwards always although we do so on each iteration if we make it to the end of the loop.
It would be nice if this could be clearer somehow.
--Tim
Quoted 14 lines · Timothy Shimmin
Quoted 10 lines · --On 24 April 2007 9:17:06 AM +1000 David Chinner <[email protected]> wrote:
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · David Chinner
Regression introduced by recent freezing fixes - we should not hold the ilock while waiting for I/O completion.
Looks good, and actually simplies the twisted maze the
xfs_sync_inodesis a little bit. And the missingIPOINTER_INSERTin theSYNC_CLOSEcase looks like an actual bugfix.
Quoted 8 lines
Quoted 6 lines
I had to look closely at that
IPOINTER_INSERTcase withSYNC_CLOSE; it was actaully working properly because you'd always end up in theSYNC_CLOSEcase having inserted a pointer earlier on in the flow of the function. It certainly wasn't obvious that it was doing the right thing, though.I find this existing code and the use of marker pointer macros a bit hard to follow.
Doesn't everyone?
Quoted 3 lines
Can you explain where "earlier on in the flow of the function" we've inserted
the marker pointer (and unlocked the inode-list lock).
I confused that with the removal of the vp == NULL checks I removed. Too many things, so little time.
So yes, this probably does fix a bug.
Quoted 1 line
It would be nice if this could be clearer somehow.
Yes, we should be looking to rip all this cruft out because most of it is redundant - the generic inode writeback does most of this for us anyway.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 5 lines · David Chinner
Quoted 1 line
It would be nice if this could be clearer somehow.
Yes, we should be looking to rip all this cruft out because most of it is redundant - the generic inode writeback does most of this for us anyway.
In theory it does the same thing. The problem is that it's really hard to verify.
Btw, before starting with this bit there's another item on my TODO list to simplify xfs_sync_inodes, and that's getting rid
of the vp == NULL case totally. Per definition all vp == NULL inodes are on mp->m_del_inodes. So instead of letting xfs_sync_inodes deals with them we should always call into xfs_finish_reclaim_all after cleaning the latter up a little and veryfing we get the same behaviour.