[PATCH] jffs2: Do not assume erase will fail

[PATCH] jffs2: Do not assume erase will fail

[Joakim Tjernlund]

Test if it did and then abort.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 fs/jffs2/nodemgmt.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 694aa5b..49ee5de 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -260,9 +260,9 @@ static int jffs2_find_nextblock(struct jffs2_sb_info *c)
 		spin_lock(&c->erase_completion_lock);
 
 		/* An erase may have failed, decreasing the
-		   amount of free space available. So we must
-		   restart from the beginning */
-		return -EAGAIN;
+		   amount of free space available. */
+		if (list_empty(&c->free_list))
+			return -EAGAIN; /* restart from the beginning */
 	}
 
 	next = c->free_list.next;
-- 
1.7.2.2

Re: [PATCH] jffs2: Do not assume erase will fail

[Artem Bityutskiy]

On Thu, 2010-10-07 at 18:29 +0200, Joakim Tjernlund wrote:
> Test if it did and then abort.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  fs/jffs2/nodemgmt.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

Pushed to l2-mtd-2.6.git, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] jffs2: Do not assume erase will fail

[David Woodhouse]

On Thu, 2010-10-07 at 18:29 +0200, Joakim Tjernlund wrote:
> 
> Test if it did and then abort.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  fs/jffs2/nodemgmt.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
> index 694aa5b..49ee5de 100644
> --- a/fs/jffs2/nodemgmt.c
> +++ b/fs/jffs2/nodemgmt.c
> @@ -260,9 +260,9 @@ static int jffs2_find_nextblock(struct jffs2_sb_info *c)
>                 spin_lock(&c->erase_completion_lock);
>  
>                 /* An erase may have failed, decreasing the
> -                  amount of free space available. So we must
> -                  restart from the beginning */
> -               return -EAGAIN;
> +                  amount of free space available. */
> +               if (list_empty(&c->free_list))
> +                       return -EAGAIN; /* restart from the beginning */ 

Hm, but there could have been more than one erase pending (or in
progress). And if one fails and another succeeds then you could have a
non-empty free_list but you could *also* now have run short of
free/freeable space so that a userspace write should now receive
-ENOSPC.

Is this really a performance issue? It should just come straight back if
the conditions are still met, surely?

And if we're hitting this code path that often, we should look at
erasing more aggressively so that we *don't* have to erase stuff on
demand.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] jffs2: Do not assume erase will fail

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 02:11:25:
>
> On Thu, 2010-10-07 at 18:29 +0200, Joakim Tjernlund wrote:
> >
> > Test if it did and then abort.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  fs/jffs2/nodemgmt.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
> > index 694aa5b..49ee5de 100644
> > --- a/fs/jffs2/nodemgmt.c
> > +++ b/fs/jffs2/nodemgmt.c
> > @@ -260,9 +260,9 @@ static int jffs2_find_nextblock(struct jffs2_sb_info *c)
> >                 spin_lock(&c->erase_completion_lock);
> >
> >                 /* An erase may have failed, decreasing the
> > -                  amount of free space available. So we must
> > -                  restart from the beginning */
> > -               return -EAGAIN;
> > +                  amount of free space available. */
> > +               if (list_empty(&c->free_list))
> > +                       return -EAGAIN; /* restart from the beginning */
>
> Hm, but there could have been more than one erase pending (or in
> progress). And if one fails and another succeeds then you could have a
> non-empty free_list but you could *also* now have run short of
> free/freeable space so that a userspace write should now receive
> -ENOSPC.

I don't see how my patch changes that, if !list_empty(&c->free_list)
then you have at least one free block so you should not run into -ENOSPC

>
> Is this really a performance issue? It should just come straight back if
> the conditions are still met, surely?

Not a perf issue. It is something I noticed while looking for the
 jffs2: Fix serious write stall due to erase


>
> And if we're hitting this code path that often, we should look at
> erasing more aggressively so that we *don't* have to erase stuff on
> demand.
>

Re: [PATCH] jffs2: Do not assume erase will fail

[Artem Bityutskiy]

