[lkml]   [2008]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [patch 0/4] [RFC] Another proportional weight IO controller
> From: Jens Axboe <>
> Date: Wed, Nov 19, 2008 03:30:07PM +0100
> On Tue, Nov 18 2008, Fabio Checconi wrote:
> > - In cfq_exit_single_io_context() and in changed_ioprio(), cic->key
> > is dereferenced without holding any lock. As I reported in [1]
> > this seems to be a problem when an exit() races with a cfq_exit_queue()
> > and in a few other cases. In BFQ we used a somehow involved
> > mechanism to avoid that, abusing rcu (of course we'll have to wait
> > the patch to talk about it :) ), but given my lack of understanding
> > of some parts of the block layer, I'd be interested in knowing if
> > the race is possible and/or if there is something more involved
> > going on that can cause the same effects.
> OK, I'm assuming this is where Nikanth got his idea for the patch from?

I think so.

> It does seem racy in spots, we can definitely proceed on getting that
> tightened up some more.
> > - set_task_ioprio() in fs/ioprio.c doesn't seem to have a write
> > memory barrier to pair with the dependent read one in
> > cfq_get_io_context().
> Agree, that needs fixing.
> > - CFQ_MIN_TT is 2ms, this can result, depending on the value of
> > HZ in timeouts of one jiffy, that may expire too early, so we are
> > just wasting time and do not actually wait for the task to present
> > its new request. Dealing with seeky traffic we've seen a lot of
> > early timeouts due to one jiffy timers expiring too early, is
> > it worth fixing or can we live with that?
> We probably just need to enfore a '2 jiffies minimum' rule for that.
> > - To detect hw tagging in BFQ we consider a sample valid iff the
> > number of requests that the scheduler could have dispatched (given
> > by cfqd->rb_queued + cfqd->rq_in_driver, i.e., the ones still into
> > the scheduler plus the ones into the driver) is higher than the
> > CFQ_HW_QUEUE_MIN threshold. This obviously caused no problems
> > during testing, but the way CFQ uses now seems a little bit
> > strange.
> Not sure this matters a whole lot, but your approach makes sense. Have
> you seen the later change to the CFQ logic from Aaron?

Yes, we started from his code. As Aaron reported, on BFQ our change
to the CIC_SEEKY logic has a bad interaction with the hw tag detection
on some workloads, but that problem should be easy to solve (test patch
posted in

> > - Initially, cic->last_request_pos is zero, so the sdist charged
> > to a task for its first seek depends on the position on the disk
> > that is accessed first, independently from its seekiness. Even
> > if there is a cap on that value, we choose to not charge the first
> > seek to processes; that resulted in less wrong predictions for
> > purely sequential loads.
> Agreed, that's is definitely off.
> > - From my understanding, with shared I/O contexts, two different
> > tasks may concurrently lookup for a cfqd into the same ioc.
> > This may result in cfq_drop_dead_cic() being called two times
> > for the same cic. Am I missing something that prevents that from
> > happening?
> That also looks problematic. I guess we need to recheck that under the
> lock when in cfq_drop_dead_cic().
> > Regarding the code splitup, do you think you'll go for the CFS(BFQ) way,
> > using a single compilation unit and including the .c files, or a layout
> > with different compilation units (like the ll_rw_blk.c splitup)?
> Different compilation units would be my preferred choice.

Ok, thank you, I'll try to put together and test some patches, and to
post them for discussion in the next few days.

 \ /
  Last update: 2008-11-19 16:51    [W:0.167 / U:2.432 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site