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