lkml.org 
[lkml]   [2008]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 0/4] [RFC] Another proportional weight IO controller
> From: Jens Axboe <jens.axboe@oracle.com>
> 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 http://lkml.org/lkml/2008/11/19/100).


> > - 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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site