Patchwork jffs2: Do not assume erase will fail

login
register
mail settings
Submitter Joakim Tjernlund
Date Oct. 7, 2010, 4:29 p.m.
Message ID <1286468986-24627-1-git-send-email-Joakim.Tjernlund@transmode.se>
Download mbox | patch
Permalink /patch/67074/
State New
Headers show

Comments

Joakim Tjernlund - Oct. 7, 2010, 4:29 p.m.
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(-)
Artem Bityutskiy - Oct. 12, 2010, 8:48 a.m.
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.
David Woodhouse - Oct. 25, 2010, 12:11 a.m.
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.
Joakim Tjernlund - Oct. 25, 2010, 6:49 a.m.
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.
>
Artem Bityutskiy - Oct. 25, 2010, 8:01 a.m.
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.
David Woodhouse - Oct. 25, 2010, 9:50 a.m.
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.
Joakim Tjernlund - Oct. 25, 2010, 10:01 a.m.
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
David Woodhouse - Oct. 25, 2010, 10:56 a.m.
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.
Joakim Tjernlund - Oct. 25, 2010, 11 a.m.
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 :)
David Woodhouse - Oct. 25, 2010, 11:01 a.m.
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
Joakim Tjernlund - Oct. 25, 2010, 11:03 a.m.
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

Patch

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;