From: Tejun Heo <tj@kernel.org>
To: Shaohua Li <shaohua.li@intel.com>
Cc: Jens Axboe <axboe@kernel.dk>, Hugh Dickins <hughd@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Stephen Rothwell <sfr@canb.auug.org.au>,
linux-next@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org,
x86@kernel.org
Subject: Re: [PATCH block:for-3.3/core] block: disable ELEVATOR_INSERT_SORT_MERGE
Date: Fri, 6 Jan 2012 07:34:52 -0800 [thread overview]
Message-ID: <20120106153452.GG6276@google.com> (raw)
In-Reply-To: <1325837757.22361.538.camel@sli10-conroe>
Hello, Shaohua.
On Fri, Jan 06, 2012 at 04:15:57PM +0800, Shaohua Li wrote:
> it's not related to the recursive merge. I forgot the detail of the
> workload why we add INSERT_SORT_MERGE, it's added several months ago
> anyway.
> Obviously we shouldn't do complex things inside the merge window. I just
> didn't agree to disable INSERT_SORT_MERGE, which will bring performance
> regression for sure. Can we just change CFQ like your first path?
But that is exactly what was broken and papering it over is exactly
the thing we shouldn't do, for now or for ever. Think about it. Your
commit f1f8cc94651 "block, cfq: fix empty queue crash caused by
request merge" fixed one symptom caused by cross cfqq merging
happening behind cfq's back without properly root-causing it. I'm not
saying it was your fault but in the end all it did was obscuring the
root cause of the problem further, making the next more subtle failure
much more difficult to diagnose.
So, if you think cross-cfqq merging is a good idea, please go ahead
and do it properly. It shouldn't happen as a side effect of two bugs
folded together.
As for quicker fix than revamping merge infrastructure, adding another
callback to explicitly ask elevator whether two requests chosen by
block core can be merged. I didn't go that route because I wasn't
sure about the benefit of doing so - I still can't see how that would
make a lot of difference without cross cfqq merging, and wanted to
stay on the safer side given the intricacy of the problem.
So, if you wanna do that and have workload which is benefited enough
to justify the extra step, let's do it after the merge window, merge
as fixes and send it back via -stable. But we shouldn't be doing that
inside merge window when all other trees are getting pushed upstream
and we don't have much time for testing ourselves or in linux-next.
Thanks.
--
tejun
next prev parent reply other threads:[~2012-01-06 15:34 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20111221174733.9ba0861e762e8d96844b060b@canb.auug.org.au>
2011-12-21 23:15 ` linux-next: Tree for Dec 21 Andrew Morton
2011-12-22 23:08 ` Andrew Morton
2011-12-22 23:20 ` Tejun Heo
2011-12-22 23:24 ` Andrew Morton
2011-12-22 23:38 ` Tejun Heo
2011-12-22 23:44 ` Andrew Morton
2011-12-22 23:46 ` Tejun Heo
2011-12-23 0:42 ` Tejun Heo
2011-12-24 5:13 ` Hugh Dickins
2011-12-25 1:02 ` [PATCH block/for-3.3/core] block: an exiting task should be allowed to create io_context Tejun Heo
2011-12-25 13:29 ` Jens Axboe
2011-12-27 22:07 ` Andrew Morton
2011-12-28 8:33 ` Hugh Dickins
2011-12-28 16:48 ` Tejun Heo
2011-12-28 17:50 ` Hugh Dickins
2011-12-28 17:55 ` Tejun Heo
2011-12-28 21:19 ` Tejun Heo
2012-01-03 17:35 ` Tejun Heo
2012-01-03 17:59 ` Tejun Heo
2012-01-03 20:09 ` Tejun Heo
2012-01-03 20:20 ` Jens Axboe
2012-01-03 22:13 ` Tejun Heo
2012-01-03 22:35 ` Tejun Heo
2012-01-05 1:24 ` Tejun Heo
2012-01-05 18:36 ` Hugh Dickins
2012-01-05 18:38 ` Tejun Heo
2012-01-06 2:17 ` [PATCH block:for-3.3/core] cfq: merged request shouldn't jump to a different cfqq Tejun Heo
2012-01-06 2:36 ` Tejun Heo
2012-01-06 3:14 ` Shaohua Li
2012-01-06 3:04 ` Tejun Heo
2012-01-06 3:30 ` Tejun Heo
2012-01-06 3:52 ` [PATCH block:for-3.3/core] block: disable ELEVATOR_INSERT_SORT_MERGE Tejun Heo
2012-01-06 4:19 ` Shaohua Li
2012-01-06 4:38 ` Tejun Heo
2012-01-06 8:15 ` Shaohua Li
2012-01-06 15:34 ` Tejun Heo [this message]
2012-01-06 3:34 ` [PATCH block:for-3.3/core] cfq: merged request shouldn't jump to a different cfqq Shaohua Li
2012-01-06 3:22 ` Tejun Heo
2012-01-06 4:15 ` Shaohua Li
2012-01-06 4:40 ` Tejun Heo
2012-01-06 2:47 ` Shaohua Li
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=20120106153452.GG6276@google.com \
--to=tj@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=axboe@kernel.dk \
--cc=hughd@google.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=sfr@canb.auug.org.au \
--cc=shaohua.li@intel.com \
--cc=x86@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).