2006-01-13 07:33:07

by Pekka Enberg

Subject: [PATCH] reiserfs: use __GFP_NOFAIL instead of yield and retry loop for allocation

From: Pekka Enberg <[email protected]>

This patch replaces yield and retry loop with __GFP_NOFAIL in
alloc_journal_list(). Compile-tested only.

Signed-off-by: Pekka Enberg <[email protected]>
---

fs/reiserfs/journal.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

Index: 2.6/fs/reiserfs/journal.c
===================================================================
--- 2.6.orig/fs/reiserfs/journal.c
+++ 2.6/fs/reiserfs/journal.c
@@ -2446,12 +2446,8 @@ static int journal_read(struct super_blo
static struct reiserfs_journal_list *alloc_journal_list(struct super_block *s)
{
struct reiserfs_journal_list *jl;
- retry:
- jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
- if (!jl) {
- yield();
- goto retry;
- }
+ jl = kzalloc(sizeof(struct reiserfs_journal_list),
+ GFP_NOFS | __GFP_NOFAIL);
INIT_LIST_HEAD(&jl->j_list);
INIT_LIST_HEAD(&jl->j_working_list);
INIT_LIST_HEAD(&jl->j_tail_bh_list);

2006-01-13 07:43:04

by Andrew Morton

Subject: Re: [PATCH] reiserfs: use __GFP_NOFAIL instead of yield and retry loop for allocation

Pekka J Enberg <[email protected]> wrote:
>
> - retry:
> - jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> - if (!jl) {
> - yield();
> - goto retry;
> - }
> + jl = kzalloc(sizeof(struct reiserfs_journal_list),
> + GFP_NOFS | __GFP_NOFAIL);

yup, that's what __GFP_NOFAIL is for: to consolidate and identify all those
places which want to lock up when we're short of memory... They all need
fixing, really.

2006-01-13 07:46:14

by Pekka Enberg

Subject: Re: [PATCH] reiserfs: use __GFP_NOFAIL instead of yield and retry loop for allocation

Hi,

Pekka J Enberg <[email protected]> wrote:
> >
> > - retry:
> > - jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> > - if (!jl) {
> > - yield();
> > - goto retry;
> > - }
> > + jl = kzalloc(sizeof(struct reiserfs_journal_list),
> > + GFP_NOFS | __GFP_NOFAIL);

On Thu, 12 Jan 2006, Andrew Morton wrote:
> yup, that's what __GFP_NOFAIL is for: to consolidate and identify all those
> places which want to lock up when we're short of memory... They all need
> fixing, really.

Out of curiosity, are there any potential problems with combining GFP_NOFS
and __GFP_NOFAIL? Can we really guarantee to give out memory if we're not
allowed to page out?

Pekka

2006-01-13 07:56:11

by Andrew Morton

Subject: Re: [PATCH] reiserfs: use __GFP_NOFAIL instead of yield and retry loop for allocation

Pekka J Enberg <[email protected]> wrote:
>
> Hi,
>
> Pekka J Enberg <[email protected]> wrote:
> > >
> > > - retry:
> > > - jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
> > > - if (!jl) {
> > > - yield();
> > > - goto retry;
> > > - }
> > > + jl = kzalloc(sizeof(struct reiserfs_journal_list),
> > > + GFP_NOFS | __GFP_NOFAIL);
>
> On Thu, 12 Jan 2006, Andrew Morton wrote:
> > yup, that's what __GFP_NOFAIL is for: to consolidate and identify all those
> > places which want to lock up when we're short of memory... They all need
> > fixing, really.
>
> Out of curiosity, are there any potential problems with combining GFP_NOFS
> and __GFP_NOFAIL? Can we really guarantee to give out memory if we're not
> allowed to page out?
>

GFP_NOFS increases the risk (relative to GFP_KERNEL) because page reclaim
can do less things than GFP_KERNEL to free memory.

GFP_NOFS allocations can still perform swapspace writes, however. GFP_NOIO
cannot even do that.

2006-01-13 21:44:12

by Hans Reiser

Subject: Re: [PATCH] reiserfs: use __GFP_NOFAIL instead of yield and retry loop for allocation

Do you guys think you could write some nice long comments on these flags
regarding what they mean and the policies for using them?

I gotta tell you, lots of people end up just guessing as best as they can.

Hans

Andrew Morton wrote:

>Pekka J Enberg <[email protected]> wrote:
>
>
>>Hi,
>>
>>Pekka J Enberg <[email protected]> wrote:
>>
>>
>>>> - retry:
>>>> - jl = kzalloc(sizeof(struct reiserfs_journal_list), GFP_NOFS);
>>>> - if (!jl) {
>>>> - yield();
>>>> - goto retry;
>>>> - }
>>>> + jl = kzalloc(sizeof(struct reiserfs_journal_list),
>>>> + GFP_NOFS | __GFP_NOFAIL);
>>>>
>>>>
>>On Thu, 12 Jan 2006, Andrew Morton wrote:
>>
>>
>>>yup, that's what __GFP_NOFAIL is for: to consolidate and identify all those
>>>places which want to lock up when we're short of memory... They all need
>>>fixing, really.
>>>
>>>
>>Out of curiosity, are there any potential problems with combining GFP_NOFS
>>and __GFP_NOFAIL? Can we really guarantee to give out memory if we're not
>>allowed to page out?
>>
>>
>>
>
>GFP_NOFS increases the risk (relative to GFP_KERNEL) because page reclaim
>can do less things than GFP_KERNEL to free memory.
>
>GFP_NOFS allocations can still perform swapspace writes, however. GFP_NOIO
>cannot even do that.
>
>
>
>