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

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

[Brian Foster]

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)
-- 
1.8.3.1

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

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

[Mark Tinguely]

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.

--Mark.

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

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

[Brian Foster]

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

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

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

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

[Mark Tinguely]

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

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

[Shaun Gosse]

> 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.
>

The problem is the else doesn't check if acceptable is set before breaking. So it can quit early, not having found what it was looking for even though it was in one of the later buffers. Pre-mature optimization...

So the patch is good as is. It improves correctness.

If the optimization were desired, though, it could be:

+ else if (acceptable)
+   break 

In addition to the remove already posted, and with proper whitespace.

Fair warning: this is without looking at the code, just inferring from what's been written in those two emails. I'm hearing my CS101 professor in my head and thinking may not be worth a couple cycles at the risk of correctness or clarity (and suggesting code blind seems risky).

Actually, this could be the first condition. If the whole point of that loop is just to make sure to set acceptable if it can be set according to the rules on the basis of at least one of the buffers, then a starting:

+ if (acceptable)
+    break

With an annotation something like "/** Optimization: Quit when acceptable has been set. Loop has no other side-effects to worry about.*/" seems reasonable (and hopefully is true). And then it's clear from the code that's the point of the loop and how it works.

Cheers,
-Shaun

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

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

[Shaun Gosse]

Ha, disregard, caught up to read the other email and I see everyone else got there too. I would've been in the thread an hour ago, but unfortunately timed call back from Microsoft support, much to my dismay.

Carry on then...

-----Original Message-----
From: xfs-bounces@oss.sgi.com [mailto:xfs-bounces@oss.sgi.com] On Behalf Of Shaun Gosse
Sent: Friday, February 28, 2014 3:33 PM
To: Mark Tinguely; Brian Foster
Cc: xfs@oss.sgi.com
Subject: RE: [PATCH] xfs: check all buffers in xfs_check_page_type()

> 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.
>

The problem is the else doesn't check if acceptable is set before breaking. So it can quit early, not having found what it was looking for even though it was in one of the later buffers. Pre-mature optimization...

So the patch is good as is. It improves correctness.

If the optimization were desired, though, it could be:

+ else if (acceptable)
+   break 

In addition to the remove already posted, and with proper whitespace.

Fair warning: this is without looking at the code, just inferring from what's been written in those two emails. I'm hearing my CS101 professor in my head and thinking may not be worth a couple cycles at the risk of correctness or clarity (and suggesting code blind seems risky).

Actually, this could be the first condition. If the whole point of that loop is just to make sure to set acceptable if it can be set according to the rules on the basis of at least one of the buffers, then a starting:

+ if (acceptable)
+    break

With an annotation something like "/** Optimization: Quit when acceptable has been set. Loop has no other side-effects to worry about.*/" seems reasonable (and hopefully is true). And then it's clear from the code that's the point of the loop and how it works.

Cheers,
-Shaun

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

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

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

[Dave Chinner]

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