xfs: check all buffers in xfs_check_page_type()
PATCH 1/1 xfs: check all buffers in xfs_check_page_type()
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.
---
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(-)
fs/xfs/xfs_aops.c +0 -2
@@ -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)
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 40 lines · 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<[email protected]>---
The only other caller to
xfs_check_page_type()isxfs_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(-)fs/xfs/xfs_aops.c +0
-0
fs/xfs/xfs_aops.c xfs_check_page_type( · line 660
Quoted 8 lines
fs/xfs/xfs_aops.c +0
-2@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 48 lines · Mark Tinguely
Quoted 47 lines · 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<[email protected]>---
The only other caller to
xfs_check_page_type()isxfs_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(-)fs/xfs/xfs_aops.c +0
-2@@ -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);
Quoted 6 lines
fs/xfs/xfs_aops.c xfs_check_page_type( · line 660
Quoted 1 line
fs/xfs/xfs_aops.c +0
-0if (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
Quoted 6 lines
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 75 lines · Brian Foster
Quoted 48 lines · Mark Tinguely
Quoted 47 lines · 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<[email protected]>---
The only other caller to
xfs_check_page_type()isxfs_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(-)fs/xfs/xfs_aops.c +0
-2@@ -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);Quoted 6 lines
fs/xfs/xfs_aops.c xfs_check_page_type( · line 660
Quoted 1 line
fs/xfs/xfs_aops.c +0
-0if (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; ...
Quoted 1 line
Brian
Good catch BTW. Yes that is what I was thinking.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 3 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
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: [email protected] [mailto:[email protected]] On Behalf Of Shaun Gosse
Sent: Friday, February 28, 2014 3:33 PM
To: Mark Tinguely; Brian Foster
Cc: [email protected]
Subject: RE: [PATCH] xfs: check all buffers in xfs_check_page_type()
Quoted 3 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 23 lines · 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.
Quoted 6 lines
Signed-off-by Brian Foster <[email protected]>---
The only other caller to
xfs_check_page_type()isxfs_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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs