[PATCH v2] ext4:Let ext4_ext_fiemap_cb() handle blocks before request range correctly.

[PATCH v2] ext4:Let ext4_ext_fiemap_cb() handle blocks before request range correctly.

[Yongqiang Yang]

v1->v2:
  Add more specific description.

To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache,
it thus collects information starting from a page's head block.
If blockszie < pagesize, the beginning blocks of a page may lie before the
range.  ext4_ext_fiemap_cb() should proceed ignoring them, because they
has been handled before.

Reported-by: Amir Goldstein <amir73il@users.sf.net>
Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
---
 fs/ext4/extents.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e363f21..ec37109 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3718,9 +3718,14 @@ out:
 
 			bh = head;
 			do {
+				if (end < newex->ec_block)
+					/* The buffer is not in
+					 * the request range.
+					 */
+					continue;
 				if (buffer_mapped(bh)) {
 					/* get the 1st mapped buffer. */
-					if (end > newex->ec_block +
+					if (end >= newex->ec_block +
 						newex->ec_len)
 						/* The buffer is out of
 						 * the request range.
-- 
1.7.5.1

Re: [PATCH v2] ext4:Let ext4_ext_fiemap_cb() handle blocks before request range correctly.

[Lukas Czerner]

On Tue, 10 May 2011, Yongqiang Yang wrote:

> v1->v2:
>   Add more specific description.
> 
> To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache,
> it thus collects information starting from a page's head block.
> If blockszie < pagesize, the beginning blocks of a page may lie before the
> range.  ext4_ext_fiemap_cb() should proceed ignoring them, because they
> has been handled before.

Thanks for the description, but I have one question below.

> 
> Reported-by: Amir Goldstein <amir73il@users.sf.net>
> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
> ---
>  fs/ext4/extents.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e363f21..ec37109 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3718,9 +3718,14 @@ out:
>  
>  			bh = head;
>  			do {
> +				if (end < newex->ec_block)
> +					/* The buffer is not in
> +					 * the request range.
> +					 */
> +					continue;
So if the condition is true, then we might leave the loop, because (bh ==
head) in the first iteration, is that desired behavior? Also if we hit
this condition in other than first iteration we will spin forever. I do
not think this is right. How did you tested this case ?

Thanks!
-Lukas

>  				if (buffer_mapped(bh)) {
>  					/* get the 1st mapped buffer. */
> -					if (end > newex->ec_block +
> +					if (end >= newex->ec_block +
>  						newex->ec_len)
>  						/* The buffer is out of
>  						 * the request range.
> 

Re: [PATCH v2] ext4:Let ext4_ext_fiemap_cb() handle blocks before request range correctly.

[Yongqiang Yang]

On Tue, May 10, 2011 at 10:35 PM, Lukas Czerner <lczerner@redhat.com> wrote:
> On Tue, 10 May 2011, Yongqiang Yang wrote:
>
>> v1->v2:
>>   Add more specific description.
>>
>> To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache,
>> it thus collects information starting from a page's head block.
>> If blockszie < pagesize, the beginning blocks of a page may lie before the
>> range.  ext4_ext_fiemap_cb() should proceed ignoring them, because they
>> has been handled before.
>
> Thanks for the description, but I have one question below.
>
>>
>> Reported-by: Amir Goldstein <amir73il@users.sf.net>
>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>> ---
>>  fs/ext4/extents.c |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index e363f21..ec37109 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -3718,9 +3718,14 @@ out:
>>
>>                       bh = head;
>>                       do {
>> +                             if (end < newex->ec_block)
>> +                                     /* The buffer is not in
>> +                                      * the request range.
>> +                                      */
>> +                                     continue;
> So if the condition is true, then we might leave the loop, because (bh ==
> head) in the first iteration, is that desired behavior? Also if we hit
> this condition in other than first iteration we will spin forever. I do
> not think this is right. How did you tested this case ?
You are right.  Maybe the patch sent out is not same as the patch with
which I tested.  What a stupid error!
I am not sure what happened now.  I will check the patch with which I
tested tomorrow.

My working tree passed xfstests 225 in both 1k blocksize and 4k
blocksize cases.

Sorry!

Yongqiang.
>
> Thanks!
> -Lukas
>
>>                               if (buffer_mapped(bh)) {
>>                                       /* get the 1st mapped buffer. */
>> -                                     if (end > newex->ec_block +
>> +                                     if (end >= newex->ec_block +
>>                                               newex->ec_len)
>>                                               /* The buffer is out of
>>                                                * the request range.
>>
>



-- 
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 v2] ext4:Let ext4_ext_fiemap_cb() handle blocks before request range correctly.

[Yongqiang Yang]

On Tue, May 10, 2011 at 11:08 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
> On Tue, May 10, 2011 at 10:35 PM, Lukas Czerner <lczerner@redhat.com> wrote:
>> On Tue, 10 May 2011, Yongqiang Yang wrote:
>>
>>> v1->v2:
>>>   Add more specific description.
>>>
>>> To get delayed-extent information, ext4_ext_fiemap_cb() lookups pagecache,
>>> it thus collects information starting from a page's head block.
>>> If blockszie < pagesize, the beginning blocks of a page may lie before the
>>> range.  ext4_ext_fiemap_cb() should proceed ignoring them, because they
>>> has been handled before.
>>
>> Thanks for the description, but I have one question below.
>>
>>>
>>> Reported-by: Amir Goldstein <amir73il@users.sf.net>
>>> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
>>> ---
>>>  fs/ext4/extents.c |    7 ++++++-
>>>  1 files changed, 6 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>> index e363f21..ec37109 100644
>>> --- a/fs/ext4/extents.c
>>> +++ b/fs/ext4/extents.c
>>> @@ -3718,9 +3718,14 @@ out:
>>>
>>>                       bh = head;
>>>                       do {
>>> +                             if (end < newex->ec_block)
>>> +                                     /* The buffer is not in
>>> +                                      * the request range.
>>> +                                      */
>>> +                                     continue;
>> So if the condition is true, then we might leave the loop, because (bh ==
>> head) in the first iteration, is that desired behavior? Also if we hit
>> this condition in other than first iteration we will spin forever. I do
>> not think this is right. How did you tested this case ?
> You are right.  Maybe the patch sent out is not same as the patch with
> which I tested.  What a stupid error!
> I am not sure what happened now.  I will check the patch with which I
> tested tomorrow.
>
> My working tree passed xfstests 225 in both 1k blocksize and 4k
> blocksize cases.
I checked the code I tested with.  Indeed I tested with this ugly
patch and xfstests 225 passed.  I tested again and results were same '
PASSED'.

I had a look into xfstests 225 and found that there are problems in
xfstests 225 so it can not find the bug induced by this ugly patch.
It seems that current xfstests 225 has a lot of problems.

I am fixing xfstests 225 to enable it find this bug.

Yongqiang.

>
> Sorry!
>
> Yongqiang.
>>
>> Thanks!
>> -Lukas
>>
>>>                               if (buffer_mapped(bh)) {
>>>                                       /* get the 1st mapped buffer. */
>>> -                                     if (end > newex->ec_block +
>>> +                                     if (end >= newex->ec_block +
>>>                                               newex->ec_len)
>>>                                               /* The buffer is out of
>>>                                                * the request range.
>>>
>>
>
>
>
> --
> Best Wishes
> Yongqiang Yang
>



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