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
-
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/