Messages in this thread | | | Date | Wed, 18 Jan 2012 20:05:03 +0100 | From | Jens Axboe <> | Subject | Re: Kernel crash in icq_free_icq_rcu |
| |
On 2012-01-18 17:36, Vivek Goyal wrote: > On Wed, Jan 18, 2012 at 05:31:06PM +0100, Jens Axboe wrote: >> On 2012-01-18 17:24, Jens Axboe wrote: >>> On 2012-01-18 17:09, Tejun Heo wrote: >>>> On Wed, Jan 18, 2012 at 09:20:05AM -0500, Vivek Goyal wrote: >>>>>> Not allocating icq if request is never going to go to elevator as elevator >>>>>> switch was happening makes sense to me. >>>>>> >>>>>> I tried this patch. It went little further and crashed at a different >>>>>> place. I think this seems to be separate merging issue Tejun is trying >>>>>> to track down. >>>>> >>>>> Applied Tejun's debug patch to return early and not call into elevator >>>>> for checking whether merge is allowed or not. Things seems to be stable >>>>> now for me. >>>> >>>> Yeah, plug merge is calling into elevator code without any >>>> synchronization, so it's bound to be broken. Given plugging is >>>> per-task, I don't think we really need to query elevator about merging >>>> bio's. The request is not on elevator and plugging is part of issuing >>>> mechanism, not scheduling, after all. Jens, what do you think? >>> >>> Hmmm. We can bypass asking the elevator, as long as we query the >>> restrictions. Does the below, by itself, resolve the crash? If yes, let >>> me cook up a patch splitting the elv and blk rq merging logic. >> >> Something like the below, completely untested. >> >> But thinking about this a bit while doing it, why is the IO scheduler >> going away while we have plugged requests that are elvpriv? > > Not calling ioscheduler during plug merge will allow merging of sync/async > requests together. I guess we wouldn't want that. The only check we can > skip in case of plug merge, is whether bio and rq beong to same task/cfqq > or not.
It's not a huge concern. Since the IO is coming from the same task, it's definitely related. And for the related cases, we pretty much always want merging anyway.
> May be separate elevator functions for plug merge (without lock) and > elevator merge (with lock) will do?
I don't think that's a good idea, just the restriction checking should be enough.
-- Jens Axboe
| |