[PATCH] block: clear all of bi_opf in bio_set_op_attrs

[PATCH] block: clear all of bi_opf in bio_set_op_attrs

[Christoph Hellwig]

Since commit 87374179 ("block: add a proper block layer data direction
encoding") we only OR the new op and flags into bi_opf in bio_set_op_attrs
instead of clearing the old value.  I've not seen any breakage with the
new behavior, but it seems dangerous.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 562ac46..67434fd 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -208,7 +208,7 @@ enum req_flag_bits {
 
 /* obsolete, don't use in new code */
 #define bio_set_op_attrs(bio, op, op_flags) \
-	((bio)->bi_opf |= (op | op_flags))
+	((bio)->bi_opf = op | op_flags)
 
 static inline bool op_is_write(unsigned int op)
 {
-- 
2.1.4

Re: [PATCH] block: clear all of bi_opf in bio_set_op_attrs

[Bart Van Assche]

On 11/09/2016 10:38 AM, Christoph Hellwig wrote:
> Since commit 87374179 ("block: add a proper block layer data direction
> encoding") we only OR the new op and flags into bi_opf in bio_set_op_attrs
> instead of clearing the old value.  I've not seen any breakage with the
> new behavior, but it seems dangerous.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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 562ac46..67434fd 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -208,7 +208,7 @@ enum req_flag_bits {
>
>  /* obsolete, don't use in new code */
>  #define bio_set_op_attrs(bio, op, op_flags) \
> -	((bio)->bi_opf |= (op | op_flags))
> +	((bio)->bi_opf = op | op_flags)
>
>  static inline bool op_is_write(unsigned int op)
>  {

Hello Christoph,

Have you verified whether or not this change affects the behavior of the 
bcache driver? From commit ad0d9e76a412:

@@ -114,7 +114,7 @@ void bch_data_verify(struct cached_dev *dc, struct 
bio *bio)
         check = bio_clone(bio, GFP_NOIO);
         if (!check)
                 return;
-       check->bi_rw |= READ_SYNC;
+       bio_set_op_attrs(check, REQ_OP_READ, READ_SYNC);

         if (bio_alloc_pages(check, GFP_NOIO))
                 goto out_put;

Additionally, since bio_set_op_attrs is a macro please surround 'op' and 
'op_flags' with parentheses or change this macro into an inline function.

Thanks,

Bart.

Re: [PATCH] block: clear all of bi_opf in bio_set_op_attrs

[Christoph Hellwig]

On Thu, Nov 10, 2016 at 08:31:11AM -0800, Bart Van Assche wrote:
> Have you verified whether or not this change affects the behavior of the 
> bcache driver? From commit ad0d9e76a412:

It doesn't, bcache only calls bch_data_verify from a read completion
handler.

> Additionally, since bio_set_op_attrs is a macro please surround 'op' and 
> 'op_flags' with parentheses or change this macro into an inline function.

Makese sense, thanks.

[PATCH] block: clear all of bi_opf in bio_set_op_attrs

[Christoph Hellwig]

Since commit 87374179 ("block: add a proper block layer data direction
encoding") we only or the new op and flags into bi_opf in bio_set_op_attrs
instead of clearing the old value.  I've not seen any breakage with the
new behavior, but it seems dangerous.

Also convert it to an inline function to make the argument passing
safer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/blk_types.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4d0044d..f57458a 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -207,8 +207,11 @@ enum req_flag_bits {
 	((req)->cmd_flags & REQ_OP_MASK)
 
 /* obsolete, don't use in new code */
-#define bio_set_op_attrs(bio, op, op_flags) \
-	((bio)->bi_opf |= (op | op_flags))
+static inline void bio_set_op_attrs(struct bio *bio, unsigned op,
+		unsigned op_flags)
+{
+	bio->bi_opf = op | op_flags;
+}
 
 static inline bool op_is_write(unsigned int op)
 {
-- 
2.1.4

Re: [PATCH] block: clear all of bi_opf in bio_set_op_attrs

[Jens Axboe]

On 11/21/2016 08:18 AM, Christoph Hellwig wrote:
> Since commit 87374179 ("block: add a proper block layer data direction
> encoding") we only or the new op and flags into bi_opf in bio_set_op_attrs
> instead of clearing the old value.  I've not seen any breakage with the
> new behavior, but it seems dangerous.
>
> Also convert it to an inline function to make the argument passing
> safer.

Agree, this is safer. Applied for 4.10.

-- 
Jens Axboe