[Top] [All Lists]

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

To: Mel Gorman <mgorman@xxxxxxx>
Subject: Re: [PATCH 020/102] xfs: warn if direct reclaim tries to writeback pages
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 14 Sep 2012 07:56:25 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, Dave Chinner <dchinner@xxxxxxxxxx>
In-reply-to: <20120914094523.GD11266@xxxxxxx>
References: <1345698180-13612-1-git-send-email-david@xxxxxxxxxxxxx> <1345698180-13612-21-git-send-email-david@xxxxxxxxxxxxx> <20120827181742.GA13970@xxxxxxxxxxxxx> <20120905113242.GL11266@xxxxxxx> <50523E20.5000508@xxxxxxx> <20120914094523.GD11266@xxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
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@xxxxxxx>

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@xxxxxxxxxx>
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@xxxxxxxxxx>
    Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
    Reviewed-by: Christoph Hellwig <hch@xxxxxx>
    Reviewed-by: Mark Tinguely <tinguely@xxxxxxx>
    Signed-off-by: Ben Myers <bpm@xxxxxxx>

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

<Prev in Thread] Current Thread [Next in Thread>