| Message ID | 1266394581-32450-2-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2010-02-17 at 09:16 +0100, Joakim Tjernlund wrote: > diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c > index b47679b..1ca2559 100644 > --- a/fs/jffs2/erase.c > +++ b/fs/jffs2/erase.c > @@ -114,6 +114,11 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count) > while (!list_empty(&c->erase_complete_list) || > !list_empty(&c->erase_pending_list)) { > > + if (signal_pending(current)) { > + spin_unlock(&c->erase_completion_lock); > + mutex_unlock(&c->erase_free_sem); > + goto done; > + } Err, I thought you would remove signal checking from this function? It really does not belong here.
Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/02/18 12:06:06: > > On Wed, 2010-02-17 at 09:16 +0100, Joakim Tjernlund wrote: > > diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c > > index b47679b..1ca2559 100644 > > --- a/fs/jffs2/erase.c > > +++ b/fs/jffs2/erase.c > > @@ -114,6 +114,11 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info > *c, int count) > > while (!list_empty(&c->erase_complete_list) || > > !list_empty(&c->erase_pending_list)) { > > > > + if (signal_pending(current)) { > > + spin_unlock(&c->erase_completion_lock); > > + mutex_unlock(&c->erase_free_sem); > > + goto done; > > + } > > Err, I thought you would remove signal checking from this function? It > really does not belong here. Slight misunderstanding, it needs to be there otherwise my patches won't do any good. Removing signal from this function basically means that we should remove count and only erase one block per invocation. Basically the while !empty loop needs to move into the GC task instead. This is a somewhat bigger surgery but doable. Jocke
On Thu, 2010-02-18 at 13:17 +0100, Joakim Tjernlund wrote: > Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/02/18 12:06:06: > > > > On Wed, 2010-02-17 at 09:16 +0100, Joakim Tjernlund wrote: > > > diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c > > > index b47679b..1ca2559 100644 > > > --- a/fs/jffs2/erase.c > > > +++ b/fs/jffs2/erase.c > > > @@ -114,6 +114,11 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info > > *c, int count) > > > while (!list_empty(&c->erase_complete_list) || > > > !list_empty(&c->erase_pending_list)) { > > > > > > + if (signal_pending(current)) { > > > + spin_unlock(&c->erase_completion_lock); > > > + mutex_unlock(&c->erase_free_sem); > > > + goto done; > > > + } > > > > Err, I thought you would remove signal checking from this function? It > > really does not belong here. > > Slight misunderstanding, it needs to be there otherwise my patches won't do any good. > Removing signal from this function basically means that we should remove count > and only erase one block per invocation. Basically the while !empty loop needs to move > into the GC task instead. This is a somewhat bigger surgery but doable. Right. You can move the signal check to the BGT, and do 1-3 erases at a time by passing cnt = 1-3. I think it is cleaner.
Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/02/18 13:22:44: > > On Thu, 2010-02-18 at 13:17 +0100, Joakim Tjernlund wrote: > > Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/02/18 12:06:06: > > > > > > On Wed, 2010-02-17 at 09:16 +0100, Joakim Tjernlund wrote: > > > > diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c > > > > index b47679b..1ca2559 100644 > > > > --- a/fs/jffs2/erase.c > > > > +++ b/fs/jffs2/erase.c > > > > @@ -114,6 +114,11 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info > > > *c, int count) > > > > while (!list_empty(&c->erase_complete_list) || > > > > !list_empty(&c->erase_pending_list)) { > > > > > > > > + if (signal_pending(current)) { > > > > + spin_unlock(&c->erase_completion_lock); > > > > + mutex_unlock(&c->erase_free_sem); > > > > + goto done; > > > > + } > > > > > > Err, I thought you would remove signal checking from this function? It > > > really does not belong here. > > > > Slight misunderstanding, it needs to be there otherwise my patches won't do any good. > > Removing signal from this function basically means that we should remove count > > and only erase one block per invocation. Basically the while !empty loop needs to move > > into the GC task instead. This is a somewhat bigger surgery but doable. > > Right. You can move the signal check to the BGT, and do 1-3 erases at a > time by passing cnt = 1-3. I think it is cleaner. Doing this warrants a return value from jffs2_erase_pending_blocks() as testing for empty lists will be clumsy in both GC and erase.c All in all this will bring some more churn in the code so I really don't won't to do all that for nothing, that is, David are you happy with this too? Question, is it really necessary to take erase_free_sem lock before testing for list_empty? Jocke
diff --git a/fs/jffs2/background.c b/fs/jffs2/background.c index 3ff50da..a8e0140 100644 --- a/fs/jffs2/background.c +++ b/fs/jffs2/background.c @@ -146,6 +146,7 @@ static int jffs2_garbage_collect_thread(void *_c) disallow_signal(SIGHUP); D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n")); + jffs2_erase_pending_blocks(c, 0); 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..1ca2559 100644 --- a/fs/jffs2/erase.c +++ b/fs/jffs2/erase.c @@ -114,6 +114,11 @@ void jffs2_erase_pending_blocks(struct jffs2_sb_info *c, int count) while (!list_empty(&c->erase_complete_list) || !list_empty(&c->erase_pending_list)) { + if (signal_pending(current)) { + spin_unlock(&c->erase_completion_lock); + mutex_unlock(&c->erase_free_sem); + goto done; + } if (!list_empty(&c->erase_complete_list)) { jeb = list_entry(c->erase_complete_list.next, struct jffs2_eraseblock, list); list_move(&jeb->list, &c->erase_checking_list); 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/super.c b/fs/jffs2/super.c index 9a80e8e..5162329 100644 --- a/fs/jffs2/super.c +++ b/fs/jffs2/super.c @@ -64,7 +64,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); }
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. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> --- fs/jffs2/background.c | 1 + fs/jffs2/erase.c | 5 +++++ fs/jffs2/nodemgmt.c | 4 ++++ fs/jffs2/super.c | 1 - 4 files changed, 10 insertions(+), 1 deletions(-)