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

From: Tejun Heo
Date: Thu Feb 09 2006 - 19:19:11 EST


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@xxxxxxxxx>


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