ext4_ext_convert_to_initialized bug found in extended FSX testing

ext4_ext_convert_to_initialized bug found in extended FSX testing

[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 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);
 			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

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Yongqiang Yang]

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
>
>



-- 
Best Wishes
Yongqiang Yang
--
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

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Allison Henderson]

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
>>
>>
>
>
>

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Allison Henderson]

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
>>
>>
>
>
>

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Mingming Cao]

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
> >
> >
> 
> 
> 

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Allison Henderson]

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
>>>
>>>
>>
>>
>>
>
>

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Mingming Cao]

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

Re: ext4_ext_convert_to_initialized bug found in extended FSX testing

[Yongqiang Yang]

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
>> >
>> >
>>
>>
>>
>
>
>



-- 
Best Wishes
Yongqiang Yang
--
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