lkml.org 
[lkml]   [2010]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC][PATCH 16/16] writeback: prevent unnecessary bdi threads wakeups
> +		if (wb_has_dirty_io(wb) && dirty_writeback_interval) {
> + unsigned long wait;
>
> - wait_jiffies = msecs_to_jiffies(dirty_writeback_interval * 10);
> - schedule_timeout(wait_jiffies);
> + wait = msecs_to_jiffies(dirty_writeback_interval * 10);
> + schedule_timeout(wait);

No need for a local variable. If you want to shorten things a bit a
schedule_timeout_msecs helper in generic code would be nice, as there
are lots of patterns like this in various kernel threads.

> void __mark_inode_dirty(struct inode *inode, int flags)
> {
> + bool wakeup_bdi;
> struct super_block *sb = inode->i_sb;
> + struct backing_dev_info *uninitialized_var(bdi);

Just initialize wakeup_bdi and bdi here - a smart compiler will defer
them until we need them, and it makes the code a lot easier to read, as
well as getting rid of the uninitialized_var hack.

> */
> if (!was_dirty) {
> - struct bdi_writeback *wb = &inode_to_bdi(inode)->wb;
> - struct backing_dev_info *bdi = wb->bdi;
> + bdi = inode_to_bdi(inode);
>
> WARN(bdi_cap_writeback_dirty(bdi) &&
> !test_bit(BDI_registered, &bdi->state),
> "bdi-%s not registered\n", bdi->name);
>
> + /*
> + * If this is the first dirty inode for this bdi, we
> + * have to wake-up the corresponding bdi thread to make
> + * sure background write-back happens later.
> + */
> + if (!wb_has_dirty_io(&bdi->wb) &&
> + bdi_cap_writeback_dirty(bdi))
> + wakeup_bdi = true;

How about redoing this as:

if (bdi_cap_writeback_dirty(bdi)) {
WARN(!test_bit(BDI_registered, &bdi->state),
"bdi-%s not registered\n", bdi->name);

/*
* If this is the first dirty inode for this
* bdi, we have to wake-up the corresponding
* flusher thread to make sure background
* writeback happens later.
*/
if (!wb_has_dirty_io(&bdi->wb))
wakeup_bdi = true;
}

> + if (wakeup_bdi) {
> + bool wakeup_default = false;
> +
> + spin_lock(&bdi->wb_lock);
> + if (unlikely(!bdi->wb.task))
> + wakeup_default = true;
> + else
> + wake_up_process(bdi->wb.task);
> + spin_unlock(&bdi->wb_lock);
> +
> + if (wakeup_default)
> + wake_up_process(default_backing_dev_info.wb.task);

Same comment about just keeping wb_lock over the
default_backing_dev_info wakup as for one of the earlier patches applies
here.


Except for these nitpicks the patch looks good to me.

> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 65cb88a..818f934 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -326,7 +326,7 @@ static unsigned long bdi_longest_inactive(void)
> unsigned long interval;
>
> interval = msecs_to_jiffies(dirty_writeback_interval * 10);
> - return max(5UL * 60 * HZ, wait_jiffies);
> + return max(5UL * 60 * HZ, interval);

So previously we just ignored interval here?



\
 
 \ /
  Last update: 2010-07-18 09:55    [W:0.217 / U:0.248 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site