On Mon, 2010-10-25 at 01:11 +0100, David Woodhouse wrote:
> On Thu, 2010-10-07 at 18:29 +0200, Joakim Tjernlund wrote:
> > 
> > Test if it did and then abort.
> > 
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  fs/jffs2/nodemgmt.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
> > index 694aa5b..49ee5de 100644
> > --- a/fs/jffs2/nodemgmt.c
> > +++ b/fs/jffs2/nodemgmt.c
> > @@ -260,9 +260,9 @@ static int jffs2_find_nextblock(struct jffs2_sb_info *c)
> >                 spin_lock(&c->erase_completion_lock);
> >  
> >                 /* An erase may have failed, decreasing the
> > -                  amount of free space available. So we must
> > -                  restart from the beginning */
> > -               return -EAGAIN;
> > +                  amount of free space available. */
> > +               if (list_empty(&c->free_list))
> > +                       return -EAGAIN; /* restart from the beginning */ 
> 
> Hm, but there could have been more than one erase pending (or in
> progress). And if one fails and another succeeds then you could have a
> non-empty free_list but you could *also* now have run short of
> free/freeable space so that a userspace write should now receive
> -ENOSPC.
> 
> Is this really a performance issue? It should just come straight back if
> the conditions are still met, surely?
> 
> And if we're hitting this code path that often, we should look at
> erasing more aggressively so that we *don't* have to erase stuff on
> demand.

David,

there are 2 patches which you seem to miss. I've re-based my l2 tree
against your today's mtd tree, and applied them on top. I've also
preserved this patch. Please, look at those.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] jffs2: Do not assume erase will fail

[David Woodhouse]

On Mon, 2010-10-25 at 08:49 +0200, Joakim Tjernlund wrote:
> David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 02:11:25:
> >
> > On Thu, 2010-10-07 at 18:29 +0200, Joakim Tjernlund wrote:
> > >
> > > Test if it did and then abort.
> > >
> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > > ---
> > >  fs/jffs2/nodemgmt.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
> > > index 694aa5b..49ee5de 100644
> > > --- a/fs/jffs2/nodemgmt.c
> > > +++ b/fs/jffs2/nodemgmt.c
> > > @@ -260,9 +260,9 @@ static int jffs2_find_nextblock(struct jffs2_sb_info *c)
> > >                 spin_lock(&c->erase_completion_lock);
> > >
> > >                 /* An erase may have failed, decreasing the
> > > -                  amount of free space available. So we must
> > > -                  restart from the beginning */
> > > -               return -EAGAIN;
> > > +                  amount of free space available. */
> > > +               if (list_empty(&c->free_list))
> > > +                       return -EAGAIN; /* restart from the beginning */
> >
> > Hm, but there could have been more than one erase pending (or in
> > progress). And if one fails and another succeeds then you could have a
> > non-empty free_list but you could *also* now have run short of
> > free/freeable space so that a userspace write should now receive
> > -ENOSPC.
> 
> I don't see how my patch changes that, if !list_empty(&c->free_list)
> then you have at least one free block so you should not run into -ENOSPC

Not for jffs2_reserve_space_gc(), you're right. But for non-GC
allocations it's different -- jffs2_reserve_space() will only allow an
allocation if you have more than a certain amount of freeable space,
according to the type of operation. And that amount of freeable space
can be reduced if an erase fails.

-- 
dwmw2

Re: [PATCH] jffs2: Do not assume erase will fail

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 11:50:01:
>
> On Mon, 2010-10-25 at 08:49 +0200, Joakim Tjernlund wrote:
> > David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 02:11:25:
> > >
> > > On Thu, 2010-10-07 at 18:29 +0200, Joakim Tjernlund wrote:
> > > >
> > > > Test if it did and then abort.
> > > >
> > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > > > ---
> > > >  fs/jffs2/nodemgmt.c |    6 +++---
> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
> > > > index 694aa5b..49ee5de 100644
> > > > --- a/fs/jffs2/nodemgmt.c
> > > > +++ b/fs/jffs2/nodemgmt.c
> > > > @@ -260,9 +260,9 @@ static int jffs2_find_nextblock(struct jffs2_sb_info *c)
> > > >                 spin_lock(&c->erase_completion_lock);
> > > >
> > > >                 /* An erase may have failed, decreasing the
> > > > -                  amount of free space available. So we must
> > > > -                  restart from the beginning */
> > > > -               return -EAGAIN;
> > > > +                  amount of free space available. */
> > > > +               if (list_empty(&c->free_list))
> > > > +                       return -EAGAIN; /* restart from the beginning */
> > >
> > > Hm, but there could have been more than one erase pending (or in
> > > progress). And if one fails and another succeeds then you could have a
> > > non-empty free_list but you could *also* now have run short of
> > > free/freeable space so that a userspace write should now receive
> > > -ENOSPC.
> >
> > I don't see how my patch changes that, if !list_empty(&c->free_list)
> > then you have at least one free block so you should not run into -ENOSPC
>
> Not for jffs2_reserve_space_gc(), you're right. But for non-GC
> allocations it's different -- jffs2_reserve_space() will only allow an
> allocation if you have more than a certain amount of freeable space,
> according to the type of operation. And that amount of freeable space
> can be reduced if an erase fails.

