Prev: Mtd: fixed coding style indent errors
Next: Use xvmalloc to store compressed chunks
From: Christoph Hellwig on 18 Jul 2010 03:50
> + 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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 20 Jul 2010 09:30
On Sun, 2010-07-18 at 03:45 -0400, Christoph Hellwig wrote:
> > + 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.

OK, do you want me to ignore the 80-lines limitation or you want me
to add schedule_timeout_msecs() as a part of this patch series?

> > 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.

OK.

> > + /*
> > + * 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;
> }

OK.

> > + 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.

I just figured that I have to add 'trace_writeback_nothread(bdi, work)'
here, just like in 'bdi_queue_work()'. I'd feel safer to call tracer
outside the spinlock. What do you think?

> > --- 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?

Yes, my fault, thanks for catching.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 21 Jul 2010 08:00
On Wed, 2010-07-21 at 12:31 +0300, Artem Bityutskiy wrote:
> spin_unlock(&inode_lock);
> +
> + if (wakeup_bdi) {
> + spin_lock(&bdi->wb_lock);
> + if (!bdi->wb.task)
> + wake_up_process(default_backing_dev_info.wb.task);
> + else
> + wake_up_process(bdi->wb.task);
> + spin_unlock(&bdi->wb_lock);
> + }
> }

Dave,

I do not know whether this stuff will end up in upstream, I did not get
any feed back from Jens so far. But if it will, I'd like to let you know
that the code quoted above is similar to the 'bdi_queue_work()'
function. And the purpose is very similar. But you added a
'trace_writeback_nothread()' call to 'bdi_queue_work()', and I think a
similar call has to be here.

Can I call 'trace_writeback_nothread()'? I guess not. Should I create
another trace point? Any hints/instructions?

Note, the patches are against Jens' tree.

Please, see linux-fsdevel or lkml for the full patch and its purposes.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Hellwig on 21 Jul 2010 08:20
Looks good,


Reviewed-by: Christoph Hellwig <hch(a)lst.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Dave Chinner on 21 Jul 2010 20:50
On Wed, Jul 21, 2010 at 02:45:41PM +0300, Artem Bityutskiy wrote:
> On Wed, 2010-07-21 at 12:31 +0300, Artem Bityutskiy wrote:
> > spin_unlock(&inode_lock);
> > +
> > + if (wakeup_bdi) {
> > + spin_lock(&bdi->wb_lock);
> > + if (!bdi->wb.task)
> > + wake_up_process(default_backing_dev_info.wb.task);
> > + else
> > + wake_up_process(bdi->wb.task);
> > + spin_unlock(&bdi->wb_lock);
> > + }
> > }
>
> Dave,
>
> I do not know whether this stuff will end up in upstream, I did not get
> any feed back from Jens so far. But if it will, I'd like to let you know
> that the code quoted above is similar to the 'bdi_queue_work()'
> function. And the purpose is very similar. But you added a
> 'trace_writeback_nothread()' call to 'bdi_queue_work()', and I think a
> similar call has to be here.

Yes, that seems like a sane thing to do ;)

> Can I call 'trace_writeback_nothread()'? I guess not. Should I create
> another trace point? Any hints/instructions?

The bdi_queue_work() tracepoints expect a work structure to be
passed in, so you can't use them (or that class of event) if you
don't have a struct wb_writeback_work.

For __mark_inode_dirty(), I'd add two new tracepoints like:

DEFINE_WRITEBACK_EVENT(writeback_wakeup);
DEFINE_WRITEBACK_EVENT(writeback_wakeup_nothread);

and place them as:

if (wakeup_bdi) {
trace_writeback_wakeup(bdi)
spin_lock(&bdi->wb_lock);
if (!bdi->wb.task) {{
trace_writeback_wakeup_nothread(bdi);
wake_up_process(default_backing_dev_info.wb.task);
} else
wake_up_process(bdi->wb.task);
spin_unlock(&bdi->wb_lock);
}

Cheers,

Dave.
--
Dave Chinner
david(a)fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3 4
Prev: Mtd: fixed coding style indent errors
Next: Use xvmalloc to store compressed chunks