* [PATCH] ext4: release page cache in ext4_mb_load_buddy error path
@ 2011-04-14 6:44 Yang Ruirui
2011-04-14 7:01 ` Amir Goldstein
2011-04-16 23:07 ` Ted Ts'o
0 siblings, 2 replies; 6+ messages in thread
From: Yang Ruirui @ 2011-04-14 6:44 UTC (permalink / raw)
To: tytso, linux-ext4, linux-kernel
Add missing page_cache_release in the error path of ext4_mb_load_buddy
Signed-off-by: Yang Ruirui <ruirui.r.yang@tieto.com>
---
fs/ext4/mballoc.c | 2 ++
1 file changed, 2 insertions(+)
--- linux-2.6.orig/fs/ext4/mballoc.c 2011-04-14 14:04:48.000000000 +0800
+++ linux-2.6/fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800
@@ -1273,6 +1273,8 @@ repeat_load_buddy:
return 0;
err:
+ if (page)
+ page_cache_release(page);
if (e4b->bd_bitmap_page)
page_cache_release(e4b->bd_bitmap_page);
if (e4b->bd_buddy_page)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: release page cache in ext4_mb_load_buddy error path
2011-04-14 6:44 [PATCH] ext4: release page cache in ext4_mb_load_buddy error path Yang Ruirui
@ 2011-04-14 7:01 ` Amir Goldstein
2011-04-14 7:29 ` Yang Rui Rui
2011-04-16 23:07 ` Ted Ts'o
1 sibling, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2011-04-14 7:01 UTC (permalink / raw)
To: Yang Ruirui; +Cc: tytso, Ext4 Developers List
Hi Yang ,
The patch looks correct, but in my opinion a nicer fix would be to set
e4b->bd_bitmap_page = page;
or
e4b->bd_buddy_page = page;
right after assigning a new value to the temp variable 'page'.
and keeping the cleanup code in the error path as it is.
It's a matter of taste and code readability.
Amir.
On Thu, Apr 14, 2011 at 9:44 AM, Yang Ruirui <ruirui.r.yang@tieto.com> wrote:
> Add missing page_cache_release in the error path of ext4_mb_load_buddy
>
> Signed-off-by: Yang Ruirui <ruirui.r.yang@tieto.com>
> ---
> fs/ext4/mballoc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> --- linux-2.6.orig/fs/ext4/mballoc.c 2011-04-14 14:04:48.000000000 +0800
> +++ linux-2.6/fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800
> @@ -1273,6 +1273,8 @@ repeat_load_buddy:
> return 0;
>
> err:
> + if (page)
> + page_cache_release(page);
> if (e4b->bd_bitmap_page)
> page_cache_release(e4b->bd_bitmap_page);
> if (e4b->bd_buddy_page)
> --
> 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
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: release page cache in ext4_mb_load_buddy error path
2011-04-14 7:01 ` Amir Goldstein
@ 2011-04-14 7:29 ` Yang Rui Rui
2011-04-14 7:46 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Yang Rui Rui @ 2011-04-14 7:29 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Yang Ruirui R, tytso, Ext4 Developers List
Hi,
Thanks for your comment. BTW, why linux-kernel mainling list is removed from cc?
Is it ext4 list prefer or something else?
On 04/14/2011 03:01 PM, Amir Goldstein wrote:
> Hi Yang ,
>
> The patch looks correct, but in my opinion a nicer fix would be to set
> e4b->bd_bitmap_page = page;
> or
> e4b->bd_buddy_page = page;
> right after assigning a new value to the temp variable 'page'.
> and keeping the cleanup code in the error path as it is.
>
> It's a matter of taste and code readability.
I agree with you for common case, but this function is not so readable already.
Two many if conditions and indent. I would prefer just fix this problem as this patch.
Then rewrite the function as more small size functions for example the get page part.
>
> Amir.
>
> On Thu, Apr 14, 2011 at 9:44 AM, Yang Ruirui<ruirui.r.yang@tieto.com> wrote:
>> Add missing page_cache_release in the error path of ext4_mb_load_buddy
>>
>> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
>> ---
>> �fs/ext4/mballoc.c | � �2 ++
>> �1 file changed, 2 insertions(+)
>>
>> --- linux-2.6.orig/fs/ext4/mballoc.c � �2011-04-14 14:04:48.000000000 +0800
>> +++ linux-2.6/fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800
>> @@ -1273,6 +1273,8 @@ repeat_load_buddy:
>> � � � �return 0;
>>
>> �err:
>> + � � � if (page)
>> + � � � � � � � page_cache_release(page);
>> � � � �if (e4b->bd_bitmap_page)
>> � � � � � � � �page_cache_release(e4b->bd_bitmap_page);
>> � � � �if (e4b->bd_buddy_page)
>> --
>> 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
>>
--
Thanks
Yang Ruirui
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: release page cache in ext4_mb_load_buddy error path
2011-04-14 7:29 ` Yang Rui Rui
@ 2011-04-14 7:46 ` Amir Goldstein
2011-04-14 8:28 ` Yang Rui Rui
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2011-04-14 7:46 UTC (permalink / raw)
To: Yang Rui Rui; +Cc: Yang Ruirui R, tytso, Ext4 Developers List
On Thu, Apr 14, 2011 at 10:29 AM, Yang Rui Rui <ruirui.r.yang@tieto.com> wrote:
> Hi,
>
> Thanks for your comment. BTW, why linux-kernel mainling list is removed from
> cc?
> Is it ext4 list prefer or something else?
ext4 list is the place to post fixes for ext4.
LKML need not be bothered with these specific patches.
>
> On 04/14/2011 03:01 PM, Amir Goldstein wrote:
>>
>> Hi Yang ,
>>
>> The patch looks correct, but in my opinion a nicer fix would be to set
>> e4b->bd_bitmap_page = page;
>> or
>> e4b->bd_buddy_page = page;
>> right after assigning a new value to the temp variable 'page'.
>> and keeping the cleanup code in the error path as it is.
>>
>> It's a matter of taste and code readability.
>
> I agree with you for common case, but this function is not so readable
> already.
> Two many if conditions and indent. I would prefer just fix this problem as
> this patch.
As I said it's a matter of taste so I'm not going to argue.
One argument in favor of your patch is that it adds fewer lines.
> Then rewrite the function as more small size functions for example the get
> page part.
>
If you are going to audit code or rework functions in mballoc.c,
I suggest that you first take look at my patches to remove alloc_semp.
Those already involve some rework in mballoc.c.
See online version at:
https://github.com/amir73il/ext4-snapshots/tree/alloc_semp
>>
>> Amir.
>>
>> On Thu, Apr 14, 2011 at 9:44 AM, Yang Ruirui<ruirui.r.yang@tieto.com>
>> wrote:
>>>
>>> Add missing page_cache_release in the error path of ext4_mb_load_buddy
>>>
>>> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
>>> ---
>>> �fs/ext4/mballoc.c | � �2 ++
>>> �1 file changed, 2 insertions(+)
>>>
>>> --- linux-2.6.orig/fs/ext4/mballoc.c � �2011-04-14 14:04:48.000000000
>>> +0800
>>> +++ linux-2.6/fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800
>>> @@ -1273,6 +1273,8 @@ repeat_load_buddy:
>>> � � � �return 0;
>>>
>>> �err:
>>> + � � � if (page)
>>> + � � � � � � � page_cache_release(page);
>>> � � � �if (e4b->bd_bitmap_page)
>>> � � � � � � � �page_cache_release(e4b->bd_bitmap_page);
>>> � � � �if (e4b->bd_buddy_page)
>>> --
>>> 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
>>>
>
>
> --
> Thanks
> Yang Ruirui
>
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: release page cache in ext4_mb_load_buddy error path
2011-04-14 7:46 ` Amir Goldstein
@ 2011-04-14 8:28 ` Yang Rui Rui
0 siblings, 0 replies; 6+ messages in thread
From: Yang Rui Rui @ 2011-04-14 8:28 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Yang Ruirui R, tytso, Ext4 Developers List
On 04/14/2011 03:46 PM, Amir Goldstein wrote:
> On Thu, Apr 14, 2011 at 10:29 AM, Yang Rui Rui<ruirui.r.yang@tieto.com> wrote:
>> Hi,
>>
>> Thanks for your comment. BTW, why linux-kernel mainling list is removed from
>> cc?
>> Is it ext4 list prefer or something else?
>
> ext4 list is the place to post fixes for ext4.
> LKML need not be bothered with these specific patches.
>
>>
>> On 04/14/2011 03:01 PM, Amir Goldstein wrote:
>>>
>>> Hi Yang ,
>>>
>>> The patch looks correct, but in my opinion a nicer fix would be to set
>>> e4b->bd_bitmap_page = page;
>>> or
>>> e4b->bd_buddy_page = page;
>>> right after assigning a new value to the temp variable 'page'.
>>> and keeping the cleanup code in the error path as it is.
>>>
>>> It's a matter of taste and code readability.
>>
>> I agree with you for common case, but this function is not so readable
>> already.
>> Two many if conditions and indent. I would prefer just fix this problem as
>> this patch.
>
> As I said it's a matter of taste so I'm not going to argue.
> One argument in favor of your patch is that it adds fewer lines.
>
>> Then rewrite the function as more small size functions for example the get
>> page part.
>>
>
> If you are going to audit code or rework functions in mballoc.c,
> I suggest that you first take look at my patches to remove alloc_semp.
> Those already involve some rework in mballoc.c.
> See online version at:
> https://github.com/amir73il/ext4-snapshots/tree/alloc_semp
Thanks, will take a look.
>
>>>
>>> Amir.
>>>
>>> On Thu, Apr 14, 2011 at 9:44 AM, Yang Ruirui<ruirui.r.yang@tieto.com>
>>> wrote:
>>>>
>>>> Add missing page_cache_release in the error path of ext4_mb_load_buddy
>>>>
>>>> Signed-off-by: Yang Ruirui<ruirui.r.yang@tieto.com>
--
Thanks
Yang Ruirui
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: release page cache in ext4_mb_load_buddy error path
2011-04-14 6:44 [PATCH] ext4: release page cache in ext4_mb_load_buddy error path Yang Ruirui
2011-04-14 7:01 ` Amir Goldstein
@ 2011-04-16 23:07 ` Ted Ts'o
1 sibling, 0 replies; 6+ messages in thread
From: Ted Ts'o @ 2011-04-16 23:07 UTC (permalink / raw)
To: Yang Ruirui; +Cc: linux-ext4, linux-kernel
On Thu, Apr 14, 2011 at 02:44:41PM +0800, Yang Ruirui wrote:
> Add missing page_cache_release in the error path of ext4_mb_load_buddy
>
> Signed-off-by: Yang Ruirui <ruirui.r.yang@tieto.com>
Thanks, added to the ext4 tree.
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-04-16 23:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-14 6:44 [PATCH] ext4: release page cache in ext4_mb_load_buddy error path Yang Ruirui
2011-04-14 7:01 ` Amir Goldstein
2011-04-14 7:29 ` Yang Rui Rui
2011-04-14 7:46 ` Amir Goldstein
2011-04-14 8:28 ` Yang Rui Rui
2011-04-16 23:07 ` Ted Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.