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

[Tejun Heo]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/