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

[Mark Tinguely <tinguely@sgi.com>]

On 02/28/14 14:36, Brian Foster wrote:
> On Fri, Feb 28, 2014 at 02:02:47PM -0600, Mark Tinguely wrote:
>> 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@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.
>>>
>>> Brian
>>>
>>>   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.
>>
>
> Hi Mark,
>
> Good point. We could pull the 'if (acceptable)' check up into the loop
> and exit as soon as we find something writeable.
>
> Alternatively, we could do something like the following and get rid of
> 'acceptable' entirely (or continue to nest the type checks if there's a
> performance concern):
>
> 	...
> 	if (buffer_unwritten(bh) && type == XFS_IO_UNWRITTEN)
> 		return 1;
> 	else if (buffer_delay(bh) && type == XFS_IO_DELALLOC)
> 		return 1;
> 	else if (buffer_dirty(bh) && buffer_mapped(bh)&&
> 		type == XFS_IO_OVERWRITE)
> 		return 1;
> 	...
>
> Brian

Good catch BTW. Yes that is what I was thinking.

--Mark.

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