* [PATCH, RFC] simplify writeback thread creation
@ 2010-07-07 22:52 Christoph Hellwig
2010-07-08 7:08 ` Artem Bityutskiy
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Christoph Hellwig @ 2010-07-07 22:52 UTC (permalink / raw)
To: axboe; +Cc: dedekind1, linux-fsdevel
Currently the per-bdi writeback thread is only created when there is
dirty any dirty data on the BDI, and it lazy exists when it's been
unused for some time.
This leads to some very complex code, and the need to keep a forker
thread around.
This patch removes all this code and simply creates the thread as part
of the bdi registration. The downside is that we use up ressoures
for possible unused devices, although that overhead is rather low,
with 8k kernel stack size on x86 and few other, even smaller ressources.
If the overhead is still considered too much I can look into starting
the thread explicitly instead of as part of the bdi registration, but
that will require a bit of code complexity, too.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/fs-writeback.c | 60 ---------
include/linux/backing-dev.h | 14 --
include/linux/writeback.h | 1
include/trace/events/writeback.h | 3
mm/backing-dev.c | 252 ++++-----------------------------------
5 files changed, 33 insertions(+), 297 deletions(-)
Index: linux-2.6/fs/fs-writeback.c
===================================================================
--- linux-2.6.orig/fs/fs-writeback.c 2010-07-07 11:26:00.000000000 -0700
+++ linux-2.6/fs/fs-writeback.c 2010-07-07 11:36:20.196688282 -0700
@@ -80,19 +80,7 @@ static void bdi_queue_work(struct backin
list_add_tail(&work->list, &bdi->work_list);
spin_unlock(&bdi->wb_lock);
- /*
- * If the default thread isn't there, make sure we add it. When
- * it gets created and wakes up, we'll run this work.
- */
- if (unlikely(!bdi->wb.task)) {
- trace_writeback_nothread(bdi, work);
- wake_up_process(default_backing_dev_info.wb.task);
- } else {
- struct bdi_writeback *wb = &bdi->wb;
-
- if (wb->task)
- wake_up_process(wb->task);
- }
+ wake_up_process(bdi->wb.task);
}
static void
@@ -107,10 +95,8 @@ __bdi_start_writeback(struct backing_dev
*/
work = kzalloc(sizeof(*work), GFP_ATOMIC);
if (!work) {
- if (bdi->wb.task) {
- trace_writeback_nowork(bdi);
- wake_up_process(bdi->wb.task);
- }
+ trace_writeback_nowork(bdi);
+ wake_up_process(bdi->wb.task);
return;
}
@@ -756,7 +742,7 @@ static long wb_check_old_data_flush(stru
/*
* Retrieve work items and do the writeback they describe
*/
-long wb_do_writeback(struct bdi_writeback *wb, int force_wait)
+static long wb_do_writeback(struct bdi_writeback *wb, int force_wait)
{
struct backing_dev_info *bdi = wb->bdi;
struct wb_writeback_work *work;
@@ -800,17 +786,8 @@ int bdi_writeback_thread(void *data)
{
struct bdi_writeback *wb = data;
struct backing_dev_info *bdi = wb->bdi;
- unsigned long last_active = jiffies;
- unsigned long wait_jiffies = -1UL;
long pages_written;
- /*
- * 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();
@@ -819,38 +796,14 @@ int bdi_writeback_thread(void *data)
*/
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);
-
- trace_writeback_thread_start(bdi);
-
while (!kthread_should_stop()) {
pages_written = wb_do_writeback(wb, 0);
trace_writeback_pages_written(pages_written);
- if (pages_written)
- last_active = jiffies;
- else if (wait_jiffies != -1UL) {
- unsigned long max_idle;
-
- /*
- * Longest period of inactivity that we tolerate. If we
- * see dirty data again later, the task will get
- * recreated automatically.
- */
- max_idle = max(5UL * 60 * HZ, wait_jiffies);
- if (time_after(jiffies, max_idle + last_active))
- break;
- }
-
if (dirty_writeback_interval) {
- wait_jiffies = msecs_to_jiffies(dirty_writeback_interval * 10);
- schedule_timeout_interruptible(wait_jiffies);
+ schedule_timeout_interruptible(msecs_to_jiffies(
+ dirty_writeback_interval * 10));
} else {
set_current_state(TASK_INTERRUPTIBLE);
if (list_empty_careful(&wb->bdi->work_list) &&
@@ -871,7 +824,6 @@ int bdi_writeback_thread(void *data)
if (!list_empty(&bdi->work_list))
wb_do_writeback(wb, 1);
- trace_writeback_thread_stop(bdi);
return 0;
}
Index: linux-2.6/include/linux/backing-dev.h
===================================================================
--- linux-2.6.orig/include/linux/backing-dev.h 2010-07-07 11:33:00.427687937 -0700
+++ linux-2.6/include/linux/backing-dev.h 2010-07-07 11:38:34.224687973 -0700
@@ -26,8 +26,6 @@ struct dentry;
* Bits in backing_dev_info.state
*/
enum bdi_state {
- BDI_pending, /* On its way to being activated */
- BDI_wb_alloc, /* Default embedded wb allocated */
BDI_async_congested, /* The async (write) queue is getting full */
BDI_sync_congested, /* The sync queue is getting full */
BDI_registered, /* bdi_register() was done */
@@ -58,7 +56,6 @@ struct bdi_writeback {
struct backing_dev_info {
struct list_head bdi_list;
- struct rcu_head rcu_head;
unsigned long ra_pages; /* max readahead in PAGE_CACHE_SIZE units */
unsigned long state; /* Always use atomic bitops on this */
unsigned int capabilities; /* Device capabilities */
@@ -306,11 +303,6 @@ static inline bool bdi_cap_swap_backed(s
return bdi->capabilities & BDI_CAP_SWAP_BACKED;
}
-static inline bool bdi_cap_flush_forker(struct backing_dev_info *bdi)
-{
- return bdi == &default_backing_dev_info;
-}
-
static inline bool mapping_cap_writeback_dirty(struct address_space *mapping)
{
return bdi_cap_writeback_dirty(mapping->backing_dev_info);
@@ -326,12 +318,6 @@ static inline bool mapping_cap_swap_back
return bdi_cap_swap_backed(mapping->backing_dev_info);
}
-static inline int bdi_sched_wait(void *word)
-{
- schedule();
- return 0;
-}
-
static inline void blk_run_backing_dev(struct backing_dev_info *bdi,
struct page *page)
{
Index: linux-2.6/include/linux/writeback.h
===================================================================
--- linux-2.6.orig/include/linux/writeback.h 2010-07-07 10:28:22.000000000 -0700
+++ linux-2.6/include/linux/writeback.h 2010-07-07 11:33:12.144687938 -0700
@@ -64,7 +64,6 @@ int writeback_inodes_sb_if_idle(struct s
void sync_inodes_sb(struct super_block *);
void writeback_inodes_wb(struct bdi_writeback *wb,
struct writeback_control *wbc);
-long wb_do_writeback(struct bdi_writeback *wb, int force_wait);
void wakeup_flusher_threads(long nr_pages);
/* writeback.h requires fs.h; it, too, is not included from here. */
Index: linux-2.6/mm/backing-dev.c
===================================================================
--- linux-2.6.orig/mm/backing-dev.c 2010-07-07 11:33:00.437687937 -0700
+++ linux-2.6/mm/backing-dev.c 2010-07-07 12:02:28.694687934 -0700
@@ -36,13 +36,10 @@ EXPORT_SYMBOL_GPL(noop_backing_dev_info)
static struct class *bdi_class;
/*
- * bdi_lock protects updates to bdi_list and bdi_pending_list, as well as
- * reader side protection for bdi_pending_list. bdi_list has RCU reader side
- * locking.
+ * bdi_lock protects updates to bdi_list.
*/
DEFINE_SPINLOCK(bdi_lock);
LIST_HEAD(bdi_list);
-LIST_HEAD(bdi_pending_list);
static struct task_struct *sync_supers_tsk;
static struct timer_list sync_supers_timer;
@@ -50,8 +47,6 @@ static struct timer_list sync_supers_tim
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);
-
#ifdef CONFIG_DEBUG_FS
#include <linux/debugfs.h>
#include <linux/seq_file.h>
@@ -266,21 +261,8 @@ int bdi_has_dirty_io(struct backing_dev_
return wb_has_dirty_io(&bdi->wb);
}
-static void bdi_flush_io(struct backing_dev_info *bdi)
-{
- struct writeback_control wbc = {
- .sync_mode = WB_SYNC_NONE,
- .older_than_this = NULL,
- .range_cyclic = 1,
- .nr_to_write = 1024,
- };
-
- writeback_inodes_wb(&bdi->wb, &wbc);
-}
-
/*
- * kupdated() used to do this. We cannot do it from the bdi_forker_task()
- * or we risk deadlocking on ->s_umount. The longer term solution would be
+ * kupdated() used to do this. The longer term solution would be
* to implement sync_supers_bdi() or similar and simply do it from the
* bdi writeback tasks individually.
*/
@@ -318,160 +300,38 @@ static void sync_supers_timer_fn(unsigne
bdi_arm_supers_timer();
}
-static int bdi_forker_task(void *ptr)
-{
- struct bdi_writeback *me = ptr;
-
- 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;
- struct bdi_writeback *wb;
-
- /*
- * 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))
- wb_do_writeback(me, 0);
-
- spin_lock_bh(&bdi_lock);
-
- /*
- * 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) {
- if (bdi->wb.task)
- continue;
- if (list_empty(&bdi->work_list) &&
- !bdi_has_dirty_io(bdi))
- continue;
-
- bdi_add_default_flusher_task(bdi);
- }
-
- set_current_state(TASK_INTERRUPTIBLE);
-
- if (list_empty(&bdi_pending_list)) {
- unsigned long wait;
-
- spin_unlock_bh(&bdi_lock);
- wait = msecs_to_jiffies(dirty_writeback_interval * 10);
- if (wait)
- schedule_timeout(wait);
- else
- schedule();
- try_to_freeze();
- continue;
- }
-
- __set_current_state(TASK_RUNNING);
-
- /*
- * This is our real job - check for pending entries in
- * bdi_pending_list, and create the tasks that got added
- */
- bdi = list_entry(bdi_pending_list.next, struct backing_dev_info,
- bdi_list);
- 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 task 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;
-
- /*
- * Add this 'bdi' to the back, 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);
- }
- }
-
- 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 task 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 int bdi_writeback_enable(struct backing_dev_info *bdi,
+ struct device *dev)
{
- 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;
+ bdi->wb.task = kthread_run(bdi_writeback_thread, &bdi->wb,
+ "bdi-%s", dev_name(dev));
+ if (IS_ERR(bdi->wb.task)) {
+ bdi->wb.task = NULL;
+ return -ENOMEM;
}
- /*
- * Check with the helper whether to proceed adding a task. 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.
- */
- if (!test_and_set_bit(BDI_pending, &bdi->state)) {
- list_del_rcu(&bdi->bdi_list);
+ spin_lock_bh(&bdi_lock);
+ list_add_rcu(&bdi->bdi_list, &bdi_list);
+ spin_unlock_bh(&bdi_lock);
- /*
- * 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);
- }
+ return 0;
}
-/*
- * Remove bdi from bdi_list, and ensure that it is no longer visible
- */
-static void bdi_remove_from_list(struct backing_dev_info *bdi)
+static void bdi_writeback_disable(struct backing_dev_info *bdi)
{
spin_lock_bh(&bdi_lock);
list_del_rcu(&bdi->bdi_list);
spin_unlock_bh(&bdi_lock);
synchronize_rcu();
+
+ /*
+ * Force unfreeze of the thread before calling kthread_stop(),
+ * otherwise it would never exit if it is currently stuck in the
+ * refrigerator.
+ */
+ thaw_process(bdi->wb.task);
+ kthread_stop(bdi->wb.task);
}
int bdi_register(struct backing_dev_info *bdi, struct device *parent,
@@ -492,10 +352,6 @@ int bdi_register(struct backing_dev_info
goto exit;
}
- spin_lock_bh(&bdi_lock);
- list_add_tail_rcu(&bdi->bdi_list, &bdi_list);
- spin_unlock_bh(&bdi_lock);
-
bdi->dev = dev;
/*
@@ -503,18 +359,10 @@ int bdi_register(struct backing_dev_info
* and add other bdi's to the list. They will get a thread created
* on-demand when they need it.
*/
- if (bdi_cap_flush_forker(bdi)) {
- struct bdi_writeback *wb = &bdi->wb;
-
- wb->task = kthread_run(bdi_forker_task, wb, "bdi-%s",
- dev_name(dev));
- if (IS_ERR(wb->task)) {
- wb->task = NULL;
- ret = -ENOMEM;
-
- bdi_remove_from_list(bdi);
+ if (bdi_cap_writeback_dirty(bdi)) {
+ ret = bdi_writeback_enable(bdi, dev);
+ if (ret)
goto exit;
- }
}
bdi_debug_register(bdi, dev_name(dev));
@@ -532,37 +380,6 @@ int bdi_register_dev(struct backing_dev_
EXPORT_SYMBOL(bdi_register_dev);
/*
- * Remove bdi from the global list and shutdown any threads we have running
- */
-static void bdi_wb_shutdown(struct backing_dev_info *bdi)
-{
- if (!bdi_cap_writeback_dirty(bdi))
- return;
-
- /*
- * If setup is pending, wait for that to complete first
- */
- wait_on_bit(&bdi->state, BDI_pending, bdi_sched_wait,
- TASK_UNINTERRUPTIBLE);
-
- /*
- * Make sure nobody finds us on the bdi_list anymore
- */
- bdi_remove_from_list(bdi);
-
- /*
- * 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.
- */
- if (bdi->wb.task) {
- thaw_process(bdi->wb.task);
- kthread_stop(bdi->wb.task);
- }
-}
-
-/*
* This bdi is going away now, make sure that no super_blocks point to it
*/
static void bdi_prune_sb(struct backing_dev_info *bdi)
@@ -583,8 +400,8 @@ void bdi_unregister(struct backing_dev_i
trace_writeback_bdi_unregister(bdi);
bdi_prune_sb(bdi);
- if (!bdi_cap_flush_forker(bdi))
- bdi_wb_shutdown(bdi);
+ if (bdi_cap_writeback_dirty(bdi))
+ bdi_writeback_disable(bdi);
bdi_debug_unregister(bdi);
device_unregister(bdi->dev);
bdi->dev = NULL;
@@ -602,7 +419,6 @@ int bdi_init(struct backing_dev_info *bd
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);
@@ -631,20 +447,6 @@ void bdi_destroy(struct backing_dev_info
{
int i;
- /*
- * Splice our entries to the default_backing_dev_info, if this
- * bdi disappears
- */
- if (bdi_has_dirty_io(bdi)) {
- struct bdi_writeback *dst = &default_backing_dev_info.wb;
-
- spin_lock(&inode_lock);
- list_splice(&bdi->wb.b_dirty, &dst->b_dirty);
- list_splice(&bdi->wb.b_io, &dst->b_io);
- list_splice(&bdi->wb.b_more_io, &dst->b_more_io);
- spin_unlock(&inode_lock);
- }
-
bdi_unregister(bdi);
for (i = 0; i < NR_BDI_STAT_ITEMS; i++)
Index: linux-2.6/include/trace/events/writeback.h
===================================================================
--- linux-2.6.orig/include/trace/events/writeback.h 2010-07-07 11:33:56.285687937 -0700
+++ linux-2.6/include/trace/events/writeback.h 2010-07-07 11:35:35.586687939 -0700
@@ -45,7 +45,6 @@ DECLARE_EVENT_CLASS(writeback_work_class
DEFINE_EVENT(writeback_work_class, name, \
TP_PROTO(struct backing_dev_info *bdi, struct wb_writeback_work *work), \
TP_ARGS(bdi, work))
-DEFINE_WRITEBACK_WORK_EVENT(writeback_nothread);
DEFINE_WRITEBACK_WORK_EVENT(writeback_queue);
DEFINE_WRITEBACK_WORK_EVENT(writeback_exec);
@@ -82,8 +81,6 @@ DEFINE_EVENT(writeback_class, name, \
DEFINE_WRITEBACK_EVENT(writeback_nowork);
DEFINE_WRITEBACK_EVENT(writeback_bdi_register);
DEFINE_WRITEBACK_EVENT(writeback_bdi_unregister);
-DEFINE_WRITEBACK_EVENT(writeback_thread_start);
-DEFINE_WRITEBACK_EVENT(writeback_thread_stop);
DECLARE_EVENT_CLASS(wbc_class,
TP_PROTO(struct writeback_control *wbc, struct backing_dev_info *bdi),
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-07 22:52 [PATCH, RFC] simplify writeback thread creation Christoph Hellwig
@ 2010-07-08 7:08 ` Artem Bityutskiy
2010-07-08 12:20 ` Jens Axboe
2010-07-08 13:22 ` Artem Bityutskiy
2 siblings, 0 replies; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-08 7:08 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: axboe, linux-fsdevel
Thanks Christoph,
I'll try to test it in my environment.
On Thu, 2010-07-08 at 00:52 +0200, Christoph Hellwig wrote:
> Currently the per-bdi writeback thread is only created when there is
> dirty any dirty data on the BDI, and it lazy exists when it's been
> unused for some time.
>
> This leads to some very complex code, and the need to keep a forker
> thread around.
I'd add here a comment that due to the complexity it is not bug-free -
e.g., I pointed to the race between thread suicide and wake-up.
Also I'd add a comment that it leads to more thread types waking up
periodically and consuming power. With just one thread type it is easier
to fix this issue.
> This patch removes all this code and simply creates the thread as part
> of the bdi registration. The downside is that we use up ressoures
> for possible unused devices, although that overhead is rather low,
> with 8k kernel stack size on x86 and few other, even smaller ressources.
s/ressources/resources/
:-)
On the other hand, there was a message that process IDs are precious
resource on large NUMA systems. So, less suicides, more free IDs :-) But
this is just a minor note.
> If the overhead is still considered too much I can look into starting
> the thread explicitly instead of as part of the bdi registration, but
> that will require a bit of code complexity, too.
Well, the threads can exit, then we need:
1. when adding bdi works and the thread exited, forr it.
2. the same should be done in __mark_inode_dirty - if the bdi thread is
dead, create it.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-07 22:52 [PATCH, RFC] simplify writeback thread creation Christoph Hellwig
2010-07-08 7:08 ` Artem Bityutskiy
@ 2010-07-08 12:20 ` Jens Axboe
2010-07-08 14:21 ` Artem Bityutskiy
2010-07-08 13:22 ` Artem Bityutskiy
2 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2010-07-08 12:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: dedekind1, linux-fsdevel
On 2010-07-08 00:52, Christoph Hellwig wrote:
> Currently the per-bdi writeback thread is only created when there is
> dirty any dirty data on the BDI, and it lazy exists when it's been
> unused for some time.
>
> This leads to some very complex code, and the need to keep a forker
> thread around.
>
> This patch removes all this code and simply creates the thread as part
> of the bdi registration. The downside is that we use up ressoures
> for possible unused devices, although that overhead is rather low,
> with 8k kernel stack size on x86 and few other, even smaller ressources.
>
> If the overhead is still considered too much I can look into starting
> the thread explicitly instead of as part of the bdi registration, but
> that will require a bit of code complexity, too.
I'm pretty sure this will come back to bite us in the ass... If we are
going to change the lazy create/exit setup, I would greatly prefer
doing it at fs mount time (or something to that effect).
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-07 22:52 [PATCH, RFC] simplify writeback thread creation Christoph Hellwig
2010-07-08 7:08 ` Artem Bityutskiy
2010-07-08 12:20 ` Jens Axboe
@ 2010-07-08 13:22 ` Artem Bityutskiy
2 siblings, 0 replies; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-08 13:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: axboe, linux-fsdevel
On Thu, 2010-07-08 at 00:52 +0200, Christoph Hellwig wrote:
> @@ -503,18 +359,10 @@ int bdi_register(struct backing_dev_info
> * and add other bdi's to the list. They will get a thread
> created
> * on-demand when they need it.
> */
Just noticed that this comment should be killed as well.
> - if (bdi_cap_flush_forker(bdi)) {
> - struct bdi_writeback *wb = &bdi->wb;
> -
> - wb->task = kthread_run(bdi_forker_task, wb, "bdi-%s",
> - dev_name(dev));
> - if (IS_ERR(wb->task)) {
> - wb->task = NULL;
> - ret = -ENOMEM;
> -
> - bdi_remove_from_list(bdi);
> + if (bdi_cap_writeback_dirty(bdi)) {
> + ret = bdi_writeback_enable(bdi, dev);
> + if (ret)
> goto exit;
>
>
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 12:20 ` Jens Axboe
@ 2010-07-08 14:21 ` Artem Bityutskiy
2010-07-08 14:59 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-08 14:21 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-fsdevel
On Thu, 2010-07-08 at 14:20 +0200, Jens Axboe wrote:
> On 2010-07-08 00:52, Christoph Hellwig wrote:
> > Currently the per-bdi writeback thread is only created when there is
> > dirty any dirty data on the BDI, and it lazy exists when it's been
> > unused for some time.
> >
> > This leads to some very complex code, and the need to keep a forker
> > thread around.
> >
> > This patch removes all this code and simply creates the thread as part
> > of the bdi registration. The downside is that we use up ressoures
> > for possible unused devices, although that overhead is rather low,
> > with 8k kernel stack size on x86 and few other, even smaller ressources.
> >
> > If the overhead is still considered too much I can look into starting
> > the thread explicitly instead of as part of the bdi registration, but
> > that will require a bit of code complexity, too.
>
> I'm pretty sure this will come back to bite us in the ass... If we are
> going to change the lazy create/exit setup, I would greatly prefer
> doing it at fs mount time (or something to that effect).
How about not starting any thread at all at the bdi registration time,
and start a bdi thread only when something for this bdi becomes dirty
(__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we
do this, then the tasks can also die by the 5min timeout, and will be
forked again when dirt/bdi works arrives?
I guess it is a bit challenging to start a task in __mark_inode_dirty(),
whis is supposed to be fast and non-sleeping, but we can just submit a
work which will start the task.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 14:21 ` Artem Bityutskiy
@ 2010-07-08 14:59 ` Jens Axboe
2010-07-08 15:23 ` Artem Bityutskiy
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2010-07-08 14:59 UTC (permalink / raw)
To: dedekind1; +Cc: Christoph Hellwig, linux-fsdevel
On 2010-07-08 16:21, Artem Bityutskiy wrote:
> On Thu, 2010-07-08 at 14:20 +0200, Jens Axboe wrote:
>> On 2010-07-08 00:52, Christoph Hellwig wrote:
>>> Currently the per-bdi writeback thread is only created when there is
>>> dirty any dirty data on the BDI, and it lazy exists when it's been
>>> unused for some time.
>>>
>>> This leads to some very complex code, and the need to keep a forker
>>> thread around.
>>>
>>> This patch removes all this code and simply creates the thread as part
>>> of the bdi registration. The downside is that we use up ressoures
>>> for possible unused devices, although that overhead is rather low,
>>> with 8k kernel stack size on x86 and few other, even smaller ressources.
>>>
>>> If the overhead is still considered too much I can look into starting
>>> the thread explicitly instead of as part of the bdi registration, but
>>> that will require a bit of code complexity, too.
>>
>> I'm pretty sure this will come back to bite us in the ass... If we are
>> going to change the lazy create/exit setup, I would greatly prefer
>> doing it at fs mount time (or something to that effect).
>
> How about not starting any thread at all at the bdi registration time,
> and start a bdi thread only when something for this bdi becomes dirty
> (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we
> do this, then the tasks can also die by the 5min timeout, and will be
> forked again when dirt/bdi works arrives?
>
> I guess it is a bit challenging to start a task in __mark_inode_dirty(),
> whis is supposed to be fast and non-sleeping, but we can just submit a
> work which will start the task.
That work would have to reside on the stack, and __mark_inode_dirty()
block on the thread startup. We can't always do that.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 14:59 ` Jens Axboe
@ 2010-07-08 15:23 ` Artem Bityutskiy
2010-07-08 17:23 ` Jens Axboe
0 siblings, 1 reply; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-08 15:23 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-fsdevel
On Thu, 2010-07-08 at 16:59 +0200, Jens Axboe wrote:
> > How about not starting any thread at all at the bdi registration time,
> > and start a bdi thread only when something for this bdi becomes dirty
> > (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we
> > do this, then the tasks can also die by the 5min timeout, and will be
> > forked again when dirt/bdi works arrives?
> >
> > I guess it is a bit challenging to start a task in __mark_inode_dirty(),
> > whis is supposed to be fast and non-sleeping, but we can just submit a
> > work which will start the task.
>
> That work would have to reside on the stack, and __mark_inode_dirty()
> block on the thread startup. We can't always do that.
We can have a pre-defined bdi->wb->task_start_work or something like
that.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 15:23 ` Artem Bityutskiy
@ 2010-07-08 17:23 ` Jens Axboe
2010-07-08 18:43 ` Artem Bityutskiy
0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2010-07-08 17:23 UTC (permalink / raw)
To: dedekind1; +Cc: Christoph Hellwig, linux-fsdevel
On 08/07/10 17.23, Artem Bityutskiy wrote:
> On Thu, 2010-07-08 at 16:59 +0200, Jens Axboe wrote:
>>> How about not starting any thread at all at the bdi registration time,
>>> and start a bdi thread only when something for this bdi becomes dirty
>>> (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we
>>> do this, then the tasks can also die by the 5min timeout, and will be
>>> forked again when dirt/bdi works arrives?
>>>
>>> I guess it is a bit challenging to start a task in __mark_inode_dirty(),
>>> whis is supposed to be fast and non-sleeping, but we can just submit a
>>> work which will start the task.
>>
>> That work would have to reside on the stack, and __mark_inode_dirty()
>> block on the thread startup. We can't always do that.
>
> We can have a pre-defined bdi->wb->task_start_work or something like
> that.
Yeah, that would work.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 17:23 ` Jens Axboe
@ 2010-07-08 18:43 ` Artem Bityutskiy
2010-07-08 18:48 ` Christoph Hellwig
0 siblings, 1 reply; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-08 18:43 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, linux-fsdevel
On Thu, 2010-07-08 at 19:23 +0200, Jens Axboe wrote:
> On 08/07/10 17.23, Artem Bityutskiy wrote:
> > On Thu, 2010-07-08 at 16:59 +0200, Jens Axboe wrote:
> >>> How about not starting any thread at all at the bdi registration time,
> >>> and start a bdi thread only when something for this bdi becomes dirty
> >>> (__mark_inode_dirty()) or a bdi work is queued (bdi_queue_work())? If we
> >>> do this, then the tasks can also die by the 5min timeout, and will be
> >>> forked again when dirt/bdi works arrives?
> >>>
> >>> I guess it is a bit challenging to start a task in __mark_inode_dirty(),
> >>> whis is supposed to be fast and non-sleeping, but we can just submit a
> >>> work which will start the task.
> >>
> >> That work would have to reside on the stack, and __mark_inode_dirty()
> >> block on the thread startup. We can't always do that.
> >
> > We can have a pre-defined bdi->wb->task_start_work or something like
> > that.
>
> Yeah, that would work.
Hmm, was thinking about this while driving home - the forker approach
has a good resilience property - if it cannot fork - it'll do the stuff
itself. I have a feeling that if something like this to be implemented
with the approach I suggested, we'll end up with similar level of
complexity that we wanted to get rid of...
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 18:43 ` Artem Bityutskiy
@ 2010-07-08 18:48 ` Christoph Hellwig
2010-07-09 7:52 ` Artem Bityutskiy
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2010-07-08 18:48 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: Jens Axboe, Christoph Hellwig, linux-fsdevel
On Thu, Jul 08, 2010 at 09:43:22PM +0300, Artem Bityutskiy wrote:
> Hmm, was thinking about this while driving home - the forker approach
> has a good resilience property - if it cannot fork - it'll do the stuff
> itself. I have a feeling that if something like this to be implemented
> with the approach I suggested, we'll end up with similar level of
> complexity that we wanted to get rid of...
Yes, the lazy starting is what adds the complexity. I think starting
it once we have any filesystem mounted on the bdi and stop it once all
filesystems are gone is a lot simpler and more elegant. It also solves
the other issue wit hall lazy schemes, that is the race betwen dirtying
data with no alive thread and the bdi going away. The current code
tries to deal with that by splicing the remaining dirty inodes to the
default BDI, but that'll just cause memory corruption in most cases,
because the BDI is references all over the writeback code and it's
gone by the point it'll actually go away. Which makes be believe this
race is a rather theoretical one.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-08 18:48 ` Christoph Hellwig
@ 2010-07-09 7:52 ` Artem Bityutskiy
2010-07-09 8:16 ` Jens Axboe
2010-07-09 14:51 ` Christoph Hellwig
0 siblings, 2 replies; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-09 7:52 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-fsdevel
On Thu, 2010-07-08 at 20:48 +0200, Christoph Hellwig wrote:
> On Thu, Jul 08, 2010 at 09:43:22PM +0300, Artem Bityutskiy wrote:
> > Hmm, was thinking about this while driving home - the forker approach
> > has a good resilience property - if it cannot fork - it'll do the stuff
> > itself. I have a feeling that if something like this to be implemented
> > with the approach I suggested, we'll end up with similar level of
> > complexity that we wanted to get rid of...
>
> Yes, the lazy starting is what adds the complexity. I think starting
> it once we have any filesystem mounted on the bdi and stop it once all
> filesystems are gone is a lot simpler and more elegant.
But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also
involve dirty data write-back.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-09 7:52 ` Artem Bityutskiy
@ 2010-07-09 8:16 ` Jens Axboe
2010-07-09 11:06 ` Theodore Tso
2010-07-09 14:51 ` Christoph Hellwig
1 sibling, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2010-07-09 8:16 UTC (permalink / raw)
To: dedekind1; +Cc: Christoph Hellwig, linux-fsdevel
On 2010-07-09 09:52, Artem Bityutskiy wrote:
> On Thu, 2010-07-08 at 20:48 +0200, Christoph Hellwig wrote:
>> On Thu, Jul 08, 2010 at 09:43:22PM +0300, Artem Bityutskiy wrote:
>>> Hmm, was thinking about this while driving home - the forker approach
>>> has a good resilience property - if it cannot fork - it'll do the stuff
>>> itself. I have a feeling that if something like this to be implemented
>>> with the approach I suggested, we'll end up with similar level of
>>> complexity that we wanted to get rid of...
>>
>> Yes, the lazy starting is what adds the complexity. I think starting
>> it once we have any filesystem mounted on the bdi and stop it once all
>> filesystems are gone is a lot simpler and more elegant.
>
> But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also
> involve dirty data write-back.
You would have to do it at device open time, if the thread does
not already exist.
Not sure this is all worth it, I think the complexity of the
lazy create/exit is a bit exaggerated.
--
Jens Axboe
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-09 8:16 ` Jens Axboe
@ 2010-07-09 11:06 ` Theodore Tso
0 siblings, 0 replies; 15+ messages in thread
From: Theodore Tso @ 2010-07-09 11:06 UTC (permalink / raw)
To: Jens Axboe; +Cc: dedekind1, Christoph Hellwig, linux-fsdevel
Chiming onto this thread late, I've definitely seen customer workloads
which used a whole fleet-load of blades connected to a SAN network,
the idea being that the blades were easily replaceable, and could take
various different responsibilities due to shifting workload, battle damage,
etc. So this would be a case where the individual blades wouldn't have
huge amounts of memory, but there would be a huge number of block
devices available on the SAN network. So at least in this case, avoiding
creating every single BDI thread until the block devices is first used
would be a major win for this sort of customer deployment.
-- Ted
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-09 7:52 ` Artem Bityutskiy
2010-07-09 8:16 ` Jens Axboe
@ 2010-07-09 14:51 ` Christoph Hellwig
2010-07-09 15:49 ` Artem Bityutskiy
1 sibling, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2010-07-09 14:51 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: Christoph Hellwig, Jens Axboe, linux-fsdevel
On Fri, Jul 09, 2010 at 10:52:59AM +0300, Artem Bityutskiy wrote:
> But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also
> involve dirty data write-back.
But only on the bdev fs inode.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH, RFC] simplify writeback thread creation
2010-07-09 14:51 ` Christoph Hellwig
@ 2010-07-09 15:49 ` Artem Bityutskiy
0 siblings, 0 replies; 15+ messages in thread
From: Artem Bityutskiy @ 2010-07-09 15:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-fsdevel
On Fri, 2010-07-09 at 16:51 +0200, Christoph Hellwig wrote:
> On Fri, Jul 09, 2010 at 10:52:59AM +0300, Artem Bityutskiy wrote:
> > But what about cases like 'dd if=/dev/zero of=/dev/sda4'? They also
> > involve dirty data write-back.
>
> But only on the bdev fs inode.
OK, I do not know details that code and how it works, need to
investigate, but I just thought it is a different case comparing to the
"create bdi task only when an FS is mounter on top". And just wanted to
point that things may be not so elegant.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2010-07-09 15:53 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-07 22:52 [PATCH, RFC] simplify writeback thread creation Christoph Hellwig
2010-07-08 7:08 ` Artem Bityutskiy
2010-07-08 12:20 ` Jens Axboe
2010-07-08 14:21 ` Artem Bityutskiy
2010-07-08 14:59 ` Jens Axboe
2010-07-08 15:23 ` Artem Bityutskiy
2010-07-08 17:23 ` Jens Axboe
2010-07-08 18:43 ` Artem Bityutskiy
2010-07-08 18:48 ` Christoph Hellwig
2010-07-09 7:52 ` Artem Bityutskiy
2010-07-09 8:16 ` Jens Axboe
2010-07-09 11:06 ` Theodore Tso
2010-07-09 14:51 ` Christoph Hellwig
2010-07-09 15:49 ` Artem Bityutskiy
2010-07-08 13:22 ` Artem Bityutskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.