[PATCH] block: Use REQ_OP_WRITE instead of its integer constant 1 in op_is_write()

[PATCH] block: Use REQ_OP_WRITE instead of its integer constant 1 in op_is_write()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

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@micron.com>
---
 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);
 }
 
 /*
-- 
2.25.1

Re: [PATCH] block: Use REQ_OP_WRITE instead of its integer constant 1 in op_is_write()

[Damien Le Moal]

On 2021/11/16 6:58, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> 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@micron.com>
> ---
>  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.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] block: Use REQ_OP_WRITE instead of its integer constant 1 in op_is_write()

[Chaitanya Kulkarni]

Bean,

>   static inline bool op_is_write(unsigned int op)
>   {
> -       return (op & 1);
> +       return (op & REQ_OP_WRITE);
>   }

As pointed out by Damien it supposed to indicate data out and not
only write request REQ_OP_WRITE.

> 
>   /*
> --
> 2.25.1
> 

Re: [PATCH] block: Use REQ_OP_WRITE instead of its integer constant 1 in op_is_write()

[Keith Busch]

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@micron.com>
> > 
> > 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@micron.com>
> > ---
> >  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.