On Tue, Nov 16, 2021 at 08:48:56AM +0900, Damien Le Moal wrote:
> On 2021/11/16 6:58, Bean Huo wrote:
> > From: Bean Huo <beanhuo@xxxxxxxxxx>
> >
> > Use the enums REQ_OP_WRITE in op_is_write() to make it less maintenance
> > requirement and more readable
> >
> > Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
> > ---
> > include/linux/blk_types.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> > index fe065c394fff..5b5924a7e754 100644
> > --- a/include/linux/blk_types.h
> > +++ b/include/linux/blk_types.h
> > @@ -463,7 +463,7 @@ static inline void bio_set_op_attrs(struct bio *bio, unsigned op,
> >
> > static inline bool op_is_write(unsigned int op)
> > {
> > - return (op & 1);
> > + return (op & REQ_OP_WRITE);
> > }
>
> See the comment for "enum req_opf":
>
> /*
> * Operations and flags common to the bio and request structures.
> * We use 8 bits for encoding the operation, and the remaining 24 for flags.
> *
> * The least significant bit of the operation number indicates the data
> * transfer direction:
> *
> * - if the least significant bit is set transfers are TO the device
> * - if the least significant bit is not set transfers are FROM the device
> *
> * If a operation does not transfer data the least significant bit has no
> * meaning.
> */
>
> So using "1" is correct. Using REQ_OP_WRITE is confusing as it seem to imply
> that op_is_write() tests for "op is REQ_OP_WRITE" instead of the intended "op is
> transferring data TO the device". If anything, op_is_write() could be renamed to
> clarify that.
Yeah, REQ_OP_WRITE is a value, not a flag. The op_is_write() tests the data
direction flag, which coincidentally happens to be the same as the current,
somewhat arbitrarily chosen value of REQ_OP_WRITE. The op could just as easily
have been set to 0xff.
