[PATCH] ext4: don't read blocks from disk after extents being swapped in move_extent_per_page()

[PATCH] ext4: don't read blocks from disk after extents being swapped in move_extent_per_page()

[Eryu Guan]

I notice ext4/307 fails occasionally on ppc64 host, reporting md5
checksum mismatch after moving data from original file to donor file.

The reason is that move_extent_per_page() calls __block_write_begin()
and block_commit_write() to write saved data from original inode blocks
to donor inode blocks, but __block_write_begin() not only maps buffer
heads but also reads block content from disk if the size is not block
size aligned. At this time the physical block number in mapped buffer
head is pointing to the donor file not the original file, and that
results in reading wrong data to page, which get written to disk in
following block_commit_write call.

This also can be reproduced by the following script on 1k block size ext4
on x86_64 host:

    mnt=/mnt/ext4
    donorfile=$mnt/donor
    testfile=$mnt/testfile
    e4compact=~/xfstests/src/e4compact

    rm -f $donorfile $testfile

    # reserve space for donor file, written by 0xaa and sync to disk to
    # avoid EBUSY on EXT4_IOC_MOVE_EXT
    xfs_io -fc "pwrite -S 0xaa 0 1m" -c "fsync" $donorfile

    # create test file written by 0xbb
    xfs_io -fc "pwrite -S 0xbb 0 1023" -c "fsync" $testfile

    # compute initial md5sum
    md5sum $testfile | tee md5sum.txt
    # drop cache, force e4compact to read data from disk
    echo 3 > /proc/sys/vm/drop_caches

    # test defrag
    echo "$testfile" | $e4compact -i -v -f $donorfile
    # check md5sum
    md5sum -c md5sum.txt

Fix it by creating & mapping buffer heads only but not reading blocks
from disk, because all the data in page is guaranteed to be up-to-date
in mext_page_mkuptodate().

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---

I tested this patch with ext4/307 and my reproducer more than 100 times on
x86_64 and ppc64 hosts, also survived ext4 4k/2k/1k xfstests on both x86_64 and
ppc64 host.

 fs/ext4/move_extent.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index fb6f117..e032a04 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -265,11 +265,12 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
 	ext4_lblk_t orig_blk_offset, donor_blk_offset;
 	unsigned long blocksize = orig_inode->i_sb->s_blocksize;
 	unsigned int tmp_data_size, data_size, replaced_size;
-	int err2, jblocks, retries = 0;
+	int i, err2, jblocks, retries = 0;
 	int replaced_count = 0;
 	int from = data_offset_in_page << orig_inode->i_blkbits;
 	int blocks_per_page = PAGE_CACHE_SIZE >> orig_inode->i_blkbits;
 	struct super_block *sb = orig_inode->i_sb;
+	struct buffer_head *bh = NULL;
 
 	/*
 	 * It needs twice the amount of ordinary journal buffers because
@@ -380,8 +381,16 @@ data_copy:
 	}
 	/* Perform all necessary steps similar write_begin()/write_end()
 	 * but keeping in mind that i_size will not change */
-	*err = __block_write_begin(pagep[0], from, replaced_size,
-				   ext4_get_block);
+	if (!page_has_buffers(pagep[0]))
+		create_empty_buffers(pagep[0], 1 << orig_inode->i_blkbits, 0);
+	bh = page_buffers(pagep[0]);
+	for (i = 0; i < data_offset_in_page; i++)
+		bh = bh->b_this_page;
+	for (i = 0; i < block_len_in_page; i++) {
+		*err = ext4_get_block(orig_inode, orig_blk_offset + i, bh, 0);
+		if (*err < 0)
+			break;
+	}
 	if (!*err)
 		*err = block_commit_write(pagep[0], from, from + replaced_size);
 
-- 
2.5.0

[PATCH] ext4: remove unused parameter "newblock" in convert_initialized_extent()

[Eryu Guan]

