[PATCH] ext4: let ext4_ext_convet_to_initialized initialize var(eh) before using it

[PATCH] ext4: let ext4_ext_convet_to_initialized initialize var(eh) before using it

[Yongqiang Yang]

ext4_ext_convert_to_initialized() does not initialize eh before using it
and this is introduced in commit 864d21652.

Cc:Eric Gouriou <egouriou@google.com>
Cc:"Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---

Hi Eric,

Was that patch tested?

 fs/ext4/extents.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9dfdf8f..2798945 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2944,6 +2944,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
 		eof_block = map->m_lblk + map->m_len;
 
 	depth = ext_depth(inode);
+	eh = path[depth].p_hdr;
 	ex = path[depth].p_ext;
 	ee_block = le32_to_cpu(ex->ee_block);
 	ee_len = ext4_ext_get_actual_len(ex);
-- 
1.7.5.1

Re: [PATCH] ext4: let ext4_ext_convet_to_initialized initialize var(eh) before using it

[Yongqiang Yang]

Hi Eric,

Sorry, Cc did not work in git.

Yongqiang.
---------- Forwarded message ----------
From: Yongqiang Yang <xiaoqiangnk@gmail.com>
Date: Tue, Nov 1, 2011 at 9:21 AM
Subject: [PATCH] ext4: let ext4_ext_convet_to_initialized initialize
var(eh) before using it
To: linux-ext4@vger.kernel.org
Cc: Yongqiang Yang <xiaoqiangnk@gmail.com>


ext4_ext_convert_to_initialized() does not initialize eh before using it
and this is introduced in commit 864d21652.

Cc:Eric Gouriou <egouriou@google.com>
Cc:"Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---

Hi Eric,

Was that patch tested?

 fs/ext4/extents.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9dfdf8f..2798945 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2944,6 +2944,7 @@ static int
