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

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

[Joakim Tjernlund]

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

This is a resend, hopefully David will ha some time to spare
this time :)

 fs/jffs2/background.c |    4 +++-
 fs/jffs2/erase.c      |    6 +++---
 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, 19 insertions(+), 10 deletions(-)

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 3ff50da..6cc014c 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -146,7 +146,9 @@ static int jffs2_garbage_collect_thread(void *_c)
 		disallow_signal(SIGHUP);
 
 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+		if (jffs2_erase_pending_blocks(c, 1))
+			/* Nothing more to do ATM */;
+		else if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
 			printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
 			goto die;
 		}
diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..be6b2b8 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -103,7 +103,7 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
 	jffs2_erase_failed(c, jeb, bad_offset);
 }
 
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 {
 	struct jffs2_eraseblock *jeb;
 
@@ -157,6 +157,8 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 	mutex_unlock(&c->erase_free_sem);
  done:
 	D1(printk(KERN_DEBUG "jffs2_erase_pending_blocks completed\n"));
+	return !list_empty(&c->erase_complete_list) ||
+		!list_empty(&c->erase_pending_list);
 }
 
 static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
@@ -167,8 +169,6 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
 	list_move_tail(&jeb->list, &c->erase_complete_list);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
-	/* Ensure that kupdated calls us again to mark them clean */
-	jffs2_erase_pending_trigger(c);
 }
 
 static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 507ed6e..4b1848c 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -464,7 +464,7 @@ int jffs2_scan_dirty_space(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb
 int jffs2_do_mount_fs(struct jffs2_sb_info *c);
 
 /* erase.c */
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
 void jffs2_free_jeb_node_refs(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
 
 #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 21a0529..155fd63 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -733,6 +733,10 @@ int jffs2_thread_should_wake(struct jffs2_sb_info *c)
 	int nr_very_dirty = 0;
 	struct jffs2_eraseblock *jeb;
 
+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list))
+		return 1;
+
 	if (c->unchecked_size) {
 		D1(printk(KERN_DEBUG "jffs2_thread_should_wake(): unchecked_size %d, checked_ino #%d\n",
 			  c->unchecked_size, c->checked_ino));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index a7f03b7..5d26362 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -140,8 +140,7 @@ void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);
 
 #endif /* WRITEBUFFER */
 
-/* erase.c */
-static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 {
 	OFNI_BS_2SFFJ(c)->s_dirt = 1;
 }
@@ -151,6 +150,12 @@ int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);
 
+/* erase.c */
+static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+{
+	jffs2_garbage_collect_trigger(c);
+}
+
 /* dir.c */
 extern const struct file_operations jffs2_dir_operations;
 extern const struct inode_operations jffs2_dir_inode_operations;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 9a80e8e..511e2d6 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -63,8 +63,6 @@ static void jffs2_write_super(struct super_block *sb)
 
 	if (!(sb->s_flags & MS_RDONLY)) {
 		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
-		jffs2_garbage_collect_trigger(c);
-		jffs2_erase_pending_blocks(c, 0);
 		jffs2_flush_wbuf_gc(c, 0);
 	}
 
diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index 5ef7bac..f319efc 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -84,7 +84,7 @@ static void jffs2_wbuf_dirties_inode(struct jffs2_sb_info *c, uint32_t ino)
 	struct jffs2_inodirty *new;
 
 	/* Mark the superblock dirty so that kupdated will flush... */
-	jffs2_erase_pending_trigger(c);
+	jffs2_dirty_trigger(c);
 
 	if (jffs2_wbuf_pending_for_ino(c, ino))
 		return;
-- 
1.6.4.4

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

[Artem Bityutskiy]

On Sat, 2010-03-20 at 11:03 +0100, Joakim Tjernlund wrote:
> 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>
> ---

Chatted with dwmw2, and he is fine with your patch, so I'll add it to my
l2 tree, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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

[Joakim Tjernlund]

Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/03/30 14:57:34:
>
> On Sat, 2010-03-20 at 11:03 +0100, Joakim Tjernlund wrote:
> > 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>
> > ---
>
> Chatted with dwmw2, and he is fine with your patch, so I'll add it to my
> l2 tree, thanks.

Thanks.

   Jocke

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

[David Woodhouse]

On Sat, 2010-03-20 at 11:03 +0100, Joakim Tjernlund wrote:
> 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>
> ---

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

I've always been very careful to keep the GC thread as an
_optimisation_. It looks like this will still work, because the actual
erase will get done from jffs2_reserve_space()... but that'll only erase
blocks immediately when we actually need them. Before, we were erasing
them in advance. I suppose that's OK though.

-- 
dwmw2

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

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/13 19:16:58:
>
> On Sat, 2010-03-20 at 11:03 +0100, Joakim Tjernlund wrote:
> > 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>
> > ---
>

uhh, I wrote this patch almost 2 months ago, can't remember the details
but I do my best.

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

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

> 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. I guess I could move the list empty
check before goto done, but that would not really change anything.

>
> I've always been very careful to keep the GC thread as an
> _optimisation_. It looks like this will still work, because the actual
> erase will get done from jffs2_reserve_space()... but that'll only erase
> blocks immediately when we actually need them. Before, we were erasing
> them in advance. I suppose that's OK though.

yes, I think so too.

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

[David Woodhouse]

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

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

[Joakim Tjernlund]

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.

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

[Joakim Tjernlund]

>
> 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(-)

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 3ff50da..6cc014c 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -146,7 +146,9 @@ static int jffs2_garbage_collect_thread(void *_c)
 		disallow_signal(SIGHUP);

 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+		if (jffs2_erase_pending_blocks(c, 1))
+			/* Nothing more to do ATM */;
+		else if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
 			printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
 			goto die;
 		}
diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..2a6cc6c 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -103,9 +103,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
 	jffs2_erase_failed(c, jeb, bad_offset);
 }

-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 {
 	struct jffs2_eraseblock *jeb;
+	int work_done = 0;

 	mutex_lock(&c->erase_free_sem);

@@ -123,6 +124,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)

 			if (!--count) {
 				D1(printk(KERN_DEBUG "Count reached. jffs2_erase_pending_blocks leaving\n"));
+				work_done = 1;
 				goto done;
 			}

@@ -157,6 +159,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 	mutex_unlock(&c->erase_free_sem);
  done:
 	D1(printk(KERN_DEBUG "jffs2_erase_pending_blocks completed\n"));
+	return work_done;
 }

 static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
@@ -167,8 +170,6 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
 	list_move_tail(&jeb->list, &c->erase_complete_list);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
