Skip to contentSkip to conversation

ext4: release page cache in ext4_mb_load_buddy error path

PATCH In review Yang Ruirui 3 participants 6 messages Apr 14, 2011 – Apr 16, 2011 ext4 file system

lore
Share & tools
lore

Yang Ruirui

author
#0 lore raw

PATCH 1/1 ext4: release page cache in ext4_mb_load_buddy error path

Add missing page_cache_release in the error path of ext4_mb_load_buddy

Signed-off-by Yang Ruirui <[email protected]>
---
 fs/ext4/mballoc.c |    2 ++
 1 file changed, 2 insertions(+)
fs/ext4/mballoc.c +2 -0
@@ -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)
#1 lore raw

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.

Quoted 14 lines · Yang Ruirui

Add missing page_cache_release in the error path of ext4_mb_load_buddy

Signed-off-by Yang Ruirui <[email protected]>

---
 fs/ext4/mballoc.c |    2 ++

 1 file changed, 2 insertions(+)
fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800 renamed from fs/ext4/mballoc.c    2011-04-14 14:04:48.000000000 +0800 +0 -0
@@ -1273,6 +1273,8 @@ repeat_load_buddy:
        return 0;

 err:
+       if (page)

Quoted 9 lines

+               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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw

Hi,

Thanks for your comment. BTW, why linux-kernel mainling list is removed from cc? Is it ext4 list prefer or something else?

Quoted 10 lines · Amir Goldstein

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.

Quoted 27 lines

Amir.

Quoted 23 lines · Yang Ruirui

Add missing page_cache_release in the error path of ext4_mb_load_buddy

Signed-off-by Yang Ruirui<[email protected]>

---
�fs/ext4/mballoc.c | � �2 ++
�1 file changed, 2 insertions(+)

fs/ext4/mballoc.c 2011-04-14 14:33:28.702958245 +0800 renamed from fs/ext4/mballoc.c � �2011-04-14 14:04:48.000000000 +0800 +0 -0
@@ -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 [email protected]
More majordomo info at �http://vger.kernel.org/majordomo-info.html

Signature · 6 lines
Thanks
Yang Ruirui
--
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
#3 lore raw
Quoted 5 lines · Yang Rui Rui

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.

Quoted 12 lines · Amir Goldstein
Quoted 10 lines

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.

Quoted 6 lines
Quoted 1 line

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.

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

Quoted 19 lines
Quoted 19 lines

Amir.

Quoted 14 lines · Yang Ruirui

Add missing page_cache_release in the error path of ext4_mb_load_buddy

Signed-off-by Yang Ruirui<[email protected]>

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

(quoted diff) +0 -0
@@ -1273,6 +1273,8 @@ repeat_load_buddy:
 � � � �return 0;
Quoted 17 lines
Quoted 11 lines
Quoted 11 lines

�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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw
Quoted 36 lines · Amir Goldstein
Quoted 5 lines · Yang Rui Rui

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.

Quoted 12 lines · Amir Goldstein
Quoted 10 lines

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.

Quoted 6 lines
Quoted 1 line

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.

Quoted 3 lines

Then rewrite the function as more small size functions for example the get page part.

Quoted 5 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

Amir.

Quoted 4 lines · Yang Ruirui

Add missing page_cache_release in the error path of ext4_mb_load_buddy

Signed-off-by Yang Ruirui<[email protected]>
Signature · 2 lines
Thanks
Yang Ruirui