linux-kernel - Re: [PATCH block:for-3.3/core] block: disable ELEVATOR_INSERT_SORT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 6 Jan 2012 07:34:52 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Shaohua Li <shaohua.li@...el.com>
Cc:	Jens Axboe <axboe@...nel.dk>, Hugh Dickins <hughd@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	linux-next@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	linux-scsi@...r.kernel.org, linux-ide@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH block:for-3.3/core] block: disable
 ELEVATOR_INSERT_SORT_MERGE

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/