> From: Jens Axboe <jens.axboe@xxxxxxxxxx> > 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. _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/virtualization
