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
