make inode reclaim wait for log I/O to complete

PATCH Superseded Lachlan McIlroy 2 participants 6 messages May 14, 2008 – May 22, 2008

Share & tools

David Chinner

2008-05-14 06:44 UTC #1 lore raw

Quoted 16 lines · 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
xfs/xfs_inode.c renamed from xfs/xfs_inode.c_1.501 +1 -1
@@ -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(

Quoted 2 lines

	xfs_inode_t	*ip)

{

You want to kill the inline on this.

Quoted 13 lines

xfs/xfs_inode.h renamed from xfs/xfs_inode.h_1.245 +1 -0

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

xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3330

Quoted 7 lines

xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0

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

xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3340

Quoted 5 lines

xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0
@@ -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.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Lachlan McIlroy

author

2008-05-22 03:40 UTC #2 lore raw

Quoted 14 lines · David Chinner

Quoted 13 lines · 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
xfs/xfs_inode.c renamed from xfs/xfs_inode.c_1.501 +0 -0
@@ -2787,7 +2787,7 @@ __xfs_iunpin_wait(
 		wait_event(ip->i_ipin_wait, (atomic_read(&ip->i_pincount) == 
 		0));
 }
 

Quoted 7 lines

Quoted 5 lines
xfs/xfs_inode.c renamed from xfs/xfs_inode.c_1.501 +1 -1
-static inline void
+inline void
xfs_iunpin_wait(
	xfs_inode_t	*ip)
{
You want to kill the inline on this.

Done.

Quoted 9 lines

xfs/xfs_inode.h renamed from xfs/xfs_inode.h_1.245 +1 -0

@@ -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 *);

Quoted 16 lines

xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3330
Quoted 12 lines
xfs/xfs_inode.h renamed from xfs/xfs_inode.h_1.245 +0 -0
 /*
 * xfs_inode.c prototypes.
xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0
@@ -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?

Quoted 11 lines

xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3340
Quoted 6 lines
xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0
@@ -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.

David Chinner

2008-05-22 04:31 UTC #3 lore raw

Quoted 5 lines · Lachlan McIlroy

Quoted 4 lines · David Chinner
Quoted 3 lines · 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.

.....

Quoted 15 lines

Quoted 6 lines
Quoted 6 lines
xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +0 -0
@@ -3324,6 +3324,7 @@ xfs_finish_reclaim(
 			 * because we're gonna reclaim the inode anyway.
 			 */
 			if (error) {
Quoted 8 lines
xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3330
Quoted 4 lines
xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0
+				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.

Quoted 6 lines

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.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Lachlan McIlroy

author

2008-05-22 08:20 UTC #4 lore raw

Quoted 20 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy
Quoted 4 lines · David Chinner
Quoted 3 lines · 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.
.....
Quoted 13 lines
Quoted 13 lines
xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3330
Quoted 10 lines
xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0
@@ -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.

Quoted 8 lines

Quoted 6 lines
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;
	}

David Chinner

2008-05-22 23:54 UTC #5 lore raw

Quoted 33 lines · Lachlan McIlroy

Quoted 20 lines · David Chinner
Quoted 5 lines · Lachlan McIlroy
Quoted 4 lines · David Chinner
Quoted 3 lines · 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.
.....
Quoted 13 lines
Quoted 13 lines
xfs/xfs_vnodeops.c xfs_finish_reclaim( · line 3330
Quoted 10 lines
xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.757 +1 -0
@@ -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.
Quoted 8 lines
Quoted 6 lines
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.

Quoted 10 lines

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

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group