Re: [PATCH] make inode reclaim wait for log I/O to complete

[David Chinner <dgc@sgi.com>]

On Thu, May 22, 2008 at 06:23:40PM +1000, Lachlan McIlroy wrote:
> David Chinner wrote:
> >On Thu, May 22, 2008 at 01:42:47PM +1000, Lachlan McIlroy wrote:
> >>David Chinner wrote:
> >>>On Wed, May 14, 2008 at 03:24:57PM +1000, Lachlan McIlroy wrote:
> >>>>An xfs inode can be destroyed before log I/O involving that inode
> >>>>is complete.  We need to wait for the inode to be unpinned before
> >>>>tearing it down.
> >.....
> >>>>--- fs/xfs/xfs_vnodeops.c_1.757	2008-05-12 12:02:45.000000000 +1000
> >>>>+++ fs/xfs/xfs_vnodeops.c	2008-05-12 12:28:15.000000000 +1000
> >>>>@@ -3324,6 +3324,7 @@ xfs_finish_reclaim(
> >>>>			 * because we're gonna reclaim the inode anyway.
> >>>>			 */
> >>>>			if (error) {
> >>>>+				xfs_iunpin_wait(ip);
> >>>>				xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >>>>				goto reclaim;
> >>>>			}
> >>>We can't get an error from xfs_iflush() from here that hasn't
> >>>already passed through xfs_iunpin_wait() in xfs_iflush().
> >>>Hence we should never see a pinned inode through this path.
> >>Okay, good point.  I'll remove that one.  I thought about removing
> >>the XFS_FORCED_SHUTDOWN() and dirty inode checks from xfs_finish_reclaim()
> >>and calling xfs_iflush() anyway.  It will abort if it's a clean inode
> >>or it will do the unpin and then abort if it's a forced shutdown.
> >>It would make the code in xfs_finish_reclaim() a bit cleaner.  I also
> >>wouldn't need to export xfs_iunpin_wait().  Thoughts?
> >
> >Sounds like a fine plan. Please comment it appropriately, though.
> 
> Sounded too easy.  Hit this assert with an inode that's still in
> the AIL on a forced shutdown.
> 
> 	/*
> 	 * If the inode isn't dirty, then just release the inode
> 	 * flush lock and do nothing.
> 	 */
> 	if (xfs_inode_clean(ip)) {
> 		ASSERT((iip != NULL) ?
> 			 !(iip->ili_item.li_flags & XFS_LI_IN_AIL) : 1);
> 		xfs_ifunlock(ip);
> 		return 0;
> 	}

That's actually not a valid assert if xfs_iflush() is being called in
the shutdown case. In all previous cases we've checked for shutdown
before calling xfs_iflush(), so that assert would never fire. Now,
if we are calling knowing we might be in the shutdown case, then
this is an invalid assert.

i.e. on shutdown the AIL list is no longer kept properly up to
date - we remove inodes from the AIL in xfs_idestroy() in the
shutdown case....

Cheers,

Dave.

-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group