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

[Jan Kara]

On Wed 01-07-15 21:38:15, Tejun Heo wrote:
> 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.

Yeah, I didn't mean this comment as something you should change even if the
series wasn't applied yet (it isn't that bad). I meant it more as a
feedback for future.
 
> 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.

In general I agree with you - separating refactoring from functional
changes is useful. I just think you took it a bit to the extreme in this
series :) When I'm reviewing patches, I'm also checking whether the
function does what it's "supposed" to do. So in case of splitting patches
like this I have to go through the series and verify that in the end we end
up with what one would expect. And sometimes the correctness is so much
easier to verify when changes are split that the extra patch chasing is
worth it.  But in simple cases like this one, merged patch would have been
easier for me. I guess it's a matter of taste...

								Honza
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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/