From 7b6d91daee5cac6402186ff224c3af39d79f4a0e Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Sat, 7 Aug 2010 18:20:39 +0200 Subject: block: unify flags for struct bio and struct request Remove the current bio flags and reuse the request flags for the bio, too. This allows to more easily trace the type of I/O from the filesystem down to the block driver. There were two flags in the bio that were missing in the requests: BIO_RW_UNPLUG and BIO_RW_AHEAD. Also I've renamed two request flags that had a superflous RW in them. Note that the flags are in bio.h despite having the REQ_ name - as blkdev.h includes bio.h that is the only way to go for now. Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/page_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'mm') diff --git a/mm/page_io.c b/mm/page_io.c index 31a3b962230..2dee975bf46 100644 --- a/mm/page_io.c +++ b/mm/page_io.c @@ -106,7 +106,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc) goto out; } if (wbc->sync_mode == WB_SYNC_ALL) - rw |= (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG); + rw |= REQ_SYNC | REQ_UNPLUG; count_vm_event(PSWPOUT); set_page_writeback(page); unlock_page(page); -- cgit v1.2.3 From c1955ce32fdb0877b7a1b22feb2669358f65be76 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Sat, 19 Jun 2010 23:08:06 +0200 Subject: writeback: remove wb_list The wb_list member of struct backing_device_info always has exactly one element. Just use the direct bdi->wb pointer instead and simplify some code. Also remove bdi_task_init which is now trivial to prepare for the next patch. Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 83 ++++++++++++++++++++------------------------------------ 1 file changed, 29 insertions(+), 54 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 123bcef13e5..6c2a09c8922 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -65,28 +65,21 @@ static void bdi_debug_init(void) static int bdi_debug_stats_show(struct seq_file *m, void *v) { struct backing_dev_info *bdi = m->private; - struct bdi_writeback *wb; + struct bdi_writeback *wb = &bdi->wb; unsigned long background_thresh; unsigned long dirty_thresh; unsigned long bdi_thresh; unsigned long nr_dirty, nr_io, nr_more_io, nr_wb; struct inode *inode; - /* - * inode lock is enough here, the bdi->wb_list is protected by - * RCU on the reader side - */ nr_wb = nr_dirty = nr_io = nr_more_io = 0; spin_lock(&inode_lock); - list_for_each_entry(wb, &bdi->wb_list, list) { - nr_wb++; - list_for_each_entry(inode, &wb->b_dirty, i_list) - nr_dirty++; - list_for_each_entry(inode, &wb->b_io, i_list) - nr_io++; - list_for_each_entry(inode, &wb->b_more_io, i_list) - nr_more_io++; - } + list_for_each_entry(inode, &wb->b_dirty, i_list) + nr_dirty++; + list_for_each_entry(inode, &wb->b_io, i_list) + nr_io++; + list_for_each_entry(inode, &wb->b_more_io, i_list) + nr_more_io++; spin_unlock(&inode_lock); get_dirty_limits(&background_thresh, &dirty_thresh, &bdi_thresh, bdi); @@ -98,19 +91,16 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) "BdiDirtyThresh: %8lu kB\n" "DirtyThresh: %8lu kB\n" "BackgroundThresh: %8lu kB\n" - "WritebackThreads: %8lu\n" "b_dirty: %8lu\n" "b_io: %8lu\n" "b_more_io: %8lu\n" "bdi_list: %8u\n" - "state: %8lx\n" - "wb_list: %8u\n", + "state: %8lx\n", (unsigned long) K(bdi_stat(bdi, BDI_WRITEBACK)), (unsigned long) K(bdi_stat(bdi, BDI_RECLAIMABLE)), K(bdi_thresh), K(dirty_thresh), - K(background_thresh), nr_wb, nr_dirty, nr_io, nr_more_io, - !list_empty(&bdi->bdi_list), bdi->state, - !list_empty(&bdi->wb_list)); + K(background_thresh), nr_dirty, nr_io, nr_more_io, + !list_empty(&bdi->bdi_list), bdi->state); #undef K return 0; @@ -270,24 +260,6 @@ static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi) INIT_LIST_HEAD(&wb->b_more_io); } -static void bdi_task_init(struct backing_dev_info *bdi, - struct bdi_writeback *wb) -{ - struct task_struct *tsk = current; - - spin_lock(&bdi->wb_lock); - list_add_tail_rcu(&wb->list, &bdi->wb_list); - spin_unlock(&bdi->wb_lock); - - tsk->flags |= PF_FLUSHER | PF_SWAPWRITE; - set_freezable(); - - /* - * Our parent may run at a different priority, just set us to normal - */ - set_user_nice(tsk, 0); -} - static int bdi_start_fn(void *ptr) { struct bdi_writeback *wb = ptr; @@ -301,7 +273,13 @@ static int bdi_start_fn(void *ptr) list_add_rcu(&bdi->bdi_list, &bdi_list); spin_unlock_bh(&bdi_lock); - bdi_task_init(bdi, wb); + current->flags |= PF_FLUSHER | PF_SWAPWRITE; + set_freezable(); + + /* + * Our parent may run at a different priority, just set us to normal + */ + set_user_nice(current, 0); /* * Clear pending bit and wakeup anybody waiting to tear us down @@ -312,12 +290,7 @@ static int bdi_start_fn(void *ptr) ret = bdi_writeback_task(wb); - /* - * Remove us from the list - */ - spin_lock(&bdi->wb_lock); - list_del_rcu(&wb->list); - spin_unlock(&bdi->wb_lock); + wb->task = NULL; /* * Flush any work that raced with us exiting. No new work @@ -326,7 +299,6 @@ static int bdi_start_fn(void *ptr) if (!list_empty(&bdi->work_list)) wb_do_writeback(wb, 1); - wb->task = NULL; return ret; } @@ -391,7 +363,13 @@ static int bdi_forker_task(void *ptr) { struct bdi_writeback *me = ptr; - bdi_task_init(me->bdi, me); + current->flags |= PF_FLUSHER | PF_SWAPWRITE; + set_freezable(); + + /* + * Our parent may run at a different priority, just set us to normal + */ + set_user_nice(current, 0); for (;;) { struct backing_dev_info *bdi, *tmp; @@ -598,8 +576,6 @@ EXPORT_SYMBOL(bdi_register_dev); */ static void bdi_wb_shutdown(struct backing_dev_info *bdi) { - struct bdi_writeback *wb; - if (!bdi_cap_writeback_dirty(bdi)) return; @@ -615,14 +591,14 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi) bdi_remove_from_list(bdi); /* - * Finally, kill the kernel threads. We don't need to be RCU + * Finally, kill the kernel thread. We don't need to be RCU * safe anymore, since the bdi is gone from visibility. Force * unfreeze of the thread before calling kthread_stop(), otherwise * it would never exet if it is currently stuck in the refrigerator. */ - list_for_each_entry(wb, &bdi->wb_list, list) { - thaw_process(wb->task); - kthread_stop(wb->task); + if (bdi->wb.task) { + thaw_process(bdi->wb.task); + kthread_stop(bdi->wb.task); } } @@ -667,7 +643,6 @@ int bdi_init(struct backing_dev_info *bdi) spin_lock_init(&bdi->wb_lock); INIT_RCU_HEAD(&bdi->rcu_head); INIT_LIST_HEAD(&bdi->bdi_list); - INIT_LIST_HEAD(&bdi->wb_list); INIT_LIST_HEAD(&bdi->work_list); bdi_wb_init(&bdi->wb, bdi); -- cgit v1.2.3 From 082439004b31adc146e96e5f1c574dd2b57dcd93 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Sat, 19 Jun 2010 23:08:22 +0200 Subject: writeback: merge bdi_writeback_task and bdi_start_fn Move all code for the writeback thread into fs/fs-writeback.c instead of splitting it over two functions in two files. Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 44 +------------------------------------------- 1 file changed, 1 insertion(+), 43 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 6c2a09c8922..bceac647e4d 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -260,48 +260,6 @@ static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi) INIT_LIST_HEAD(&wb->b_more_io); } -static int bdi_start_fn(void *ptr) -{ - struct bdi_writeback *wb = ptr; - struct backing_dev_info *bdi = wb->bdi; - int ret; - - /* - * Add us to the active bdi_list - */ - spin_lock_bh(&bdi_lock); - list_add_rcu(&bdi->bdi_list, &bdi_list); - spin_unlock_bh(&bdi_lock); - - current->flags |= PF_FLUSHER | PF_SWAPWRITE; - set_freezable(); - - /* - * Our parent may run at a different priority, just set us to normal - */ - set_user_nice(current, 0); - - /* - * Clear pending bit and wakeup anybody waiting to tear us down - */ - clear_bit(BDI_pending, &bdi->state); - smp_mb__after_clear_bit(); - wake_up_bit(&bdi->state, BDI_pending); - - ret = bdi_writeback_task(wb); - - wb->task = NULL; - - /* - * Flush any work that raced with us exiting. No new work - * will be added, since this bdi isn't discoverable anymore. - */ - if (!list_empty(&bdi->work_list)) - wb_do_writeback(wb, 1); - - return ret; -} - int bdi_has_dirty_io(struct backing_dev_info *bdi) { return wb_has_dirty_io(&bdi->wb); @@ -425,7 +383,7 @@ static int bdi_forker_task(void *ptr) spin_unlock_bh(&bdi_lock); wb = &bdi->wb; - wb->task = kthread_run(bdi_start_fn, wb, "flush-%s", + wb->task = kthread_run(bdi_writeback_thread, wb, "flush-%s", dev_name(bdi->dev)); /* * If task creation fails, then readd the bdi to -- cgit v1.2.3 From 455b2864686d3591b3b2f39eb46290c95f76471f Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Wed, 7 Jul 2010 13:24:06 +1000 Subject: writeback: Initial tracing support Trace queue/sched/exec parts of the writeback loop. This provides insight into when and why flusher threads are scheduled to run. e.g a sync invocation leaves traces like: sync-[...]: writeback_queue: bdi 8:0: sb_dev 8:1 nr_pages=7712 sync_mode=0 kupdate=0 range_cyclic=0 background=0 flush-8:0-[...]: writeback_exec: bdi 8:0: sb_dev 8:1 nr_pages=7712 sync_mode=0 kupdate=0 range_cyclic=0 background=0 This also lays the foundation for adding more writeback tracing to provide deeper insight into the whole writeback path. The original tracing code is from Jens Axboe, though this version is a rewrite as a result of the code being traced changing significantly. Signed-off-by: Dave Chinner Signed-off-by: Jens Axboe --- mm/backing-dev.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index bceac647e4d..ac78a333618 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -10,6 +10,7 @@ #include #include #include +#include static atomic_long_t bdi_seq = ATOMIC_LONG_INIT(0); @@ -518,6 +519,7 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent, bdi_debug_register(bdi, dev_name(dev)); set_bit(BDI_registered, &bdi->state); + trace_writeback_bdi_register(bdi); exit: return ret; } @@ -578,6 +580,7 @@ static void bdi_prune_sb(struct backing_dev_info *bdi) void bdi_unregister(struct backing_dev_info *bdi) { if (bdi->dev) { + trace_writeback_bdi_unregister(bdi); bdi_prune_sb(bdi); if (!bdi_cap_flush_forker(bdi)) -- cgit v1.2.3 From 028c2dd184c097809986684f2f0627eea5529fea Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Wed, 7 Jul 2010 13:24:07 +1000 Subject: writeback: Add tracing to balance_dirty_pages Tracing high level background writeback events is good, but it doesn't give the entire picture. Add visibility into write throttling to catch IO dispatched by foreground throttling of processing dirtying lots of pages. Signed-off-by: Dave Chinner Signed-off-by: Jens Axboe --- mm/page-writeback.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'mm') diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 37498ef6154..d556cd829af 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -34,6 +34,7 @@ #include #include #include +#include /* * After a CPU has dirtied this many pages, balance_dirty_pages_ratelimited @@ -535,11 +536,13 @@ static void balance_dirty_pages(struct address_space *mapping, * threshold otherwise wait until the disk writes catch * up. */ + trace_wbc_balance_dirty_start(&wbc, bdi); if (bdi_nr_reclaimable > bdi_thresh) { writeback_inodes_wb(&bdi->wb, &wbc); pages_written += write_chunk - wbc.nr_to_write; get_dirty_limits(&background_thresh, &dirty_thresh, &bdi_thresh, bdi); + trace_wbc_balance_dirty_written(&wbc, bdi); } /* @@ -565,6 +568,7 @@ static void balance_dirty_pages(struct address_space *mapping, if (pages_written >= write_chunk) break; /* We've done our duty */ + trace_wbc_balance_dirty_wait(&wbc, bdi); __set_current_state(TASK_INTERRUPTIBLE); io_schedule_timeout(pause); -- cgit v1.2.3 From 9e094383b60066996fbc3b53891324e5d2ec858d Mon Sep 17 00:00:00 2001 From: Dave Chinner Date: Wed, 7 Jul 2010 13:24:08 +1000 Subject: writeback: Add tracing to write_cache_pages Add a trace event to the ->writepage loop in write_cache_pages to give visibility into how the ->writepage call is changing variables within the writeback control structure. Of most interest is how wbc->nr_to_write changes from call to call, especially with filesystems that write multiple pages in ->writepage. Signed-off-by: Dave Chinner Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/page-writeback.c | 1 + 1 file changed, 1 insertion(+) (limited to 'mm') diff --git a/mm/page-writeback.c b/mm/page-writeback.c index d556cd829af..3d2111a2223 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -933,6 +933,7 @@ continue_unlock: if (!clear_page_dirty_for_io(page)) goto continue_unlock; + trace_wbc_writepage(wbc, mapping->backing_dev_info); ret = (*writepage)(page, wbc, data); if (unlikely(ret)) { if (ret == AOP_WRITEPAGE_ACTIVATE) { -- cgit v1.2.3 From 6f904ff0e39ea88f81eb77e8dfb4e1238492f0a8 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:11 +0300 Subject: writeback: harmonize writeback threads naming The write-back code mixes words "thread" and "task" for the same things. This is not a big deal, but still an inconsistency. hch: a convention I tend to use and I've seen in various places is to always use _task for the storage of the task_struct pointer, and thread everywhere else. This especially helps with having foo_thread for the actual thread and foo_task for a global variable keeping the task_struct pointer This patch renames: * 'bdi_add_default_flusher_task()' -> 'bdi_add_default_flusher_thread()' * 'bdi_forker_task()' -> 'bdi_forker_thread()' because bdi threads are 'bdi_writeback_thread()', so these names are more consistent. This patch also amends commentaries and makes them refer the forker and bdi threads as "thread", not "task". Also, while on it, make 'bdi_add_default_flusher_thread()' declaration use 'static void' instead of 'void static' and make checkpatch.pl happy. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index ac78a333618..4e9ed2a8521 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -50,7 +50,7 @@ static struct timer_list sync_supers_timer; static int bdi_sync_supers(void *); static void sync_supers_timer_fn(unsigned long); -static void bdi_add_default_flusher_task(struct backing_dev_info *bdi); +static void bdi_add_default_flusher_thread(struct backing_dev_info *bdi); #ifdef CONFIG_DEBUG_FS #include @@ -279,10 +279,10 @@ static void bdi_flush_io(struct backing_dev_info *bdi) } /* - * kupdated() used to do this. We cannot do it from the bdi_forker_task() + * kupdated() used to do this. We cannot do it from the bdi_forker_thread() * or we risk deadlocking on ->s_umount. The longer term solution would be * to implement sync_supers_bdi() or similar and simply do it from the - * bdi writeback tasks individually. + * bdi writeback thread individually. */ static int bdi_sync_supers(void *unused) { @@ -318,7 +318,7 @@ static void sync_supers_timer_fn(unsigned long unused) bdi_arm_supers_timer(); } -static int bdi_forker_task(void *ptr) +static int bdi_forker_thread(void *ptr) { struct bdi_writeback *me = ptr; @@ -354,7 +354,7 @@ static int bdi_forker_task(void *ptr) !bdi_has_dirty_io(bdi)) continue; - bdi_add_default_flusher_task(bdi); + bdi_add_default_flusher_thread(bdi); } set_current_state(TASK_INTERRUPTIBLE); @@ -376,7 +376,7 @@ static int bdi_forker_task(void *ptr) /* * This is our real job - check for pending entries in - * bdi_pending_list, and create the tasks that got added + * bdi_pending_list, and create the threads that got added */ bdi = list_entry(bdi_pending_list.next, struct backing_dev_info, bdi_list); @@ -387,7 +387,7 @@ static int bdi_forker_task(void *ptr) wb->task = kthread_run(bdi_writeback_thread, wb, "flush-%s", dev_name(bdi->dev)); /* - * If task creation fails, then readd the bdi to + * If thread creation fails, then readd the bdi to * the pending list and force writeout of the bdi * from this forker thread. That will free some memory * and we can try again. @@ -430,10 +430,10 @@ static void bdi_add_to_pending(struct rcu_head *head) } /* - * Add the default flusher task that gets created for any bdi + * Add the default flusher thread that gets created for any bdi * that has dirty data pending writeout */ -void static bdi_add_default_flusher_task(struct backing_dev_info *bdi) +static void bdi_add_default_flusher_thread(struct backing_dev_info *bdi) { if (!bdi_cap_writeback_dirty(bdi)) return; @@ -445,10 +445,10 @@ void static bdi_add_default_flusher_task(struct backing_dev_info *bdi) } /* - * Check with the helper whether to proceed adding a task. Will only + * Check with the helper whether to proceed adding a thread. Will only * abort if we two or more simultanous calls to - * bdi_add_default_flusher_task() occured, further additions will block - * waiting for previous additions to finish. + * bdi_add_default_flusher_thread() occured, further additions will + * block waiting for previous additions to finish. */ if (!test_and_set_bit(BDI_pending, &bdi->state)) { list_del_rcu(&bdi->bdi_list); @@ -506,7 +506,7 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent, if (bdi_cap_flush_forker(bdi)) { struct bdi_writeback *wb = &bdi->wb; - wb->task = kthread_run(bdi_forker_task, wb, "bdi-%s", + wb->task = kthread_run(bdi_forker_thread, wb, "bdi-%s", dev_name(dev)); if (IS_ERR(wb->task)) { wb->task = NULL; -- cgit v1.2.3 From 94eac5e62364df4e605e451218ee6024a7ba664f Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:12 +0300 Subject: writeback: fix possible race when creating bdi threads This patch fixes a very unlikely race condition on the bdi forker thread error path: when bdi thread creation fails, 'bdi->wb.task' may contain the error code for a short period of time. If at the same time someone submits a work to this bdi, we can end up with an oops 'bdi_queue_work()' while executing 'wake_up_process(wb->task)'. This patch fixes the issue by introducing a temporary variable 'task' and storing the possible error code there, so that 'wb->task' would never take erroneous values. Note, this race is very unlikely and I never hit it, so it is theoretical, but nevertheless worth fixing. This patch also merges 2 comments which were previously separate. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 4e9ed2a8521..327e36d1d62 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -331,8 +331,8 @@ static int bdi_forker_thread(void *ptr) set_user_nice(current, 0); for (;;) { + struct task_struct *task; struct backing_dev_info *bdi, *tmp; - struct bdi_writeback *wb; /* * Temporary measure, we want to make sure we don't see @@ -383,29 +383,23 @@ static int bdi_forker_thread(void *ptr) list_del_init(&bdi->bdi_list); spin_unlock_bh(&bdi_lock); - wb = &bdi->wb; - wb->task = kthread_run(bdi_writeback_thread, wb, "flush-%s", - dev_name(bdi->dev)); - /* - * If thread creation fails, then readd the bdi to - * the pending list and force writeout of the bdi - * from this forker thread. That will free some memory - * and we can try again. - */ - if (IS_ERR(wb->task)) { - wb->task = NULL; - + task = kthread_run(bdi_writeback_thread, &bdi->wb, "flush-%s", + dev_name(bdi->dev)); + if (IS_ERR(task)) { /* - * Add this 'bdi' to the back, so we get - * a chance to flush other bdi's to free - * memory. + * If thread creation fails, then readd the bdi back to + * the list and force writeout of the bdi from this + * forker thread. That will free some memory and we can + * try again. Add it to the tail so we get a chance to + * flush other bdi's to free memory. */ spin_lock_bh(&bdi_lock); list_add_tail(&bdi->bdi_list, &bdi_pending_list); spin_unlock_bh(&bdi_lock); bdi_flush_io(bdi); - } + } else + bdi->wb.task = task; } return 0; -- cgit v1.2.3 From c5f7ad233b8805dae06e694538d8095b19f3c560 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:13 +0300 Subject: writeback: do not lose wake-ups in the forker thread - 1 Currently the forker thread can lose wake-ups which may lead to unnecessary delays in processing bdi works. E.g., consider the following scenario. 1. 'bdi_forker_thread()' walks the 'bdi_list', finds out there is nothing to do, and is about to finish the loop. 2. A bdi thread decides to exit because it was inactive for long time. 3. 'bdi_queue_work()' adds a work to the bdi which just exited, so it wakes up the forker thread. 4. but 'bdi_forker_thread()' executes 'set_current_state(TASK_INTERRUPTIBLE)' and goes sleep. We lose a wake-up. Losing the wake-up is not fatal, but this means that the bdi work processing will be delayed by up to 5 sec. This race is theoretical, I never hit it, but it is worth fixing. The fix is to execute 'set_current_state(TASK_INTERRUPTIBLE)' _before_ walking 'bdi_list', not after. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 327e36d1d62..b1dc2d4b9cd 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -342,6 +342,7 @@ static int bdi_forker_thread(void *ptr) wb_do_writeback(me, 0); spin_lock_bh(&bdi_lock); + set_current_state(TASK_INTERRUPTIBLE); /* * Check if any existing bdi's have dirty data without @@ -357,8 +358,6 @@ static int bdi_forker_thread(void *ptr) bdi_add_default_flusher_thread(bdi); } - set_current_state(TASK_INTERRUPTIBLE); - if (list_empty(&bdi_pending_list)) { unsigned long wait; -- cgit v1.2.3 From c4ec7908c2c5125f75fabd100e7a95626a6883ee Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:14 +0300 Subject: writeback: do not lose wake-ups in the forker thread - 2 Currently, if someone submits jobs for the default bdi, we can lose wake-up events. E.g., this can happen if 'bdi_queue_work()' is called when 'bdi_forker_thread()' is executing code after 'wb_do_writeback(me, 0)', but before 'set_current_state(TASK_INTERRUPTIBLE)'. This situation is unlikely, and the result is not very severe - we'll just delay the execution of the work, but this is still not very nice. This patch fixes the issue by checking whether the default bdi has works before the forker thread goes sleep. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index b1dc2d4b9cd..72e6eb96efe 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -358,6 +358,10 @@ static int bdi_forker_thread(void *ptr) bdi_add_default_flusher_thread(bdi); } + /* Keep working if default bdi still has things to do */ + if (!list_empty(&me->bdi->work_list)) + __set_current_state(TASK_RUNNING); + if (list_empty(&bdi_pending_list)) { unsigned long wait; -- cgit v1.2.3 From 080dcec41709be72613133f695be75b98dd43e88 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:16 +0300 Subject: writeback: simplify bdi code a little This patch simplifies bdi code a little by removing the 'pending_list' which is redundant. Indeed, currently the forker thread ('bdi_forker_thread()') is working like this: 1. In a loop, fetch all bdi's which have works but have no writeback thread and move them to the 'pending_list'. 2. If the list is empty, sleep for 5 sec. 3. Otherwise, take one bdi from the list, fork the writeback thread for this bdi, and repeat the loop. IOW, it first moves everything to the 'pending_list', then process only one element, and so on. This patch simplifies the algorithm, which is now as follows. 1. Find the first bdi which has a work and remove it from the global list of bdi's (bdi_list). 2. If there was not such bdi, sleep 5 sec. 3. Fork the writeback thread for this bdi and repeat the loop. IOW, now we find the first bdi to process, process it, and so on. This is simpler and involves less lists. The bonus now is that we can get rid of a couple of functions, as well as remove complications which involve 'rcu_call()' and 'bdi->rcu_head'. This patch also makes sure we use 'list_add_tail_rcu()', instead of plain 'list_add_tail()', but this piece of code is going to be removed in the next patch anyway. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 82 +++++++++++++------------------------------------------- 1 file changed, 18 insertions(+), 64 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 72e6eb96efe..dbc66815a0f 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -50,8 +50,6 @@ static struct timer_list sync_supers_timer; static int bdi_sync_supers(void *); static void sync_supers_timer_fn(unsigned long); -static void bdi_add_default_flusher_thread(struct backing_dev_info *bdi); - #ifdef CONFIG_DEBUG_FS #include #include @@ -331,6 +329,7 @@ static int bdi_forker_thread(void *ptr) set_user_nice(current, 0); for (;;) { + bool fork = false; struct task_struct *task; struct backing_dev_info *bdi, *tmp; @@ -349,23 +348,30 @@ static int bdi_forker_thread(void *ptr) * a thread registered. If so, set that up. */ list_for_each_entry_safe(bdi, tmp, &bdi_list, bdi_list) { + if (!bdi_cap_writeback_dirty(bdi)) + continue; if (bdi->wb.task) continue; if (list_empty(&bdi->work_list) && !bdi_has_dirty_io(bdi)) continue; - bdi_add_default_flusher_thread(bdi); + WARN(!test_bit(BDI_registered, &bdi->state), + "bdi %p/%s is not registered!\n", bdi, bdi->name); + + list_del_rcu(&bdi->bdi_list); + fork = true; + break; } + spin_unlock_bh(&bdi_lock); /* Keep working if default bdi still has things to do */ if (!list_empty(&me->bdi->work_list)) __set_current_state(TASK_RUNNING); - if (list_empty(&bdi_pending_list)) { + if (!fork) { unsigned long wait; - spin_unlock_bh(&bdi_lock); wait = msecs_to_jiffies(dirty_writeback_interval * 10); if (wait) schedule_timeout(wait); @@ -378,13 +384,13 @@ static int bdi_forker_thread(void *ptr) __set_current_state(TASK_RUNNING); /* - * This is our real job - check for pending entries in - * bdi_pending_list, and create the threads that got added + * Set the pending bit - if someone will try to unregister this + * bdi - it'll wait on this bit. */ - bdi = list_entry(bdi_pending_list.next, struct backing_dev_info, - bdi_list); - list_del_init(&bdi->bdi_list); - spin_unlock_bh(&bdi_lock); + set_bit(BDI_pending, &bdi->state); + + /* Make sure no one uses the picked bdi */ + synchronize_rcu(); task = kthread_run(bdi_writeback_thread, &bdi->wb, "flush-%s", dev_name(bdi->dev)); @@ -397,7 +403,7 @@ static int bdi_forker_thread(void *ptr) * flush other bdi's to free memory. */ spin_lock_bh(&bdi_lock); - list_add_tail(&bdi->bdi_list, &bdi_pending_list); + list_add_tail_rcu(&bdi->bdi_list, &bdi_list); spin_unlock_bh(&bdi_lock); bdi_flush_io(bdi); @@ -408,57 +414,6 @@ static int bdi_forker_thread(void *ptr) return 0; } -static void bdi_add_to_pending(struct rcu_head *head) -{ - struct backing_dev_info *bdi; - - bdi = container_of(head, struct backing_dev_info, rcu_head); - INIT_LIST_HEAD(&bdi->bdi_list); - - spin_lock(&bdi_lock); - list_add_tail(&bdi->bdi_list, &bdi_pending_list); - spin_unlock(&bdi_lock); - - /* - * We are now on the pending list, wake up bdi_forker_task() - * to finish the job and add us back to the active bdi_list - */ - wake_up_process(default_backing_dev_info.wb.task); -} - -/* - * Add the default flusher thread that gets created for any bdi - * that has dirty data pending writeout - */ -static void bdi_add_default_flusher_thread(struct backing_dev_info *bdi) -{ - if (!bdi_cap_writeback_dirty(bdi)) - return; - - if (WARN_ON(!test_bit(BDI_registered, &bdi->state))) { - printk(KERN_ERR "bdi %p/%s is not registered!\n", - bdi, bdi->name); - return; - } - - /* - * Check with the helper whether to proceed adding a thread. Will only - * abort if we two or more simultanous calls to - * bdi_add_default_flusher_thread() occured, further additions will - * block waiting for previous additions to finish. - */ - if (!test_and_set_bit(BDI_pending, &bdi->state)) { - list_del_rcu(&bdi->bdi_list); - - /* - * We must wait for the current RCU period to end before - * moving to the pending list. So schedule that operation - * from an RCU callback. - */ - call_rcu(&bdi->rcu_head, bdi_add_to_pending); - } -} - /* * Remove bdi from bdi_list, and ensure that it is no longer visible */ @@ -599,7 +554,6 @@ int bdi_init(struct backing_dev_info *bdi) bdi->max_ratio = 100; bdi->max_prop_frac = PROP_FRAC_BASE; spin_lock_init(&bdi->wb_lock); - INIT_RCU_HEAD(&bdi->rcu_head); INIT_LIST_HEAD(&bdi->bdi_list); INIT_LIST_HEAD(&bdi->work_list); -- cgit v1.2.3 From 78c40cb6581a74adc48821f3de6b864a54d4c34d Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:17 +0300 Subject: writeback: do not remove bdi from bdi_list The forker thread removes bdis from 'bdi_list' before forking the bdi thread. But this is wrong for at least 2 reasons. Reason #1: if we temporary remove a bdi from the list, we may miss works which would otherwise be given to us. Reason #2: this is racy; indeed, 'bdi_wb_shutdown()' expects that bdis are always in the 'bdi_list' (see 'bdi_remove_from_list()'), and when it races with the forker thread, it can shut down the bdi thread at the same time as the forker creates it. This patch makes sure the forker thread never removes bdis from 'bdi_list' (which was suggested by Christoph Hellwig). In order to make sure that we do not race with 'bdi_wb_shutdown()', we have to hold the 'bdi_lock' while walking the 'bdi_list' and setting the 'BDI_pending' flag. NOTE! The error path is interesting. Currently, when we fail to create a bdi thread, we move the bdi to the tail of 'bdi_list'. But if we never remove the bdi from the list, we cannot move it to the tail either, because then we can mess up the RCU readers which walk the list. And also, we'll have the race described above in "Reason #2". But I not think that adding to the tail is any important so I just do not do that. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index dbc66815a0f..672c17bb32d 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -331,7 +331,7 @@ static int bdi_forker_thread(void *ptr) for (;;) { bool fork = false; struct task_struct *task; - struct backing_dev_info *bdi, *tmp; + struct backing_dev_info *bdi; /* * Temporary measure, we want to make sure we don't see @@ -347,7 +347,7 @@ static int bdi_forker_thread(void *ptr) * Check if any existing bdi's have dirty data without * a thread registered. If so, set that up. */ - list_for_each_entry_safe(bdi, tmp, &bdi_list, bdi_list) { + list_for_each_entry(bdi, &bdi_list, bdi_list) { if (!bdi_cap_writeback_dirty(bdi)) continue; if (bdi->wb.task) @@ -359,8 +359,13 @@ static int bdi_forker_thread(void *ptr) WARN(!test_bit(BDI_registered, &bdi->state), "bdi %p/%s is not registered!\n", bdi, bdi->name); - list_del_rcu(&bdi->bdi_list); fork = true; + + /* + * Set the pending bit - if someone will try to + * unregister this bdi - it'll wait on this bit. + */ + set_bit(BDI_pending, &bdi->state); break; } spin_unlock_bh(&bdi_lock); @@ -383,29 +388,13 @@ static int bdi_forker_thread(void *ptr) __set_current_state(TASK_RUNNING); - /* - * Set the pending bit - if someone will try to unregister this - * bdi - it'll wait on this bit. - */ - set_bit(BDI_pending, &bdi->state); - - /* Make sure no one uses the picked bdi */ - synchronize_rcu(); - task = kthread_run(bdi_writeback_thread, &bdi->wb, "flush-%s", dev_name(bdi->dev)); if (IS_ERR(task)) { /* - * If thread creation fails, then readd the bdi back to - * the list and force writeout of the bdi from this - * forker thread. That will free some memory and we can - * try again. Add it to the tail so we get a chance to - * flush other bdi's to free memory. + * If thread creation fails, force writeout of the bdi + * from the thread. */ - spin_lock_bh(&bdi_lock); - list_add_tail_rcu(&bdi->bdi_list, &bdi_list); - spin_unlock_bh(&bdi_lock); - bdi_flush_io(bdi); } else bdi->wb.task = task; -- cgit v1.2.3 From adf392407076b85816d48714fb8eeaedb2157884 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:19 +0300 Subject: writeback: restructure bdi forker loop a little This patch re-structures the bdi forker a little: 1. Add 'bdi_cap_flush_forker(bdi)' condition check to the bdi loop. The reason for this is that the forker thread can start _before_ the 'BDI_registered' flag is set (see 'bdi_register()'), so the WARN() statement will fire for the default bdi. I observed this warning at boot-up. 2. Introduce an enum 'action' and use "switch" statement in the outer loop. This is a preparation to the further patch which will teach the forker thread killing bdi threads, so we'll have another case in the "switch" statement. This change was suggested by Christoph Hellwig. This patch is just a small step towards the coming change where the forker thread will kill the bdi threads. It should simplify reviewing the following changes, which would otherwise be larger. This patch also amends comments a little. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 69 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 30 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 672c17bb32d..e104e32c2ee 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -329,9 +329,12 @@ static int bdi_forker_thread(void *ptr) set_user_nice(current, 0); for (;;) { - bool fork = false; struct task_struct *task; struct backing_dev_info *bdi; + enum { + NO_ACTION, /* Nothing to do */ + FORK_THREAD, /* Fork bdi thread */ + } action = NO_ACTION; /* * Temporary measure, we want to make sure we don't see @@ -348,25 +351,31 @@ static int bdi_forker_thread(void *ptr) * a thread registered. If so, set that up. */ list_for_each_entry(bdi, &bdi_list, bdi_list) { - if (!bdi_cap_writeback_dirty(bdi)) - continue; - if (bdi->wb.task) - continue; - if (list_empty(&bdi->work_list) && - !bdi_has_dirty_io(bdi)) + bool have_dirty_io; + + if (!bdi_cap_writeback_dirty(bdi) || + bdi_cap_flush_forker(bdi)) continue; WARN(!test_bit(BDI_registered, &bdi->state), "bdi %p/%s is not registered!\n", bdi, bdi->name); - fork = true; + have_dirty_io = !list_empty(&bdi->work_list) || + wb_has_dirty_io(&bdi->wb); /* - * Set the pending bit - if someone will try to - * unregister this bdi - it'll wait on this bit. + * If the bdi has work to do, but the thread does not + * exist - create it. */ - set_bit(BDI_pending, &bdi->state); - break; + if (!bdi->wb.task && have_dirty_io) { + /* + * Set the pending bit - if someone will try to + * unregister this bdi - it'll wait on this bit. + */ + set_bit(BDI_pending, &bdi->state); + action = FORK_THREAD; + break; + } } spin_unlock_bh(&bdi_lock); @@ -374,30 +383,30 @@ static int bdi_forker_thread(void *ptr) if (!list_empty(&me->bdi->work_list)) __set_current_state(TASK_RUNNING); - if (!fork) { - unsigned long wait; + switch (action) { + case FORK_THREAD: + __set_current_state(TASK_RUNNING); + task = kthread_run(bdi_writeback_thread, &bdi->wb, "flush-%s", + dev_name(bdi->dev)); + if (IS_ERR(task)) { + /* + * If thread creation fails, force writeout of + * the bdi from the thread. + */ + bdi_flush_io(bdi); + } else + bdi->wb.task = task; + break; - wait = msecs_to_jiffies(dirty_writeback_interval * 10); - if (wait) - schedule_timeout(wait); + case NO_ACTION: + if (dirty_writeback_interval) + schedule_timeout(msecs_to_jiffies(dirty_writeback_interval * 10)); else schedule(); try_to_freeze(); + /* Back to the main loop */ continue; } - - __set_current_state(TASK_RUNNING); - - task = kthread_run(bdi_writeback_thread, &bdi->wb, "flush-%s", - dev_name(bdi->dev)); - if (IS_ERR(task)) { - /* - * If thread creation fails, force writeout of the bdi - * from the thread. - */ - bdi_flush_io(bdi); - } else - bdi->wb.task = task; } return 0; -- cgit v1.2.3 From fff5b85aa4225a7be157f208277a055822039a9e Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:20 +0300 Subject: writeback: move bdi threads exiting logic to the forker thread Currently, bdi threads can decide to exit if there were no useful activities for 5 minutes. However, this causes nasty races: we can easily oops in the 'bdi_queue_work()' if the bdi thread decides to exit while we are waking it up. And even if we do not oops, but the bdi tread exits immediately after we wake it up, we'd lose the wake-up event and have an unnecessary delay (up to 5 secs) in the bdi work processing. This patch makes the forker thread to be the central place which not only creates bdi threads, but also kills them if they were inactive long enough. This better design-wise. Another reason why this change was done is to prepare for the further changes which will prevent the bdi threads from waking up every 5 sec and wasting power. Indeed, when the task does not wake up periodically anymore, it won't be able to exit either. This patch also moves the the 'wake_up_bit()' call from the bdi thread to the forker thread as well. So now the forker thread sets the BDI_pending bit, then forks the task or kills it, then clears the bit and wakes up the waiting process. The only process which may wain on the bit is 'bdi_wb_shutdown()'. This function was changed as well - now it first removes the bdi from the 'bdi_list', then waits on the 'BDI_pending' bit. Once it wakes up, it is guaranteed that the forker thread won't race with it, because the bdi is not visible. Note, the forker thread sets the 'BDI_pending' bit under the 'bdi->wb_lock' which is essential for proper serialization. And additionally, when we change 'bdi->wb.task', we now take the 'bdi->work_lock', to make sure that we do not lose wake-ups which we otherwise would when raced with, say, 'bdi_queue_work()'. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 58 insertions(+), 11 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index e104e32c2ee..9c1c199f88c 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -316,6 +316,18 @@ static void sync_supers_timer_fn(unsigned long unused) bdi_arm_supers_timer(); } +/* + * Calculate the longest interval (jiffies) bdi threads are allowed to be + * inactive. + */ +static unsigned long bdi_longest_inactive(void) +{ + unsigned long interval; + + interval = msecs_to_jiffies(dirty_writeback_interval * 10); + return max(5UL * 60 * HZ, interval); +} + static int bdi_forker_thread(void *ptr) { struct bdi_writeback *me = ptr; @@ -329,11 +341,12 @@ static int bdi_forker_thread(void *ptr) set_user_nice(current, 0); for (;;) { - struct task_struct *task; + struct task_struct *task = NULL; struct backing_dev_info *bdi; enum { NO_ACTION, /* Nothing to do */ FORK_THREAD, /* Fork bdi thread */ + KILL_THREAD, /* Kill inactive bdi thread */ } action = NO_ACTION; /* @@ -346,10 +359,6 @@ static int bdi_forker_thread(void *ptr) spin_lock_bh(&bdi_lock); set_current_state(TASK_INTERRUPTIBLE); - /* - * Check if any existing bdi's have dirty data without - * a thread registered. If so, set that up. - */ list_for_each_entry(bdi, &bdi_list, bdi_list) { bool have_dirty_io; @@ -376,6 +385,25 @@ static int bdi_forker_thread(void *ptr) action = FORK_THREAD; break; } + + spin_lock(&bdi->wb_lock); + /* + * If there is no work to do and the bdi thread was + * inactive long enough - kill it. The wb_lock is taken + * to make sure no-one adds more work to this bdi and + * wakes the bdi thread up. + */ + if (bdi->wb.task && !have_dirty_io && + time_after(jiffies, bdi->wb.last_active + + bdi_longest_inactive())) { + task = bdi->wb.task; + bdi->wb.task = NULL; + spin_unlock(&bdi->wb_lock); + set_bit(BDI_pending, &bdi->state); + action = KILL_THREAD; + break; + } + spin_unlock(&bdi->wb_lock); } spin_unlock_bh(&bdi_lock); @@ -394,8 +422,20 @@ static int bdi_forker_thread(void *ptr) * the bdi from the thread. */ bdi_flush_io(bdi); - } else + } else { + /* + * The spinlock makes sure we do not lose + * wake-ups when racing with 'bdi_queue_work()'. + */ + spin_lock(&bdi->wb_lock); bdi->wb.task = task; + spin_unlock(&bdi->wb_lock); + } + break; + + case KILL_THREAD: + __set_current_state(TASK_RUNNING); + kthread_stop(task); break; case NO_ACTION: @@ -407,6 +447,13 @@ static int bdi_forker_thread(void *ptr) /* Back to the main loop */ continue; } + + /* + * Clear pending bit and wakeup anybody waiting to tear us down. + */ + clear_bit(BDI_pending, &bdi->state); + smp_mb__after_clear_bit(); + wake_up_bit(&bdi->state, BDI_pending); } return 0; @@ -490,15 +537,15 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi) return; /* - * If setup is pending, wait for that to complete first + * Make sure nobody finds us on the bdi_list anymore */ - wait_on_bit(&bdi->state, BDI_pending, bdi_sched_wait, - TASK_UNINTERRUPTIBLE); + bdi_remove_from_list(bdi); /* - * Make sure nobody finds us on the bdi_list anymore + * If setup is pending, wait for that to complete first */ - bdi_remove_from_list(bdi); + wait_on_bit(&bdi->state, BDI_pending, bdi_sched_wait, + TASK_UNINTERRUPTIBLE); /* * Finally, kill the kernel thread. We don't need to be RCU -- cgit v1.2.3 From 253c34e9b10c30d3064be654b5b78fbc1a8b1896 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:21 +0300 Subject: writeback: prevent unnecessary bdi threads wakeups Finally, we can get rid of unnecessary wake-ups in bdi threads, which are very bad for battery-driven devices. There are two types of activities bdi threads do: 1. process bdi works from the 'bdi->work_list' 2. periodic write-back So there are 2 sources of wake-up events for bdi threads: 1. 'bdi_queue_work()' - submits bdi works 2. '__mark_inode_dirty()' - adds dirty I/O to bdi's The former already has bdi wake-up code. The latter does not, and this patch adds it. '__mark_inode_dirty()' is hot-path function, but this patch adds another 'spin_lock(&bdi->wb_lock)' there. However, it is taken only in rare cases when the bdi has no dirty inodes. So adding this spinlock should be fine and should not affect performance. This patch makes sure bdi threads and the forker thread do not wake-up if there is nothing to do. The forker thread will nevertheless wake up at least every 5 min. to check whether it has to kill a bdi thread. This can also be optimized, but is not worth it. This patch also tidies up the warning about unregistered bid, and turns it from an ugly crocodile to a simple 'WARN()' statement. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 9c1c199f88c..a9a08d88a74 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -439,10 +439,17 @@ static int bdi_forker_thread(void *ptr) break; case NO_ACTION: - if (dirty_writeback_interval) - schedule_timeout(msecs_to_jiffies(dirty_writeback_interval * 10)); + if (!wb_has_dirty_io(me) || !dirty_writeback_interval) + /* + * There are no dirty data. The only thing we + * should now care about is checking for + * inactive bdi threads and killing them. Thus, + * let's sleep for longer time, save energy and + * be friendly for battery-driven devices. + */ + schedule_timeout(bdi_longest_inactive()); else - schedule(); + schedule_timeout(msecs_to_jiffies(dirty_writeback_interval * 10)); try_to_freeze(); /* Back to the main loop */ continue; -- cgit v1.2.3 From 6467716a37673e8d47b4984eb19839bdad0a8353 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:22 +0300 Subject: writeback: optimize periodic bdi thread wakeups Whe the first inode for a bdi is marked dirty, we wake up the bdi thread which should take care of the periodic background write-out. However, the write-out will actually start only 'dirty_writeback_interval' centisecs later, so we can delay the wake-up. This change was requested by Nick Piggin who pointed out that if we delay the wake-up, we weed out 2 unnecessary contex switches, which matters because '__mark_inode_dirty()' is a hot-path function. This patch introduces a new function - 'bdi_wakeup_thread_delayed()', which sets up a timer to wake-up the bdi thread and returns. So the wake-up is delayed. We also delete the timer in bdi threads just before writing-back. And synchronously delete it when unregistering bdi. At the unregister point the bdi does not have any users, so no one can arm it again. Since now we take 'bdi->wb_lock' in the timer, which can execute in softirq context, we have to use 'spin_lock_bh()' for 'bdi->wb_lock'. This patch makes this change as well. This patch also moves the 'bdi_wb_init()' function down in the file to avoid forward-declaration of 'bdi_wakeup_thread_delayed()'. Signed-off-by: Artem Bityutskiy Signed-off-by: Jens Axboe --- mm/backing-dev.c | 73 +++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 16 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index a9a08d88a74..cfff7225138 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -248,17 +248,6 @@ static int __init default_bdi_init(void) } subsys_initcall(default_bdi_init); -static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi) -{ - memset(wb, 0, sizeof(*wb)); - - wb->bdi = bdi; - wb->last_old_flush = jiffies; - INIT_LIST_HEAD(&wb->b_dirty); - INIT_LIST_HEAD(&wb->b_io); - INIT_LIST_HEAD(&wb->b_more_io); -} - int bdi_has_dirty_io(struct backing_dev_info *bdi) { return wb_has_dirty_io(&bdi->wb); @@ -316,6 +305,43 @@ static void sync_supers_timer_fn(unsigned long unused) bdi_arm_supers_timer(); } +static void wakeup_timer_fn(unsigned long data) +{ + struct backing_dev_info *bdi = (struct backing_dev_info *)data; + + spin_lock_bh(&bdi->wb_lock); + if (bdi->wb.task) { + wake_up_process(bdi->wb.task); + } else { + /* + * When bdi tasks are inactive for long time, they are killed. + * In this case we have to wake-up the forker thread which + * should create and run the bdi thread. + */ + wake_up_process(default_backing_dev_info.wb.task); + } + spin_unlock_bh(&bdi->wb_lock); +} + +/* + * This function is used when the first inode for this bdi is marked dirty. It + * wakes-up the corresponding bdi thread which should then take care of the + * periodic background write-out of dirty inodes. Since the write-out would + * starts only 'dirty_writeback_interval' centisecs from now anyway, we just + * set up a timer which wakes the bdi thread up later. + * + * Note, we wouldn't bother setting up the timer, but this function is on the + * fast-path (used by '__mark_inode_dirty()'), so we save few context switches + * by delaying the wake-up. + */ +void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi) +{ + unsigned long timeout; + + timeout = msecs_to_jiffies(dirty_writeback_interval * 10); + mod_timer(&bdi->wb.wakeup_timer, jiffies + timeout); +} + /* * Calculate the longest interval (jiffies) bdi threads are allowed to be * inactive. @@ -353,8 +379,10 @@ static int bdi_forker_thread(void *ptr) * Temporary measure, we want to make sure we don't see * dirty data on the default backing_dev_info */ - if (wb_has_dirty_io(me) || !list_empty(&me->bdi->work_list)) + if (wb_has_dirty_io(me) || !list_empty(&me->bdi->work_list)) { + del_timer(&me->wakeup_timer); wb_do_writeback(me, 0); + } spin_lock_bh(&bdi_lock); set_current_state(TASK_INTERRUPTIBLE); @@ -386,7 +414,7 @@ static int bdi_forker_thread(void *ptr) break; } - spin_lock(&bdi->wb_lock); + spin_lock_bh(&bdi->wb_lock); /* * If there is no work to do and the bdi thread was * inactive long enough - kill it. The wb_lock is taken @@ -403,7 +431,7 @@ static int bdi_forker_thread(void *ptr) action = KILL_THREAD; break; } - spin_unlock(&bdi->wb_lock); + spin_unlock_bh(&bdi->wb_lock); } spin_unlock_bh(&bdi_lock); @@ -427,9 +455,9 @@ static int bdi_forker_thread(void *ptr) * The spinlock makes sure we do not lose * wake-ups when racing with 'bdi_queue_work()'. */ - spin_lock(&bdi->wb_lock); + spin_lock_bh(&bdi->wb_lock); bdi->wb.task = task; - spin_unlock(&bdi->wb_lock); + spin_unlock_bh(&bdi->wb_lock); } break; @@ -586,6 +614,7 @@ void bdi_unregister(struct backing_dev_info *bdi) if (bdi->dev) { trace_writeback_bdi_unregister(bdi); bdi_prune_sb(bdi); + del_timer_sync(&bdi->wb.wakeup_timer); if (!bdi_cap_flush_forker(bdi)) bdi_wb_shutdown(bdi); @@ -596,6 +625,18 @@ void bdi_unregister(struct backing_dev_info *bdi) } EXPORT_SYMBOL(bdi_unregister); +static void bdi_wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi) +{ + memset(wb, 0, sizeof(*wb)); + + wb->bdi = bdi; + wb->last_old_flush = jiffies; + INIT_LIST_HEAD(&wb->b_dirty); + INIT_LIST_HEAD(&wb->b_io); + INIT_LIST_HEAD(&wb->b_more_io); + setup_timer(&wb->wakeup_timer, wakeup_timer_fn, (unsigned long)bdi); +} + int bdi_init(struct backing_dev_info *bdi) { int i, err; -- cgit v1.2.3 From b5048a6cb5455a16bdff26a4c5ae9534f070d94c Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:23 +0300 Subject: writeback: remove unnecessary init_timer call The 'setup_timer()' function also calls 'init_timer()', so the extra 'init_timer()' call is not needed. Indeed, 'setup_timer()' is basically 'init_timer()' plus callback function and data pointers initialization. Signed-off-by: Artem Bityutskiy Reviewed-by: Christoph Hellwig Signed-off-by: Jens Axboe --- mm/backing-dev.c | 1 - 1 file changed, 1 deletion(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index cfff7225138..99890831d55 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -236,7 +236,6 @@ static int __init default_bdi_init(void) sync_supers_tsk = kthread_run(bdi_sync_supers, NULL, "sync_supers"); BUG_ON(IS_ERR(sync_supers_tsk)); - init_timer(&sync_supers_timer); setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0); bdi_arm_supers_timer(); -- cgit v1.2.3 From 603320239fb436f175c8b6bfa43d5023c47a6dc2 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:24 +0300 Subject: writeback: add new tracepoints Add 2 new trace points to the periodic write-back wake up case, just like we do in the 'bdi_queue_work()' function. Namely, introduce: 1. trace_writeback_wake_thread(bdi) 2. trace_writeback_wake_forker_thread(bdi) The first event is triggered every time we wake up a bdi thread to start periodic background write-out. The second event is triggered only when the bdi thread does not exist and should be created by the forker thread. This patch was suggested by Dave Chinner and Christoph Hellwig. Signed-off-by: Artem Bityutskiy Signed-off-by: Jens Axboe --- mm/backing-dev.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 99890831d55..9008c4e207f 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -310,6 +310,7 @@ static void wakeup_timer_fn(unsigned long data) spin_lock_bh(&bdi->wb_lock); if (bdi->wb.task) { + trace_writeback_wake_thread(bdi); wake_up_process(bdi->wb.task); } else { /* @@ -317,6 +318,7 @@ static void wakeup_timer_fn(unsigned long data) * In this case we have to wake-up the forker thread which * should create and run the bdi thread. */ + trace_writeback_wake_forker_thread(bdi); wake_up_process(default_backing_dev_info.wb.task); } spin_unlock_bh(&bdi->wb_lock); -- cgit v1.2.3 From c284de61db31669cce547ffc99efda971146719d Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 25 Jul 2010 14:29:25 +0300 Subject: writeback: cleanup bdi_register This patch makes sure we first initialize everything and set the BDI_registered flag, and only after this we add the bdi to 'bdi_list'. Current code adds the bdi to the list too early, and as a result I the WARN(!test_bit(BDI_registered, &bdi->state) in bdi forker is triggered. Also, it is in general good practice to make things visible only when they are fully initialized. Also, this patch does few micro clean-ups: 1. Removes the 'exit' label which does not do anything, just returns. This allows to get rid of few braces and 'ret' variable and make the code smaller. 2. If 'kthread_run()' fails, remove the error code it returns, not hard-coded '-ENOMEM'. Theoretically, some day 'kthread_run()' can return something else. Also, in case of failure it is not necessary to set 'bdi->wb.task' to NULL. Signed-off-by: Artem Bityutskiy Signed-off-by: Jens Axboe --- mm/backing-dev.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 9008c4e207f..0b8ee66993d 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -511,23 +511,16 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent, const char *fmt, ...) { va_list args; - int ret = 0; struct device *dev; if (bdi->dev) /* The driver needs to use separate queues per device */ - goto exit; + return 0; va_start(args, fmt); dev = device_create_vargs(bdi_class, parent, MKDEV(0, 0), bdi, fmt, args); va_end(args); - if (IS_ERR(dev)) { - ret = PTR_ERR(dev); - goto exit; - } - - spin_lock_bh(&bdi_lock); - list_add_tail_rcu(&bdi->bdi_list, &bdi_list); - spin_unlock_bh(&bdi_lock); + if (IS_ERR(dev)) + return PTR_ERR(dev); bdi->dev = dev; @@ -541,20 +534,19 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent, wb->task = kthread_run(bdi_forker_thread, wb, "bdi-%s", dev_name(dev)); - if (IS_ERR(wb->task)) { - wb->task = NULL; - ret = -ENOMEM; - - bdi_remove_from_list(bdi); - goto exit; - } + if (IS_ERR(wb->task)) + return PTR_ERR(wb->task); } bdi_debug_register(bdi, dev_name(dev)); set_bit(BDI_registered, &bdi->state); + + spin_lock_bh(&bdi_lock); + list_add_tail_rcu(&bdi->bdi_list, &bdi_list); + spin_unlock_bh(&bdi_lock); + trace_writeback_bdi_register(bdi); -exit: - return ret; + return 0; } EXPORT_SYMBOL(bdi_register); -- cgit v1.2.3 From 6bf05d03ea8d00bb53e0642f94a5a6815be2edb6 Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Wed, 4 Aug 2010 13:34:31 +0200 Subject: writeback: fix bad _bh spinlock nesting Fix a bug where a lock is _bh nested within another _bh lock, but forgets to use the _bh variant for unlock. Further more, it's not necessary to test _bh locks, the inner lock can just use spin_lock(). So fix up the bug by making that change. Signed-off-by: Jens Axboe --- mm/backing-dev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'mm') diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 0b8ee66993d..08d357522e7 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -415,7 +415,8 @@ static int bdi_forker_thread(void *ptr) break; } - spin_lock_bh(&bdi->wb_lock); + spin_lock(&bdi->wb_lock); + /* * If there is no work to do and the bdi thread was * inactive long enough - kill it. The wb_lock is taken @@ -432,7 +433,7 @@ static int bdi_forker_thread(void *ptr) action = KILL_THREAD; break; } - spin_unlock_bh(&bdi->wb_lock); + spin_unlock(&bdi->wb_lock); } spin_unlock_bh(&bdi_lock); -- cgit v1.2.3