xfs: delalloc, dio and corruption...
v2 cover xfs: delalloc, dio and corruption...
Hi folks,
This is version 2 of the DIO vs delalloc patchset I posted here:
http://oss.sgi.com/archives/xfs/2014-03/msg00313.html
The changes to this version are:
- the bug fix to patch 2 that Brain noticed,
- I dropped the delalloc extent splittting patch because with the fix to patch 2 I can't trigger that bug anymore, and that patch was causing transaction overruns in xfs/297. Hence without an existing reproducer, I won't try to fix that problem.
- the last patch is new, and is a bug in the collapse range code where it fails to shift the last N extents correctly if there are N delalloc extents before the shifted range.
With these 6 patches, all of the xfstests fsx/fsstress tests pass on 1k, 2k and 4k block size filesystems, with and without CRCs enabled, on 1, 2 and 16p test VMs.
I'm much happier with these patches now - I don't think that there are more problems lurking, but only time will tell. I'd like to get these fixes to Linus for 3.15 (probably for -rc2), so eyeballs and testing would be appreciated.
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/6 xfs: kill buffers over failed write ranges properly
From: Dave Chinner <[email protected]>
When a write fails, if we don't clear the delalloc flags from the buffers over the failed range, they can persist beyond EOF and cause problems. writeback will see the pages int eh page cache, see they are dirty and continually retry the write, assuming that the page beyond EOF is just racing with a truncate. The page will eventually be released due to some other operation (e.g. direct IO), and it will not pass through invalidation because it is dirty. Hence it will be released with buffer_delay set on it, and trigger warnings in xfs_vm_releasepage() and assert fail in xfs_file_aio_write_direct because invalidation failed and we didn't write the corect amount.
This causes failures on block size < page size filesystems in fsx and fsstress workloads run by xfstests.
Fix it by completely trashing any state on the buffer that could be used to imply that it contains valid data when the delalloc range over the buffer is punched out during the failed write handling.
--- fs/xfs/xfs_aops.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/xfs/xfs_aops.c +10 -0
@@ -1566,6 +1566,16 @@ xfs_vm_write_failed(xfs_vm_kill_delalloc_range (inode, block_offset, block_offset + bh->b_size); + +/* +* This buffer does not contain data anymore. make sure anyone +* who finds it knows that for certain. +*/ +clear_buffer_delay (bh); +clear_buffer_uptodate (bh); +clear_buffer_mapped (bh); +clear_buffer_new (bh); +clear_buffer_dirty (bh); } }
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/6] xfs: kill buffers over failed write ranges properly
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 2/6 xfs: write failure beyond EOF truncates too much data
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
If we fail a write beyond EOF and have to handle it in xfs_vm_write_begin(), we truncate the inode back to the current inode size. This doesn't take into account the fact that we may have already made successful writes to the same page (in the case of block size < page size) and hence we can truncate the page cache away from blocks with valid data in them. If these blocks are delayed allocation blocks, we now have a mismatch between the page cache and the extent tree, and this will trigger - at minimum - a delayed block count mismatch assert when the inode is evicted from the cache. We can also trip over it when block mapping for direct IO - this is the most common symptom seen from fsx and fsstress when run from xfstests.
Fix it by only truncating away the exact range we are updating state for in this write_begin call.
--- fs/xfs/xfs_aops.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
fs/xfs/xfs_aops.c +11 -2
@@ -1609,12 +1609,21 @@ xfs_vm_write_begin( status =__block_write_begin (page, pos, len, xfs_get_blocks);if (unlikely (status)) {struct inode *inode = mapping->host; +size_t isize =i_size_read (inode);xfs_vm_write_failed (inode, page, pos, len);unlock_page (page); -if (pos + len >i_size_read (inode)) -truncate_pagecache (inode,i_size_read (inode)); +/* +* If the write is beyond EOF, we only want to kill blocks +* allocated in this write, not blocks that were previously +* written successfully. +*/ +if (pos + len > isize) { +ssize_t start =max_t (ssize_t, pos, isize); + +truncate_pagecache_range (inode, start, pos + len); + }page_cache_release (page); page =NULL ;
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/6] xfs: write failure beyond EOF truncates too much data
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 3/6 xfs: xfs_vm_write_end truncates too much on failure
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Similar to the write_begin problem, xfs-vm_write_end will truncate back to the old EOF, potentially removing page cache from over the top of delalloc blocks with valid data in them. Fix this by truncating back to just the start of the failed write.
--- fs/xfs/xfs_aops.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
fs/xfs/xfs_aops.c +10 -4
@@ -1634,9 +1634,12 @@ xfs_vm_write_begin( }/* -* On failure, we only need to kill delalloc blocks beyond EOF because they -* will never be written. For blocks within EOF, generic_write_end() zeros them -* so they are safe to leave alone and be written with all the other valid data. +* On failure, we only need to kill delalloc blocks beyond EOF in the range of +* this specific write because they will never be written. Previous writes +* beyond EOF where block allocation succeeded do not need to be trashed, so +* only new blocks from this write should be trashed. For blocks within +* EOF, generic_write_end() zeros them so they are safe to leave alone and be +* written with all the other valid data. */ STATIC int xfs_vm_write_end(
@@ -1659,8 +1662,11 @@ xfs_vm_write_end(loff_t to = pos + len;if (to > isize) { -truncate_pagecache (inode, isize); +/* only kill blocks in this write beyond EOF */ +if (pos > isize) + isize = pos;xfs_vm_kill_delalloc_range (inode, isize, to); +truncate_pagecache_range (inode, isize, to); } }return ret;
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/6] xfs: xfs_vm_write_end truncates too much on failure
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Similar to the
write_beginproblem, xfs-vm_write_end will truncate back to the old EOF, potentially removing page cache from over the top of delalloc blocks with valid data in them. Fix this by truncating back to just the start of the failed write.Signed-off-by Dave Chinner <[email protected]>
Looks good,
Any reason you used max_t in the previous one, and opencode it here?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/6] xfs: xfs_vm_write_end truncates too much on failure
Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Similar to the
write_beginproblem, xfs-vm_write_end will truncate back to the old EOF, potentially removing page cache from over the top of delalloc blocks with valid data in them. Fix this by truncating back to just the start of the failed write.
Quoted 7 lines
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>Looks good,
Reviewed-by Christoph Hellwig <[email protected]>Any reason you used
max_tin the previous one, and opencode it here?
No, just made the changes at different times. I'll fix them both to use max_t....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 4/6 xfs: zeroing space needs to punch delalloc blocks
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When we are zeroing space andit is covered by a delalloc range, we need to punch the delalloc range out before we truncate the page cache. Failing to do so leaves and inconsistency between the page cache and the extent tree, which we later trip over when doing direct IO over the same range.
--- fs/xfs/xfs_bmap_util.c | 13 ++++++++++++- fs/xfs/xfs_trace.h | 1 + 2 files changed, 13 insertions(+), 1 deletion(-)
fs/xfs/xfs_bmap_util.c +12 -1
@@ -1418,6 +1418,8 @@ xfs_zero_file_space(xfs_off_t end_boundary;int error; +trace_xfs_zero_file_space (ip); + granularity =max_t (uint,1 << mp->m_sb.sb_blocklog,PAGE_CACHE_SIZE );/*
@@ -1432,9 +1434,18 @@ xfs_zero_file_space(ASSERT (end_boundary <= offset + len);if (start_boundary < end_boundary -1 ) { -/* punch out the page cache over the conversion range */ +/* +* punch out delayed allocation blocks and the page cache over +* the conversion range +*/ +xfs_ilock (ip,XFS_ILOCK_EXCL ); + error =xfs_bmap_punch_delalloc_range (ip, +XFS_B_TO_FSB (mp, start_boundary), +XFS_B_TO_FSB (mp, end_boundary - start_boundary)); +xfs_iunlock (ip,XFS_ILOCK_EXCL );truncate_pagecache_range (VFS_I (ip), start_boundary, end_boundary -1 ); +/* convert the blocks */ error = xfs_alloc_file_space(ip, start_boundary, end_boundary - start_boundary - 1,
fs/xfs/xfs_trace.h +1 -0
@@ -603,6 +603,7 @@ DEFINE_INODE_EVENT(xfs_readlink);DEFINE_INODE_EVENT (xfs_inactive_symlink);DEFINE_INODE_EVENT (xfs_alloc_file_space);DEFINE_INODE_EVENT (xfs_free_file_space); +DEFINE_INODE_EVENT (xfs_zero_file_space);DEFINE_INODE_EVENT (xfs_collapse_file_space);DEFINE_INODE_EVENT (xfs_readdir);#ifdef CONFIG_XFS_POSIX_ACL
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/6] xfs: zeroing space needs to punch delalloc blocks
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When we are zeroing space andit is covered by a delalloc range, we need to punch the delalloc range out before we truncate the page cache. Failing to do so leaves and inconsistency between the page cache and the extent tree, which we later trip over when doing direct IO over the same range.
Looks good.
Which test found this?
fs/xfs/xfs_bmap_util.c xfs_zero_file_space( · line 1443
Quoted 12 lines
fs/xfs/xfs_bmap_util.c +7
-1@@ -1432,9 +1434,18 @@ xfs_zero_file_space(ASSERT (end_boundary <= offset + len);if (start_boundary < end_boundary -1 ) { -/* punch out the page cache over the conversion range */ +/* +* punch out delayed allocation blocks and the page cache over +* the conversion range +*/ +xfs_ilock (ip,XFS_ILOCK_EXCL ); + error = xfs_bmap_punch_delalloc_range(ip, + XFS_B_TO_FSB(mp, start_boundary),
Shouldn't this be XFS_B_TO_FSBT?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/6] xfs: zeroing space needs to punch delalloc blocks
Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When we are zeroing space andit is covered by a delalloc range, we need to punch the delalloc range out before we truncate the page cache. Failing to do so leaves and inconsistency between the page cache and the extent tree, which we later trip over when doing direct IO over the same range.
Quoted 3 lines
Looks good.
Which test found this?
The fsstress tests - any of them will do. It showed after I merged Lukas' FALLOC_FL_ZERO_RANGE support patches for fsstress and fsx into xfstests, but it wasn't reliably triggering until I fixed the __xfs_get_blocks DIO bug that Al found...
Quoted 14 lines
fs/xfs/xfs_bmap_util.c xfs_zero_file_space( · line 1443
Quoted 12 lines
fs/xfs/xfs_bmap_util.c +7
-1@@ -1432,9 +1434,18 @@ xfs_zero_file_space(ASSERT (end_boundary <= offset + len);if (start_boundary < end_boundary -1 ) { -/* punch out the page cache over the conversion range */ +/* +* punch out delayed allocation blocks and the page cache over +* the conversion range +*/ +xfs_ilock (ip,XFS_ILOCK_EXCL ); + error = xfs_bmap_punch_delalloc_range(ip, + XFS_B_TO_FSB(mp, start_boundary),Shouldn't this be
XFS_B_TO_FSBT?
XFS_B_TO_FSB gives the same result given that we've already rounded start_boundary to block granularity....
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/6] xfs: zeroing space needs to punch delalloc blocks
Quoted 2 lines · Dave Chinner
XFS_B_TO_FSBgives the same result given that we've already roundedstart_boundaryto block granularity....
It does, but XFS_B_TO_FSBT is one less operation, and it's the better example for a start offset when people start copy & pasting it.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/6] xfs: zeroing space needs to punch delalloc blocks
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Dave Chinner
XFS_B_TO_FSBgives the same result given that we've already roundedstart_boundaryto block granularity....It does, but
XFS_B_TO_FSBTis one less operation, and it's the better example for a start offset when people start copy & pasting it.
*nod*.
Fixed it.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Christoph Hellwig
Reviewed-byRe: [PATCH 4/6] xfs: zeroing space needs to punch delalloc blocks
Quoted 1 line · Dave Chinner
Fixed it.
with that little tweak. And yes, we'll need that series ASAP - I not hit the asserts everytime I do an xfstests run.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 5/6 xfs: don't map ranges that span EOF for direct IO
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Al Viro tracked down the problem that has caused generic/263 to fail on XFS since the test was introduced. If is caused by xfs_get_blocks() mapping a single extent that spans EOF without marking it as buffer-new() so that the direct Io code does not zero the tail of the block at the new EOF. This is a long standing bug that has been around for many, many years.
Because xfs_get_blocks() starts the map before EOF, it can't set buffer_new(), because that causes he direct Io code to also zero unaligned sectors at the head of the IO. This would overwrite valid data with zeros, and hence we cannot validly return a single extent that spans EOF to direct IO.
Fix this by detecting a mapping that spans EOF and truncate it down to EOF. This results in the the direct IO code doing the right thing for unaligned data blocks before EOF, and then returning to get another mapping for the region beyond EOF which XFS treats correctly by setting buffer_new() on it. This makes direct Io behave correctly w.r.t. tail block zeroing beyond EOF, and fsx is happy about that.
Again, thanks to Al Viro for finding what I couldn't.
--- fs/xfs/xfs_aops.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
fs/xfs/xfs_aops.c +14 -0
@@ -1344,6 +1344,14 @@ __xfs_get_blocks(/* * If this is O_DIRECT or the mpage code calling tell them how large * the mapping is, so that we can avoid repeated get_blocks calls. +* +* If the mapping spans EOF, then we have to break the mapping up as the +* mapping for blocks beyond EOF must be marked new so that sub block +* regions can be correctly zeroed. We can't do this for mappings within +* EOF unless the mapping was just allocated or is unwritten, otherwise +* the callers would overwrite existing data with zeros. Hence we have +* to split the mapping into a range up to and including EOF, and a +* second mapping for beyond EOF. */ if (direct || size > (1 << inode->i_blkbits)) {xfs_off_t mapping_size;
@@ -1354,6 +1362,12 @@ __xfs_get_blocks(ASSERT (mapping_size >0 );if (mapping_size > size) mapping_size = size; +if (offset <i_size_read (inode) && + offset + mapping_size >=i_size_read (inode)) { +/* limit mapping to block that spans EOF */ + mapping_size =roundup (i_size_read (inode) - offset, +1 << inode->i_blkbits); + }if (mapping_size >LONG_MAX ) mapping_size =LONG_MAX ;
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/6] xfs: don't map ranges that span EOF for direct IO
Looks good,
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 6/6 xfs: collapse range is delalloc challenged.
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
FSX has been detecting data corruption after to collapse range calls. The key observation is that the offset of the last extent in the file was not being shifted, and hence when the file size was adjusted it was truncating away data because the extents handled been correctly shifted.
Tracing indicated that before the collapse, the extent list looked like:
....
ino 0x5788 state idx 6 offset 26 block 195904 count 10 flag 0 ino 0x5788 state idx 7 offset 39 block 195917 count 35 flag 0 ino 0x5788 state idx 8 offset 86 block 195964 count 32 flag 0
and after the shift of 2 blocks:
ino 0x5788 state idx 6 offset 24 block 195904 count 10 flag 0 ino 0x5788 state idx 7 offset 37 block 195917 count 35 flag 0 ino 0x5788 state idx 8 offset 86 block 195964 count 32 flag 0
Note that the last extent did not change offset. After the changing of the file size:
ino 0x5788 state idx 6 offset 24 block 195904 count 10 flag 0 ino 0x5788 state idx 7 offset 37 block 195917 count 35 flag 0 ino 0x5788 state idx 8 offset 86 block 195964 count 30 flag 0
You can see that the last extent had it's length truncated, indicating that we've lost data.
The reason for this is that the xfs_bmap_shift_extents() loop uses XFS_IFORK_NEXTENTS() to determine how many extents are in the inode. This, unfortunately, doesn't take into account delayed allocation extents - it's a count of physically allocated extents - and hence when the file being collapsed has a delalloc extent like this one does prior to the range being collapsed:
....
ino 0x5788 state idx 4 offset 11 block 4503599627239429 count 1 flag 0
....
it gets the count wrong and terminates the shift loop early.
Fix it by using the in-memory extent array size that includes delayed allocation extents to determine the number of extents on the inode.
--- fs/xfs/xfs_bmap.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
fs/xfs/xfs_bmap.c +11 -6
@@ -5413,6 +5413,7 @@ xfs_bmap_shift_extents(int whichfork =XFS_DATA_FORK ;int logflags;xfs_filblks_t blockcount =0 ; +int total_extents; if (unlikely(XFS_TEST_ERROR( (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
@@ -5429,7 +5430,6 @@ xfs_bmap_shift_extents(ASSERT (current_ext !=NULL ); ifp =XFS_IFORK_PTR (ip, whichfork); -if (!(ifp->if_flags &XFS_IFEXTENTS )) {/* Read in all the extents */ error =xfs_iread_extents (tp, ip, whichfork);
@@ -5456,7 +5456,6 @@ xfs_bmap_shift_extents(/* We are going to change core inode */ logflags =XFS_ILOG_CORE ; -if (ifp->if_flags &XFS_IFBROOT ) { cur =xfs_bmbt_init_cursor (mp, tp, ip, whichfork); cur->bc_private.b.firstblock = *firstblock;
@@ -5467,8 +5466,14 @@ xfs_bmap_shift_extents( logflags |=XFS_ILOG_DEXT ; } -while (nexts++ < num_exts && - *current_ext <XFS_IFORK_NEXTENTS (ip, whichfork)) { +/* +* There may be delalloc extents in the data fork before the range we +* are collapsing out, so we cannot +* use the count of real extents here. Instead we have to calculate it +* from the incore fork. +*/ + total_extents = ifp->if_bytes /sizeof (xfs_bmbt_rec_t); +while (nexts++ < num_exts && *current_ext < total_extents) { gotp =xfs_iext_get_ext (ifp, *current_ext);xfs_bmbt_get_all (gotp, &got);
@@ -5556,10 +5561,11 @@ xfs_bmap_shift_extents( } (*current_ext)++; + total_extents = ifp->if_bytes /sizeof (xfs_bmbt_rec_t); }/* Check if we are done */ -if (*current_ext ==XFS_IFORK_NEXTENTS (ip, whichfork)) +if (*current_ext == total_extents) *done =1 ; del_cursor:
@@ -5568,6 +5574,5 @@ del_cursor: error ? XFS_BTREE_ERROR : XFS_BTREE_NOERROR);xfs_trans_log_inode (tp, ip, logflags); -return error; }
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/6] xfs: collapse range is delalloc challenged.
fs/xfs/xfs_bmap.c xfs_bmap_shift_extents( · line 5475
Quoted 8 lines
fs/xfs/xfs_bmap.c +7
-1- *current_ext < XFS_IFORK_NEXTENTS(ip, whichfork)) { +/* +* There may be delalloc extents in the data fork before the range we +* are collapsing out, so we cannot +* use the count of real extents here. Instead we have to calculate it +* from the incore fork. +*/ + total_extents = ifp->if_bytes /sizeof (xfs_bmbt_rec_t);
Between the amount of times we have this calculation opencoded, and the confusing nature of XFS_IFORK_NEXTENTS it might be time to introduce a macro to get the proper number of incore extents.
But no need to do this now, so:
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 18 lines · Dave Chinner
Hi folks,
This is version 2 of the DIO vs delalloc patchset I posted here:
http://oss.sgi.com/archives/xfs/2014-03/msg00313.html
The changes to this version are:
- the bug fix to patch 2 that Brain noticed,
- I dropped the delalloc extent splittting patch because with the fix to patch 2 I can't trigger that bug anymore, and that patch was causing transaction overruns in xfs/297. Hence without an existing reproducer, I won't try to fix that problem.
- the last patch is new, and is a bug in the collapse range code where it fails to shift the last N extents correctly if there are N delalloc extents before the shifted range.
Quoted 9 lines
With these 6 patches, all of the xfstests fsx/fsstress tests pass on 1k, 2k and 4k block size filesystems, with and without CRCs enabled, on 1, 2 and 16p test VMs.
I'm much happier with these patches now - I don't think that there are more problems lurking, but only time will tell. I'd like to get these fixes to Linus for 3.15 (probably for -rc2), so eyeballs and testing would be appreciated.
The series looks pretty good to me now with the latest fix. I was previously tripping all over the delalloc asserts. With this set (and also running with finobt enabled), my tests ran clean on a 4k fs.
I've seen generic/270 still hit an assert once or twice on a 1k block fs (as noted on irc), but that's proven rather difficult to reproduce. I'll continue beating on it a bit, but otherwise:
Brian
Quoted 8 lines
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs