Re: [PATCH] block: kill not-so-popular simple flag testing macros

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch kills the following request flag testing macros.
>>
>> blk_noretry_request()
>> blk_rq_started()
>> blk_pm_suspend_request()
>> blk_pm_resume_request()
>> blk_sorted_rq()
>> blk_barrier_rq()
>> blk_fua_rq()
>>
>> All above macros test for single request flag and not used widely and
>> seem to contribute more to obscurity than readability.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
> heh, I guess that's a manner of opinion :)
> 
> To me, your patch makes things less readable.  Example:
> 
>> -    int is_barrier = blk_fs_request(rq) && blk_barrier_rq(rq);
>> +    int is_barrier = blk_fs_request(rq) && rq->flags & REQ_HARDBARRIER;
> 
> 
> After your change is applied, the above statement is no longer visually 
> consistent with itself.  The reader must decode two different styles of 
> test in his brain, as opposed to one.
> 

Hello, Jeff.

Yeah, I didn't like that line either, but the thing that triggered this 
patch is this message from Linus.  This is a reply to Andrew's 
dropped-from-rc message so not on lkml.


> I'm applying this under protest.
> 
> The code may be right, but it visually makes _zero_ sense.
> 
> The fact that "blk_barrier_rq()" only triggers on hard barriers means that 
> it does what you describe in the changelog, but read the code and see how 
> little sense it makes when you _read_ it. You just checked that the rq has 
> a barrier (either a soft or a hard one), and now you check "again" whether 
> it has a barrier.
> 
> That's what it looks like from reading the code. Confusing. And thus prone 
> to bugs.
> 

The code he was talking about looks like.

if (rq->flags & (REQ_SOFTBARRIER | REQ_HARDBARRIER) {
	....
	if (blk_barrier_rq(rq))
		q->ordcolor ^= 1;
	....
}

We could add blk_barrier_rq() to test for both flags and add 
blk_hardbarrier_rq() and blk_softbarrier_rq(), but the flags are not 
used that widely and some other flag testing macros are used only few 
times in core block layer, so I determined to kill infrequently used macros.

I agree that the change makes code harder to eyes in some places, but it 
doesn't obfuscate the code and results in less maintenance overhead in 
general.  Remember a few testing macros is okay but remembering a dozen 
gets a bit annoying, IMHO.

Thanks.

-- 
tejun