* [PATCH] ext4: fix ZERO_RANGE bug hidden by flag aliasing
2014-08-31 20:00 [PATCH] ext4: fix accidental flag aliasing in ext4_map_blocks flags Theodore Ts'o
@ 2014-09-01 18:33 ` Theodore Ts'o
2014-09-02 19:01 ` [PATCH] ext4: fix accidental flag aliasing in ext4_map_blocks flags Andreas Dilger
1 sibling, 0 replies; 4+ messages in thread
From: Theodore Ts'o @ 2014-09-01 18:33 UTC (permalink / raw)
To: Ext4 Developers List; +Cc: Namjae Jeon
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@mit.edu>
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@mit.edu>
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@mit.edu>
Cc: Namjae Jeon <namjae.jeon@samsung.com>
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;
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] ext4: fix accidental flag aliasing in ext4_map_blocks flags
2014-08-31 20:00 [PATCH] ext4: fix accidental flag aliasing in ext4_map_blocks flags Theodore Ts'o
2014-09-01 18:33 ` [PATCH] ext4: fix ZERO_RANGE bug hidden by flag aliasing Theodore Ts'o
@ 2014-09-02 19:01 ` Andreas Dilger
2014-09-03 1:44 ` [PATCH] ext4: renumber EXT4_EX_* flags to avoid flag aliasing problems flags Theodore Ts'o
1 sibling, 1 reply; 4+ messages in thread
From: Andreas Dilger @ 2014-09-02 19:01 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Ext4 Developers List
[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]
On Aug 31, 2014, at 2:00 PM, Theodore Ts'o <tytso@mit.edu> 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.
Why not move EXT4_EX_*CACHE flags to the end of the range (e.g. 0x8000
and 0x4000) to minimize potential conflicts in the future?
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> fs/ext4/ext4.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index cf3ad75..550b4f9 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -569,6 +569,7 @@ enum {
> #define EXT4_GET_BLOCKS_NO_PUT_HOLE 0x0200
> /* Convert written extents to unwritten */
> #define EXT4_GET_BLOCKS_CONVERT_UNWRITTEN 0x0400
> +/* DO NOT ASSIGN ADDITIONAL FLAG VALUES WITHOUT ADJUSTING THE FLAGS BELOW
It might be useful here to mention the EXT4_EX_* flags explicitly?
In my experience, time and incremental changes will make it less
clear what "FLAGS BELOW" means...
Cheers, Andreas
> /*
> * The bit position of these flags must not overlap with any of the
> @@ -579,8 +580,8 @@ enum {
> * caching the extents when reading from the extent tree while a
> * truncate or punch hole operation is in progress.
> */
> -#define EXT4_EX_NOCACHE 0x0400
> -#define EXT4_EX_FORCE_CACHE 0x0800
> +#define EXT4_EX_NOCACHE 0x0800
> +#define EXT4_EX_FORCE_CACHE 0x1000
>
> /*
> * Flags used by ext4_free_blocks
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread