| Message ID | 1286468986-24627-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 <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. >
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.
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.
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
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.
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 :)
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
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
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;
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(-)