lkml.org 
[lkml]   [2006]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
DateFri, 10 Feb 2006 09:21:04 +0900
FromTejun Heo <>
SubjectRe: [PATCH] block: kill not-so-popular simple flag testing macros
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
-
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/

\
 
 \ /
  Last update: 2006-02-10 01:23    [from the cache]
©2003-2010