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

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

[Lachlan McIlroy]

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.

Lachlan

--- fs/xfs/xfs_inode.c_1.501	2008-05-12 14:45:17.000000000 +1000
+++ fs/xfs/xfs_inode.c	2008-05-12 12:23:48.000000000 +1000
@@ -2787,7 +2787,7 @@ __xfs_iunpin_wait(
 		wait_event(ip->i_ipin_wait, (atomic_read(&ip->i_pincount) == 0));
 }
 
-static inline void
+inline void
 xfs_iunpin_wait(
 	xfs_inode_t	*ip)
 {
--- fs/xfs/xfs_inode.h_1.245	2008-05-12 14:45:20.000000000 +1000
+++ fs/xfs/xfs_inode.h	2008-05-12 12:31:37.000000000 +1000
@@ -481,6 +481,7 @@ void		xfs_ifunlock(xfs_inode_t *);
 void		xfs_ireclaim(xfs_inode_t *);
 int		xfs_finish_reclaim(xfs_inode_t *, int, int);
 int		xfs_finish_reclaim_all(struct xfs_mount *, int);
+void		xfs_iunpin_wait(xfs_inode_t *);
 
 /*
  * xfs_inode.c prototypes.
--- 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;
 			}
@@ -3336,6 +3337,7 @@ xfs_finish_reclaim(
 	}
 
 	xfs_ifunlock(ip);
+	xfs_iunpin_wait(ip);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 
  reclaim:

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

[David Chinner]

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.
> 
> Lachlan
> 
> --- fs/xfs/xfs_inode.c_1.501	2008-05-12 14:45:17.000000000 +1000
> +++ fs/xfs/xfs_inode.c	2008-05-12 12:23:48.000000000 +1000
> @@ -2787,7 +2787,7 @@ __xfs_iunpin_wait(
> 		wait_event(ip->i_ipin_wait, (atomic_read(&ip->i_pincount) == 
> 		0));
> }
> 
> -static inline void
> +inline void
> xfs_iunpin_wait(
> 	xfs_inode_t	*ip)
> {

You want to kill the inline on this.

> --- fs/xfs/xfs_inode.h_1.245	2008-05-12 14:45:20.000000000 +1000
> +++ fs/xfs/xfs_inode.h	2008-05-12 12:31:37.000000000 +1000
> @@ -481,6 +481,7 @@ void		xfs_ifunlock(xfs_inode_t *);
> void		xfs_ireclaim(xfs_inode_t *);
> int		xfs_finish_reclaim(xfs_inode_t *, int, int);
> int		xfs_finish_reclaim_all(struct xfs_mount *, int);
> +void		xfs_iunpin_wait(xfs_inode_t *);
> 
> /*
>  * xfs_inode.c prototypes.
> --- 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.

> @@ -3336,6 +3337,7 @@ xfs_finish_reclaim(
> 	}
> 
> 	xfs_ifunlock(ip);
> +	xfs_iunpin_wait(ip);

If we are not shutting down the filesystem, how do we get a pinned
inode here?  A pinned inode is a dirty inode and should be caught by
the above code. Is the crash occurring when a force shutdown is in
progress?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

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

[Lachlan McIlroy]

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.
>>
>> Lachlan
>>
>> --- fs/xfs/xfs_inode.c_1.501	2008-05-12 14:45:17.000000000 +1000
>> +++ fs/xfs/xfs_inode.c	2008-05-12 12:23:48.000000000 +1000
>> @@ -2787,7 +2787,7 @@ __xfs_iunpin_wait(
>> 		wait_event(ip->i_ipin_wait, (atomic_read(&ip->i_pincount) == 
>> 		0));
>> }
>>
>> -static inline void
>> +inline void
>> xfs_iunpin_wait(
>> 	xfs_inode_t	*ip)
>> {
> 
> You want to kill the inline on this.

Done.

> 
>> --- fs/xfs/xfs_inode.h_1.245	2008-05-12 14:45:20.000000000 +1000
>> +++ fs/xfs/xfs_inode.h	2008-05-12 12:31:37.000000000 +1000
>> @@ -481,6 +481,7 @@ void		xfs_ifunlock(xfs_inode_t *);
>> void		xfs_ireclaim(xfs_inode_t *);
>> int		xfs_finish_reclaim(xfs_inode_t *, int, int);
>> int		xfs_finish_reclaim_all(struct xfs_mount *, int);
>> +void		xfs_iunpin_wait(xfs_inode_t *);
>>
>> /*
>>  * xfs_inode.c prototypes.
>> --- 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?

> 
>> @@ -3336,6 +3337,7 @@ xfs_finish_reclaim(
>> 	}
>>
>> 	xfs_ifunlock(ip);
>> +	xfs_iunpin_wait(ip);
> 
> If we are not shutting down the filesystem, how do we get a pinned
> inode here?  A pinned inode is a dirty inode and should be caught by
> the above code. Is the crash occurring when a force shutdown is in
> progress?

Yes, sorry, forgot to mention this was during a forced shutdown.

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

[David Chinner]

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.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

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

[Lachlan McIlroy]

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

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

[David Chinner]

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