Patchwork jffs2: Move erasing from write_super to GC.

login
register
mail settings
Submitter David Woodhouse
Date May 18, 2010, 11:42 p.m.
Message ID <1274226122.6930.8416.camel@macbook.infradead.org>
Download mbox | patch
Permalink /patch/52925/
State New
Headers show

Comments

David Woodhouse - May 18, 2010, 11:42 p.m.
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().
Joakim Tjernlund - May 19, 2010, 9:47 a.m.
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.
David Woodhouse - May 19, 2010, 10:05 a.m.
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.
Joakim Tjernlund - May 19, 2010, 10:36 a.m.
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
David Woodhouse - May 19, 2010, 4:49 p.m.
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.
Joakim Tjernlund - May 19, 2010, 8:06 p.m.
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
David Woodhouse - May 19, 2010, 9:24 p.m.
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.

Patch

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;