-	/* Ensure that kupdated calls us again to mark them clean */
-	jffs2_erase_pending_trigger(c);
 }

 static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 507ed6e..4b1848c 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -464,7 +464,7 @@ int jffs2_scan_dirty_space(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb
 int jffs2_do_mount_fs(struct jffs2_sb_info *c);

 /* erase.c */
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
 void jffs2_free_jeb_node_refs(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);

 #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 21a0529..155fd63 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -733,6 +733,10 @@ int jffs2_thread_should_wake(struct jffs2_sb_info *c)
 	int nr_very_dirty = 0;
 	struct jffs2_eraseblock *jeb;

+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list))
+		return 1;
+
 	if (c->unchecked_size) {
 		D1(printk(KERN_DEBUG "jffs2_thread_should_wake(): unchecked_size %d, checked_ino #%d\n",
 			  c->unchecked_size, c->checked_ino));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index a7f03b7..5d26362 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -140,8 +140,7 @@ void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);

 #endif /* WRITEBUFFER */

-/* erase.c */
-static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 {
 	OFNI_BS_2SFFJ(c)->s_dirt = 1;
 }
@@ -151,6 +150,12 @@ int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);

+/* erase.c */
+static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+{
+	jffs2_garbage_collect_trigger(c);
+}
+
 /* dir.c */
 extern const struct file_operations jffs2_dir_operations;
 extern const struct inode_operations jffs2_dir_inode_operations;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 9a80e8e..511e2d6 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -63,8 +63,6 @@ static void jffs2_write_super(struct super_block *sb)

 	if (!(sb->s_flags & MS_RDONLY)) {
 		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
-		jffs2_garbage_collect_trigger(c);
-		jffs2_erase_pending_blocks(c, 0);
 		jffs2_flush_wbuf_gc(c, 0);
 	}

diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index 5ef7bac..f319efc 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -84,7 +84,7 @@ static void jffs2_wbuf_dirties_inode(struct jffs2_sb_info *c, uint32_t ino)
 	struct jffs2_inodirty *new;

 	/* Mark the superblock dirty so that kupdated will flush... */
-	jffs2_erase_pending_trigger(c);
+	jffs2_dirty_trigger(c);

 	if (jffs2_wbuf_pending_for_ino(c, ino))
 		return;
--
1.6.4.4

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

[Joakim Tjernlund]

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

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

[David Woodhouse]

On Fri, 2010-05-14 at 14:12 +0200, Joakim Tjernlund wrote:
> @@ -167,8 +170,6 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
>         list_move_tail(&jeb->list, &c->erase_complete_list);
>         spin_unlock(&c->erase_completion_lock);
>         mutex_unlock(&c->erase_free_sem);
> -       /* Ensure that kupdated calls us again to mark them clean */
> -       jffs2_erase_pending_trigger(c);
>  }
> 
>  static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset) 

Why remove this? If you have asynchronous erases, with the erase
callback (and hence this jffs2_erase_succeeded function) getting called
asynchronously, then we _do_ want to trigger the GC thread to run, to
that it can write the cleanmarker to the block and refile it on the
empty_list.

-- 
dwmw2

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

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/18 17:46:27:
>
> On Fri, 2010-05-14 at 14:12 +0200, Joakim Tjernlund wrote:
> > @@ -167,8 +170,6 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info
> *c, struct jffs2_eraseblo
> >         list_move_tail(&jeb->list, &c->erase_complete_list);
> >         spin_unlock(&c->erase_completion_lock);
> >         mutex_unlock(&c->erase_free_sem);
> > -       /* Ensure that kupdated calls us again to mark them clean */
> > -       jffs2_erase_pending_trigger(c);
> >  }
> >
> >  static void jffs2_erase_failed(struct jffs2_sb_info *c, struct
> jffs2_eraseblock *jeb, uint32_t bad_offset)
>
> Why remove this? If you have asynchronous erases, with the erase
> callback (and hence this jffs2_erase_succeeded function) getting called
> asynchronously, then we _do_ want to trigger the GC thread to run, to
> that it can write the cleanmarker to the block and refile it on the
> empty_list.

hmm, I haven't given async erases much thought(is anyone using that?)
but yes it looks premature to remove this. Could you add that back or
do you want me to submit another patch?

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

[David Woodhouse]

On Fri, 2010-05-14 at 14:12 +0200, Joakim Tjernlund wrote:
> +/* erase.c */
> +static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
> +{
> +       jffs2_garbage_collect_trigger(c);
> +} 

Hrm, and now everything which calls jffs2_erase_pending_trigger() needs
_not_ to be holding c->erase_completion_lock, or it'll deadlock...

Eraseblock at 0x001c0000 completely dirtied. Removing from (dirty?) list...                                                                                                      
...and adding to erase_pending_list                                                                                                                                              
                                                                                                                                                                                 
=============================================                                                                                                                                    
[ INFO: possible recursive locking detected ]                                                                                                                                    
2.6.34-rc7 #2                                                                                                                                                                    
---------------------------------------------                                                                                                                                    
dbench/4263 is trying to acquire lock:                                                                                                                                           
 (&(&c->erase_completion_lock)->rlock){+.+...}, at: [<ffffffffa011ae0e>] jffs2_garbage_collect_trigger+0x19/0x4e [jffs2]                                                         
                                                                                                                                                                                 
but task is already holding lock:                                                                                                                                                
 (&(&c->erase_completion_lock)->rlock){+.+...}, at: [<ffffffffa01115cd>] jffs2_mark_node_obsolete+0xcb/0x737 [jffs2]                                                             
                                                                                                                                                                                 
other info that might help us debug this:                                                                                                                                        
5 locks held by dbench/4263:                                                                                                                                                     
 #0:  (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<ffffffff8107aeda>] generic_file_aio_write+0x47/0xa8                                                                           
 #1:  (&c->alloc_sem){+.+.+.}, at: [<ffffffffa011239f>] jffs2_reserve_space+0x71/0x39e [jffs2]                                                                                   
 #2:  (&f->sem){+.+.+.}, at: [<ffffffffa0115c73>] jffs2_write_inode_range+0xb0/0x2f3 [jffs2]                                                                                     
 #3:  (&c->erase_free_sem){+.+...}, at: [<ffffffffa01115b6>] jffs2_mark_node_obsolete+0xb4/0x737 [jffs2]                                                                         
 #4:  (&(&c->erase_completion_lock)->rlock){+.+...}, at: [<ffffffffa01115cd>] jffs2_mark_node_obsolete+0xcb/0x737 [jffs2]                                                        
                                                                                                                                                                                 
