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

[Lachlan McIlroy <lachlan@sgi.com>]

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;
	}