ext4_ext_convert_to_initialized(handle_t *handle,
               eof_block = map->m_lblk + map->m_len;

       depth = ext_depth(inode);
+       eh = path[depth].p_hdr;
       ex = path[depth].p_ext;
       ee_block = le32_to_cpu(ex->ee_block);
       ee_len = ext4_ext_get_actual_len(ex);
--
1.7.5.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: [PATCH] ext4: let ext4_ext_convet_to_initialized initialize var(eh) before using it

[Ted Ts'o]

On Tue, Nov 01, 2011 at 09:21:21AM +0800, Yongqiang Yang wrote:
> ext4_ext_convert_to_initialized() does not initialize eh before using it
> and this is introduced in commit 864d21652.
> 
> Cc:Eric Gouriou <egouriou@google.com>
> Cc:"Theodore Ts'o" <tytso@mit.edu>
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>

>  		eof_block = map->m_lblk + map->m_len;
>  
>  	depth = ext_depth(inode);
> +	eh = path[depth].p_hdr;
>  	ex = path[depth].p_ext;
>  	ee_block = le32_to_cpu(ex->ee_block);
>  	ee_len = ext4_ext_get_actual_len(ex);

Hmmm, nice catch.

Looks like Eric dropped this line when he forward ported this patch to
v3.1.  Interestingly, I did test this using xfstests, and it didn't
complain.  Which probably means we don't have a good test coverage
that triggers the specific preconditions of this optimization.  Oops.
I'll fix this up now.

Eric, when you have a chance, could you work up an xfstests test that
automates the various tests that you ran manually when you developed
this patch?  Thanks!!

						- Ted

Re: [PATCH] ext4: let ext4_ext_convet_to_initialized initialize var(eh) before using it

[Eric Gouriou]

[Resend of my earlier message with HTML gunk removed and one edit. ]

On Tue, Nov 1, 2011 at 15:52, Ted Ts'o <tytso@mit.edu> wrote:
>
> On Tue, Nov 01, 2011 at 09:21:21AM +0800, Yongqiang Yang wrote:
> > ext4_ext_convert_to_initialized() does not initialize eh before using it
> > and this is introduced in commit 864d21652.
> >
> > Cc:Eric Gouriou <egouriou@google.com>
> > Cc:"Theodore Ts'o" <tytso@mit.edu>
> > Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>
> >               eof_block = map->m_lblk + map->m_len;
> >
> >       depth = ext_depth(inode);
> > +     eh = path[depth].p_hdr;
> >       ex = path[depth].p_ext;
> >       ee_block = le32_to_cpu(ex->ee_block);
> >       ee_len = ext4_ext_get_actual_len(ex);
>
> Hmmm, nice catch.
>
> Looks like Eric dropped this line when he forward ported this patch to
> v3.1.

Indeed I screwed up. Apologies for the trouble. I tested the patch thoroughly
on our kernel version, ported it to ~ 2.6.39 and tested. This was a few months
ago and could not find the time to complete the work then. When I got a chance
to resume the effort, the upstream kernel had changed but I was not supposed
to even build it due to security concerns with the kernel.org sources.
So I redid
the port blind, verified [the file] built but did not test.

>  Interestingly, I did test this using xfstests, and it didn't
> complain.  Which probably means we don't have a good test coverage
> that triggers the specific preconditions of this optimization.  Oops.
> I'll fix this up now.
>
> Eric, when you have a chance, could you work up an xfstests test that
> automates the various tests that you ran manually when you developed
> this patch?  Thanks!!

Sure, but the "chance" may not manifest itself soon.

 Eric

>
>                                                - Ted
--
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: [PATCH] ext4: let ext4_ext_convet_to_initialized initialize var(eh) before using it

[Eric Sandeen]

On 11/2/11 3:22 AM, Eric Gouriou wrote:
> [Resend of my earlier message with HTML gunk removed and one edit. ]
> 
> On Tue, Nov 1, 2011 at 15:52, Ted Ts'o <tytso@mit.edu> wrote:
>>
>> On Tue, Nov 01, 2011 at 09:21:21AM +0800, Yongqiang Yang wrote:
>>> ext4_ext_convert_to_initialized() does not initialize eh before using it
>>> and this is introduced in commit 864d21652.
>>>
>>> Cc:Eric Gouriou <egouriou@google.com>
>>> Cc:"Theodore Ts'o" <tytso@mit.edu>
>>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>>
>>>               eof_block = map->m_lblk + map->m_len;
>>>
>>>       depth = ext_depth(inode);
>>> +     eh = path[depth].p_hdr;
>>>       ex = path[depth].p_ext;
>>>       ee_block = le32_to_cpu(ex->ee_block);
>>>       ee_len = ext4_ext_get_actual_len(ex);
>>
>> Hmmm, nice catch.
>>
>> Looks like Eric dropped this line when he forward ported this patch to
>> v3.1.
> 
> Indeed I screwed up. Apologies for the trouble. I tested the patch thoroughly
> on our kernel version, ported it to ~ 2.6.39 and tested. This was a few months
> ago and could not find the time to complete the work then. When I got a chance
> to resume the effort, the upstream kernel had changed but I was not supposed
> to even build it due to security concerns with the kernel.org sources.
> So I redid
> the port blind, verified [the file] built but did not test.
> 
>>  Interestingly, I did test this using xfstests, and it didn't
>> complain.  Which probably means we don't have a good test coverage
>> that triggers the specific preconditions of this optimization.  Oops.
>> I'll fix this up now.
>>
>> Eric, when you have a chance, could you work up an xfstests test that
>> automates the various tests that you ran manually when you developed
>> this patch?  Thanks!!
> 
> Sure, but the "chance" may not manifest itself soon.

Which probably means "never" :(

This is definitely a "do as I say not as I (always) do" but in general:
having testcases used for testing commits, and not putting them into
the existing regression suite, is bad development practice.  It should
be a priority for all of us.

I know sometimes it is difficult or impossible (my latest xattr race testcase
requires (for now) a bunch of libraries from Ceph, and I haven't found a way
around that yet) but "I don't have time" is a poor excuse.

How did you do the tests?  I'd be glad to give you a hand with the formalized
testcase if you need it.

Thanks,

-Eric (Sandeen)

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