stack backtrace:                                                                                                                                                                 
Pid: 4263, comm: dbench Not tainted 2.6.34-rc7 #2                                                                                                                                
Call Trace:                                                                                                                                                                      
 [<ffffffff8105d40b>] __lock_acquire+0x1633/0x16cd                                                                                                                               
 [<ffffffff8105a16f>] ? trace_hardirqs_off+0xd/0xf                                                                                                                               
 [<ffffffff814d6d71>] ? mutex_lock_nested+0x2c7/0x31a                                                                                                                            
 [<ffffffff8105b1c7>] ? trace_hardirqs_on_caller+0x10c/0x130                                                                                                                     
 [<ffffffff8105d4fc>] lock_acquire+0x57/0x6d                                                                                                                                     
 [<ffffffffa011ae0e>] ? jffs2_garbage_collect_trigger+0x19/0x4e [jffs2]                                                                                                          
 [<ffffffff814d7fdd>] _raw_spin_lock+0x3b/0x4a                                                                                                                                   
 [<ffffffffa011ae0e>] ? jffs2_garbage_collect_trigger+0x19/0x4e [jffs2]                                                                                                          
 [<ffffffffa011ae0e>] jffs2_garbage_collect_trigger+0x19/0x4e [jffs2]                                                                                                            
 [<ffffffffa01118a9>] jffs2_mark_node_obsolete+0x3a7/0x737 [jffs2]                                                                                                               
 [<ffffffff814d4e51>] ? printk+0x3c/0x3e                                                                                                                                         
 [<ffffffffa011022c>] jffs2_obsolete_node_frag+0x2a/0x48 [jffs2]                                                                                                                 
 [<ffffffffa01108e9>] jffs2_add_full_dnode_to_inode+0x2f3/0x3cc [jffs2]                                                                                                          
 [<ffffffffa0115dc6>] jffs2_write_inode_range+0x203/0x2f3 [jffs2]                                                                                                                
 [<ffffffffa010f94e>] jffs2_write_end+0x176/0x25b [jffs2]                                                                                                                        
 [<ffffffff810790ca>] generic_file_buffered_write+0x188/0x282                                                                                                                    

-- 
dwmw2

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

[David Woodhouse]

On Tue, 2010-05-18 at 19:37 +0100, David Woodhouse wrote:
> 
> Hrm, and now everything which calls jffs2_erase_pending_trigger()
> needs
> _not_ to be holding c->erase_completion_lock, or it'll deadlock...

Playing with this incremental patch now...

I also moved the erasing into jffs2_garbage_collect_pass() itself rather
than having both callers of jffs2_garbage_collect_pass() do it for
themselves, under different circumstances. I think the one in
jffs2_reserve_space() was buggy anyway.

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 6cc014c..8c9ffdc 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -21,12 +21,11 @@
 
 static int jffs2_garbage_collect_thread(void *);
 
-void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c)
+void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
 {
-	spin_lock(&c->erase_completion_lock);
+	BUG_ON(spin_trylock(&c->erase_completion_lock));
 	if (c->gc_task && jffs2_thread_should_wake(c))
 		send_sig(SIGHUP, c->gc_task, 1);
-	spin_unlock(&c->erase_completion_lock);
 }
 
 /* This must only ever be called when no GC thread is currently running */
@@ -146,9 +145,8 @@ static int jffs2_garbage_collect_thread(void *_c)
 		disallow_signal(SIGHUP);
 
 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-		if (jffs2_erase_pending_blocks(c, 1))
-			/* Nothing more to do ATM */;
-		else if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+
+		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
 			printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
 			goto die;
 		}
diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index 2a6cc6c..e8638f8 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -168,8 +168,11 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
 	mutex_lock(&c->erase_free_sem);
 	spin_lock(&c->erase_completion_lock);
 	list_move_tail(&jeb->list, &c->erase_complete_list);
+	/* Wake the GC thread to mark them clean */
+	jffs2_erase_pending_trigger(c);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
+	wake_up(&c->erase_wait);
 }
 
 static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
@@ -488,9 +491,9 @@ filebad:
 
 refile:
 	/* Stick it back on the list from whence it came and come back later */
-	jffs2_erase_pending_trigger(c);
 	mutex_lock(&c->erase_free_sem);
 	spin_lock(&c->erase_completion_lock);
+	jffs2_erase_pending_trigger(c);
 	list_move(&jeb->list, &c->erase_complete_list);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index 3b6f2fa..1ea4a84 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -214,6 +214,19 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
 		return ret;
 	}
 
+	/* If there are any blocks which need erasing, erase them now */
+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list)) {
+		spin_unlock(&c->erase_completion_lock);
+		D1(printk(KERN_DEBUG "jffs2_garbage_collect_pass() erasing pending blocks\n"));
+		if (jffs2_erase_pending_blocks(c, 1)) {
+			mutex_unlock(&c->alloc_sem);
+			return 0;
+		}
+		D1(printk(KERN_DEBUG "No progress from erasing blocks; doing GC anyway\n"));
+		spin_lock(&c->erase_completion_lock);
+	}
+
 	/* First, work out which block we're garbage-collecting */
 	jeb = c->gcblock;
 
@@ -222,7 +235,7 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
 
 	if (!jeb) {
 		/* Couldn't find a free block. But maybe we can just erase one and make 'progress'? */
-		if (!list_empty(&c->erase_pending_list)) {
+		if (c->nr_erasing_blocks) {
 			spin_unlock(&c->erase_completion_lock);
 			mutex_unlock(&c->alloc_sem);
 			return -EAGAIN;
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 4779301..8ec436a 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -115,9 +115,21 @@ int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize,
 			spin_unlock(&c->erase_completion_lock);
 
 			ret = jffs2_garbage_collect_pass(c);
-
-			if (ret == -EAGAIN)
-				jffs2_erase_pending_blocks(c, 1);
+			if (ret == -EAGAIN) {
+				spin_lock(&c->erase_completion_lock);
+				if (c->nr_erasing_blocks &&
+				    list_empty(&c->erase_pending_list) &&
+				    list_empty(&c->erase_complete_list)) {
+					DECLARE_WAITQUEUE(wait, current);
+					set_current_state(TASK_UNINTERRUPTIBLE);
+					add_wait_queue(&c->erase_wait, &wait);
+					D1(printk(KERN_DEBUG "%s waiting for erase to complete\n", __func__));
+					spin_unlock(&c->erase_completion_lock);
+					
+					schedule();
+				} else
+					spin_unlock(&c->erase_completion_lock);
+			}
 			else if (ret)
 				return ret;
 
@@ -469,7 +481,9 @@ struct jffs2_raw_node_ref *jffs2_add_physical_node_ref(struct jffs2_sb_info *c,
 void jffs2_complete_reservation(struct jffs2_sb_info *c)
 {
 	D1(printk(KERN_DEBUG "jffs2_complete_reservation()\n"));
-	jffs2_garbage_collect_trigger(c);
+	spin_lock(&c->erase_completion_lock);
+	jffs2_erase_pending_trigger(c);
+	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->alloc_sem);
 }
 
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index 5d26362..64ac455 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -148,13 +148,7 @@ static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 /* background.c */
 int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
-void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);
-
-/* erase.c */
-static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
-{
-	jffs2_garbage_collect_trigger(c);
-}
+void jffs2_erase_pending_trigger(struct jffs2_sb_info *c);
 
 /* dir.c */
 extern const struct file_operations jffs2_dir_operations;
diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index 696686c..1312552 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -260,7 +260,9 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
 			ret = -EIO;
 			goto out;
 		}
+		spin_lock(&c->erase_completion_lock);
 		jffs2_erase_pending_trigger(c);
+		spin_unlock(&c->erase_completion_lock);
 	}
 	ret = 0;
  out:


