Re: [PATCH] bcache: fix double bio_endio completion in detached_dev_end_io

Stephen Zhang
2026-01-19T06:54:30Z

Coly Li <colyli@fnnas.com> 于2026年1月18日周日 19:49写道：
>
> On Thu, Jan 15, 2026 at 12:29:15AM +0800, Christoph Hellwig wrote:
> > Can you please test this patch?
> >
>
> Shida,
> can you also test it and confirm? We need to get the fix in within 6.19 cycle.
>
> Yes, we need to make a dicision ASAP.
> I prefer the clone bio solution, and it looks fine to me.
>
> Thanks in advance.
>
> Coly Li
>
>
>
>
> > commit d14f13516f60424f3cffc6d1837be566e360a8a3
> > Author: Christoph Hellwig <hch@lst.de>
> > Date: Tue Jan 13 09:53:34 2026 +0100
> >
> > bcache: clone bio in detached_dev_do_request
> >
> > Not-yet-Signed-off-by: Christoph Hellwig <hch@lst.de>
> >

Thank you, Coly and Christoph, for giving me the opportunity to continue
your outstanding work on this patch.

If given the chance to complete the next steps, I will begin by adding a
proper commit message:

bcache: use bio cloning for detached device requests

Previously, bcache hijacked the bi_end_io and bi_private fields of the incoming
bio when the backing device was in a detached state. This is fragile and
breaks if the bio is needed to be processed by other layers.

This patch transitions to using a cloned bio embedded within a private
structure.
This ensures the original bio's metadata remains untouched.

Co-developed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>

Additionally, I would like to conduct a thorough code review to identify any
potential issues that may not be easily caught through normal testing.

> > diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> > index 82fdea7dea7a..9e7b59121313 100644
> > --- a/drivers/md/bcache/request.c
> > +++ b/drivers/md/bcache/request.c
> > @@ -1078,67 +1078,66 @@ static CLOSURE_CALLBACK(cached_dev_nodata)
> > }
> >
> > struct detached_dev_io_private {
> > - struct bcache_device *d;
> > unsigned long start_time;
> > - bio_end_io_t *bi_end_io;
> > - void *bi_private;
> > - struct block_device *orig_bdev;
> > + struct bio *orig_bio;
> > + struct bio bio;
> > };
> >
> > static void detached_dev_end_io(struct bio *bio)
> > {
> > - struct detached_dev_io_private *ddip;
> > -
> > - ddip = bio->bi_private;
> > - bio->bi_end_io = ddip->bi_end_io;
> > - bio->bi_private = ddip->bi_private;
> > + struct detached_dev_io_private *ddip =
> > + container_of(bio, struct detached_dev_io_private, bio);
> > + struct bio *orig_bio = ddip->orig_bio;
> >
> > /* Count on the bcache device */
> > - bio_end_io_acct_remapped(bio, ddip->start_time, ddip->orig_bdev);
> > + bio_end_io_acct(orig_bio, ddip->start_time);
> >
> > if (bio->bi_status) {
> > - struct cached_dev *dc = container_of(ddip->d,
> > - struct cached_dev, disk);
> > + struct cached_dev *dc = bio->bi_private;
> > +
> > /* should count I/O error for backing device here */
> > bch_count_backing_io_errors(dc, bio);
> > + orig_bio->bi_status = bio->bi_status;
> > }
> >

bio_init_clone must be paired with a bio_uninit() call before the
memory is freed?

+ bio_uninit(bio);

Thanks,
Shida

> > kfree(ddip);
> > - bio_endio(bio);
> > + bio_endio(orig_bio);
> > }
> >
> > -static void detached_dev_do_request(struct bcache_device *d, struct bio *bio,
> > - struct block_device *orig_bdev, unsigned long start_time)
> > +static void detached_dev_do_request(struct bcache_device *d,
> > + struct bio *orig_bio, unsigned long start_time)
> > {
> > struct detached_dev_io_private *ddip;
> > struct cached_dev *dc = container_of(d, struct cached_dev, disk);
> >
> > + if (bio_op(orig_bio) == REQ_OP_DISCARD &&
> > + !bdev_max_discard_sectors(dc->bdev)) {
> > + bio_endio(orig_bio);
> > + return;
> > + }
> > +
> > /*
> > * no need to call closure_get(&dc->disk.cl),
> > * because upper layer had already opened bcache device,
> > * which would call closure_get(&dc->disk.cl)
> > */
> > ddip = kzalloc(sizeof(struct detached_dev_io_private), GFP_NOIO);
> > - if (!ddip) {
> > - bio->bi_status = BLK_STS_RESOURCE;
> > - bio_endio(bio);
> > - return;
> > - }
> > -
> > - ddip->d = d;
> > + if (!ddip)
> > + goto enomem;
> > + if (bio_init_clone(dc->bdev, &ddip->bio, orig_bio, GFP_NOIO))
> > + goto free_ddip;
> > /* Count on the bcache device */
> > - ddip->orig_bdev = orig_bdev;
> > ddip->start_time = start_time;
> > - ddip->bi_end_io = bio->bi_end_io;
> > - ddip->bi_private = bio->bi_private;
> > - bio->bi_end_io = detached_dev_end_io;
> > - bio->bi_private = ddip;
> > -
> > - if ((bio_op(bio) == REQ_OP_DISCARD) &&
> > - !bdev_max_discard_sectors(dc->bdev))
> > - detached_dev_end_io(bio);
> > - else
> > - submit_bio_noacct(bio);
> > + ddip->orig_bio = orig_bio;
> > + ddip->bio.bi_end_io = detached_dev_end_io;
> > + ddip->bio.bi_private = dc;
> > + submit_bio_noacct(&ddip->bio);
> > + return;
> > +free_ddip:
> > + kfree(ddip);
> > +enomem:
> > + orig_bio->bi_status = BLK_STS_RESOURCE;
> > + bio_endio(orig_bio);
> > }
> >
> > static void quit_max_writeback_rate(struct cache_set *c,
> > @@ -1214,10 +1213,10 @@ void cached_dev_submit_bio(struct bio *bio)
> >
> > start_time = bio_start_io_acct(bio);
> >
> > - bio_set_dev(bio, dc->bdev);
> > bio->bi_iter.bi_sector += dc->sb.data_offset;
> >
> > if (cached_dev_get(dc)) {
> > + bio_set_dev(bio, dc->bdev);
> > s = search_alloc(bio, d, orig_bdev, start_time);
> > trace_bcache_request_start(s->d, bio);
> >
> > @@ -1237,9 +1236,10 @@ void cached_dev_submit_bio(struct bio *bio)
> > else
> > cached_dev_read(dc, s);
> > }
> > - } else
> > + } else {
> > /* I/O request sent to backing device */
> > - detached_dev_do_request(d, bio, orig_bdev, start_time);
> > + detached_dev_do_request(d, bio, start_time);
> > + }
> > }
> >
> > static int cached_dev_ioctl(struct bcache_device *d, blk_mode_t mode,