| Message ID | 20211115215819.28787-1-huobean@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Use REQ_OP_WRITE instead of its integer constant 1 in op_is_write() | expand |
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.
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 >
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.
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); } /*