| Message ID | 1266249781-27970-1-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2010-02-15 at 17:03 +0100, Joakim Tjernlund wrote: > Erasing blocks is a form of GC and therefor it should live in the > GC task. By moving it there two problems will be solved: > 1) umounting 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 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(-) > > 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; > + } I think this is not very good split of functionality. 'jffs2_erase_pending_blocks()' should be stupid, it should just erase as many EBs as it was asked to. It should have zero knowledge about signals. I think you should remove the signals checking from here and move that to the GC thread. Also, I think you should call it with count = 1, so that you would erase only one EB at one iteration. This way you will let the GC thread do other things with less latency as well.
Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/02/16 09:57:52: > > On Mon, 2010-02-15 at 17:03 +0100, Joakim Tjernlund wrote: > > Erasing blocks is a form of GC and therefor it should live in the > > GC task. By moving it there two problems will be solved: > > 1) umounting 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 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(-) > > > > 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; > > + } > > I think this is not very good split of functionality. > 'jffs2_erase_pending_blocks()' should be stupid, it should just erase as > many EBs as it was asked to. It should have zero knowledge about > signals. hmm, you have a point there. > > I think you should remove the signals checking from here and move that > to the GC thread. > > Also, I think you should call it with count = 1, so that you would erase > only one EB at one iteration. This way you will let the GC thread do > other things with less latency as well. If we reduce the count to 1 one might just as well remove count from jffs2_erase_pending_blocks() and make it just erase one block on each invocation, not sure I like that as it will be more overhead when erasing many blocks. I figure erasing blocks is the most rewarding form of GC so there should be no need to reduce latency for other things, can you think of a reason? Jocke
On Tue, 2010-02-16 at 10:18 +0100, Joakim Tjernlund wrote: > > Also, I think you should call it with count = 1, so that you would erase > > only one EB at one iteration. This way you will let the GC thread do > > other things with less latency as well. > > If we reduce the count to 1 one might just as well remove count from > jffs2_erase_pending_blocks() and make it just erase one block on each invocation, > not sure I like that as it will be more overhead when erasing many blocks. > > I figure erasing blocks is the most rewarding form of GC so there should > be no need to reduce latency for other things, can you think of a reason? Hmm, yes, you are right. Please, discard my "erase only 1" suggestion.
Artem Bityutskiy <dedekind1@gmail.com> wrote on 2010/02/16 10:45:35: > > On Tue, 2010-02-16 at 10:18 +0100, Joakim Tjernlund wrote: > > > Also, I think you should call it with count = 1, so that you would erase > > > only one EB at one iteration. This way you will let the GC thread do > > > other things with less latency as well. > > > > If we reduce the count to 1 one might just as well remove count from > > jffs2_erase_pending_blocks() and make it just erase one block on each invocation, > > not sure I like that as it will be more overhead when erasing many blocks. > > > > I figure erasing blocks is the most rewarding form of GC so there should > > be no need to reduce latency for other things, can you think of a reason? > > Hmm, yes, you are right. Please, discard my "erase only 1" suggestion. OK. Both you and Kai-Uwe wants back jffs2_erase_pending_trigger() so I will redo this part. Jocke BTW, I see UBIFS is using CRC16 as well as CRC32. Is there a reason you are using CRC16 in UBIFS. Any particular reason for that? CRC16 is much slower than crc32 in linux since the CRC32 impl. was optimized by yours truly long time ago just because it was impacting mount time for JFFS2. Jocke
On Tue, 2010-02-16 at 12:49 +0100, Joakim Tjernlund wrote: > BTW, I see UBIFS is using CRC16 as well as CRC32. Is there a reason > you are using CRC16 in UBIFS. Any particular reason for that? CRC16 is > much slower than crc32 in linux since the CRC32 impl. was optimized > by yours truly long time ago just because it was impacting mount time > for JFFS2. We use crc16 only for LPT (Leb Properties Tree). The tree is very optimized to be as small as possible, and we protect very short pieces of data, this is why crc16 was choosed. Everything else is protected with crc32.
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 therefor it should live in the GC task. By moving it there two problems will be solved: 1) umounting 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 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(-)