RE: [PATCH 5/5] ext4: Fix extent merging in ext4_ext_shift_path_extents()

RE: [PATCH 5/5] ext4: Fix extent merging in ext4_ext_shift_path_extents()

[Namjae Jeon]

> There is a bug in ext4_ext_shift_path_extents() where if we actually
> manage to merge a extent we would skip shifting the next extent. This
> will result in in one extent in the extent tree not being properly
> shifted.
> 
> This is causing failure in various xfstests tests using fsx or fsstress
> with collapse range support. It will also cause file system corruption
> which looks something like:
> 
>  e2fsck 1.42.9 (4-Feb-2014)
>  Pass 1: Checking inodes, blocks, and sizes
>  Inode 20 has out of order extents
>         (invalid logical block 3, physical block 492938, len 2)
>  Clear? yes
>  ...
> 
> when running e2fsck.
> 
> It's also very easily reproducible just by running fsx without any
> parameters. I can usually hit the problem within a minute.
> 
> Fix it by increasing ex_start only if we're not merging the extent.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
You can add : Reviewed-by: Namjae Jeon <namjae.jeon@samsung.com>

Thanks!

Re: [PATCH 5/5] ext4: Fix extent merging in ext4_ext_shift_path_extents()

[Theodore Ts'o]

Thanks, applied.

					- Ted

[PATCH 1/5] ext4: Use filemap_write_and_wait_range() correctly in collapse range

[Lukas Czerner]

Currently we're passing -1 as lend argumnet for
filemap_write_and_wait_range() which is wrong since lend is signed type
so it would cause some confusion and we might not write_and_wait for the
entire range we're expecting to write.

Fix it by using LLONG_MAX instead.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ff823b7..821c1d4 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5378,7 +5378,7 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	punch_stop = (offset + len) >> EXT4_BLOCK_SIZE_BITS(sb);
 
 	/* Write out all dirty pages */
-	ret = filemap_write_and_wait_range(inode->i_mapping, offset, -1);
+	ret = filemap_write_and_wait_range(inode->i_mapping, offset, LLONG_MAX);
 	if (ret)
 		return ret;
 
-- 
1.8.3.1

[PATCH 5/5] ext4: Fix extent merging in ext4_ext_shift_path_extents()

[Lukas Czerner]

There is a bug in ext4_ext_shift_path_extents() where if we actually
manage to merge a extent we would skip shifting the next extent. This
will result in in one extent in the extent tree not being properly
shifted.

This is causing failure in various xfstests tests using fsx or fsstress
with collapse range support. It will also cause file system corruption
which looks something like:

 e2fsck 1.42.9 (4-Feb-2014)
 Pass 1: Checking inodes, blocks, and sizes
 Inode 20 has out of order extents
        (invalid logical block 3, physical block 492938, len 2)
 Clear? yes
 ...

when running e2fsck.

It's also very easily reproducible just by running fsx without any
parameters. I can usually hit the problem within a minute.

Fix it by increasing ex_start only if we're not merging the extent.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 84bb668..5fa31cb 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5230,13 +5230,14 @@ ext4_ext_shift_path_extents(struct ext4_ext_path *path, ext4_lblk_t shift,
 
 			while (ex_start <= ex_last) {
 				ex_start->ee_block -= shift;
-				if (ex_start >
-					EXT_FIRST_EXTENT(path[depth].p_hdr)) {
-					if (ext4_ext_try_to_merge_right(inode,
-						path, ex_start - 1))
-						ex_last--;
-				}
-				ex_start++;
+				/* Try to merge to the left. */
+				if ((ex_start >
+				     EXT_FIRST_EXTENT(path[depth].p_hdr)) &&
+				    ext4_ext_try_to_merge_right(inode,
+							path, ex_start - 1))
+					ex_last--;
+				else
+					ex_start++;
 			}
 			err = ext4_ext_dirty(handle, inode, path + depth);
 			if (err)
-- 
1.8.3.1