From: Shida Zhang The `__bio_chain_endio` function, which was intended to serve only as a flag, contains faulty completion logic. When called, it decrements the `bi_remaining` count on the *next* bio in the chain without checking its own. Consider a bio chain where `bi_remaining` is decremented as each bio in the chain completes. For example, if a chain consists of three bios (bio1 -> bio2 -> bio3) with bi_remaining count: 1->2->2 if the bio completes in the reverse order, there will be a problem. if bio 3 completes first, it will become: 1->2->1 then bio 2 completes: 1->1->0 Because `bi_remaining` has reached zero, the final `end_io` callback for the entire chain is triggered, even though not all bios in the chain have actually finished processing. As a quick fix, removing `__bio_chain_endio` and allowing the standard `bio_endio` to handle the completion logic should resolve this issue, as `bio_endio` correctly manages the `bi_remaining` counter. Signed-off-by: Shida Zhang --- block/bio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/bio.c b/block/bio.c index b3a79285c27..55c2c1a0020 100644 --- a/block/bio.c +++ b/block/bio.c @@ -322,7 +322,7 @@ static struct bio *__bio_chain_endio(struct bio *bio) static void bio_chain_endio(struct bio *bio) { - bio_endio(__bio_chain_endio(bio)); + bio_endio(bio); } /** -- 2.34.1 From: Shida Zhang Andreas point out that multiple completions can race setting bi_status. The check (parent->bi_status) and the subsequent write are not an atomic operation. The value of parent->bi_status could have changed between the time you read it for the if check and the time you write to it. So we use cmpxchg to fix the race, as suggested by Christoph. Suggested-by: Andreas Gruenbacher Suggested-by: Christoph Hellwig Signed-off-by: Shida Zhang --- block/bio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/block/bio.c b/block/bio.c index 55c2c1a0020..aa43435c15f 100644 --- a/block/bio.c +++ b/block/bio.c @@ -313,9 +313,12 @@ EXPORT_SYMBOL(bio_reset); static struct bio *__bio_chain_endio(struct bio *bio) { struct bio *parent = bio->bi_private; + blk_status_t *status = &parent->bi_status; + blk_status_t new_status = bio->bi_status; + + if (new_status != BLK_STS_OK) + cmpxchg(status, BLK_STS_OK, new_status); - if (bio->bi_status && !parent->bi_status) - parent->bi_status = bio->bi_status; bio_put(bio); return parent; } -- 2.34.1 From: Shida Zhang Don't call bio->bi_end_io() directly. Use the bio_endio() helper function instead, which handles completion more safely and uniformly. Suggested-by: Christoph Hellwig Signed-off-by: Shida Zhang --- drivers/md/bcache/request.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c index af345dc6fde..82fdea7dea7 100644 --- a/drivers/md/bcache/request.c +++ b/drivers/md/bcache/request.c @@ -1104,7 +1104,7 @@ static void detached_dev_end_io(struct bio *bio) } kfree(ddip); - bio->bi_end_io(bio); + bio_endio(bio); } static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, @@ -1121,7 +1121,7 @@ static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, ddip = kzalloc(sizeof(struct detached_dev_io_private), GFP_NOIO); if (!ddip) { bio->bi_status = BLK_STS_RESOURCE; - bio->bi_end_io(bio); + bio_endio(bio); return; } @@ -1136,7 +1136,7 @@ static void detached_dev_do_request(struct bcache_device *d, struct bio *bio, if ((bio_op(bio) == REQ_OP_DISCARD) && !bdev_max_discard_sectors(dc->bdev)) - bio->bi_end_io(bio); + detached_dev_end_io(bio); else submit_bio_noacct(bio); } -- 2.34.1 From: Shida Zhang Now that all potential callers of bio_chain_endio have been eliminated, completely prohibit any future calls to this function. Suggested-by: Ming Lei Suggested-by: Andreas Gruenbacher Suggested-by: Christoph Hellwig Signed-off-by: Shida Zhang --- block/bio.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/block/bio.c b/block/bio.c index aa43435c15f..2473a2c0d2f 100644 --- a/block/bio.c +++ b/block/bio.c @@ -323,8 +323,13 @@ static struct bio *__bio_chain_endio(struct bio *bio) return parent; } +/** + * This function should only be used as a flag and must never be called. + * If execution reaches here, it indicates a serious programming error. + */ static void bio_chain_endio(struct bio *bio) { + BUG_ON(1); bio_endio(bio); } -- 2.34.1 From: Shida Zhang Export the bio_chain_and_submit function to make it available as a common utility. This will allow replacing repetitive bio chaining patterns found in multiple locations throughout the codebase. Signed-off-by: Shida Zhang --- block/bio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/bio.c b/block/bio.c index 2473a2c0d2f..6102e2577cb 100644 --- a/block/bio.c +++ b/block/bio.c @@ -371,6 +371,7 @@ struct bio *bio_chain_and_submit(struct bio *prev, struct bio *new) } return new; } +EXPORT_SYMBOL_GPL(bio_chain_and_submit); struct bio *blk_next_bio(struct bio *bio, struct block_device *bdev, unsigned int nr_pages, blk_opf_t opf, gfp_t gfp) -- 2.34.1 From: Shida Zhang Replace duplicate bio chaining logic with the common bio_chain_and_submit helper function. Signed-off-by: Shida Zhang --- fs/gfs2/lops.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c index 9c8c305a75c..0073fd7c454 100644 --- a/fs/gfs2/lops.c +++ b/fs/gfs2/lops.c @@ -487,8 +487,7 @@ static struct bio *gfs2_chain_bio(struct bio *prev, unsigned int nr_iovecs) new = bio_alloc(prev->bi_bdev, nr_iovecs, prev->bi_opf, GFP_NOIO); bio_clone_blkg_association(new, prev); new->bi_iter.bi_sector = bio_end_sector(prev); - bio_chain(new, prev); - submit_bio(prev); + bio_chain_and_submit(prev, new); return new; } -- 2.34.1 From: Shida Zhang Replace duplicate bio chaining logic with the common bio_chain_and_submit helper function. Signed-off-by: Shida Zhang --- fs/xfs/xfs_bio_io.c | 3 +-- fs/xfs/xfs_buf.c | 3 +-- fs/xfs/xfs_log.c | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c index 2a736d10eaf..4a6577b0789 100644 --- a/fs/xfs/xfs_bio_io.c +++ b/fs/xfs/xfs_bio_io.c @@ -38,8 +38,7 @@ xfs_rw_bdev( bio_max_vecs(count - done), prev->bi_opf, GFP_KERNEL); bio->bi_iter.bi_sector = bio_end_sector(prev); - bio_chain(prev, bio); - submit_bio(prev); + bio_chain_and_submit(prev, bio); } done += added; } while (done < count); diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 773d959965d..c26bd28edb4 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1357,8 +1357,7 @@ xfs_buf_submit_bio( split = bio_split(bio, bp->b_maps[map].bm_len, GFP_NOFS, &fs_bio_set); split->bi_iter.bi_sector = bp->b_maps[map].bm_bn; - bio_chain(split, bio); - submit_bio(split); + bio_chain_and_submit(split, bio); } bio->bi_iter.bi_sector = bp->b_maps[map].bm_bn; submit_bio(bio); diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 603e85c1ab4..f4c9ad1d148 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1687,8 +1687,7 @@ xlog_write_iclog( split = bio_split(&iclog->ic_bio, log->l_logBBsize - bno, GFP_NOIO, &fs_bio_set); - bio_chain(split, &iclog->ic_bio); - submit_bio(split); + bio_chain_and_submit(split, &iclog->ic_bio); /* restart at logical offset zero for the remainder */ iclog->ic_bio.bi_iter.bi_sector = log->l_logBBstart; -- 2.34.1 From: Shida Zhang Replace duplicate bio chaining logic with the common bio_chain_and_submit helper function. Signed-off-by: Shida Zhang --- fs/squashfs/block.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c index a05e3793f93..5818e473255 100644 --- a/fs/squashfs/block.c +++ b/fs/squashfs/block.c @@ -126,8 +126,7 @@ static int squashfs_bio_read_cached(struct bio *fullbio, if (bio) { bio_trim(bio, start_idx * PAGE_SECTORS, (end_idx - start_idx) * PAGE_SECTORS); - bio_chain(bio, new); - submit_bio(bio); + bio_chain_and_submit(bio, new); } bio = new; -- 2.34.1 From: Shida Zhang Replace duplicate bio chaining logic with the common bio_chain_and_submit helper function. Signed-off-by: Shida Zhang --- fs/ntfs3/fsntfs.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c index c7a2f191254..35685ee4ed2 100644 --- a/fs/ntfs3/fsntfs.c +++ b/fs/ntfs3/fsntfs.c @@ -1514,11 +1514,7 @@ int ntfs_bio_pages(struct ntfs_sb_info *sbi, const struct runs_tree *run, len = ((u64)clen << cluster_bits) - off; new_bio: new = bio_alloc(bdev, nr_pages - page_idx, op, GFP_NOFS); - if (bio) { - bio_chain(bio, new); - submit_bio(bio); - } - bio = new; + bio = bio_chain_and_submit(bio, new); bio->bi_iter.bi_sector = lbo >> 9; while (len) { @@ -1611,11 +1607,7 @@ int ntfs_bio_fill_1(struct ntfs_sb_info *sbi, const struct runs_tree *run) len = (u64)clen << cluster_bits; new_bio: new = bio_alloc(bdev, BIO_MAX_VECS, REQ_OP_WRITE, GFP_NOFS); - if (bio) { - bio_chain(bio, new); - submit_bio(bio); - } - bio = new; + bio = bio_chain_and_submit(bio, new); bio->bi_iter.bi_sector = lbo >> 9; for (;;) { -- 2.34.1 From: Shida Zhang Replace duplicate bio chaining logic with the common bio_chain_and_submit helper function. Signed-off-by: Shida Zhang --- drivers/block/zram/zram_drv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index a4307465753..084de60ebaf 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -730,8 +730,7 @@ static void read_from_bdev_async(struct zram *zram, struct page *page, bio = bio_alloc(zram->bdev, 1, parent->bi_opf, GFP_NOIO); bio->bi_iter.bi_sector = entry * (PAGE_SIZE >> 9); __bio_add_page(bio, page, PAGE_SIZE, 0); - bio_chain(bio, parent); - submit_bio(bio); + bio_chain_and_submit(bio, parent); } static int zram_writeback_slots(struct zram *zram, struct zram_pp_ctl *ctl) -- 2.34.1 From: Shida Zhang Replace duplicate bio chaining logic with the common bio_chain_and_submit helper function. Signed-off-by: Shida Zhang --- drivers/nvdimm/nd_virtio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c index c3f07be4aa2..e6ec7ceee9b 100644 --- a/drivers/nvdimm/nd_virtio.c +++ b/drivers/nvdimm/nd_virtio.c @@ -122,8 +122,7 @@ int async_pmem_flush(struct nd_region *nd_region, struct bio *bio) return -ENOMEM; bio_clone_blkg_association(child, bio); child->bi_iter.bi_sector = -1; - bio_chain(child, bio); - submit_bio(child); + bio_chain_and_submit(child, bio); return 0; } if (virtio_pmem_flush(nd_region)) -- 2.34.1 From: Shida Zhang Replace repetitive bio chaining patterns with bio_chain_and_submit. Note that while the parameter order (prev vs new) differs from the original code, the chaining order does not affect bio chain functionality. Signed-off-by: Shida Zhang --- drivers/nvme/target/io-cmd-bdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c index 8d246b8ca60..4af45659bd2 100644 --- a/drivers/nvme/target/io-cmd-bdev.c +++ b/drivers/nvme/target/io-cmd-bdev.c @@ -312,8 +312,7 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req) opf, GFP_KERNEL); bio->bi_iter.bi_sector = sector; - bio_chain(bio, prev); - submit_bio(prev); + bio_chain_and_submit(prev, bio); } sector += sg->length >> 9; -- 2.34.1