[Top] [All Lists]

Re: [PATCH] xfs: check all buffers in xfs_check_page_type()

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: check all buffers in xfs_check_page_type()
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 28 Feb 2014 14:02:47 -0600
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1393615369-41882-1-git-send-email-bfoster@xxxxxxxxxx>
References: <1393615369-41882-1-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 02/28/14 13:22, Brian Foster wrote:
xfs_aops_discard_page() was introduced in the following commit:

   xfs: truncate delalloc extents when IO fails in writeback

... to clean up left over delalloc ranges after I/O failure in
->writepage(). generic/224 tests for this scenario and occasionally
reproduces panics on sub-4k blocksize filesystems.

The cause of this is failure to clean up the delalloc range on a
page where the first buffer does not match one of the expected
states of xfs_check_page_type(). If a buffer is not unwritten,
delayed or dirty&mapped, xfs_check_page_type() stops and
immediately returns 0.

The stress test of generic/224 creates a scenario where the first
several buffers of a page with delayed buffers are mapped&uptodate
and some subsequent buffer is delayed. If the ->writepage() happens
to fail for this page, xfs_aops_discard_page() incorrectly skips
the entire page.

Modify xfs_aops_discard_page() to iterate all of the page buffers
to ensure a delayed buffer does not go undetected.

Signed-off-by: Brian Foster<bfoster@xxxxxxxxxx>

The only other caller to xfs_check_page_type() is xfs_convert_page(). I
think this is safe with respect to that codepath, given the additional
imap checks therein and whatnot, but thoughts appreciated.


  fs/xfs/xfs_aops.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index db2cfb0..5962a9f 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -655,8 +655,6 @@ xfs_check_page_type(
                                acceptable += (type == XFS_IO_DELALLOC);
                        else if (buffer_dirty(bh) && buffer_mapped(bh))
                                acceptable += (type == XFS_IO_OVERWRITE);
-                       else
-                               break;
                } while ((bh = bh->b_this_page) != head);

                if (acceptable)

Is there any reason to scan all the buffers when we all we want is an indication that at least one is acceptable? Maybe there are generally not may buffers to a page to make it worthwhile.


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