Messages in this thread Patch in this message | | | From | Dongsheng Yang <> | Subject | Re: [PATCH] bcache: dont reset bio opf in bch_data_insert_start | Date | Thu, 28 Jan 2021 17:10:18 +0800 |
| |
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);
Thanx
Yang
| |