-- 
dwmw2

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

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/18 20:59:12:
>
> On Tue, 2010-05-18 at 19:37 +0100, David Woodhouse wrote:
> >
> > Hrm, and now everything which calls jffs2_erase_pending_trigger()
> > needs
> > _not_ to be holding c->erase_completion_lock, or it'll deadlock...
>
> Playing with this incremental patch now...

oops, this got a lot more complicated. I will have closer look tmw.

BTW, I had a much simpler patch to start with but it was reject because
it tested for signals in erase_pending_blocks(). Look for
  jffs2: Move erasing from write_super to GC.
and
  jffs2: Make jffs2_erase_pending_trigger() initiate GC.
if you want to look.

>
> I also moved the erasing into jffs2_garbage_collect_pass() itself rather
> than having both callers of jffs2_garbage_collect_pass() do it for
> themselves, under different circumstances. I think the one in
> jffs2_reserve_space() was buggy anyway.
>
> diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
> index 6cc014c..8c9ffdc 100644
> --- a/fs/jffs2/background.c
> +++ b/fs/jffs2/background.c
> @@ -21,12 +21,11 @@
>
>  static int jffs2_garbage_collect_thread(void *);
>
> -void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c)
> +void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
>  {
> -   spin_lock(&c->erase_completion_lock);
> +   BUG_ON(spin_trylock(&c->erase_completion_lock));
>     if (c->gc_task && jffs2_thread_should_wake(c))
>        send_sig(SIGHUP, c->gc_task, 1);
> -   spin_unlock(&c->erase_completion_lock);
>  }
>
>  /* This must only ever be called when no GC thread is currently running */
> @@ -146,9 +145,8 @@ static int jffs2_garbage_collect_thread(void *_c)
>        disallow_signal(SIGHUP);
>
>        D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
> -      if (jffs2_erase_pending_blocks(c, 1))
> -         /* Nothing more to do ATM */;
> -      else if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
> +
> +      if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
>           printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
>           goto die;
>        }
> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> index 2a6cc6c..e8638f8 100644
> --- a/fs/jffs2/erase.c
> +++ b/fs/jffs2/erase.c
> @@ -168,8 +168,11 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info
> *c, struct jffs2_eraseblo
>     mutex_lock(&c->erase_free_sem);
>     spin_lock(&c->erase_completion_lock);
>     list_move_tail(&jeb->list, &c->erase_complete_list);
> +   /* Wake the GC thread to mark them clean */
> +   jffs2_erase_pending_trigger(c);
>     spin_unlock(&c->erase_completion_lock);
>     mutex_unlock(&c->erase_free_sem);
> +   wake_up(&c->erase_wait);
>  }
>
>  static void jffs2_erase_failed(struct jffs2_sb_info *c, struct
> jffs2_eraseblock *jeb, uint32_t bad_offset)
> @@ -488,9 +491,9 @@ filebad:
>
>  refile:
>     /* Stick it back on the list from whence it came and come back later */
> -   jffs2_erase_pending_trigger(c);
>     mutex_lock(&c->erase_free_sem);
>     spin_lock(&c->erase_completion_lock);
> +   jffs2_erase_pending_trigger(c);
>     list_move(&jeb->list, &c->erase_complete_list);
>     spin_unlock(&c->erase_completion_lock);
>     mutex_unlock(&c->erase_free_sem);
> diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
> index 3b6f2fa..1ea4a84 100644
> --- a/fs/jffs2/gc.c
> +++ b/fs/jffs2/gc.c
> @@ -214,6 +214,19 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
>        return ret;
>     }
>
> +   /* If there are any blocks which need erasing, erase them now */
> +   if (!list_empty(&c->erase_complete_list) ||
> +       !list_empty(&c->erase_pending_list)) {
> +      spin_unlock(&c->erase_completion_lock);
> +      D1(printk(KERN_DEBUG "jffs2_garbage_collect_pass() erasing pending blocks\n"));
> +      if (jffs2_erase_pending_blocks(c, 1)) {
> +         mutex_unlock(&c->alloc_sem);
> +         return 0;
> +      }
> +      D1(printk(KERN_DEBUG "No progress from erasing blocks; doing GC anyway\n"));
> +      spin_lock(&c->erase_completion_lock);
> +   }
> +
>     /* First, work out which block we're garbage-collecting */
>     jeb = c->gcblock;
>
> @@ -222,7 +235,7 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
>
>     if (!jeb) {
>        /* Couldn't find a free block. But maybe we can just erase one and make
> 'progress'? */
> -      if (!list_empty(&c->erase_pending_list)) {
> +      if (c->nr_erasing_blocks) {
>           spin_unlock(&c->erase_completion_lock);
>           mutex_unlock(&c->alloc_sem);
>           return -EAGAIN;
> diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
> index 4779301..8ec436a 100644
> --- a/fs/jffs2/nodemgmt.c
> +++ b/fs/jffs2/nodemgmt.c
> @@ -115,9 +115,21 @@ int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize,
>           spin_unlock(&c->erase_completion_lock);
>
>           ret = jffs2_garbage_collect_pass(c);
> -
> -         if (ret == -EAGAIN)
> -            jffs2_erase_pending_blocks(c, 1);
> +         if (ret == -EAGAIN) {
> +            spin_lock(&c->erase_completion_lock);
> +            if (c->nr_erasing_blocks &&
> +                list_empty(&c->erase_pending_list) &&
> +                list_empty(&c->erase_complete_list)) {
> +               DECLARE_WAITQUEUE(wait, current);
> +               set_current_state(TASK_UNINTERRUPTIBLE);
> +               add_wait_queue(&c->erase_wait, &wait);
> +               D1(printk(KERN_DEBUG "%s waiting for erase to complete\n", __func__));
> +               spin_unlock(&c->erase_completion_lock);
> +
> +               schedule();
> +            } else
> +               spin_unlock(&c->erase_completion_lock);
> +         }
>           else if (ret)
>              return ret;
>
> @@ -469,7 +481,9 @@ struct jffs2_raw_node_ref *jffs2_add_physical_node_ref
> (struct jffs2_sb_info *c,
>  void jffs2_complete_reservation(struct jffs2_sb_info *c)
>  {
>     D1(printk(KERN_DEBUG "jffs2_complete_reservation()\n"));
> -   jffs2_garbage_collect_trigger(c);
> +   spin_lock(&c->erase_completion_lock);
> +   jffs2_erase_pending_trigger(c);
> +   spin_unlock(&c->erase_completion_lock);
>     mutex_unlock(&c->alloc_sem);
>  }
>
> diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
> index 5d26362..64ac455 100644
> --- a/fs/jffs2/os-linux.h
> +++ b/fs/jffs2/os-linux.h
> @@ -148,13 +148,7 @@ static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
>  /* background.c */
>  int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
>  void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
> -void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);
> -
> -/* erase.c */
> -static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
> -{
> -   jffs2_garbage_collect_trigger(c);
> -}
> +void jffs2_erase_pending_trigger(struct jffs2_sb_info *c);
>
>  /* dir.c */
>  extern const struct file_operations jffs2_dir_operations;
> diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
> index 696686c..1312552 100644
> --- a/fs/jffs2/scan.c
> +++ b/fs/jffs2/scan.c
> @@ -260,7 +260,9 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
>           ret = -EIO;
>           goto out;
>        }
> +      spin_lock(&c->erase_completion_lock);
>        jffs2_erase_pending_trigger(c);
> +      spin_unlock(&c->erase_completion_lock);
>     }
>     ret = 0;
>   out:
>
>
> --
> dwmw2
>
>
>

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

[David Woodhouse]

On Wed, 2010-05-19 at 00:44 +0200, Joakim Tjernlund wrote:
> 
> oops, this got a lot more complicated. I will have closer look tmw.

It's not so bad -- the whole thing looks like this (I'll follow it up
with a global s/jffs2_erase_pending_trigger/jffs2_gc_thread_trigger/).

