As the member fe_len defined in struct ext4_free_extent is expressed as
number of clusters, the variable "size" computation is wrong, we need to
first translate it to block number, then to bytes.
Signed-off-by: Xiaoguang Wang <[email protected]>
---
fs/ext4/mballoc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 7f72f50..9a543b5 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
size = 8 * 1024 * 1024;
} else {
start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
- size = ac->ac_o_ex.fe_len << bsbits;
+ size = EXT4_C2B(EXT4_SB(ac->ac_sb),
+ ac->ac_o_ex.fe_len) << bsbits;
}
size = size >> bsbits;
start = start_off >> bsbits;
--
1.8.2.1
On Thu, 10 Jul 2014, Xiaoguang Wang wrote:
> Date: Thu, 10 Jul 2014 13:40:36 +0800
> From: Xiaoguang Wang <[email protected]>
> To: [email protected]
> Cc: [email protected], [email protected],
> Xiaoguang Wang <[email protected]>
> Subject: [PATCH] ext4: fix wrong size computation in
> ext4_mb_normalize_request()
>
> As the member fe_len defined in struct ext4_free_extent is expressed as
> number of clusters, the variable "size" computation is wrong, we need to
> first translate it to block number, then to bytes.
>
> Signed-off-by: Xiaoguang Wang <[email protected]>
> ---
> fs/ext4/mballoc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7f72f50..9a543b5 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
> size = 8 * 1024 * 1024;
> } else {
> start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
> - size = ac->ac_o_ex.fe_len << bsbits;
> + size = EXT4_C2B(EXT4_SB(ac->ac_sb),
> + ac->ac_o_ex.fe_len) << bsbits;
There is a patch out there up for more discussion which rewrites a
whole bunch of code in ext4_mb_normalize_request(). But more
importantly this fix, while correct is not going to change anything
since this condition will never be run.
Btw, I take back the correct part since it seems that there is a
a possibility of overflow. This should be better.
size = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
ac->ac_o_ex.fe_len) << bsbits;
Thanks!
-Lukas
> }
> size = size >> bsbits;
> start = start_off >> bsbits;
>
Hi,
On 07/10/2014 05:25 PM, Luk?? Czerner wrote:
> On Thu, 10 Jul 2014, Xiaoguang Wang wrote:
>
>> Date: Thu, 10 Jul 2014 13:40:36 +0800
>> From: Xiaoguang Wang <[email protected]>
>> To: [email protected]
>> Cc: [email protected], [email protected],
>> Xiaoguang Wang <[email protected]>
>> Subject: [PATCH] ext4: fix wrong size computation in
>> ext4_mb_normalize_request()
>>
>> As the member fe_len defined in struct ext4_free_extent is expressed as
>> number of clusters, the variable "size" computation is wrong, we need to
>> first translate it to block number, then to bytes.
>>
>> Signed-off-by: Xiaoguang Wang <[email protected]>
>> ---
>> fs/ext4/mballoc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 7f72f50..9a543b5 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>> size = 8 * 1024 * 1024;
>> } else {
>> start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
>> - size = ac->ac_o_ex.fe_len << bsbits;
>> + size = EXT4_C2B(EXT4_SB(ac->ac_sb),
>> + ac->ac_o_ex.fe_len) << bsbits;
>
> There is a patch out there up for more discussion which rewrites a
> whole bunch of code in ext4_mb_normalize_request(). But more
> importantly this fix, while correct is not going to change anything
> since this condition will never be run.
Yeah, I have read your patch and related discussions, indeed this condition
would never be run, thanks for your explanation. But given that currently your
patch is not merged, still this code is wrong and should be fixed.
I'm new to ext4:) Whether should I continue to send a v2 version to fix this issue? thanks!
Regards,
Xiaoguang Wang
>
> Btw, I take back the correct part since it seems that there is a
> a possibility of overflow. This should be better.
>
> size = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
> ac->ac_o_ex.fe_len) << bsbits;
>
> Thanks!
> -Lukas
>
>> }
>> size = size >> bsbits;
>> start = start_off >> bsbits;
>>
> .
>
On Fri, 11 Jul 2014, Xiaoguang Wang wrote:
> Date: Fri, 11 Jul 2014 11:41:08 +0800
> From: Xiaoguang Wang <[email protected]>
> To: Luk?? Czerner <[email protected]>
> Cc: [email protected], [email protected], [email protected]
> Subject: Re: [PATCH] ext4: fix wrong size computation in
> ext4_mb_normalize_request()
>
> Hi,
>
> On 07/10/2014 05:25 PM, Luk?? Czerner wrote:
> > On Thu, 10 Jul 2014, Xiaoguang Wang wrote:
> >
> >> Date: Thu, 10 Jul 2014 13:40:36 +0800
> >> From: Xiaoguang Wang <[email protected]>
> >> To: [email protected]
> >> Cc: [email protected], [email protected],
> >> Xiaoguang Wang <[email protected]>
> >> Subject: [PATCH] ext4: fix wrong size computation in
> >> ext4_mb_normalize_request()
> >>
> >> As the member fe_len defined in struct ext4_free_extent is expressed as
> >> number of clusters, the variable "size" computation is wrong, we need to
> >> first translate it to block number, then to bytes.
> >>
> >> Signed-off-by: Xiaoguang Wang <[email protected]>
> >> ---
> >> fs/ext4/mballoc.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> >> index 7f72f50..9a543b5 100644
> >> --- a/fs/ext4/mballoc.c
> >> +++ b/fs/ext4/mballoc.c
> >> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
> >> size = 8 * 1024 * 1024;
> >> } else {
> >> start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
> >> - size = ac->ac_o_ex.fe_len << bsbits;
> >> + size = EXT4_C2B(EXT4_SB(ac->ac_sb),
> >> + ac->ac_o_ex.fe_len) << bsbits;
> >
> > There is a patch out there up for more discussion which rewrites a
> > whole bunch of code in ext4_mb_normalize_request(). But more
> > importantly this fix, while correct is not going to change anything
> > since this condition will never be run.
>
> Yeah, I have read your patch and related discussions, indeed this condition
> would never be run, thanks for your explanation. But given that currently your
> patch is not merged, still this code is wrong and should be fixed.
> I'm new to ext4:) Whether should I continue to send a v2 version to fix this issue? thanks!
Please do, it's still worth fixing.
Thanks!
-Lukas
>
> Regards,
> Xiaoguang Wang
> >
> > Btw, I take back the correct part since it seems that there is a
> > a possibility of overflow. This should be better.
> >
> > size = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
> > ac->ac_o_ex.fe_len) << bsbits;
> >
> > Thanks!
> > -Lukas
> >
> >> }
> >> size = size >> bsbits;
> >> start = start_off >> bsbits;
> >>
> > .
> >
>
>
As the member fe_len defined in struct ext4_free_extent is expressed as
number of clusters, the variable "size" computation is wrong, we need to
first translate fe_len to block number, then to bytes.
Signed-off-by: Xiaoguang Wang <[email protected]>
---
fs/ext4/mballoc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 7f72f50..975a35e 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
size = 8 * 1024 * 1024;
} else {
start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
- size = ac->ac_o_ex.fe_len << bsbits;
+ size = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
+ ac->ac_o_ex.fe_len) << bsbits;
}
size = size >> bsbits;
start = start_off >> bsbits;
--
1.8.2.1
On Fri, 11 Jul 2014, Xiaoguang Wang wrote:
> Date: Fri, 11 Jul 2014 16:34:20 +0800
> From: Xiaoguang Wang <[email protected]>
> To: [email protected]
> Cc: [email protected], [email protected],
> Xiaoguang Wang <[email protected]>
> Subject: [PATCH v2] ext4: fix wrong size computation in
> ext4_mb_normalize_request()
>
> As the member fe_len defined in struct ext4_free_extent is expressed as
> number of clusters, the variable "size" computation is wrong, we need to
> first translate fe_len to block number, then to bytes.
>
> Signed-off-by: Xiaoguang Wang <[email protected]>
> ---
> fs/ext4/mballoc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 7f72f50..975a35e 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -3076,7 +3076,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
> size = 8 * 1024 * 1024;
> } else {
> start_off = (loff_t)ac->ac_o_ex.fe_logical << bsbits;
> - size = ac->ac_o_ex.fe_len << bsbits;
> + size = (loff_t)EXT4_C2B(EXT4_SB(ac->ac_sb),
> + ac->ac_o_ex.fe_len) << bsbits;
The indentation is wrong, otherwise it looks good.
Thanks!
Reviewed-by: Lukas Czerner <[email protected]>
> }
> size = size >> bsbits;
> start = start_off >> bsbits;
>
On Fri, Jul 11, 2014 at 04:34:20PM +0800, Xiaoguang Wang wrote:
> As the member fe_len defined in struct ext4_free_extent is expressed as
> number of clusters, the variable "size" computation is wrong, we need to
> first translate fe_len to block number, then to bytes.
>
> Signed-off-by: Xiaoguang Wang <[email protected]>
Thanks, applied.
- Ted