aboutsummaryrefslogtreecommitdiff
path: root/mm/backing-dev.c
diff options
context:
space:
mode:
authorArtem Bityutskiy <Artem.Bityutskiy@nokia.com>2010-07-25 14:29:17 +0300
committerJens Axboe <jaxboe@fusionio.com>2010-08-07 18:53:56 +0200
commit78c40cb6581a74adc48821f3de6b864a54d4c34d (patch)
tree9ef19e68f6c28ded34f42de8a21b139006bfceaa /mm/backing-dev.c
parent080dcec41709be72613133f695be75b98dd43e88 (diff)
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 <Artem.Bityutskiy@nokia.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Jens Axboe <jaxboe@fusionio.com>
Diffstat (limited to 'mm/backing-dev.c')
-rw-r--r--mm/backing-dev.c31
1 files changed, 10 insertions, 21 deletions
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;