It could perhaps be broken into smaller simple patches along the lines
of:
 - Add 'work_done' return value from jffs2_erase_pending_blocks()
 - Call jffs2_erase_pending_blocks() from jffs2_g_c_pass()
 - Fix the conditions under which jffs2_g_c_pass() will return -EINVAL,
   fix the way that jffs2_reserve_space() will respond to that, and fix
   jffs2_erase_succeeded() to actually wake up the erase_wait queue.
 - Remove locking from jffs2_garbage_collect_trigger() and require that
   callers have c->erase_completion_lock already held.
 - Change most callers of jffs2_erase_pending_trigger() to call
   jffs2_garbage_collect_trigger() instead. To comply with the locking
   rules, this may involve moving the call up or down a few lines, or
   sometimes adding a new lock/unlock pair around it.
 - Remove jffs2_erase_pending_blocks() call from jffs2_write_super().
 - Rename jffs2_erase_pending_trigger() and its only remaining caller
   in wbuf.c to jffs2_dirty_trigger().


diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 3ff50da..39c65ad 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -21,12 +21,11 @@
 
 static int jffs2_garbage_collect_thread(void *);
 
-void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c)
+void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
 {
-	spin_lock(&c->erase_completion_lock);
+	BUG_ON(spin_trylock(&c->erase_completion_lock));
 	if (c->gc_task && jffs2_thread_should_wake(c))
 		send_sig(SIGHUP, c->gc_task, 1);
-	spin_unlock(&c->erase_completion_lock);
 }
 
 /* This must only ever be called when no GC thread is currently running */
diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..e8638f8 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -103,9 +103,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
 	jffs2_erase_failed(c, jeb, bad_offset);
 }
 
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 {
 	struct jffs2_eraseblock *jeb;
+	int work_done = 0;
 
 	mutex_lock(&c->erase_free_sem);
 
@@ -123,6 +124,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 
 			if (!--count) {
 				D1(printk(KERN_DEBUG "Count reached. jffs2_erase_pending_blocks leaving\n"));
+				work_done = 1;
 				goto done;
 			}
 
@@ -157,6 +159,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 	mutex_unlock(&c->erase_free_sem);
  done:
 	D1(printk(KERN_DEBUG "jffs2_erase_pending_blocks completed\n"));
+	return work_done;
 }
 
 static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
@@ -165,10 +168,11 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
 	mutex_lock(&c->erase_free_sem);
 	spin_lock(&c->erase_completion_lock);
 	list_move_tail(&jeb->list, &c->erase_complete_list);
+	/* Wake the GC thread to mark them clean */
+	jffs2_erase_pending_trigger(c);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
-	/* Ensure that kupdated calls us again to mark them clean */
-	jffs2_erase_pending_trigger(c);
+	wake_up(&c->erase_wait);
 }
 
 static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
@@ -487,9 +491,9 @@ filebad:
 
 refile:
 	/* Stick it back on the list from whence it came and come back later */
-	jffs2_erase_pending_trigger(c);
 	mutex_lock(&c->erase_free_sem);
 	spin_lock(&c->erase_completion_lock);
+	jffs2_erase_pending_trigger(c);
 	list_move(&jeb->list, &c->erase_complete_list);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index 3b6f2fa..1ea4a84 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -214,6 +214,19 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
 		return ret;
 	}
 
+	/* If there are any blocks which need erasing, erase them now */
+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list)) {
+		spin_unlock(&c->erase_completion_lock);
+		D1(printk(KERN_DEBUG "jffs2_garbage_collect_pass() erasing pending blocks\n"));
+		if (jffs2_erase_pending_blocks(c, 1)) {
+			mutex_unlock(&c->alloc_sem);
+			return 0;
+		}
+		D1(printk(KERN_DEBUG "No progress from erasing blocks; doing GC anyway\n"));
+		spin_lock(&c->erase_completion_lock);
+	}
+
 	/* First, work out which block we're garbage-collecting */
 	jeb = c->gcblock;
 
