| Message ID | 20110414064441.GA3499@darkstar |
|---|---|
| State | Accepted, archived |
| Headers | show |
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
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 >>
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
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>
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 -- 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
--- 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)
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(+) -- 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