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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux ARM Kernel]     [Linux Filesystem Development]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux