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

[David Woodhouse <dwmw2@infradead.org>]

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.

> > 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?
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? 

>  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.

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