Review: Fix sub-page zeroing for buffered writes into unwritten extents

Review: Fix sub-page zeroing for buffered writes into unwritten extents

[David Chinner]

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

Re: Review: Fix sub-page zeroing for buffered writes into unwritten extents

[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?  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

David Chinner wrote:
> 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.

Re: Review: Fix sub-page zeroing for buffered writes into unwritten extents

[David Chinner]

On Wed, Jan 24, 2007 at 04:44:04PM +0000, Lachlan McIlroy wrote:
> 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.

> And calling
> bhv_vop_bmap() with flags set to BMAPI_WRITE will cause xfs_iomap()
> to set iomap_flags to IOMAP_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.

> The combination of create=1 and
> iomap_flags=IOMAP_NEW in __xfs_get_blocks() should result in calling
> set_buffer_new(), right?

Yes, it would, but unwritten extents are not new extents.....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: Fix sub-page zeroing for buffered writes into unwritten extents

[Lachlan McIlroy]

David Chinner wrote:
> On Wed, Jan 24, 2007 at 04:44:04PM +0000, Lachlan McIlroy wrote:
> 
>>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.
> 
> 
>>And calling
>>bhv_vop_bmap() with flags set to BMAPI_WRITE will cause xfs_iomap()
>>to set iomap_flags to IOMAP_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.

I must have skimmed over the break statement.  Your fix makes sense to me now.

> 
> 
>>The combination of create=1 and
>>iomap_flags=IOMAP_NEW in __xfs_get_blocks() should result in calling
>>set_buffer_new(), right?
> 
> 
> Yes, it would, but unwritten extents are not new extents.....
> 
> Cheers,
> 
> Dave.