On 5/15/25 9:47 PM, Christoph Hellwig wrote:
On Wed, May 14, 2025 at 01:29:36PM -0700, Bart Van Assche wrote:
/*
* Now assemble so we handle the lowest level first.
*/
+ bio_list_on_stack[0] = bio_list_on_stack[1];
bio_list_merge(&bio_list_on_stack[0], &lower);
bio_list_merge(&bio_list_on_stack[0], &same);
- bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
If I read this code correctly, this means that we no keep processing bios
that already were on bio_list_on_stack[0] and the beginning of the loop
in the next iteration(s) instead of finishing off the ones created by
this iteration, which could lead to exhaustion of resources like mempool.
Note that this is a big if - the code is really hard to read, it should
really grow a data structure for the on-stack list that has named members
for both lists instead of the array magic.. :(
I'm still trying to understand your problem given that it wasn't
described much. What I could think it is that bio_split_to_limits through
bio_submit_split first re-submits the remainder bio using
submit_bio_noacct, which the above should place on the same list and then
later the stacking block drivers also submit the bio split off at the
beginning, unlike blk-mq drivers that process it directly. But given
that this resubmission should be on the lower list above I don't
see how it causes problems.
Agreed that this should be root-caused. To my own frustration I do not
yet have a full root-cause analysis. What I have done to obtain more
information is to make the kernel issue a warning the first time a bio
is added out-of-order at the end of the bio list. The following output
appeared (sde is the zoned block device at the bottom of the stack):
[ 71.312492][ T1] bio_list_insert_sorted: inserting in the middle
of a bio list
[ 71.313483][ T1] print_bio_list(sde) sector 0x1b7520 size 0x10
[ 71.313034][ T1] bio_list_insert_sorted(sde) sector 0x1b7120 size
0x400
[ ... ]
[ 71.368117][ T163] WARNING: CPU: 4 PID: 163 at block/blk-core.c:725
bio_list_insert_sorted+0x144/0x18c
[ 71.386664][ T163] Workqueue: writeback wb_workfn (flush-253:49)
[ 71.387110][ T163] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT
-SSBS BTYPE=--)
[ 71.393772][ T163] Call trace:
[ 71.393988][ T163] bio_list_insert_sorted+0x144/0x18c
[ 71.394338][ T163] submit_bio_noacct_nocheck+0xd8/0x4f4
[ 71.394696][ T163] submit_bio_noacct+0x32c/0x50c
[ 71.395017][ T163] bio_submit_split+0xf0/0x1f8
[ 71.395349][ T163] bio_split_rw+0xdc/0xf0
[ 71.395631][ T163] blk_mq_submit_bio+0x320/0x940
[ 71.395970][ T163] __submit_bio+0xa4/0x1c4
[ 71.396260][ T163] submit_bio_noacct_nocheck+0x1c0/0x4f4
[ 71.396623][ T163] submit_bio_noacct+0x32c/0x50c
[ 71.396942][ T163] submit_bio+0x17c/0x198
[ 71.397222][ T163] f2fs_submit_write_bio+0x94/0x154
[ 71.397604][ T163] __submit_merged_bio+0x80/0x204
[ 71.397933][ T163] __submit_merged_write_cond+0xd0/0x1fc
[ 71.398297][ T163] f2fs_submit_merged_write+0x24/0x30
[ 71.398646][ T163] f2fs_sync_node_pages+0x5ec/0x64c
[ 71.398999][ T163] f2fs_write_node_pages+0xe8/0x1dc
[ 71.399338][ T163] do_writepages+0xe4/0x2f8
[ 71.399673][ T163] __writeback_single_inode+0x84/0x6e4
[ 71.400036][ T163] writeback_sb_inodes+0x2cc/0x5c0
[ 71.400369][ T163] wb_writeback+0x134/0x550
[ 71.400662][ T163] wb_workfn+0x154/0x588
[ 71.400937][ T163] process_one_work+0x26c/0x65c
[ 71.401271][ T163] worker_thread+0x33c/0x498
[ 71.401575][ T163] kthread+0x110/0x134
[ 71.401844][ T163] ret_from_fork+0x10/0x20
I think that the above call stack indicates the following:
f2fs_submit_write_bio() submits a bio to a dm driver, that the dm driver
submitted a bio for the lower driver (SCSI core), that the bio for the
lower driver is split by bio_split_rw(), and that the second half of the
split bio triggers the above out-of-order warning.
This new patch should address the concerns brought up in your latest
email:
diff --git a/block/blk-core.c b/block/blk-core.c
index 411f005e6b1f..aa270588272a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -649,6 +649,26 @@ static void __submit_bio(struct bio *bio)
blk_finish_plug(&plug);
}
+/*
+ * Insert a bio in LBA order. If no bio for the same bdev with a higher
LBA is
+ * found, append at the end.
+ */
+static void bio_list_insert_sorted(struct bio_list *bl, struct bio *bio)
+{
+ struct block_device *bdev = bio->bi_bdev;
+ struct bio **pprev = &bl->head, *next;
+ sector_t sector = bio->bi_iter.bi_sector;
+
+ for (next = *pprev; next; pprev = &next->bi_next, next = next->bi_next)
+ if (next->bi_bdev == bdev && sector < next->bi_iter.bi_sector)
+ break;
+
+ bio->bi_next = next;
+ *pprev = bio;
+ if (!next)
+ bl->tail = bio;
+}
+
/*
* The loop in this function may be a bit non-obvious, and so deserves
some
* explanation:
@@ -706,7 +726,8 @@ static void __submit_bio_noacct(struct bio *bio)
*/
bio_list_merge(&bio_list_on_stack[0], &lower);
bio_list_merge(&bio_list_on_stack[0], &same);
- bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
+ while ((bio = bio_list_pop(&bio_list_on_stack[1])))
+ bio_list_insert_sorted(&bio_list_on_stack[0], bio);
} while ((bio = bio_list_pop(&bio_list_on_stack[0])));
current->bio_list = NULL;
@@ -746,7 +767,7 @@ void submit_bio_noacct_nocheck(struct bio *bio)
* it is active, and then process them after it returned.
*/
if (current->bio_list)
- bio_list_add(¤t->bio_list[0], bio);
+ bio_list_insert_sorted(¤t->bio_list[0], bio);
else if (!bdev_test_flag(bio->bi_bdev, BD_HAS_SUBMIT_BIO))
__submit_bio_noacct_mq(bio);
else
