On 07/10/2009 04:18 PM, Tejun Heo wrote: > Hello, Christoph. > > Christoph Hellwig wrote: >> On Thu, Jul 09, 2009 at 09:45:24AM +0900, Tejun Heo wrote: >>> What's more disturbing to me is the different between RQ and BIO >>> flags. __REQ_* are bit positions, REQ_* are masks while BIO_* are bit >>> positions. Sadly it seems it's already too late to change that. I >>> personally an not a big fan of simple accessors or flags defined as >>> bit positions. They seem to obscure things without much benefit. >> flags as bit positions generally only make sense if you use >> test/set/clear_bit, otherwise they just confuse things. > first please make a distinction between test/set/clear_bit and test/__set/__clear_bit the former is not an option since it's not what we need. I too, do not like the lower-case accessors for upper-case bits like: blk_failfast_dev() && blk_failfast_transport() which give nothing and confuse the grepping of sets-vs-clears. But I do like the use of __set/__clear_bit of flags. grepping is clear and code semantics are more correct. Actually I prefer when a construct like bio or request have two accessors set/clear_flags, which abstract out not the bits but the flags member. Say when things evolve in the future it is easer to adapted. What can be more clear then rq_set_flags(req, QUEUE_FLAG_QUEUED) then rq_clear_flags(req, QUEUE_FLAG_QUEUED) later. > Another shortcoming of bit position flags is masking / multi flag > operations. It's just awful. I think it's always better to define > flags as masks even when it's used with test/set/clear_bit(). If such > usages are common enough, we can easily add test/set/clear_bit_mask(). > The conversion from mask to bit would be constant most of the time and > it's not like fls/ffs() are expensive. > That's why I suggested the set/clear_flags() variable size macro which can set/clear multiple bit-flags at same cost of masks, only that the compiler calculates the mask in compile time. This can also be good for the greps above. .eg: test_flags(&rq->cmd_flags, REQ_FAILFAST_DEV, REQ_FAILFAST_TRANSPORT, REQ_FAILFAST_DRIVER); >> And the accessors are pretty annoying, especially in the block >> layer. Trying to find the places where a BIO flag has an actual >> effect is pretty painful due to the mix of the different flags and >> the accessors. > > Yeap, fully agreed. > As said, yes, the the lower-case accessors for upper-case bits does nothing, but use __set/__clear/test is a different matter that can also replace the sugary need of these. > Thanks. > Thanks Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