@@ -222,7 +235,7 @@ int jffs2_garbage_collect_pass(struct jffs2_sb_info *c)
 
 	if (!jeb) {
 		/* Couldn't find a free block. But maybe we can just erase one and make 'progress'? */
-		if (!list_empty(&c->erase_pending_list)) {
+		if (c->nr_erasing_blocks) {
 			spin_unlock(&c->erase_completion_lock);
 			mutex_unlock(&c->alloc_sem);
 			return -EAGAIN;
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 36d7a84..a881a42 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -464,7 +464,7 @@ int jffs2_scan_dirty_space(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb
 int jffs2_do_mount_fs(struct jffs2_sb_info *c);
 
 /* erase.c */
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
 void jffs2_free_jeb_node_refs(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
 
 #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 191359d..8ec436a 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -115,9 +115,21 @@ int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize,
 			spin_unlock(&c->erase_completion_lock);
 
 			ret = jffs2_garbage_collect_pass(c);
-
-			if (ret == -EAGAIN)
-				jffs2_erase_pending_blocks(c, 1);
+			if (ret == -EAGAIN) {
+				spin_lock(&c->erase_completion_lock);
+				if (c->nr_erasing_blocks &&
+				    list_empty(&c->erase_pending_list) &&
+				    list_empty(&c->erase_complete_list)) {
+					DECLARE_WAITQUEUE(wait, current);
+					set_current_state(TASK_UNINTERRUPTIBLE);
+					add_wait_queue(&c->erase_wait, &wait);
+					D1(printk(KERN_DEBUG "%s waiting for erase to complete\n", __func__));
+					spin_unlock(&c->erase_completion_lock);
+					
+					schedule();
+				} else
+					spin_unlock(&c->erase_completion_lock);
+			}
 			else if (ret)
 				return ret;
 
@@ -469,7 +481,9 @@ struct jffs2_raw_node_ref *jffs2_add_physical_node_ref(struct jffs2_sb_info *c,
 void jffs2_complete_reservation(struct jffs2_sb_info *c)
 {
 	D1(printk(KERN_DEBUG "jffs2_complete_reservation()\n"));
-	jffs2_garbage_collect_trigger(c);
+	spin_lock(&c->erase_completion_lock);
+	jffs2_erase_pending_trigger(c);
+	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->alloc_sem);
 }
 
@@ -732,6 +746,10 @@ int jffs2_thread_should_wake(struct jffs2_sb_info *c)
 	int nr_very_dirty = 0;
 	struct jffs2_eraseblock *jeb;
 
+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list))
+		return 1;
+
 	if (c->unchecked_size) {
 		D1(printk(KERN_DEBUG "jffs2_thread_should_wake(): unchecked_size %d, checked_ino #%d\n",
 			  c->unchecked_size, c->checked_ino));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index a7f03b7..64ac455 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -140,8 +140,7 @@ void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);
 
 #endif /* WRITEBUFFER */
 
-/* erase.c */
-static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 {
 	OFNI_BS_2SFFJ(c)->s_dirt = 1;
 }
@@ -149,7 +148,7 @@ static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
 /* background.c */
 int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
-void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);
+void jffs2_erase_pending_trigger(struct jffs2_sb_info *c);
 
 /* dir.c */
 extern const struct file_operations jffs2_dir_operations;
diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index 696686c..1312552 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -260,7 +260,9 @@ int jffs2_scan_medium(struct jffs2_sb_info *c)
 			ret = -EIO;
 			goto out;
 		}
+		spin_lock(&c->erase_completion_lock);
 		jffs2_erase_pending_trigger(c);
+		spin_unlock(&c->erase_completion_lock);
 	}
 	ret = 0;
  out:
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 9a80e8e..511e2d6 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -63,8 +63,6 @@ static void jffs2_write_super(struct super_block *sb)
 
 	if (!(sb->s_flags & MS_RDONLY)) {
 		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
-		jffs2_garbage_collect_trigger(c);
-		jffs2_erase_pending_blocks(c, 0);
 		jffs2_flush_wbuf_gc(c, 0);
 	}
 
diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index 5ef7bac..f319efc 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -84,7 +84,7 @@ static void jffs2_wbuf_dirties_inode(struct jffs2_sb_info *c, uint32_t ino)
 	struct jffs2_inodirty *new;
 
 	/* Mark the superblock dirty so that kupdated will flush... */
-	jffs2_erase_pending_trigger(c);
+	jffs2_dirty_trigger(c);
 
 	if (jffs2_wbuf_pending_for_ino(c, ino))
 		return;

-- 
dwmw2

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

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/19 01:42:02:
>
> On Wed, 2010-05-19 at 00:44 +0200, Joakim Tjernlund wrote:
> >
> > oops, this got a lot more complicated. I will have closer look tmw.
>
> It's not so bad -- the whole thing looks like this (I'll follow it up
> with a global s/jffs2_erase_pending_trigger/jffs2_gc_thread_trigger/).
>
> It could perhaps be broken into smaller simple patches along the lines
> of:
>  - Add 'work_done' return value from jffs2_erase_pending_blocks()
>  - Call jffs2_erase_pending_blocks() from jffs2_g_c_pass()
>  - Fix the conditions under which jffs2_g_c_pass() will return -EINVAL,
>    fix the way that jffs2_reserve_space() will respond to that, and fix
>    jffs2_erase_succeeded() to actually wake up the erase_wait queue.
>  - Remove locking from jffs2_garbage_collect_trigger() and require that
>    callers have c->erase_completion_lock already held.
>  - Change most callers of jffs2_erase_pending_trigger() to call
>    jffs2_garbage_collect_trigger() instead. To comply with the locking
>    rules, this may involve moving the call up or down a few lines, or
>    sometimes adding a new lock/unlock pair around it.
>  - Remove jffs2_erase_pending_blocks() call from jffs2_write_super().
>  - Rename jffs2_erase_pending_trigger() and its only remaining caller
>    in wbuf.c to jffs2_dirty_trigger().

I have tried this on my 2.6.33 tree and it seems to work OK.
I am currenltly copying lots of files and deleting them in a loop,
so far so good :) Needed to "tweak":

