review: don't hold ilock when calling vn_iowait

review: don't hold ilock when calling vn_iowait

[David Chinner]

Regression introduced by recent freezing fixes - we should
not hold the ilock while waiting for I/O completion.

Cheers,

Dave.
-- 
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) &&

Re: review: don't hold ilock when calling vn_iowait

[Christoph Hellwig]

On Mon, Apr 23, 2007 at 09:03:03AM +1000, David Chinner wrote:
> 
> 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.

Re: review: don't hold ilock when calling vn_iowait

[David Chinner]

On Mon, Apr 23, 2007 at 10:43:38PM +0100, Christoph Hellwig wrote:
> On Mon, Apr 23, 2007 at 09:03:03AM +1000, David Chinner wrote:
> > 
> > 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.

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.

> 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: review: don't hold ilock when calling vn_iowait

[Timothy Shimmin]

--On 24 April 2007 9:17:06 AM +1000 David Chinner <dgc@sgi.com> wrote:

> On Mon, Apr 23, 2007 at 10:43:38PM +0100, Christoph Hellwig wrote:
>> On Mon, Apr 23, 2007 at 09:03:03AM +1000, David Chinner wrote:
>> >
>> > 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.
>
> 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.
>
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

Re: review: don't hold ilock when calling vn_iowait

[David Chinner]

On Tue, Apr 24, 2007 at 12:00:38PM +1000, Timothy Shimmin wrote:
> 
> 
> --On 24 April 2007 9:17:06 AM +1000 David Chinner <dgc@sgi.com> wrote:
> 
> >On Mon, Apr 23, 2007 at 10:43:38PM +0100, Christoph Hellwig wrote:
> >>On Mon, Apr 23, 2007 at 09:03:03AM +1000, David Chinner wrote:
> >>>
> >>> 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.
> >
> >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.
> >
> I find this existing code and the use of marker pointer macros a bit hard 
> to follow.

Doesn't everyone?

> 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.

> 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: review: don't hold ilock when calling vn_iowait

[Christoph Hellwig]

On Tue, Apr 24, 2007 at 01:08:26PM +1000, David Chinner wrote:
> > 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.