The "newblock" parameter is not used in convert_initialized_extent(),
remove it.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---
 fs/ext4/extents.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0ffabaf..54be692 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3928,7 +3928,7 @@ static int
 convert_initialized_extent(handle_t *handle, struct inode *inode,
 			   struct ext4_map_blocks *map,
 			   struct ext4_ext_path **ppath, int flags,
-			   unsigned int allocated, ext4_fsblk_t newblock)
+			   unsigned int allocated)
 {
 	struct ext4_ext_path *path = *ppath;
 	struct ext4_extent *ex;
@@ -4347,7 +4347,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			    (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN)) {
 				allocated = convert_initialized_extent(
 						handle, inode, map, &path,
-						flags, allocated, newblock);
+						flags, allocated;
 				goto out2;
 			} else if (!ext4_ext_is_unwritten(ex))
 				goto out;
-- 
2.5.0

Re: [PATCH] ext4: remove unused parameter "newblock" in convert_initialized_extent()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Hi Eryu,

[auto build test ERROR on ext4/dev]
[also build test ERROR on v4.5-rc3 next-20160210]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Eryu-Guan/ext4-remove-unused-parameter-newblock-in-convert_initialized_extent/20160210-232337
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: x86_64-randconfig-x012-201606 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   fs/ext4/extents.c: In function 'ext4_ext_map_blocks':
>> fs/ext4/extents.c:4350:23: error: expected ')' before ';' token
          flags, allocated;
                          ^
>> fs/ext4/extents.c:4352:4: error: expected ';' before '}' token
       } else if (!ext4_ext_is_unwritten(ex))
       ^

vim +4350 fs/ext4/extents.c

  4344				 * caller wants to convert it to unwritten.
  4345				 */
  4346				if ((!ext4_ext_is_unwritten(ex)) &&
  4347				    (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN)) {
  4348					allocated = convert_initialized_extent(
  4349							handle, inode, map, &path,
> 4350							flags, allocated;
  4351					goto out2;
> 4352				} else if (!ext4_ext_is_unwritten(ex))
  4353					goto out;
  4354	
  4355				ret = ext4_ext_handle_unwritten_extents(

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 30591 bytes --]

[PATCH v2] ext4: remove unused parameter "newblock" in convert_initialized_extent()

[Eryu Guan]

The "newblock" parameter is not used in convert_initialized_extent(),
remove it.

Signed-off-by: Eryu Guan <guaneryu@gmail.com>
---

v2:
- Fix build error in v1, I tested the good patch and sent the wrong one...

 fs/ext4/extents.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0ffabaf..3753ceb 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3928,7 +3928,7 @@ static int
 convert_initialized_extent(handle_t *handle, struct inode *inode,
 			   struct ext4_map_blocks *map,
 			   struct ext4_ext_path **ppath, int flags,
-			   unsigned int allocated, ext4_fsblk_t newblock)
+			   unsigned int allocated)
 {
 	struct ext4_ext_path *path = *ppath;
 	struct ext4_extent *ex;
@@ -4347,7 +4347,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			    (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN)) {
 				allocated = convert_initialized_extent(
 						handle, inode, map, &path,
-						flags, allocated, newblock);
+						flags, allocated);
 				goto out2;
 			} else if (!ext4_ext_is_unwritten(ex))
 				goto out;
-- 
2.5.0

Re: [PATCH] ext4: don't read blocks from disk after extents being swapped in move_extent_per_page()

[Theodore Ts'o]

On Wed, Feb 10, 2016 at 11:16:45PM +0800, Eryu Guan wrote:
> I notice ext4/307 fails occasionally on ppc64 host, reporting md5
> checksum mismatch after moving data from original file to donor file.
> 
> The reason is that move_extent_per_page() calls __block_write_begin()
> and block_commit_write() to write saved data from original inode blocks
> to donor inode blocks, but __block_write_begin() not only maps buffer
> heads but also reads block content from disk if the size is not block
> size aligned. At this time the physical block number in mapped buffer
> head is pointing to the donor file not the original file, and that
> results in reading wrong data to page, which get written to disk in
> following block_commit_write call.

Thanks, applied.

					- Ted

Re: [PATCH v2] ext4: remove unused parameter "newblock" in convert_initialized_extent()

[Theodore Ts'o]

On Thu, Feb 11, 2016 at 12:09:04AM +0800, Eryu Guan wrote:
> The "newblock" parameter is not used in convert_initialized_extent(),
> remove it.
> 
> Signed-off-by: Eryu Guan <guaneryu@gmail.com>

Applied, thanks.

					- Ted