>
>
> diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
> index 3ff50da..39c65ad 100644
> --- a/fs/jffs2/background.c
> +++ b/fs/jffs2/background.c
> @@ -21,12 +21,11 @@
>
>  static int jffs2_garbage_collect_thread(void *);
>
> -void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c)
> +void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
>  {
> -   spin_lock(&c->erase_completion_lock);
> +   BUG_ON(spin_trylock(&c->erase_completion_lock));

This looks like a typo and I had to change that to:
BUG_ON(spin_is_locked(&c->erase_completion_lock));

>     if (c->gc_task && jffs2_thread_should_wake(c))
>        send_sig(SIGHUP, c->gc_task, 1);
> -   spin_unlock(&c->erase_completion_lock);
>  }
>
>  /* This must only ever be called when no GC thread is currently running */
> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> index b47679b..e8638f8 100644
> --- a/fs/jffs2/erase.c
> +++ b/fs/jffs2/erase.c
> @@ -103,9 +103,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
>     jffs2_erase_failed(c, jeb, bad_offset);
>  }
>
> -void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
> +int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
>  {
>     struct jffs2_eraseblock *jeb;
> +   int work_done = 0;
>
>     mutex_lock(&c->erase_free_sem);
>
> @@ -123,6 +124,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
>
>           if (!--count) {
>              D1(printk(KERN_DEBUG "Count reached. jffs2_erase_pending_blocks
> leaving\n"));
> +            work_done = 1;

work_done = 1; should move outside the if, otherwise it might get set if count is big.

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

[David Woodhouse]

On Wed, 2010-05-19 at 11:47 +0200, Joakim Tjernlund wrote:
> 
> This looks like a typo and I had to change that to:
> BUG_ON(spin_is_locked(&c->erase_completion_lock)); 

Hm, no -- we want it to BUG() if the spinlock is _NOT_ locked.

You're building on UP, without preemption or spinlock debugging. In that
case, spinlocks are a no-op. So spin_trylock() _always_ succeeds, and
spin_is_locked() always returns 0.

It should be lockdep_assert_held(), I think.

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

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

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/19 12:05:31:
>
> On Wed, 2010-05-19 at 11:47 +0200, Joakim Tjernlund wrote:
> >
> > This looks like a typo and I had to change that to:
> > BUG_ON(spin_is_locked(&c->erase_completion_lock));
>
> Hm, no -- we want it to BUG() if the spinlock is _NOT_ locked.
>
> You're building on UP, without preemption or spinlock debugging. In that
> case, spinlocks are a no-op. So spin_trylock() _always_ succeeds, and
> spin_is_locked() always returns 0.
>
> It should be lockdep_assert_held(), I think.

Ah, now I remember. There was an discussion about this on LKML a while
ago.
But would not assert_spin_locked() be better? On the
other hand lockdep_assert_held() will work too. Depends on what you
want?

       Jocke

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

[David Woodhouse]

On Wed, 2010-05-19 at 11:47 +0200, Joakim Tjernlund wrote:
> 
> > It could perhaps be broken into smaller simple patches along the lines
> > of:
> >  - Add 'work_done' return value from jffs2_erase_pending_blocks()
> >  - Call jffs2_erase_pending_blocks() from jffs2_g_c_pass()
> >  - Fix the conditions under which jffs2_g_c_pass() will return -EINVAL,
> >    fix the way that jffs2_reserve_space() will respond to that, and fix
> >    jffs2_erase_succeeded() to actually wake up the erase_wait queue.
> >  - Remove locking from jffs2_garbage_collect_trigger() and require that
> >    callers have c->erase_completion_lock already held.
> >  - Change most callers of jffs2_erase_pending_trigger() to call
> >    jffs2_garbage_collect_trigger() instead. To comply with the locking
> >    rules, this may involve moving the call up or down a few lines, or
> >    sometimes adding a new lock/unlock pair around it.
> >  - Remove jffs2_erase_pending_blocks() call from jffs2_write_super().
> >  - Rename jffs2_erase_pending_trigger() and its only remaining caller
> >    in wbuf.c to jffs2_dirty_trigger().
> 
> I have tried this on my 2.6.33 tree and it seems to work OK.
> I am currenltly copying lots of files and deleting them in a loop,
> so far so good :) Needed to "tweak":

OK, this is now pushed, in some semblance of the above form. I think I
got the attribution right on the bits which you wrote. Thanks again.

-- 
dwmw2

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

[Joakim Tjernlund]

David Woodhouse <dwmw2@infradead.org> wrote on 2010/05/19 18:49:08:
>
> On Wed, 2010-05-19 at 11:47 +0200, Joakim Tjernlund wrote:
> >
> > > It could perhaps be broken into smaller simple patches along the lines
> > > of:
> > >  - Add 'work_done' return value from jffs2_erase_pending_blocks()
> > >  - Call jffs2_erase_pending_blocks() from jffs2_g_c_pass()
> > >  - Fix the conditions under which jffs2_g_c_pass() will return -EINVAL,
> > >    fix the way that jffs2_reserve_space() will respond to that, and fix
> > >    jffs2_erase_succeeded() to actually wake up the erase_wait queue.
> > >  - Remove locking from jffs2_garbage_collect_trigger() and require that
> > >    callers have c->erase_completion_lock already held.
> > >  - Change most callers of jffs2_erase_pending_trigger() to call
> > >    jffs2_garbage_collect_trigger() instead. To comply with the locking
> > >    rules, this may involve moving the call up or down a few lines, or
> > >    sometimes adding a new lock/unlock pair around it.
> > >  - Remove jffs2_erase_pending_blocks() call from jffs2_write_super().
> > >  - Rename jffs2_erase_pending_trigger() and its only remaining caller
> > >    in wbuf.c to jffs2_dirty_trigger().
> >
> > I have tried this on my 2.6.33 tree and it seems to work OK.
> > I am currenltly copying lots of files and deleting them in a loop,
> > so far so good :) Needed to "tweak":
>
> OK, this is now pushed, in some semblance of the above form. I think I
> got the attribution right on the bits which you wrote. Thanks again.

Thanks, I am sure you get attribution right, but where did you push?
Can't find it in http://git.infradead.org/mtd-2.6.git

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

[David Woodhouse]

On Wed, 2010-05-19 at 22:06 +0200, Joakim Tjernlund wrote:
> > OK, this is now pushed, in some semblance of the above form. I think I
> > got the attribution right on the bits which you wrote. Thanks again.
> 
> Thanks, I am sure you get attribution right, but where did you push?
> Can't find it in http://git.infradead.org/mtd-2.6.git

Oh, looks like I didn't push it anywhere; just committed it locally.
It's there now; sorry.

-- 
dwmw2

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

[Joakim Tjernlund]

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      |    6 +++---
 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, 19 insertions(+), 10 deletions(-)

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 3ff50da..6cc014c 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -146,7 +146,9 @@ static int jffs2_garbage_collect_thread(void *_c)
 		disallow_signal(SIGHUP);
 
 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+		if (jffs2_erase_pending_blocks(c, 1))
+			/* Nothing more to do ATM */;
+		else if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
 			printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
 			goto die;
 		}
diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..be6b2b8 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -103,7 +103,7 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
 	jffs2_erase_failed(c, jeb, bad_offset);
 }
 
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 {
 	struct jffs2_eraseblock *jeb;
 
@@ -157,6 +157,8 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 	mutex_unlock(&c->erase_free_sem);
  done:
 	D1(printk(KERN_DEBUG "jffs2_erase_pending_blocks completed\n"));
+	return !list_empty(&c->erase_complete_list) ||
+		!list_empty(&c->erase_pending_list);
 }
 
 static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
@@ -167,8 +169,6 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
 	list_move_tail(&jeb->list, &c->erase_complete_list);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
