[PATCH 020/102] xfs: warn if direct reclaim tries to writeback pages

[Mark Tinguely]

On 09/14/12 04:45, Mel Gorman wrote:
> On Thu, Sep 13, 2012 at 03:12:16PM -0500, Mark Tinguely wrote:
>> On 09/05/12 06:32, Mel Gorman wrote:
>>> On Mon, Aug 27, 2012 at 02:17:42PM -0400, Christoph Hellwig wrote:
>>>> On Thu, Aug 23, 2012 at 03:01:38PM +1000, Dave Chinner wrote:
>>>>> From: Mel Gorman<mgorman at suse.de>
>>>>>
>>>>> Upstream commit: 94054fa3fca1fd78db02cb3d68d5627120f0a1d4
>>>>>
>>>>> Direct reclaim should never writeback pages.  For now, handle the
>>>>> situation and warn about it.  Ultimately, this will be a BUG_ON.
>>>>
>>>> Is this actually the case on 3.0-stable?
>>>>
>>>
>>> No, it is not. AFAIK, 3.0-stable does not contain [ee72886d: mm: vmscan:
>>> do not writeback filesystem pages in direct reclaim] which is the absolute
>>> minimum required for commit [94054fa3: xfs: warn if direct reclaim tries
>>> to writeback pages] to make sense.
>>>
>>
>> I hit this warning testing on a linux-30.42 with a x86_64.
>>
>
> Just to be clear, this was linux 3.0.42 with this series of patches
> applied on top, right?
>
>> WARNING: at fs/xfs/linux-2.6/xfs_aops.c:961 xfs_vm_writepage+0x63c/0x6a0()
>> Hardware name: S2721-533 Thunder i7501 Pro
>> Modules linked in: ext4 jbd2 crc16
>> Pid: 12122, comm: cp Not tainted 3.0.42 #2
>
> I ask because on 3.0.42 this warning is not present. This patch should
> not be merged to 3.0-stable.
>

Correct. Linux-3.0.42 with Dave 102 patches, I removed patch 94 and 95 
per list comments. I added Brian Foster's patch as 103 <below>

I am testing xfstest 109 and 273 in a loop for shutdown worker races - 
none found yet. That message happened in the first loop, and never 
occurred again.

And the machine was a x86_32 not 64.

patch 103:


Subject: xfs: check for stale inode before acquiring iflock on push
From: Brian Foster <bfoster at redhat.com>
Upstream commit: 9a3a5dab63461b84213052888bf38a962b22d035

     An inode in the AIL can be flush locked and marked stale if
     a cluster free transaction occurs at the right time. The
     inode item is then marked as flushing, which causes xfsaild
     to spin and leaves the filesystem stalled. This is
     reproduced by running xfstests 273 in a loop for an
     extended period of time.

     Check for stale inodes before the flush lock. This marks
     the inode as pinned, leads to a log flush and allows the
     filesystem to proceed.

     Signed-off-by: Brian Foster <bfoster at redhat.com>
     Reviewed-by: Dave Chinner <dchinner at redhat.com>
     Reviewed-by: Christoph Hellwig <hch at lst.de>
     Reviewed-by: Mark Tinguely <tinguely at sgi.com>
     Signed-off-by: Ben Myers <bpm at sgi.com>


Index: linux-3.0.42/fs/xfs/xfs_inode_item.c
===================================================================
--- linux-3.0.42.orig/fs/xfs/xfs_inode_item.c
+++ linux-3.0.42/fs/xfs/xfs_inode_item.c
@@ -507,6 +507,21 @@ xfs_inode_item_trylock(
  	if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
  		return XFS_ITEM_LOCKED;

+	/*
+	 * Re-check the pincount now that we stabilized the value by
+	 * taking the ilock.
+	 */
+	if (xfs_ipincount(ip) > 0) {
+		xfs_iunlock(ip, XFS_ILOCK_SHARED);
+		return XFS_ITEM_PINNED;
+	}
+
+	/* Stale items should force out the iclog */
+	if (ip->i_flags & XFS_ISTALE) {
+		xfs_iunlock(ip, XFS_ILOCK_SHARED);
+		return XFS_ITEM_PINNED;
+	}
+
  	if (!xfs_iflock_nowait(ip)) {
  		/*
  		 * inode has already been flushed to the backing buffer,
@@ -516,13 +531,6 @@ xfs_inode_item_trylock(
  		return XFS_ITEM_PUSHBUF;
  	}

-	/* Stale items should force out the iclog */
-	if (ip->i_flags & XFS_ISTALE) {
-		xfs_ifunlock(ip);
-		xfs_iunlock(ip, XFS_ILOCK_SHARED);
-		return XFS_ITEM_PINNED;
-	}
-
  #ifdef DEBUG
  	if (!XFS_FORCED_SHUTDOWN(ip->i_mount)) {
  		ASSERT(iip->ili_fields != 0);