Message ID | 1274226122.6930.8416.camel@macbook.infradead.org |
---|---|
State | New, archived |
Headers | show |
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.
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 <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
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.
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
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.
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;