-	/* Ensure that kupdated calls us again to mark them clean */
-	jffs2_erase_pending_trigger(c);
 }
 
 static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 507ed6e..4b1848c 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -464,7 +464,7 @@ int jffs2_scan_dirty_space(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb
 int jffs2_do_mount_fs(struct jffs2_sb_info *c);
 
 /* erase.c */
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
 void jffs2_free_jeb_node_refs(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
 
 #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 21a0529..155fd63 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -733,6 +733,10 @@ int jffs2_thread_should_wake(struct jffs2_sb_info *c)
 	int nr_very_dirty = 0;
 	struct jffs2_eraseblock *jeb;
 
+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list))
+		return 1;
+
 	if (c->unchecked_size) {
 		D1(printk(KERN_DEBUG "jffs2_thread_should_wake(): unchecked_size %d, checked_ino #%d\n",
 			  c->unchecked_size, c->checked_ino));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index a7f03b7..5d26362 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -140,8 +140,7 @@ void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);
 
 #endif /* WRITEBUFFER */
 
-/* erase.c */
-static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 {
 	OFNI_BS_2SFFJ(c)->s_dirt = 1;
 }
@@ -151,6 +150,12 @@ int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);
 
+/* erase.c */
+static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+{
+	jffs2_garbage_collect_trigger(c);
+}
+
 /* dir.c */
 extern const struct file_operations jffs2_dir_operations;
 extern const struct inode_operations jffs2_dir_inode_operations;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 9a80e8e..511e2d6 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -63,8 +63,6 @@ static void jffs2_write_super(struct super_block *sb)
 
 	if (!(sb->s_flags & MS_RDONLY)) {
 		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
-		jffs2_garbage_collect_trigger(c);
-		jffs2_erase_pending_blocks(c, 0);
 		jffs2_flush_wbuf_gc(c, 0);
 	}
 
diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index 5ef7bac..f319efc 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -84,7 +84,7 @@ static void jffs2_wbuf_dirties_inode(struct jffs2_sb_info *c, uint32_t ino)
 	struct jffs2_inodirty *new;
 
 	/* Mark the superblock dirty so that kupdated will flush... */
-	jffs2_erase_pending_trigger(c);
+	jffs2_dirty_trigger(c);
 
 	if (jffs2_wbuf_pending_for_ino(c, ino))
 		return;
-- 
1.6.4.4

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

[Joakim Tjernlund]

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(-)

diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c
index 3ff50da..6cc014c 100644
--- a/fs/jffs2/background.c
+++ b/fs/jffs2/background.c
@@ -146,7 +146,9 @@ static int jffs2_garbage_collect_thread(void *_c)
 		disallow_signal(SIGHUP);

 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
-		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
+		if (jffs2_erase_pending_blocks(c, 1))
+			/* Nothing more to do ATM */;
+		else if (jffs2_garbage_collect_pass(c) == -ENOSPC) {
 			printk(KERN_NOTICE "No space for garbage collection. Aborting GC thread\n");
 			goto die;
 		}
diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index b47679b..2a6cc6c 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -103,9 +103,10 @@ static void jffs2_erase_block(struct jffs2_sb_info *c,
 	jffs2_erase_failed(c, jeb, bad_offset);
 }

-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 {
 	struct jffs2_eraseblock *jeb;
+	int work_done = 0;

 	mutex_lock(&c->erase_free_sem);

@@ -123,6 +124,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)

 			if (!--count) {
 				D1(printk(KERN_DEBUG "Count reached. jffs2_erase_pending_blocks leaving\n"));
+				work_done = 1;
 				goto done;
 			}

@@ -157,6 +159,7 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count)
 	mutex_unlock(&c->erase_free_sem);
  done:
 	D1(printk(KERN_DEBUG "jffs2_erase_pending_blocks completed\n"));
+	return work_done;
 }

 static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
@@ -167,8 +170,6 @@ static void jffs2_erase_succeeded(struct jffs2_sb_info *c, struct jffs2_eraseblo
 	list_move_tail(&jeb->list, &c->erase_complete_list);
 	spin_unlock(&c->erase_completion_lock);
 	mutex_unlock(&c->erase_free_sem);
-	/* Ensure that kupdated calls us again to mark them clean */
-	jffs2_erase_pending_trigger(c);
 }

 static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 507ed6e..4b1848c 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -464,7 +464,7 @@ int jffs2_scan_dirty_space(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb
 int jffs2_do_mount_fs(struct jffs2_sb_info *c);

 /* erase.c */
-void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
+int jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count);
 void jffs2_free_jeb_node_refs(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);

 #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
diff --git a/fs/jffs2/nodemgmt.c b/fs/jffs2/nodemgmt.c
index 21a0529..155fd63 100644
--- a/fs/jffs2/nodemgmt.c
+++ b/fs/jffs2/nodemgmt.c
@@ -733,6 +733,10 @@ int jffs2_thread_should_wake(struct jffs2_sb_info *c)
 	int nr_very_dirty = 0;
 	struct jffs2_eraseblock *jeb;

+	if (!list_empty(&c->erase_complete_list) ||
+	    !list_empty(&c->erase_pending_list))
+		return 1;
+
 	if (c->unchecked_size) {
 		D1(printk(KERN_DEBUG "jffs2_thread_should_wake(): unchecked_size %d, checked_ino #%d\n",
 			  c->unchecked_size, c->checked_ino));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index a7f03b7..5d26362 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -140,8 +140,7 @@ void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);

 #endif /* WRITEBUFFER */

-/* erase.c */
-static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 {
 	OFNI_BS_2SFFJ(c)->s_dirt = 1;
 }
@@ -151,6 +150,12 @@ int jffs2_start_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c);
 void jffs2_garbage_collect_trigger(struct jffs2_sb_info *c);

+/* erase.c */
+static inline void jffs2_erase_pending_trigger(struct jffs2_sb_info *c)
+{
+	jffs2_garbage_collect_trigger(c);
+}
+
 /* dir.c */
 extern const struct file_operations jffs2_dir_operations;
 extern const struct inode_operations jffs2_dir_inode_operations;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 9a80e8e..511e2d6 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -63,8 +63,6 @@ static void jffs2_write_super(struct super_block *sb)

 	if (!(sb->s_flags & MS_RDONLY)) {
 		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
-		jffs2_garbage_collect_trigger(c);
-		jffs2_erase_pending_blocks(c, 0);
 		jffs2_flush_wbuf_gc(c, 0);
 	}

diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index 5ef7bac..f319efc 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -84,7 +84,7 @@ static void jffs2_wbuf_dirties_inode(struct jffs2_sb_info *c, uint32_t ino)
 	struct jffs2_inodirty *new;

 	/* Mark the superblock dirty so that kupdated will flush... */
-	jffs2_erase_pending_trigger(c);
+	jffs2_dirty_trigger(c);

 	if (jffs2_wbuf_pending_for_ino(c, ino))
 		return;
--
1.6.4.4