But jffs2_find_nextblock is only about finding ONE block, isn't it?

     Jocke

Re: [PATCH] jffs2: Do not assume erase will fail

[David Woodhouse]

On Mon, 2010-10-25 at 11:01 +0300, Artem Bityutskiy wrote:
> 
> there are 2 patches which you seem to miss. I've re-based my l2 tree
> against your today's mtd tree, and applied them on top. I've also
> preserved this patch. Please, look at those. 

The 'fix NAND pwrite in raw mode' patch is redundant -- the problem is
fixed by Jon Povey's patch in commit cdcf12b2, which looks almost
identical to the way I suggested it be fixed.

The callback from m25p80 was dropped by its author, and rightly so.

Thanks, again, for keeping track.

-- 
dwmw2

Re: [PATCH] jffs2: Do not assume erase will fail

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 12:56:48:
>
> On Mon, 2010-10-25 at 11:01 +0300, Artem Bityutskiy wrote:
> >
> > there are 2 patches which you seem to miss. I've re-based my l2 tree
> > against your today's mtd tree, and applied them on top. I've also
> > preserved this patch. Please, look at those.
>
> The 'fix NAND pwrite in raw mode' patch is redundant -- the problem is
> fixed by Jon Povey's patch in commit cdcf12b2, which looks almost
> identical to the way I suggested it be fixed.
>
> The callback from m25p80 was dropped by its author, and rightly so.
>
> Thanks, again, for keeping track.

But jffs2: Fix serious write stall due to erase
from me remains :)

Re: [PATCH] jffs2: Do not assume erase will fail

[David Woodhouse]

On Mon, 2010-10-25 at 13:00 +0200, Joakim Tjernlund wrote:
> David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 12:56:48:
> >
> > On Mon, 2010-10-25 at 11:01 +0300, Artem Bityutskiy wrote:
> > >
> > > there are 2 patches which you seem to miss. I've re-based my l2 tree
> > > against your today's mtd tree, and applied them on top. I've also
> > > preserved this patch. Please, look at those.
> >
> > The 'fix NAND pwrite in raw mode' patch is redundant -- the problem is
> > fixed by Jon Povey's patch in commit cdcf12b2, which looks almost
> > identical to the way I suggested it be fixed.
> >
> > The callback from m25p80 was dropped by its author, and rightly so.
> >
> > Thanks, again, for keeping track.
> 
> But jffs2: Fix serious write stall due to erase
> from me remains :)

http://git.infradead.org/mtd-2.6.git/commitdiff/81cfc9f1

-- 
dwmw2

Re: [PATCH] jffs2: Do not assume erase will fail

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 13:01:27:
>
> On Mon, 2010-10-25 at 13:00 +0200, Joakim Tjernlund wrote:
> > David Woodhouse <dwmw2@infradead.org> wrote on 2010/10/25 12:56:48:
> > >
> > > On Mon, 2010-10-25 at 11:01 +0300, Artem Bityutskiy wrote:
> > > >
> > > > there are 2 patches which you seem to miss. I've re-based my l2 tree
> > > > against your today's mtd tree, and applied them on top. I've also
> > > > preserved this patch. Please, look at those.
> > >
> > > The 'fix NAND pwrite in raw mode' patch is redundant -- the problem is
> > > fixed by Jon Povey's patch in commit cdcf12b2, which looks almost
> > > identical to the way I suggested it be fixed.
> > >
> > > The callback from m25p80 was dropped by its author, and rightly so.
> > >
> > > Thanks, again, for keeping track.
> >
> > But jffs2: Fix serious write stall due to erase
> > from me remains :)
>
> http://git.infradead.org/mtd-2.6.git/commitdiff/81cfc9f1

Oh, you had already taken it. Thanks

    Jocke