[PATCH] jffs2: Move erasing from write_super to GC.

[Joakim Tjernlund]

>
> >
> > David Woodhouse <dwmw2 at 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?