make inode reclaim synchronise with xfs_iflush_done()

On a forced shutdown, xfs_finish_reclaim() will skip flushing the inode. If the inode flush lock is not already held and there is an outstanding xfs_iflush_done() then we might free the inode prematurely. By acquiring and releasing the flush lock we will synchronise with xfs_iflush_done().

(can ppl please post patches in line rather than as attachments as it's really hard to quote attachments)

Quoted 4 lines · Lachlan McIlroy

On a forced shutdown, xfs_finish_reclaim() will skip flushing the inode. If the inode flush lock is not already held and there is an outstanding xfs_iflush_done() then we might free the inode prematurely. By acquiring and releasing the flush lock we will synchronise with xfs_iflush_done().

Yes, That could happen. Good catch. Comments on the code below.

Quoted 3 lines

Alternatively we could take a hold on the inode when when issuing I/Os with xfs_iflush_done() and release it in xfs_iflush_done(). Would this be a better approach?

No - we can't take a hold on the inode because it may not have a linux inode attached to it and hence there's nothing to hold the reference count (we use the linux inode reference count for this).

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

@@ -3762,20 +3762,29 @@ xfs_finish_reclaim(
 				goto reclaim;
 			}
 			xfs_iflock(ip); /* synchronize with xfs_iflush_done */
+			xfs_ifunlock(ip);
 		}
 

Why do you unlock it here? If it is left locked, there is absolutely no chance for the inode to be flushed again after this point.

xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.726 +12 -4

 		ASSERT(ip->i_update_core == 0);
 		ASSERT(ip->i_itemp == NULL ||
 		       ip->i_itemp->ili_format.ilf_fields == 0);
 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
-	} else if (locked) {
+	} else {
 		/*
 		 * We are not interested in doing an iflush if we're
 		 * in the process of shutting down the filesystem forcibly.
 		 * So, just reclaim the inode.
-		 */
-		xfs_ifunlock(ip);
-		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		 * 
+		 * If the flush lock is not already held then temporarily
+		 * acquire it to synchronize with xfs_iflush_done.
+		 */
+		if (locked) {
+			xfs_ifunlock(ip);
+			xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		} else {
+			xfs_iflock(ip);
+			xfs_ifunlock(ip);
+		}
 	}

Oh, that just makes it messy :/

How about locking the inode unconditionally before the entire if..else statement like:

xfs/xfs_vnodeops.c renamed from xfs/xfs_vnodeops.c_1.726 +5 -6

+	if (!locked) {
+		xfs_ilock(ip, XFS_ILOCK_EXCL);
+		xfs_iflock(ip);
+	}
        if (!XFS_FORCED_SHUTDOWN(ip->i_mount)) {
-		if (!locked) {
-			xfs_ilock(ip, XFS_ILOCK_EXCL);
-			xfs_iflock(ip);
-		}
 
.....
-	} else if (locked) {
+	} else {
 		/*
 		 * We are not interested in doing an iflush if we're
 		 * in the process of shutting down the filesystem forcibly.
 		 * So, just reclaim the inode.
-		xfs_ifunlock(ip);

		xfs_iunlock(ip, XFS_ILOCK_EXCL);

}
reclaim:

That would mean we always go to xfs_ireclaim() with the flush lock held and hence a guarantee that no more I/O can be issued on the inode.