Commit Message

>> David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/14 12:35:04:> >> > On Fri, 2010-05-14 at 12:10 +0200, Joakim Tjernlund wrote:> > > The only callers of jffs2_erase_pending_blocks() now call it with a> > > > 'count' argument of 1. So perhaps it's now misnamed and the 's' and the> > > > extra argument should be dropped?> > >> > > I didn't want to change to much and who knows, maybe someone wants> > > to erase more than one block in the future. Removing the> > > count could be an add on patch once this patch has proven itself.> >> > Yeah, that makes some sense.> >> > > > I don't much like the calculation you've added to the end of that> > > > function either, which really ought to be under locks (even though right> > > > now I suspect it doesn't hurt). Why recalculate that at all, though --> > >> > > Why does a simple list test need locks?> >> > Because it's not just about the test itself. It's also about the memory> > barriers. Some other CPU could have changed the list (under locks) but> > unless you have the memory barrier which is implicit in the spinlock,> > you might see old data.>> old data doesn't matter here I think.>> >> > > > why not keep a 'ret' variable which defaults to 0 but is set to 1 just> > > > before the 'goto done' which is the only way out of the function where> > > > the return value should be non-zero anyway?> > >> > > That would not be the same, would it? One wants to know if the lists> > > are empty AFTER erasing count blocks.> >> > Hm, does one? There's precisely one place we use this return value, in> > the GC thread. Can you explain the logic of what you were doing there?>> Sure, return 1 if there are more blocks left in the list after> erasing count. That way the caller knows if there are any block left> to erase.>> > It looks like you really wanted it to return a flag saying whether it> > actually _did_ anything or not. And if it did, that's your work for this> > GC wakeup and you don't call jffs2_garbage_collect_pass(). Why are you> > returning a value which tells whether there's more work to do?>> hmm, I guess the simpler method like you suggested would work too.> Details are a bit fuzzy now.>> >> > > I guess I could move the list empty> > > check before goto done, but that would not really change anything.> >> > Ah, yes. Instead of setting ret=1 at the 'goto done', you'd actually do> > the test somewhere there too, before dropping the locks. Assuming that> > this really is the return value you need to return, rather than a simple> > 'work_done' flag.
How about this then? I have changed jffs2_erase_pending_blocks() to use
the simpler work_done flag:
From a2173115bbb4544ff0652232a89426a55d32db61 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Mon, 15 Feb 2010 08:40:33 +0100
Subject: [PATCH] jffs2: Move erasing from write_super to GC.
Erasing blocks is a form of GC and therefore it should live in the
GC task. By moving it there two problems will be solved:
1) unmounting will not hang until all pending blocks has
been erased.
2) Erasing can be paused by sending a SIGSTOP to the GC thread which
allowes for time critical tasks to work in peace.
Since erasing now is in the GC thread, erases should trigger
the GC task instead.
wbuf.c still wants to flush its buffer via write_super so
invent jffs2_dirty_trigger() and use that in wbuf.
Remove surplus call to jffs2_erase_pending_trigger() in erase.c
and remove jffs2_garbage_collect_trigger() from write_super as
of now write_super() should only commit dirty data to disk.
Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
fs/jffs2/background.c | 4 +++-
fs/jffs2/erase.c | 7 ++++---
fs/jffs2/nodelist.h | 2 +-
fs/jffs2/nodemgmt.c | 4 ++++
fs/jffs2/os-linux.h | 9 +++++++--
fs/jffs2/super.c | 2 --
fs/jffs2/wbuf.c | 2 +-
7 files changed, 20 insertions(+), 10 deletions(-)
--
1.6.4.4

Comments

>> >> > David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/14 12:35:04:> > >> > > On Fri, 2010-05-14 at 12:10 +0200, Joakim Tjernlund wrote:> > > > The only callers of jffs2_erase_pending_blocks() now call it with a> > > > > 'count' argument of 1. So perhaps it's now misnamed and the 's' and the> > > > > extra argument should be dropped?> > > >> > > > I didn't want to change to much and who knows, maybe someone wants> > > > to erase more than one block in the future. Removing the> > > > count could be an add on patch once this patch has proven itself.> > >> > > Yeah, that makes some sense.> > >> > > > > I don't much like the calculation you've added to the end of that> > > > > function either, which really ought to be under locks (even though right> > > > > now I suspect it doesn't hurt). Why recalculate that at all, though --> > > >> > > > Why does a simple list test need locks?> > >> > > Because it's not just about the test itself. It's also about the memory> > > barriers. Some other CPU could have changed the list (under locks) but> > > unless you have the memory barrier which is implicit in the spinlock,> > > you might see old data.> >> > old data doesn't matter here I think.> >> > >> > > > > why not keep a 'ret' variable which defaults to 0 but is set to 1 just> > > > > before the 'goto done' which is the only way out of the function where> > > > > the return value should be non-zero anyway?> > > >> > > > That would not be the same, would it? One wants to know if the lists> > > > are empty AFTER erasing count blocks.> > >> > > Hm, does one? There's precisely one place we use this return value, in> > > the GC thread. Can you explain the logic of what you were doing there?> >> > Sure, return 1 if there are more blocks left in the list after> > erasing count. That way the caller knows if there are any block left> > to erase.> >> > > It looks like you really wanted it to return a flag saying whether it> > > actually _did_ anything or not. And if it did, that's your work for this> > > GC wakeup and you don't call jffs2_garbage_collect_pass(). Why are you> > > returning a value which tells whether there's more work to do?> >> > hmm, I guess the simpler method like you suggested would work too.> > Details are a bit fuzzy now.> >> > >> > > > I guess I could move the list empty> > > > check before goto done, but that would not really change anything.> > >> > > Ah, yes. Instead of setting ret=1 at the 'goto done', you'd actually do> > > the test somewhere there too, before dropping the locks. Assuming that> > > this really is the return value you need to return, rather than a simple> > > 'work_done' flag.>> How about this then? I have changed jffs2_erase_pending_blocks() to use> the simpler work_done flag:
Ping?