Hi all,
While trying to add more punch hole tests to xfstest, I found that test 252 hangs on ext4 due to a loop in xfsprogs that does not exit. XFS gets out of this loop because there is logic in the loop that looks for the last extent flag and breaks out. But it looks like ext4 does not return a last extent when the file has a hole at the end. I am not sure if this is the correct behavior or not, so I will copy the ext4 folks on this too. Below is a copy of the fix for xfsprogs:
diff --git a/io/fiemap.c b/io/fiemap.c
index fa990cc..81fc92c 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -246,7 +246,7 @@ fiemap_f(
flg_w, _("FLAGS"));
}
- while (!last && ((cur_extent + 1) != max_extents)) {
+ while (!last && (cur_extent <= max_extents)) {
if (max_extents)
num_extents = min(num_extents,
max_extents - (cur_extent + 1));
It looks like the loop enters with last=0, cur_extents=0, and max_extents = 0, and on the first iteration cur_extents get set to 2, so we dont see ((cur_extent + 1) == max_extents for a very long time. I doubt the logic was meant to work that way, so this patch should fix it, but I wanted to make sure that the fiemap for ext4 is working as intended too. Feed back appreciated! Thx all!
Allison Henderson
On Tue, May 24, 2011 at 4:34 AM, Allison Henderson
<[email protected]> wrote:
> Hi all,
>
> While trying to add more punch hole tests to xfstest, I found that test 252 hangs on ext4 due to a loop in xfsprogs that does not exit. ?XFS gets out of this loop because there is logic in the loop that looks for the last extent flag and breaks out. ?But it looks like ext4 does not return a last extent when the file has a hole at the end. ?I am not sure if this is the correct behavior or not, so I will copy the ext4 folks on this too. ?Below is a copy of the fix for xfsprogs:
Hi there,
What's blocksize of the tested ext4? For now, ext4 returns
LAST_EXTENT if the logical offset covered by the extent is greater
than file size, so if there is a hole at the end, no last extent is
returned. Thx!
Yongqiang.
>
> diff --git a/io/fiemap.c b/io/fiemap.c
> index fa990cc..81fc92c 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -246,7 +246,7 @@ fiemap_f(
> ? ? ? ? ? ? ? ? ? ? ? flg_w, _("FLAGS"));
> ? ? ? ?}
>
> - ? ? ? while (!last && ((cur_extent + 1) != max_extents)) {
> + ? ? ? while (!last && (cur_extent <= max_extents)) {
> ? ? ? ? ? ? ? ?if (max_extents)
> ? ? ? ? ? ? ? ? ? ? ? ?num_extents = min(num_extents,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?max_extents - (cur_extent + 1));
>
>
> It looks like the loop enters with last=0, cur_extents=0, and max_extents = 0, and on the first iteration cur_extents get set to 2, so we dont see ((cur_extent + 1) == max_extents for a very long time. ?I doubt the logic was meant to work that way, so this patch should fix it, but I wanted to make sure that the fiemap for ext4 is working as intended too. ?Feed back appreciated! ?Thx all!
>
> Allison Henderson
> --
> 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
>
--
Best Wishes
Yongqiang Yang
On 5/23/2011 6:16 PM, Yongqiang Yang wrote:
> On Tue, May 24, 2011 at 4:34 AM, Allison Henderson
> <[email protected]> wrote:
>> Hi all,
>>
>> While trying to add more punch hole tests to xfstest, I found that test 252 hangs on ext4 due to a loop in xfsprogs that does not exit. XFS gets out of this loop because there is logic in the loop that looks for the last extent flag and breaks out. But it looks like ext4 does not return a last extent when the file has a hole at the end. I am not sure if this is the correct behavior or not, so I will copy the ext4 folks on this too. Below is a copy of the fix for xfsprogs:
>
> Hi there,
>
> What's blocksize of the tested ext4? For now, ext4 returns
> LAST_EXTENT if the logical offset covered by the extent is greater
> than file size, so if there is a hole at the end, no last extent is
> returned. Thx!
>
> Yongqiang.
Hi there,
The block size I've been using is 4096. As long as that behavior is
expected, I think the test will be ok with just the xfsprogs fix,
though. Thx!
Allison Henderson
>
>>
>> diff --git a/io/fiemap.c b/io/fiemap.c
>> index fa990cc..81fc92c 100644
>> --- a/io/fiemap.c
>> +++ b/io/fiemap.c
>> @@ -246,7 +246,7 @@ fiemap_f(
>> flg_w, _("FLAGS"));
>> }
>>
>> - while (!last&& ((cur_extent + 1) != max_extents)) {
>> + while (!last&& (cur_extent<= max_extents)) {
>> if (max_extents)
>> num_extents = min(num_extents,
>> max_extents - (cur_extent + 1));
>>
>>
>> It looks like the loop enters with last=0, cur_extents=0, and max_extents = 0, and on the first iteration cur_extents get set to 2, so we dont see ((cur_extent + 1) == max_extents for a very long time. I doubt the logic was meant to work that way, so this patch should fix it, but I wanted to make sure that the fiemap for ext4 is working as intended too. Feed back appreciated! Thx all!
>>
>> Allison Henderson
>> --
>> 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
>>
>
>
>
On 5/23/2011 1:34 PM, Allison Henderson wrote:
> Hi all,
>
> While trying to add more punch hole tests to xfstest, I found that test 252 hangs on ext4 due to a loop in xfsprogs that does not exit. XFS gets out of this loop because there is logic in the loop that looks for the last extent flag and breaks out. But it looks like ext4 does not return a last extent when the file has a hole at the end. I am not sure if this is the correct behavior or not, so I will copy the ext4 folks on this too. Below is a copy of the fix for xfsprogs:
>
> diff --git a/io/fiemap.c b/io/fiemap.c
> index fa990cc..81fc92c 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -246,7 +246,7 @@ fiemap_f(
> flg_w, _("FLAGS"));
> }
>
> - while (!last&& ((cur_extent + 1) != max_extents)) {
> + while (!last&& (cur_extent<= max_extents)) {
> if (max_extents)
> num_extents = min(num_extents,
> max_extents - (cur_extent + 1));
>
>
> It looks like the loop enters with last=0, cur_extents=0, and max_extents = 0, and on the first iteration cur_extents get set to 2, so we dont see ((cur_extent + 1) == max_extents for a very long time. I doubt the logic was meant to work that way, so this patch should fix it, but I wanted to make sure that the fiemap for ext4 is working as intended too. Feed back appreciated! Thx all!
>
> Allison Henderson
>
Hi all,
I haven't heard much back on this patch, so Im just poking this thread
to make sure it doesn't get forgotten. I have some patches out there for
punch hole, and I'm currently looking at fixing up some older punch hole
tests in the dmapi code, but they wont do much good for ext4 with out
this fix. If I could get a quick peek from some one on the xfs list for
this patch, that would be much appreciated. Thx all!
Allison Henderson
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs
On Thu, May 26, 2011 at 11:32:57AM -0700, Allison Henderson wrote:
> On 5/23/2011 1:34 PM, Allison Henderson wrote:
> >Hi all,
> >
> >While trying to add more punch hole tests to xfstest, I found
> >that test 252 hangs on ext4 due to a loop in xfsprogs that does
> >not exit. XFS gets out of this loop because there is logic in
> >the loop that looks for the last extent flag and breaks out. But
> >it looks like ext4 does not return a last extent when the file
> >has a hole at the end. I am not sure if this is the correct
> >behavior or not, so I will copy the ext4 folks on this too.
> >Below is a copy of the fix for xfsprogs:
> >
> >diff --git a/io/fiemap.c b/io/fiemap.c
> >index fa990cc..81fc92c 100644
> >--- a/io/fiemap.c
> >+++ b/io/fiemap.c
> >@@ -246,7 +246,7 @@ fiemap_f(
> > flg_w, _("FLAGS"));
> > }
> >
> >- while (!last&& ((cur_extent + 1) != max_extents)) {
> >+ while (!last&& (cur_extent<= max_extents)) {
> > if (max_extents)
> > num_extents = min(num_extents,
> > max_extents - (cur_extent + 1));
> >
> >
> >It looks like the loop enters with last=0, cur_extents=0, and
> >max_extents = 0, and on the first iteration cur_extents get set
> >to 2, so we dont see ((cur_extent + 1) == max_extents for a very
> >long time. I doubt the logic was meant to work that way, so this
> >patch should fix it, but I wanted to make sure that the fiemap
> >for ext4 is working as intended too. Feed back appreciated! Thx
> >all!
> >
> >Allison Henderson
> >
> Hi all,
>
> I haven't heard much back on this patch, so Im just poking this
> thread to make sure it doesn't get forgotten. I have some patches
> out there for punch hole, and I'm currently looking at fixing up
> some older punch hole tests in the dmapi code, but they wont do much
> good for ext4 with out this fix. If I could get a quick peek from
> some one on the xfs list for this patch, that would be much
> appreciated. Thx all!
If ext4 is not setting the last extent flag on the last extent then
that's an ext4 bug that the test has detected, right? And so you
should be fixing ext4 rather than modifying the test to hide the
different behaviour?
Cheer
--
Dave Chinner
[email protected]
On 5/23/2011 7:38 PM, Allison Henderson wrote:
> On 5/23/2011 6:16 PM, Yongqiang Yang wrote:
>> On Tue, May 24, 2011 at 4:34 AM, Allison Henderson
>> <[email protected]> wrote:
>>> Hi all,
>>>
>>> While trying to add more punch hole tests to xfstest, I found that
>>> test 252 hangs on ext4 due to a loop in xfsprogs that does not exit.
>>> XFS gets out of this loop because there is logic in the loop that
>>> looks for the last extent flag and breaks out. But it looks like ext4
>>> does not return a last extent when the file has a hole at the end. I
>>> am not sure if this is the correct behavior or not, so I will copy
>>> the ext4 folks on this too. Below is a copy of the fix for xfsprogs:
>>
>> Hi there,
>>
>> What's blocksize of the tested ext4? For now, ext4 returns
>> LAST_EXTENT if the logical offset covered by the extent is greater
>> than file size, so if there is a hole at the end, no last extent is
>> returned. Thx!
>>
>> Yongqiang.
>
> Hi there,
>
> The block size I've been using is 4096. As long as that behavior is
> expected, I think the test will be ok with just the xfsprogs fix,
> though. Thx!
>
> Allison Henderson
>
>>
>>>
>>> diff --git a/io/fiemap.c b/io/fiemap.c
>>> index fa990cc..81fc92c 100644
>>> --- a/io/fiemap.c
>>> +++ b/io/fiemap.c
>>> @@ -246,7 +246,7 @@ fiemap_f(
>>> flg_w, _("FLAGS"));
>>> }
>>>
>>> - while (!last&& ((cur_extent + 1) != max_extents)) {
>>> + while (!last&& (cur_extent<= max_extents)) {
>>> if (max_extents)
>>> num_extents = min(num_extents,
>>> max_extents - (cur_extent + 1));
>>>
>>>
>>> It looks like the loop enters with last=0, cur_extents=0, and
>>> max_extents = 0, and on the first iteration cur_extents get set to 2,
>>> so we dont see ((cur_extent + 1) == max_extents for a very long time.
>>> I doubt the logic was meant to work that way, so this patch should
>>> fix it, but I wanted to make sure that the fiemap for ext4 is working
>>> as intended too. Feed back appreciated! Thx all!
>>>
>>> Allison Henderson
>>> --
>>> 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
>>>
>>
>>
>>
>
>> Hi all,
>>
>> I haven't heard much back on this patch, so Im just poking this
>> thread to make sure it doesn't get forgotten. I have some patches
>> out there for punch hole, and I'm currently looking at fixing up
>> some older punch hole tests in the dmapi code, but they wont do much
>> good for ext4 with out this fix. If I could get a quick peek from
>> some one on the xfs list for this patch, that would be much
>> appreciated. Thx all!
>
> If ext4 is not setting the last extent flag on the last extent then
> that's an ext4 bug that the test has detected, right? And so you
> should be fixing ext4 rather than modifying the test to hide the
> different behaviour?
>
> Cheer
>
>-- Dave Chinner [email protected]
Hi All,
Sorry, I should have poked the thread with Yongqiang's response, so I
will move the dialog into this thread. At the moment, it sounds like
the fiemap for ext4 is working as intended. Yongqiang, do you agree
that the fiemap for ext4 should be changed? I think you are more
familiar with this part of the code than I am, and I just want to make
sure we find a solution that everyone is happy with. Thx!
Allison Henderson
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs
On Tue, May 31, 2011 at 11:25 PM, Allison Henderson
<[email protected]> wrote:
> On 5/23/2011 7:38 PM, Allison Henderson wrote:
>>
>> On 5/23/2011 6:16 PM, Yongqiang Yang wrote:
>>>
>>> On Tue, May 24, 2011 at 4:34 AM, Allison Henderson
>>> <[email protected]> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> While trying to add more punch hole tests to xfstest, I found that
>>>> test 252 hangs on ext4 due to a loop in xfsprogs that does not exit.
>>>> XFS gets out of this loop because there is logic in the loop that
>>>> looks for the last extent flag and breaks out. But it looks like ext4
>>>> does not return a last extent when the file has a hole at the end. I
>>>> am not sure if this is the correct behavior or not, so I will copy
>>>> the ext4 folks on this too. Below is a copy of the fix for xfsprogs:
>>>
>>> Hi there,
>>>
>>> What's blocksize of the tested ext4? For now, ext4 returns
>>> LAST_EXTENT if the logical offset covered by the extent is greater
>>> than file size, so if there is a hole at the end, no last extent is
>>> returned. Thx!
>>>
>>> Yongqiang.
>>
>> Hi there,
>>
>> The block size I've been using is 4096. As long as that behavior is
>> expected, I think the test will be ok with just the xfsprogs fix,
>> though. Thx!
>>
>> Allison Henderson
>>
>>>
>>>>
>>>> diff --git a/io/fiemap.c b/io/fiemap.c
>>>> index fa990cc..81fc92c 100644
>>>> --- a/io/fiemap.c
>>>> +++ b/io/fiemap.c
>>>> @@ -246,7 +246,7 @@ fiemap_f(
>>>> flg_w, _("FLAGS"));
>>>> }
>>>>
>>>> - while (!last&& ((cur_extent + 1) != max_extents)) {
>>>> + while (!last&& (cur_extent<= max_extents)) {
>>>> if (max_extents)
>>>> num_extents = min(num_extents,
>>>> max_extents - (cur_extent + 1));
>>>>
>>>>
>>>> It looks like the loop enters with last=0, cur_extents=0, and
>>>> max_extents = 0, and on the first iteration cur_extents get set to 2,
>>>> so we dont see ((cur_extent + 1) == max_extents for a very long time.
>>>> I doubt the logic was meant to work that way, so this patch should
>>>> fix it, but I wanted to make sure that the fiemap for ext4 is working
>>>> as intended too. Feed back appreciated! Thx all!
>>>>
>>>> Allison Henderson
>>>> --
>>>> 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
>>>>
>>>
>>>
>>>
>>
>
>
>>> Hi all,
>>>
>>> I haven't heard much back on this patch, so Im just poking this
>>> thread to make sure it doesn't get forgotten. I have some patches
>>> out there for punch hole, and I'm currently looking at fixing up
>>> some older punch hole tests in the dmapi code, but they wont do much
>>> good for ext4 with out this fix. If I could get a quick peek from
>>> some one on the xfs list for this patch, that would be much
>>> appreciated. Thx all!
>>
>> If ext4 is not setting the last extent flag on the last extent then
>> that's an ext4 bug that the test has detected, right? And so you
>> should be fixing ext4 rather than modifying the test to hide the
>> different behaviour?
>>
>> Cheer
>>
>>-- Dave Chinner [email protected]
>
> Hi All,
>
> Sorry, I should have poked the thread with Yongqiang's response, so I will
> move the dialog into this thread. At the moment, it sounds like the fiemap
> for ext4 is working as intended. Yongqiang, do you agree that the fiemap
> for ext4 should be changed? I think you are more familiar with this part of
Yes, I agree. ext4 should return LAST extent. I am thinking if we
can find a new solution collecting extents.
Maybe we can insert delayed extents into extent tree. This way fiemap
will be simpler and much more efficient.
I would like to throw out the proposal inserting delayed-extents into
extent tree. What will it bring? AFAIK it will bring:
1. We need to down i_data_sem in delayed write-path to insert
delayed-extents into the tree without journaling it.
2. When we come to block allocation, we can convert delayed-extents to
normal extents.
There is a problem that the solution can be only used in ordered mode.
So what are your opinions？
Yongqiang.
> the code than I am, and I just want to make sure we find a solution that
> everyone is happy with. Thx!
>
> Allison Henderson
>
>
>
>
>
>> _______________________________________________
>> xfs mailing list
>> [email protected]
>> http://oss.sgi.com/mailman/listinfo/xfs
>
>
--
Best Wishes
Yongqiang Yang
On 5/31/2011 6:58 PM, Yongqiang Yang wrote:
> On Tue, May 31, 2011 at 11:25 PM, Allison Henderson
> <[email protected]> wrote:
>> On 5/23/2011 7:38 PM, Allison Henderson wrote:
>>>
>>> On 5/23/2011 6:16 PM, Yongqiang Yang wrote:
>>>>
>>>> On Tue, May 24, 2011 at 4:34 AM, Allison Henderson
>>>> <[email protected]> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> While trying to add more punch hole tests to xfstest, I found that
>>>>> test 252 hangs on ext4 due to a loop in xfsprogs that does not exit.
>>>>> XFS gets out of this loop because there is logic in the loop that
>>>>> looks for the last extent flag and breaks out. But it looks like ext4
>>>>> does not return a last extent when the file has a hole at the end. I
>>>>> am not sure if this is the correct behavior or not, so I will copy
>>>>> the ext4 folks on this too. Below is a copy of the fix for xfsprogs:
>>>>
>>>> Hi there,
>>>>
>>>> What's blocksize of the tested ext4? For now, ext4 returns
>>>> LAST_EXTENT if the logical offset covered by the extent is greater
>>>> than file size, so if there is a hole at the end, no last extent is
>>>> returned. Thx!
>>>>
>>>> Yongqiang.
>>>
>>> Hi there,
>>>
>>> The block size I've been using is 4096. As long as that behavior is
>>> expected, I think the test will be ok with just the xfsprogs fix,
>>> though. Thx!
>>>
>>> Allison Henderson
>>>
>>>>
>>>>>
>>>>> diff --git a/io/fiemap.c b/io/fiemap.c
>>>>> index fa990cc..81fc92c 100644
>>>>> --- a/io/fiemap.c
>>>>> +++ b/io/fiemap.c
>>>>> @@ -246,7 +246,7 @@ fiemap_f(
>>>>> flg_w, _("FLAGS"));
>>>>> }
>>>>>
>>>>> - while (!last&& ((cur_extent + 1) != max_extents)) {
>>>>> + while (!last&& (cur_extent<= max_extents)) {
>>>>> if (max_extents)
>>>>> num_extents = min(num_extents,
>>>>> max_extents - (cur_extent + 1));
>>>>>
>>>>>
>>>>> It looks like the loop enters with last=0, cur_extents=0, and
>>>>> max_extents = 0, and on the first iteration cur_extents get set to 2,
>>>>> so we dont see ((cur_extent + 1) == max_extents for a very long time.
>>>>> I doubt the logic was meant to work that way, so this patch should
>>>>> fix it, but I wanted to make sure that the fiemap for ext4 is working
>>>>> as intended too. Feed back appreciated! Thx all!
>>>>>
>>>>> Allison Henderson
>>>>> --
>>>>> 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
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>>> Hi all,
>>>>
>>>> I haven't heard much back on this patch, so Im just poking this
>>>> thread to make sure it doesn't get forgotten. I have some patches
>>>> out there for punch hole, and I'm currently looking at fixing up
>>>> some older punch hole tests in the dmapi code, but they wont do much
>>>> good for ext4 with out this fix. If I could get a quick peek from
>>>> some one on the xfs list for this patch, that would be much
>>>> appreciated. Thx all!
>>>
>>> If ext4 is not setting the last extent flag on the last extent then
>>> that's an ext4 bug that the test has detected, right? And so you
>>> should be fixing ext4 rather than modifying the test to hide the
>>> different behaviour?
>>>
>>> Cheer
>>>
>>> -- Dave Chinner [email protected]
>>
>> Hi All,
>>
>> Sorry, I should have poked the thread with Yongqiang's response, so I will
>> move the dialog into this thread. At the moment, it sounds like the fiemap
>> for ext4 is working as intended. Yongqiang, do you agree that the fiemap
>> for ext4 should be changed? I think you are more familiar with this part of
> Yes, I agree. ext4 should return LAST extent. I am thinking if we
> can find a new solution collecting extents.
>
> Maybe we can insert delayed extents into extent tree. This way fiemap
> will be simpler and much more efficient.
>
> I would like to throw out the proposal inserting delayed-extents into
> extent tree. What will it bring? AFAIK it will bring:
> 1. We need to down i_data_sem in delayed write-path to insert
> delayed-extents into the tree without journaling it.
>
> 2. When we come to block allocation, we can convert delayed-extents to
> normal extents.
>
> There is a problem that the solution can be only used in ordered mode.
> So what are your opinions？
>
> Yongqiang.
Hi All,
Well, I am not yet familiar with how the code for ordered mode works, so
I may not be much help here, but I do think that if we could get it to
work, it would help simplify a lot of things including fiemap and punch
hole. I know that the earlier versions of punch hole were a little
complicated because of the different mechanisms needed to identify
mapped extents, delayed extents and holes. Eventually what we did was
to flush out the data in the region to be punched out in order to avoid
race conditions, and also to simplify the logic. This has introduced
some problems in some of the existing xfstests punch hole tests, because
the tests want to see a hole in unwritten extents instead of written
extents. I was working on some optimization patches to avoid this, but
if this proposal is put in place, I think I could optimize that fix a
lot. And I know the fiemap routines would get simpler sense they would
only have to deal with the extent tree. If we decide to do what you
propose, I will wait on doing any punch hole or fiemap patches to take
advantage of that. I'll poke around with the modes too to see if
there's any thing we can do to get around that problem. Thx!
Allison Henderson
>
>> the code than I am, and I just want to make sure we find a solution that
>> everyone is happy with. Thx!
>>
>> Allison Henderson
>>
>>
>>
>>
>>
>>> _______________________________________________
>>> xfs mailing list
>>> [email protected]
>>> http://oss.sgi.com/mailman/listinfo/xfs
>>
>>
>
>
>