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@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. 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@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
