All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: Tejun Heo <tj@kernel.org>, Shaohua Li <shaohua.li@intel.com>,
	linux kernel mailing list <linux-kernel@vger.kernel.org>
Subject: Re: Kernel crash in icq_free_icq_rcu
Date: Wed, 18 Jan 2012 20:05:03 +0100	[thread overview]
Message-ID: <4F1717DF.8060809@kernel.dk> (raw)
In-Reply-To: <20120118163638.GD30204@redhat.com>

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


  parent reply	other threads:[~2012-01-18 19:05 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-01-17 20:18 Kernel crash in icq_free_icq_rcu Vivek Goyal
2012-01-17 20:19 ` Jens Axboe
2012-01-17 20:40   ` Vivek Goyal
2012-01-17 20:42     ` Jens Axboe
2012-01-17 20:58       ` Vivek Goyal
2012-01-17 21:01         ` Vivek Goyal
2012-01-17 21:48 ` Tejun Heo
2012-01-17 22:07   ` Vivek Goyal
2012-01-18  1:01     ` Shaohua Li
2012-01-18  1:03       ` Tejun Heo
2012-01-18  1:05         ` Shaohua Li
2012-01-18  1:11           ` Tejun Heo
2012-01-18  1:30             ` Shaohua Li
2012-01-18  2:26               ` Shaohua Li
2012-01-18  4:23                 ` Shaohua Li
2012-01-18  6:03               ` Shaohua Li
2012-01-18 13:51                 ` Vivek Goyal
2012-01-18 14:20                   ` Vivek Goyal
2012-01-18 16:09                     ` Tejun Heo
2012-01-18 16:24                       ` Jens Axboe
2012-01-18 16:31                         ` Jens Axboe
2012-01-18 16:36                           ` Vivek Goyal
2012-01-18 17:10                             ` Tejun Heo
2012-01-18 19:07                               ` Jens Axboe
2012-01-18 19:05                             ` Jens Axboe [this message]
2012-01-18 16:55                           ` Tejun Heo
2012-01-18 16:07                 ` Tejun Heo
2012-01-19  1:41                   ` [patch]block: fix NULL icq_cache reference Shaohua Li
2012-01-19  1:43                     ` Tejun Heo
2012-01-19  8:20                     ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F1717DF.8060809@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=shaohua.li@intel.com \
    --cc=tj@kernel.org \
    --cc=vgoyal@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.