| Message ID | 4DC97C32.2020203@linux.vnet.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, May 11, 2011 at 1:56 AM, Allison Henderson <achender@linux.vnet.ibm.com> wrote: > 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! > > Signed-off-by: Allison Henderson <achender@us.ibm.com> > --- > :100644 100644 e363f21... ce69450... M fs/ext4/extents.c > fs/ext4/extents.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index e363f21..ce69450 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -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 of MAP_NEW flag. Right logic? I can not see the error and the meaning of ee_len - allocated - map->m_len. Thanks, Yongqiang. > 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 > >
On 5/10/2011 6:47 PM, Yongqiang Yang wrote: > On Wed, May 11, 2011 at 1:56 AM, Allison Henderson > <achender@linux.vnet.ibm.com> wrote: >> 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? 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 > > 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! >> >> Signed-off-by: Allison Henderson<achender@us.ibm.com> >> --- >> :100644 100644 e363f21... ce69450... M fs/ext4/extents.c >> fs/ext4/extents.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index e363f21..ce69450 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -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 of MAP_NEW flag. > > Right logic? > > > I can not see the error and the meaning of ee_len - allocated - map->m_len. > > Thanks, > Yongqiang. > > >> 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 >> >> > > > -- 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
On 5/10/2011 6:47 PM, Yongqiang Yang wrote: > On Wed, May 11, 2011 at 1:56 AM, Allison Henderson > <achender@linux.vnet.ibm.com> wrote: >> 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? > 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 > 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! >> >> Signed-off-by: Allison Henderson<achender@us.ibm.com> >> --- >> :100644 100644 e363f21... ce69450... M fs/ext4/extents.c >> fs/ext4/extents.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> index e363f21..ce69450 100644 >> --- a/fs/ext4/extents.c >> +++ b/fs/ext4/extents.c >> @@ -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 of MAP_NEW flag. > > Right logic? > > > I can not see the error and the meaning of ee_len - allocated - map->m_len. > > Thanks, > Yongqiang. > > >> 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 >> >> > > > -- 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
On Wed, 2011-05-11 at 09:47 +0800, Yongqiang Yang wrote: > On Wed, May 11, 2011 at 1:56 AM, Allison Henderson > <achender@linux.vnet.ibm.com> wrote: > > 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! > > > > Signed-off-by: Allison Henderson <achender@us.ibm.com> > > --- > > :100644 100644 e363f21... ce69450... M fs/ext4/extents.c > > fs/ext4/extents.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > index e363f21..ce69450 100644 > > --- a/fs/ext4/extents.c > > +++ b/fs/ext4/extents.c > > @@ -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 of MAP_NEW flag. > > 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 > > I can not see the error and the meaning of ee_len - allocated - map->m_len. > > Thanks, > Yongqiang. > > > > 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 > > > > > > > -- 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
On 5/11/2011 6:15 PM, Mingming Cao wrote: > On Wed, 2011-05-11 at 09:47 +0800, Yongqiang Yang wrote: >> On Wed, May 11, 2011 at 1:56 AM, Allison Henderson >> <achender@linux.vnet.ibm.com> wrote: >>> 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! >>> >>> Signed-off-by: Allison Henderson<achender@us.ibm.com> >>> --- >>> :100644 100644 e363f21... ce69450... M fs/ext4/extents.c >>> fs/ext4/extents.c | 3 ++- >>> 1 files changed, 2 insertions(+), 1 deletions(-) >>> >>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >>> index e363f21..ce69450 100644 >>> --- a/fs/ext4/extents.c >>> +++ b/fs/ext4/extents.c >>> @@ -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 of MAP_NEW flag. >> >> 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 > > >> >> 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 >> >> Thanks, >> Yongqiang. >> >> >>> 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 >>> >>> >> >> >> > > -- 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
On Wed, 2011-05-11 at 18:15 -0700, Mingming Cao wrote: > On Wed, 2011-05-11 at 09:47 +0800, Yongqiang Yang wrote: > > On Wed, May 11, 2011 at 1:56 AM, Allison Henderson > > <achender@linux.vnet.ibm.com> wrote: > > > 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! > > > > > > Signed-off-by: Allison Henderson <achender@us.ibm.com> > > > --- > > > :100644 100644 e363f21... ce69450... M fs/ext4/extents.c > > > fs/ext4/extents.c | 3 ++- > > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > > > index e363f21..ce69450 100644 > > > --- a/fs/ext4/extents.c > > > +++ b/fs/ext4/extents.c > > > @@ -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 of MAP_NEW flag. > > > > 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; > } > 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 > > > Mingming > > > > > > I can not see the error and the meaning of ee_len - allocated - map->m_len. > > > > Thanks, > > Yongqiang. > > > > > > > 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 > > > > > > > > > > > > > > > -- > 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 -- 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
On Thu, May 12, 2011 at 9:15 AM, Mingming Cao <cmm@us.ibm.com> wrote: > On Wed, 2011-05-11 at 09:47 +0800, Yongqiang Yang wrote: >> On Wed, May 11, 2011 at 1:56 AM, Allison Henderson >> <achender@linux.vnet.ibm.com> wrote: >> > 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! >> > >> > Signed-off-by: Allison Henderson <achender@us.ibm.com> >> > --- >> > :100644 100644 e363f21... ce69450... M fs/ext4/extents.c >> > fs/ext4/extents.c | 3 ++- >> > 1 files changed, 2 insertions(+), 1 deletions(-) >> > >> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c >> > index e363f21..ce69450 100644 >> > --- a/fs/ext4/extents.c >> > +++ b/fs/ext4/extents.c >> > @@ -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 of MAP_NEW flag. >> >> Right logic? >> > Hi Mingming, Sorry for late response. > 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. > > 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); 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(). > + ext4_ext_try_to_merge(inode, path, ex); > + err = ext4_ext_dirty(handle, inode, path + depth); > + goto out; > } > > > > Mingming > > >> >> I can not see the error and the meaning of ee_len - allocated - map->m_len. >> >> Thanks, >> Yongqiang. >> >> >> > 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 >> > >> > >> >> >> > > >
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index e363f21..ce69450 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -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);
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! Signed-off-by: Allison Henderson <achender@us.ibm.com> --- :100644 100644 e363f21... ce69450... M fs/ext4/extents.c fs/ext4/extents.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)