Re: [PATCH] bcache: dont reset bio opf in bch_data_insert_start

[Dongsheng Yang]

Hi Christoph,

在 2021/1/28 星期四 下午 5:10, Dongsheng Yang 写道:
> Hi Christop:
>
> 在 2021/1/28 星期四 上午 1:37, Christoph Hellwig 写道:
> > But the old code is also completely broken.  We can't just OR in
> > the op, as that implicitly assumes the old op was 0 (REQ_OP_READ).
>
>
> Yes, indeed, there is an assume that the op is just possible to be 0 
> (REQ_OP_READ) or 1 (REQ_OP_WRITE).
>
> REQ_OP_WRITE is from cached_dev_submit_bio() which would be submitted by 
> upper user.
>
> REQ_OP_READ is from bcache itself, such as cached_dev_read_done() (when 
> we found cache miss, we will read
>
> data from backing and then we want to insert it into cache device. then 
> there is a read bio with data reach here, we
>
> need to set the bio_op to REQ_OP_WRITE, and send this bio to cache device).
>
> > Please fix this to explicitly set the exact op and flags that you want
> > instead of this fragile magic.blk_rq_map_kern
>
> This commit only want to fix the logic bug introduced in ad0d9e76a412 
> ("bcache: use bio op accessors"),
>
> that's more likely a partial revert.
>
>
> I agree that we can make it more clearly and explicitly.
>
> But I found there is no accessor to set op only, besides, the 
> bio_set_op_attrs() was marked as obsolete.
>
> There are some others doing similar things as below:
>
> blk_rq_map_kern():
>
> bio->bi_opf &= ~REQ_OP_MASK;
>
> bio->bi_opf |= req_op(rq);
>
>
> So what about below:
>
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index c7cadaafa947..bacc7366002f 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -244,7 +244,14 @@ static void bch_data_insert_start(struct closure *cl)
>                  trace_bcache_cache_insert(k);
>                  bch_keylist_push(&op->insert_keys);
>
> -               bio_set_op_attrs(n, REQ_OP_WRITE, 0);
> +               /*
> +                * n here would be REQ_OP_READ, if
> +                * we are inserting data read from
> +                * backing device in cache miss or
> +                * inserting data in movinggc.
> +                */
> +               n->bi_opf &= ~REQ_OP_MASK;
> +               n->bi_opf |= REQ_OP_WRITE;
>                  bch_submit_bbio(n, op->c, k, 0);
>          } while (n != bio);

Another solution is introducing an accessor to set op only, something 
like bio_set_op(). Then we should keep the bcache patch as what it was 
to fix the bug.

And send another patch to introduce bio_set_op():

diff --git a/block/blk-map.c b/block/blk-map.c
index 6e804892d5ec..83bc33a59fa5 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -587,9 +587,7 @@ static int __blk_rq_map_user_iov(struct request *rq,
        if (IS_ERR(bio))
                return PTR_ERR(bio);

-       bio->bi_opf &= ~REQ_OP_MASK;
-       bio->bi_opf |= req_op(rq);
-
+       bio_set_op(bio, req_op(rq));
        orig_bio = bio;

        /*
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index eb734f7ddaac..d8839300805e 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -244,7 +244,13 @@ static void bch_data_insert_start(struct closure *cl)
                trace_bcache_cache_insert(k);
                bch_keylist_push(&op->insert_keys);

-               n->bi_opf |= REQ_OP_WRITE;
+               /*
+                * n here would be REQ_OP_READ, if
+                * we are inserting data read from
+                * backing device in cache miss or
+                * inserting data in movinggc.
+                */
+               bio_set_op(n, REQ_OP_WRITE);
                bch_submit_bbio(n, op->c, k, 0);
        } while (n != bio);

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index b3fc5d3dd8ea..2affd3269bdc 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -439,6 +439,12 @@ static inline void bio_set_op_attrs(struct bio 
*bio, unsigned op,
        bio->bi_opf = op | op_flags;
 }

+static inline void bio_set_op(struct bio *bio, unsigned op)
+{
+       bio->bi_opf &= ~REQ_OP_MASK;
+       bio->bi_opf |= op;
+}
+
 static inline bool op_is_write(unsigned int op)
 {
        return (op & 1);

> Thanx
>
> Yang