Re: [PATCH 28/51] writeback, blkcg: restructure blk_{set|clear}_queue_congested()

Path	csiph.com!aioe.org!news.servidellagleba.it!bofh.it!news.nic.it!robomod
From	Tejun Heo <tj@kernel.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH 28/51] writeback, blkcg: restructure blk_{set\|clear}_queue_congested()
Date	Thu, 02 Jul 2015 03:40:01 +0200
Message-ID	<pHBDP-4uw-9@gated-at.bofh.it> (permalink)
References	<pt2wi-1zh-3@gated-at.bofh.it> <pt2G0-1Lc-53@gated-at.bofh.it> <pH5kB-5vh-3@gated-at.bofh.it>
X-Original-To	Jan Kara <jack@suse.cz>
Dkim-Signature	v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=Opvd5hQEUtR5BpHhJ/1S79O1ZXB7UBxOP+xxvCjfHec=; b=RhEWaem2Yh/QK/sJa1qz0SYrHdaPrMndsSHyPt6+9kEYwNU4wV1Zwv4p3dthFV7sT5 CG1nvQRQI9qzKRZACH2Zy7ihzEJm2eTn0o9nrVdPWd74alWg3Rq5+UZny9iN0AOm1BG/ /y3Y0yKtHT5ER4wfVALspGpz1gM0t71MPRblhmO5iIYzM1iLqM856rIadYhbdm2KQB6y PwzvqC08pw/zSAzcEEEgjXjx38lyqNl+y3yEkqHkYH47ClXtQu1cSEUzjXHaNK98J6qH JYeelYjyni4PgvUeAhrtr23ZONttNuGXjcGK/y8Z+B420qJtKN/O2vc4WWGWJGtMSuyT fyIA==
X-Received	by 10.140.85.85 with SMTP id m79mr39114132qgd.37.1435801098684; Wed, 01 Jul 2015 18:38:18 -0700 (PDT)
MIME-Version	1.0
Content-Type	text/plain; charset=us-ascii
Content-Disposition	inline
User-Agent	Mutt/1.5.23 (2014-03-12)
Sender	robomod@news.nic.it
List-ID	<linux-kernel.vger.kernel.org>
X-Mailing-List	linux-kernel@vger.kernel.org
Approved	robomod@news.nic.it
Lines	41
Organization	linux.* mail to news gateway
X-Original-Cc	axboe@kernel.dk, linux-kernel@vger.kernel.org, hch@infradead.org, hannes@cmpxchg.org, linux-fsdevel@vger.kernel.org, vgoyal@redhat.com, lizefan@huawei.com, cgroups@vger.kernel.org, linux-mm@kvack.org, mhocko@suse.cz, clm@fb.com, fengguang.wu@intel.com, david@fromorbit.com, gthelen@google.com, khlebnikov@yandex-team.ru
X-Original-Date	Wed, 1 Jul 2015 21:38:15 -0400
X-Original-Message-ID	<20150702013815.GE26440@mtj.duckdns.org>
X-Original-References	<1432329245-5844-1-git-send-email-tj@kernel.org> <1432329245-5844-29-git-send-email-tj@kernel.org> <20150630150254.GN7252@quack.suse.cz>
X-Original-Sender	linux-kernel-owner@vger.kernel.org
Xref	aioe.org linux.kernel:1175646

Show key headers only | View raw

Hello, Jan.

On Tue, Jun 30, 2015 at 05:02:54PM +0200, Jan Kara wrote:
> BTW, I'd prefer if this was merged with the following patch. I was
> wondering for a while about the condition at the beginning of
> blk_clear_congested() only to learn it gets modified to the one I'd expect
> in the following patch :)

The patches are already merged, it's a bit too late to discuss but I
usually try to keep each step quite granular.  e.g. I try hard to
avoid combining code relocation / restructuring with actual functional
changes so that the code change A -> B -> C where B is functionally
identical to A and C is different from B only where the actual
functional changes occur.

I think your argument is that as C is the final form, introducing B is
actually harder for reviewing.  I have to disagree with that pretty
strongly.  When you only think about the functional transformations A
-> C might seem easier but given that we also want to verify the
changes - both during development and review - it's far more
beneficial to go through the intermediate stage as that isolates
functional changes from mere code transformation.

Another thing to consider is that there's a difference when one is
reviewing a patch series as a whole tracking the development of big
picture and later when somebody tries to debug or bisect a bug the
patchset introduces.  At that point, the general larger flow isn't
really in the picture and combining structural and functional changes
may make understanding what's going on significantly harder in
addition to making such errors more likely and less detectable in the
first place.

Thanks.

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

Back to linux.kernel | Previous | Next | Find similar | Unroll thread

Thread

Re: [PATCH 28/51] writeback, blkcg: restructure  blk_{set|clear}_queue_congested() Tejun Heo <tj@kernel.org> - 2015-07-02 03:40 +0200

csiph-web