lkml.org 
[lkml]   [2007]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch] Give kjournald a IOPRIO_CLASS_RT io priority
On Mon, Oct 15 2007, Andrew Morton wrote:
> On Mon, 15 Oct 2007 10:46:47 -0700
> Arjan van de Ven <arjan@infradead.org> wrote:
>
> >
> > Subject: Give kjournald a IOPRIO_CLASS_RT io priority
> > From: Arjan van de Ven <arjan@linux.intel.com>
> >
> > With latencytop, I noticed that the (in memory) atime updates during a
> > kernel build had latencies of 600 msec or longer; this is obviously not so
> > nice behavior. Other EXT3 journal related operations had similar or even
> > longer latencies.
> >
> > Digging into this a bit more, it appears to be an interaction between EXT3
> > and CFQ in that CFQ tries to be fair to everyone, including kjournald.
> > However, in reality, kjournald is "special" in that it does a lot of journal
> > work and effectively this leads to a twisted kind of "mass priority
> > inversion" type of behavior.
> >
> > The good news is that CFQ already has the infrastructure to make certain
> > processes special... JBD just wasn't using that quite yet.
> >
> > The patch below makes kjournald of the IOPRIO_CLASS_RT priority to break
> > this priority inversion behavior. With this patch, the latencies for atime
> > updates (and similar operation) go down by a factor of 3x to 4x !
> >
>
> Seems a pretty fundamental change which could do with some careful
> benchmarking, methinks.
>
> See, your patch amounts to "do more seeks to improve one test case".
> Surely other testcases will worsen. What are they?

Yes, completely agree! I think Arjans patch makes a heap of sense, but
some numbers would be great to see.

> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> >
> >
> > diff -purN linux-2.6.23-rc9.org/fs/jbd/journal.c linux-2.6.23-rc9.lt/fs/jbd/journal.c
> > --- linux-2.6.23-rc9.org/fs/jbd/journal.c 2007-10-02 05:24:52.000000000 +0200
> > +++ linux-2.6.23-rc9.lt/fs/jbd/journal.c 2007-10-14 00:06:55.000000000 +0200
> > @@ -35,6 +35,7 @@
> > #include <linux/kthread.h>
> > #include <linux/poison.h>
> > #include <linux/proc_fs.h>
> > +#include <linux/ioprio.h>
> >
> > #include <asm/uaccess.h>
> > #include <asm/page.h>
> > @@ -131,6 +132,8 @@ static int kjournald(void *arg)
> > printk(KERN_INFO "kjournald starting. Commit interval %ld seconds\n",
> > journal->j_commit_interval / HZ);
> >
> > + current->ioprio = (IOPRIO_CLASS_RT << IOPRIO_CLASS_SHIFT) | 4;
> > +
>
> Might be worth a code comment?

It should not be merged as-is, instead I'll provide a function to do
this. It should also set current->io_context->ioprio_changed. Since no
IO has been done yet at this point it doesn't matter. But we should cut
a piece of set_task_ioprio() out and provide that as a kernel helper for
this sort of thing.

Even just writing it as:

current->ioprio = (IOPRIO_CLASS_RT << IOPRIO_CLASS_SHIFT) | IOPRIO_NORM;

would be more readable. Or perhaps this would suffice, given the above
restriction that IO hasn't been done yet:

current->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, IOPRIO_NORM);

--
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-10-15 21:31    [W:0.085 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site