Review: Fix sub-page zeroing for buffered writes into unwritten extents
Simple test case:
prealloc large file
write 3000 bytes to the middle of the file
read back file
The data in the block where the 3000 bytes was written has non-zero garbage around it both in memory and on disk.
The problem is a buffer mapping problem. When we copy data into an unwritten buffer, we have the create flag set which means we map the buffer. We then mark the buffer as unwritten, and do some more checks. Because the buffer is mapped, we do not set the buffer_new() flag on the buffer, which means when we return to the generic code, it does not do sub-block zeroing of the unwritten areas of the block.
The following patch fixes the problem. Comments?
Cheers,
Dave.
Signature · 35 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/linux-2.6/xfs_aops.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_aops.c 2007-01-23 18:40:45.255241599 +1100
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c 2007-01-23 18:49:13.345681246 +1100
@@ -1282,13 +1282,18 @@ __xfs_get_blocks(
bh_result->b_bdev = iomap.iomap_target->bt_bdev;
/*
- * If we previously allocated a block out beyond eof and we are
- * now coming back to use it then we will need to flag it as new
- * even if it has a disk address.
+ * If we previously allocated a block out beyond eof and we are now
+ * coming back to use it then we will need to flag it as new even if it
+ * has a disk address.
+ *
+ * With sub-block writes into unwritten extents we also need to mark
+ * the buffer as new so that the unwritten parts of the buffer gets
+ * correctly zeroed.
*/
if (create &&
((!buffer_mapped(bh_result) && !buffer_uptodate(bh_result)) ||
- (offset >= i_size_read(inode)) || (iomap.iomap_flags & IOMAP_NEW)))
+ (offset >= i_size_read(inode)) ||
+ (iomap.iomap_flags & (IOMAP_NEW|IOMAP_UNWRITTEN))))
set_buffer_new(bh_result);
if (iomap.iomap_flags & IOMAP_DELAY) {Dave,
I'm trying to understand what the sequence of events is here.
If we write to an unwritten extent then will __xfs_get_blocks() be called with create=1 and flags=BMAPI_WRITE? And calling bhv_vop_bmap() with flags set to BMAPI_WRITE will cause xfs_iomap() to set iomap_flags to IOMAP_NEW? The combination of create=1 and iomap_flags=IOMAP_NEW in __xfs_get_blocks() should result in calling set_buffer_new(), right?
I must be missing something...
Lachlan
Quoted 22 lines · David Chinner
Simple test case:
prealloc large file
write 3000 bytes to the middle of the file
read back fileThe data in the block where the 3000 bytes was written has non-zero garbage around it both in memory and on disk.
The problem is a buffer mapping problem. When we copy data into an unwritten buffer, we have the create flag set which means we map the buffer. We then mark the buffer as unwritten, and do some more checks. Because the buffer is mapped, we do not set the
buffer_new()flag on the buffer, which means when we return to the generic code, it does not do sub-block zeroing of the unwritten areas of the block.The following patch fixes the problem. Comments?
Cheers,
Dave.
Quoted 6 lines · Lachlan McIlroy
Dave,
I'm trying to understand what the sequence of events is here.
If we write to an unwritten extent then will
__xfs_get_blocks()be called with create=1 and flags=BMAPI_WRITE?
Yup.
Quoted 3 lines
And calling
bhv_vop_bmap()with flags set toBMAPI_WRITEwill causexfs_iomap()to setiomap_flagstoIOMAP_NEW?
Only if we allocate an extent in xfs_iomap:
In xfs_iomap:
258 phase2:
259 switch (flags & (BMAPI_WRITE|BMAPI_ALLOCATE|BMAPI_UNWRITTEN)) {
260 case BMAPI_WRITE:
261 /* If we found an extent, return it */
262 if (nimaps &&
263 (imap.br_startblock != HOLESTARTBLOCK) &&
264 (imap.br_startblock != DELAYSTARTBLOCK)) {
265 xfs_iomap_map_trace(XFS_IOMAP_WRITE_MAP, io,
266 offset, count, iomapp, &imap, flags);
267 break;
268 }We found an extent - an unwritten extent - which means we have a map and the startblock is a real number (i.e. not a hole or delalloc region). Hence we break here and never set the IOMAP_NEW flag which is correct because we didn't just do an allocation.
Quoted 3 lines
The combination of create=1 and
iomap_flags=IOMAP_NEW in__xfs_get_blocks()should result in callingset_buffer_new(), right?
Yes, it would, but unwritten extents are not new extents.....
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 35 lines · David Chinner
Quoted 6 lines · Lachlan McIlroy
Dave,
I'm trying to understand what the sequence of events is here.
If we write to an unwritten extent then will
__xfs_get_blocks()be called with create=1 and flags=BMAPI_WRITE?Yup.
Quoted 3 lines
And calling
bhv_vop_bmap()with flags set toBMAPI_WRITEwill causexfs_iomap()to setiomap_flagstoIOMAP_NEW?Only if we allocate an extent in
xfs_iomap:In
xfs_iomap:258 phase2: 259 switch (flags & (BMAPI_WRITE|BMAPI_ALLOCATE|BMAPI_UNWRITTEN)) { 260 case BMAPI_WRITE: 261 /* If we found an extent, return it */ 262 if (nimaps && 263 (imap.br_startblock != HOLESTARTBLOCK) && 264 (imap.br_startblock != DELAYSTARTBLOCK)) { 265 xfs_iomap_map_trace(XFS_IOMAP_WRITE_MAP, io, 266 offset, count, iomapp, &imap, flags); 267 break; 268 }
Quoted 4 lines
We found an extent - an unwritten extent - which means we have a map and the startblock is a real number (i.e. not a hole or delalloc region). Hence we break here and never set the
IOMAP_NEWflag which is correct because we didn't just do an allocation.
I must have skimmed over the break statement. Your fix makes sense to me now.
Quoted 12 lines
Quoted 5 lines
The combination of create=1 and
iomap_flags=IOMAP_NEW in__xfs_get_blocks()should result in callingset_buffer_new(), right?Yes, it would, but unwritten extents are not new extents.....
Cheers,
Dave.