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

[Dave Chinner <david@fromorbit.com>]

On Fri, Feb 28, 2014 at 02:22:49PM -0500, 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@redhat.com>
> ---
> 
> 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.

Just to close the loop ifor everyone else on the IRC discussion
Brian and I had - removing the break statement is likely to cause
problems with xfs_convert_page().

What xfs_convert_page() assumes is that xfs_check_page_type() will
return true iff the first iand subsequent buffers on the page match
the given type and can be written back.  Skipping over buffers that
have unknown contents is incorrect behaviour - if the first buffer
on the page is unmapped, then it should break and return false.

However, xfs_aops_discard_page() requires it to check all buffers on
the page for delalloc state so that we can punch them correctly, and
so breaking out at the first unwriteable buffer is a bug.

Hence to fix this, we need to change the way xfs_convert_page()
works. It needs to stop processing buffesr in it's main loop
whenever "done" gets set so that it stops at the same point that
xfs_check_page_type() stops checking the buffers on the page. Once
that is done, then we can modify xfs_check_page_type() to return
true when it finds the first buffer of a given type on the page
or false if it finds an unmapped buffer and we are looking for
IO_DELALLOC....

And it needs a decent set of comments, too :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs