ext4_ext_convert_to_initialized bug found in extended FSX testing
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the ext4_ext_convert_to_initialized and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
---
:100644 100644 e363f21... ce69450... M fs/ext4/extents.c
fs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
fs/ext4/extents.c +2 -1
@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,/* case 3 */ zero_ex.ee_block =cpu_to_le32 (map->m_lblk + map->m_len); - zero_ex.ee_len =cpu_to_le16 (allocated - map->m_len); + zero_ex.ee_len =cpu_to_le16 (ee_len - + allocated - map->m_len);ext4_ext_store_pblock (&zero_ex,ext4_ext_pblock (ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout (inode, &zero_ex);
Signature · 1 line
1.7.1
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initialized
What do you mean zeros here?
Some useful data is zeroed?
and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 13 lines
Signed-off-by Allison Henderson <[email protected]>---
:100644 100644 e363f21... ce69450... M fs/ext4/extents.cfs/ext4/extents.c| 3 ++-1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle, /* case 3 */
Quoted 5 lines
zero_ex.ee_block =
cpu_to_le32(map->m_lblk + map->m_len);
- zero_ex.ee_len =cpu_to_le16(allocated - map->m_len);
+ zero_ex.ee_len = cpu_to_le16(ee_len -
+ allocated -map->m_len);
The logic is that we splits [ee_block, ee_block + ee_len) into [ee_block, map->m_blk) that is uninitialized and [map->m_blk, ee_block
+ ee_len) that is initialized. We need to zero [map->m_lblk +map->m_len, ee_block + ee_len).
and [map->m_lblk, map->m_lblk + map->m_len) is zeroed by upper layer because of MAP_NEW flag.
Right logic?
I can not see the error and the meaning of ee_len - allocated - map->m_len.
Thanks,
Yongqiang.
Quoted 7 lines
ext4_ext_store_pblock(&zero_ex,
ext4_ext_pblock(ex)+map->m_lblk-ee_block); err =ext4_ext_zeroout(inode, &zero_ex); --
1.7.1
Signature · 6 lines
Best Wishes Yongqiang Yang -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · Yongqiang Yang
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initializedWhat do you mean zeros here?
Some useful data is zeroed?
Resending this note because I didn't see it show up in the mailing list.
Maybe a duplicate:
Oh sorry, I phrased that wrong. The test was failing because it did not contain zeros when it should have. I traced it down to a map write operation that was trying to write data in an extent that had been created unwritten by a previous fallocate. The unwritten extent had to be split, but the last part of the extent was not zero'd out all the way, so it was causing some erroneous data to show up after the region that was being written. This fix appears to correct it though.
Allison Henderson
Quoted 48 lines
and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 12 lines
Signed-off-by Allison Henderson<[email protected]>---
:100644 100644 e363f21... ce69450... Mfs/ext4/extents.cfs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_ext_convert_to_initialized(handle_t *handle, · line 2823
Quoted 6 lines
fs/ext4/extents.c +2
-1@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,/* case 3 */ zero_ex.ee_block =cpu_to_le32 (map->m_lblk + map->m_len); - zero_ex.ee_len =cpu_to_le16 (allocated - map->m_len); + zero_ex.ee_len =cpu_to_le16 (ee_len - + allocated - map->m_len);The logic is that we splits [
ee_block,ee_block+ee_len) into [ee_block,map->m_blk) that is uninitialized and [map->m_blk,ee_block
+ee_len) that is initialized. We need to zero [map->m_lblk+map->m_len,ee_block+ee_len).
and [map->m_lblk,map->m_lblk+map->m_len) is zeroed by upper layer because ofMAP_NEWflag.Right logic?
I can not see the error and the meaning of
ee_len- allocated -map->m_len.Thanks,
Yongqiang.Quoted 7 lines
fs/ext4/extents.c +0
-1ext4_ext_store_pblock (&zero_ex,ext4_ext_pblock (ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout (inode,&zero_ex); --1.7.1
Quoted 8 lines · Yongqiang Yang
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initializedWhat do you mean zeros here?
Some useful data is zeroed?
Oh sorry, I phrased that wrong. The test was failing because it did not contain zeros when it should have. I traced it down to a map write operation that was trying to write data in an extent that had been created unwritten by a previous fallocate. The unwritten extent had to be split, but the last part of the extent was not zero'd out all the way, so it was causing some erroneous data to show up after the region that was being written. This fix appears to correct it though.
Allison Henderson
Quoted 49 lines
and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 12 lines
Signed-off-by Allison Henderson<[email protected]>---
:100644 100644 e363f21... ce69450... Mfs/ext4/extents.cfs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_ext_convert_to_initialized(handle_t *handle, · line 2823
Quoted 6 lines
fs/ext4/extents.c +2
-1@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,/* case 3 */ zero_ex.ee_block =cpu_to_le32 (map->m_lblk + map->m_len); - zero_ex.ee_len =cpu_to_le16 (allocated - map->m_len); + zero_ex.ee_len =cpu_to_le16 (ee_len - + allocated - map->m_len);The logic is that we splits [
ee_block,ee_block+ee_len) into [ee_block,map->m_blk) that is uninitialized and [map->m_blk,ee_block
+ee_len) that is initialized. We need to zero [map->m_lblk+map->m_len,ee_block+ee_len).
and [map->m_lblk,map->m_lblk+map->m_len) is zeroed by upper layer because ofMAP_NEWflag.Right logic?
I can not see the error and the meaning of
ee_len- allocated -map->m_len.Thanks,
Yongqiang.Quoted 7 lines
fs/ext4/extents.c +0
-1ext4_ext_store_pblock (&zero_ex,ext4_ext_pblock (ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout (inode,&zero_ex); --1.7.1
Quoted 38 lines · Yongqiang Yang
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initializedWhat do you mean zeros here?
Some useful data is zeroed?and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 12 lines
Signed-off-by Allison Henderson <[email protected]>---
:100644 100644 e363f21... ce69450... Mfs/ext4/extents.cfs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_ext_convert_to_initialized(handle_t *handle, · line 2823
Quoted 6 lines
fs/ext4/extents.c +2
-1@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,/* case 3 */ zero_ex.ee_block =cpu_to_le32 (map->m_lblk + map->m_len); - zero_ex.ee_len =cpu_to_le16 (allocated - map->m_len); + zero_ex.ee_len =cpu_to_le16 (ee_len - + allocated - map->m_len);The logic is that we splits [
ee_block,ee_block+ee_len) into [ee_block,map->m_blk) that is uninitialized and [map->m_blk,ee_block
+ee_len) that is initialized. We need to zero [map->m_lblk+map->m_len,ee_block+ee_len).
and [map->m_lblk,map->m_lblk+map->m_len) is zeroed by upper layer because ofMAP_NEWflag.
Quoted 2 lines
Right logic?
Hmm, the logic in case 3 is-- if ex2[map->m_blk, map->m_blk+m_len] and ex3 together[map->mblk+m_len+1, map->m_blk+allocated] total length (allocated)is < than 7 blocks, then we zero out the entire ex2 and ext3, there is no need to do split.
I think zero_ex.ee_len should be "allocated". Look at the original code (before the extents splits cleanup patches), it will zero out entire [map->mblk, map->m_blk+allocated] and don't do split anymore.
something like this, not a patch, but show what I think the right fix.
if (allocated > map->m_len) {
if (allocated <= EXT4_EXT_ZERO_LEN &&
(EXT4_EXT_MAY_ZEROOUT & split_flag)) {
/* case 3 */
zero_ex.ee_block =
cpu_to_le32(map->m_lblk + map->m_len);
- zero_ex.ee_len = cpu_to_le16(allocated - map->m_len);
zero_ex.ee_len = cpu_to_le16(allocated);
ext4_ext_store_pblock(&zero_ex,
ext4_ext_pblock(ex) + map->m_lblk - ee_block);
err = ext4_ext_zeroout(inode, &zero_ex);
if (err)
goto out;- split_map.m_lblk = map->m_lblk;
- split_map.m_len = allocated;
+ ext4_ext_mark_initialized(ex);
+ ext4_ext_try_to_merge(inode, path, ex);
+ err = ext4_ext_dirty(handle, inode, path + depth);
+ goto out;
}
Mingming
Quoted 17 lines
I can not see the error and the meaning of
ee_len- allocated -map->m_len.Thanks,
Yongqiang.Quoted 7 lines
fs/ext4/extents.c +0
-1ext4_ext_store_pblock (&zero_ex,ext4_ext_pblock (ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout (inode, &zero_ex); --1.7.1
Quoted 78 lines · Mingming Cao
Quoted 38 lines · Yongqiang Yang
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initializedWhat do you mean zeros here?
Some useful data is zeroed?and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 12 lines
Signed-off-by Allison Henderson<[email protected]>---
:100644 100644 e363f21... ce69450... Mfs/ext4/extents.cfs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_ext_convert_to_initialized(handle_t *handle, · line 2823
Quoted 6 lines
fs/ext4/extents.c +2
-1@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,/* case 3 */ zero_ex.ee_block =cpu_to_le32 (map->m_lblk + map->m_len); - zero_ex.ee_len =cpu_to_le16 (allocated - map->m_len); + zero_ex.ee_len =cpu_to_le16 (ee_len - + allocated - map->m_len);The logic is that we splits [
ee_block,ee_block+ee_len) into [ee_block,map->m_blk) that is uninitialized and [map->m_blk,ee_block
+ee_len) that is initialized. We need to zero [map->m_lblk+map->m_len,ee_block+ee_len).
and [map->m_lblk,map->m_lblk+map->m_len) is zeroed by upper layer because ofMAP_NEWflag.Quoted 2 lines
Right logic?
Hmm, the logic in case 3 is-- if ex2[map->m_blk, map->m_blk+m_len] and ex3 together[map->mblk+m_len+1, map->m_blk+allocated] total length (allocated)is< than 7 blocks, then we zero out the entire ex2 and ext3, there is no need to do split.
I think zero_ex.ee_len should be "allocated". Look at the original code (before the extents splits cleanup patches), it will zero out entire [
map->mblk, map->m_blk+allocated] and don't do split anymore.something like this, not a patch, but show what I think the right fix.
if (allocated> map->m_len) { if (allocated<= EXT4_EXT_ZERO_LEN&& (EXT4_EXT_MAY_ZEROOUT& split_flag)) { /* case 3 */ zero_ex.ee_block = cpu_to_le32(map->m_lblk + map->m_len);(quoted diff) +4
-3- zero_ex.ee_len = cpu_to_le16(allocated - map->m_len); zero_ex.ee_len = cpu_to_le16(allocated); ext4_ext_store_pblock(&zero_ex, ext4_ext_pblock(ex) + map->m_lblk - ee_block); err = ext4_ext_zeroout(inode,&zero_ex); if (err) goto out; - split_map.m_lblk = map->m_lblk; - split_map.m_len = allocated; + ext4_ext_mark_initialized(ex); + ext4_ext_try_to_merge(inode, path, ex); + err = ext4_ext_dirty(handle, inode, path + depth); + goto out; }
Quoted 5 lines
Mingming
Quoted 2 lines
I can not see the error and the meaning of
ee_len- allocated -map->m_len.
Hi Yongqiang,
Sorry I didnt see your extra question down here. Initially I had read "allocated" to be the length of ex1, but now I see that it is the length of ex2+ex3. So ee_len - allocated - map->m_len was supposed to be ex3, but I think Mingming has the right idea now with zeroing out all of "allocated".
Allison Henderson
Quoted 17 lines
Quoted 15 lines
Thanks,
Yongqiang.Quoted 7 lines
fs/ext4/extents.c +0
-1ext4_ext_store_pblock (&zero_ex,ext4_ext_pblock (ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout (inode,&zero_ex); --1.7.1
Quoted 66 lines · Mingming Cao
Quoted 38 lines · Yongqiang Yang
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initializedWhat do you mean zeros here?
Some useful data is zeroed?and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 12 lines
Signed-off-by Allison Henderson <[email protected]>---
:100644 100644 e363f21... ce69450... Mfs/ext4/extents.cfs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0fs/ext4/extents.c static int ext4_ext_convert_to_initialized(handle_t *handle, · line 2823
Quoted 6 lines
fs/ext4/extents.c +2
-1@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,/* case 3 */ zero_ex.ee_block =cpu_to_le32 (map->m_lblk + map->m_len); - zero_ex.ee_len =cpu_to_le16 (allocated - map->m_len); + zero_ex.ee_len =cpu_to_le16 (ee_len - + allocated - map->m_len);The logic is that we splits [
ee_block,ee_block+ee_len) into [ee_block,map->m_blk) that is uninitialized and [map->m_blk,ee_block
+ee_len) that is initialized. We need to zero [map->m_lblk+map->m_len,ee_block+ee_len).
and [map->m_lblk,map->m_lblk+map->m_len) is zeroed by upper layer because ofMAP_NEWflag.Quoted 2 lines
Right logic?
Hmm, the logic in case 3 is-- if ex2[map->m_blk, map->m_blk+m_len] and ex3 together[map->mblk+m_len+1, map->m_blk+allocated] total length (allocated)is < than 7 blocks, then we zero out the entire ex2 and ext3, there is no need to do split.
I think zero_ex.ee_len should be "allocated". Look at the original code (before the extents splits cleanup patches), it will zero out entire [
map->mblk, map->m_blk+allocated] and don't do split anymore.something like this, not a patch, but show what I think the right fix.
if (allocated > map->m_len) { if (allocated <= EXT4_EXT_ZERO_LEN && (EXT4_EXT_MAY_ZEROOUT & split_flag)) { /* case 3 */ zero_ex.ee_block = cpu_to_le32(map->m_lblk + map->m_len);(quoted diff) +0
-1- zero_ex.ee_len = cpu_to_le16(allocated - map->m_len); zero_ex.ee_len = cpu_to_le16(allocated); ext4_ext_store_pblock(&zero_ex, ext4_ext_pblock(ex) + map->m_lblk - ee_block);
Review
Quoted 10 lines
(quoted diff) +4
-2err = ext4_ext_zeroout(inode, &zero_ex); if (err) goto out; - split_map.m_lblk = map->m_lblk; - split_map.m_len = allocated; + ext4_ext_mark_initialized(ex); + ext4_ext_try_to_merge(inode, path, ex); + err = ext4_ext_dirty(handle, inode, path + depth); + goto out; }
Hi there,
I realized that we still need to insert the zeroed out extents, do one split. But the zeroed out extent length is the whole "allocated" space.so above is not entirely right. I think there maybe more issue with the extent split cleanup code...
Basically the issue is the zero-out length is not calculated properly. and doesn't match the split length. in case 3 and 2, we both just zero out the small portion that less then the water mark, do one split. And the split extent length should match the zero out length. Which I think current cleanup patch don't.
In case 3, since the split/zerout happen at the start of requested logical offset, we could return the whole zeroed-out length; but in case 2, since the split/zeroout happen at the end of requested range, we should only return the length of mapped(zince being zerout)blocks that start from the requested logical offset. ext4_ext_map_blocks() will return the mapped blocks back to the caller.
Hmm,. it might be easier to comment in the [PATCH v2 3/3] ext4:Reimplement convert and split_unwritten. and the other patch directly.
Mingming
Quoted 28 lines
Mingming
Quoted 17 lines
I can not see the error and the meaning of
ee_len- allocated -map->m_len.Thanks,
Yongqiang.Quoted 7 lines
fs/ext4/extents.c +0
-1ext4_ext_store_pblock (&zero_ex,ext4_ext_pblock (ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout (inode, &zero_ex); --1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 39 lines · Mingming Cao
Quoted 38 lines · Yongqiang Yang
Quoted 3 lines · Allison Henderson
Hi All,
We've been trying to get punch hole through some extended fsx tests, and I ran across some other tests that were failing because the test file contained zeros where it shouldn't. I made this fix to the
ext4_ext_convert_to_initializedWhat do you mean zeros here?
Some useful data is zeroed?and the test has been running smooth for about an hour now. Yongqiang, this one looks like it may have been associated with the split extents clean up patch. Would you mind taking a look at this fix and giving it your ok if it looks good? Thx!
Quoted 13 lines
Signed-off-by Allison Henderson <[email protected]>---
:100644 100644 e363f21... ce69450... M fs/ext4/extents.cfs/ext4/extents.c| 3 ++-1 files changed, 2 insertions(+), 1 deletions(-)fs/ext4/extents.c +0
-0@@ -2819,7 +2819,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle, /* case 3 */Quoted 5 lines
zero_ex.ee_block =
cpu_to_le32(map->m_lblk + map->m_len);
- zero_ex.ee_len =cpu_to_le16(allocated - map->m_len);
+ zero_ex.ee_len = cpu_to_le16(ee_len -
+ allocated -map->m_len);The logic is that we splits [
ee_block,ee_block+ee_len) into [ee_block,map->m_blk) that is uninitialized and [map->m_blk,ee_block
+ee_len) that is initialized. We need to zero [map->m_lblk+map->m_len,ee_block+ee_len).
and [map->m_lblk,map->m_lblk+map->m_len) is zeroed by upper layer because ofMAP_NEWflag.
Quoted 3 lines
Quoted 2 lines
Right logic?
Hi Mingming,
Sorry for late response.
Quoted 4 lines
Hmm, the logic in case 3 is-- if ex2[map->m_blk, map->m_blk+m_len] and ex3 together[map->mblk+m_len+1, map->m_blk+allocated] total length (allocated)is < than 7 blocks, then we zero out the entire ex2 and ext3, there is no need to do split.
I only zero out ext3 because ext2 is the requested extent so it will be flushed with data that application writes. So zeroing ext3 is enough.
Quoted 11 lines
I think zero_ex.ee_len should be "allocated". Look at the original code (before the extents splits cleanup patches), it will zero out entire [
map->mblk, map->m_blk+allocated] and don't do split anymore.something like this, not a patch, but show what I think the right fix.
Quoted 16 lines
if (allocated >
map->m_len) {
if (allocated <=EXT4_EXT_ZERO_LEN&& (EXT4_EXT_MAY_ZEROOUT&split_flag)) {
/* case 3 */
zero_ex.ee_block =
cpu_to_le32(map->m_lblk +map->m_len);
- zero_ex.ee_len =cpu_to_le16(allocated - map->m_len); zero_ex.ee_len =cpu_to_le16(allocated);ext4_ext_store_pblock(&zero_ex, ext4_ext_pblock(ex) + map->m_lblk - ee_block); err =ext4_ext_zeroout(inode, &zero_ex);
if (err)
goto out;
- split_map.m_lblk =map->m_lblk;
- split_map.m_len = allocated;
+ext4_ext_mark_initialized(ex);
Nope. ex is initially uninitialized, it is split into two extents [ee_block, map->m_lblk) and [map->m_lblk, ee_block + ee_len). the 1st should be uninitialized while the 2nd one should be initialized and this is
done in ext4_split_extent().
Quoted 30 lines
+
ext4_ext_try_to_merge(inode, path, ex);
+ err =ext4_ext_dirty(handle, inode, path + depth);
+ goto out;
}Mingming
Quoted 17 lines
I can not see the error and the meaning of
ee_len- allocated -map->m_len.Thanks,
Yongqiang.Quoted 7 lines
ext4_ext_store_pblock(&zero_ex,
ext4_ext_pblock(ex)+map->m_lblk-ee_block); err =ext4_ext_zeroout(inode, &zero_ex); --
1.7.1
Signature · 6 lines
Best Wishes Yongqiang Yang -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html