[Top] [All Lists]

Re: [PATCH 04/18] xfs: page type check in writeback only checks last buf

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 04/18] xfs: page type check in writeback only checks last buffer
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Mon, 16 Apr 2012 11:15:29 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1334319061-12968-5-git-send-email-david@xxxxxxxxxxxxx>
References: <1334319061-12968-1-git-send-email-david@xxxxxxxxxxxxx> <1334319061-12968-5-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 04/13/12 07:10, Dave Chinner wrote:
From: Dave Chinner<dchinner@xxxxxxxxxx>

xfs_is_delayed_page() checks to see if a page has buffers matching
the given IO type passed in. It does so by walking the buffer heads
on the page and checking if the state flags match the IO type.

However, the "acceptable" variable that is calculated is overwritten
every time a new buffer is checked. Hence if the first buffer on the
page is of the right type, this state is lost if the second buffer
is not of the correct type. This means that xfs_aops_discard_page()
may not discard delalloc regions when it is supposed to, and
xfs_convert_page() may not cluster IO as efficiently as possible.

This problem only occurs on filesystems with a block size smaller
than page size.

Also, rename xfs_is_delayed_page() to xfs_check_page_type() to
better describe what it is doing - it is not delalloc specific

The problem was first noticed by Peter Watkins.

Signed-off-by: Dave Chinner<dchinner@xxxxxxxxxx>
  fs/xfs/xfs_aops.c |   12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 0783def..2fc12db 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -623,7 +623,7 @@ xfs_map_at_offset(
   * or delayed allocate extent.
  STATIC int
        struct page             *page,
        unsigned int            type)
@@ -637,11 +637,11 @@ xfs_is_delayed_page(
                bh = head = page_buffers(page);
                do {
                        if (buffer_unwritten(bh))
-                               acceptable = (type == IO_UNWRITTEN);
+                               acceptable += (type == IO_UNWRITTEN);
                        else if (buffer_delay(bh))
-                               acceptable = (type == IO_DELALLOC);
+                               acceptable += (type == IO_DELALLOC);
                        else if (buffer_dirty(bh)&&  buffer_mapped(bh))
-                               acceptable = (type == IO_OVERWRITE);
+                               acceptable += (type == IO_OVERWRITE);

Looks good.

Could short-cut and return 1 on the first acceptable buffer rather than scanning the entire too.

Reviewed-by: Mark Tinguely <tinguely@xxxxxxx>

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