lkml.org 
[lkml]   [2021]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] bcache: dont reset bio opf in bch_data_insert_start
From
Date
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
>

\
 
 \ /
  Last update: 2021-01-28 12:15    [W:0.046 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site