ext4_ext_convert_to_initialized() does not initialize eh before using it
and this is introduced in commit 864d21652.
Cc:Eric Gouriou <[email protected]>
Cc:"Theodore Ts'o" <[email protected]>
Signed-off-by: Yongqiang Yang <[email protected]>
---
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
Hi Eric,
Sorry, Cc did not work in git.
Yongqiang.
---------- Forwarded message ----------
From: Yongqiang Yang <[email protected]>
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: [email protected]
Cc: Yongqiang Yang <[email protected]>
ext4_ext_convert_to_initialized() does not initialize eh before using it
and this is introduced in commit 864d21652.
Cc:Eric Gouriou <[email protected]>
Cc:"Theodore Ts'o" <[email protected]>
Signed-off-by: Yongqiang Yang <[email protected]>
---
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
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 <[email protected]>
> Cc:"Theodore Ts'o" <[email protected]>
> Signed-off-by: Yongqiang Yang <[email protected]>
> 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
[Resend of my earlier message with HTML gunk removed and one edit. ]
On Tue, Nov 1, 2011 at 15:52, Ted Ts'o <[email protected]> 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 <[email protected]>
> > Cc:"Theodore Ts'o" <[email protected]>
> > Signed-off-by: Yongqiang Yang <[email protected]>
>
> > 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
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 <[email protected]> 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 <[email protected]>
>>> Cc:"Theodore Ts'o" <[email protected]>
>>> Signed-off-by: Yongqiang Yang <[email protected]>
>>
>>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html