On Sun, Aug 31, 2014 at 04:00:40PM -0400, Theodore Ts'o wrote:
> Commit b8a8684502a0f introduced an accidental flag aliasing between
> EXT4_EX_NOCACHE and EXT4_GET_BLOCKS_CONVERT_UNWRITTEN.
>
> Fortunately, this didn't introduce any untorward side effects --- we
> got lucky. Nevertheless, fix this and leave a warning to hopefully
> avoid this from happening in the future.
>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
I spoke too soon. It turns out this flag aliasing was _hiding_ a bug.
The following patch seems to address the problem. We probably need to
do further cleanup of the extent handling code so it's clearer what's
going on, so for now, I'm going to stick with a more conservative fix.
- Ted
commit 3d714c69e1beba1e07e64e9adc72d446b249be18
Author: Theodore Ts'o <tytso@xxxxxxx>
Date: Mon Sep 1 14:32:09 2014 -0400
ext4: fix ZERO_RANGE bug hidden by flag aliasing
We accidently aliased EXT4_EX_NOCACHE and EXT4_GET_CONVERT_UNWRITTEN
falgs, which apparently was hiding a bug that was unmasked when this
flag aliasing issue was addressed (see the subsequent commit). The
reproduction case was:
fsx -N 10000 -l 500000 -r 4096 -t 4096 -w 4096 -Z -R -W /vdb/junk
... which would cause fsx to report corruption in the data file.
The fix we have is a bit of an overkill, but I'd much rather be
conservative for now, and we can optimize ZERO_RANGE_FL handling
later. The fact that we need to zap the extent_status cache for the
inode is unfortunate, but correctness is far more important than
performance.
Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
Cc: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index bc3b49f..4571b5d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4808,7 +4808,8 @@ static long ext4_zero_range(struct file *file, loff_t offset,
max_blocks -= lblk;
flags = EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT |
- EXT4_GET_BLOCKS_CONVERT_UNWRITTEN;
+ EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |
+ EXT4_EX_NOCACHE;
if (mode & FALLOC_FL_KEEP_SIZE)
flags |= EXT4_GET_BLOCKS_KEEP_SIZE;
@@ -4846,15 +4847,21 @@ static long ext4_zero_range(struct file *file, loff_t offset,
ext4_inode_block_unlocked_dio(inode);
inode_dio_wait(inode);
+ ret = ext4_alloc_file_blocks(file, lblk, max_blocks, new_size,
+ flags, mode);
+ if (ret)
+ goto out_dio;
/*
* Remove entire range from the extent status tree.
+ *
+ * ext4_es_remove_extent(inode, lblk, max_blocks) is
+ * NOT sufficient. I'm not sure why this is the case,
+ * but let's be conservative and remove the extent
+ * status tree for the entire inode. There should be
+ * no outstanding delalloc extents thanks to the
+ * filemap_write_and_wait_range() call above.
*/
- ret = ext4_es_remove_extent(inode, lblk, max_blocks);
- if (ret)
- goto out_dio;
-
- ret = ext4_alloc_file_blocks(file, lblk, max_blocks, new_size,
- flags, mode);
+ ret = ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
if (ret)
goto out_dio;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
