On Tue, Dec 29 2020 at  4:18am -0500,
dannyshih [off-list ref] wrote:

From: Danny Shih <redacted>

We found out that split bios might handle not in order when a big bio
had split by blk_queue_split() and also split in stacking block device,
such as md device because chunk size boundary limit.

Stacking block device normally use submit_bio_noacct() add the remaining
bio to current->bio_list's tail after they split original bio. Therefore,
when bio split first time, the last part of bio was add to bio_list.
After then, when bio split second time, the middle part of bio was add to
bio_list. Results that the middle part is now behind the last part of bio.

For example:
	There is a RAID0 md device, with max_sectors_kb = 2 KB,
	and chunk_size = 1 KB

	1. a read bio come to md device wants to read 0-7 KB
	2. In blk_queue_split(), bio split into (0-1), (2-7),
	   and send (2-7) back to md device

	   current->bio_list = bio_list_on_stack[0]: (md 2-7)
	3. RAID0 split bio (0-1) into (0) and (1), since chunk size is 1 KB
	   and send (1) back to md device

	   bio_list_on_stack[0]: (md 2-7) -> (md 1)
	4. remap and send (0) to lower layer device

	   bio_list_on_stack[0]: (md 2-7) -> (md 1) -> (lower 0)
	5. __submit_bio_noacct() sorting bio let lower bio handle firstly
	   bio_list_on_stack[0]: (lower 0) -> (md 2-7) -> (md 1)
	   pop (lower 0)
	   move bio_list_on_stack[0] to bio_list_on_stack[1]

	   bio_list_on_stack[1]: (md 2-7) -> (md 1)
	6. after handle lower bio, it handle (md 2-7) firstly, and split
	   in blk_queue_split() into (2-3), (4-7), send (4-7) back

	   bio_list_on_stack[0]: (md 4-7)
	   bio_list_on_stack[1]: (md 1)
	7. RAID0 split bio (2-3) into (2) and (3) and send (3) back

	   bio_list_on_stack[0]: (md 4-7) -> (md 3)
	   bio_list_on_stack[1]: (md 1)
	...
	In the end, the split bio handle's order will become
	0 -> 2 -> 4 -> 6 -> 7 -> 5 -> 3 -> 1

Reverse the order of same queue bio when sorting bio in
__submit_bio_noacct() can solve this issue, but it might influence
too much. So we provide alternative version of submit_bio_noacct(),
named submit_bio_noacct_add_head(), for the case which need to add bio
to the head of current->bio_list. And replace submit_bio_noacct() with
submit_bio_noacct_add_head() in block device layer when we want to
split bio and send remaining back to itself.

Ordering aside, you cannot split more than once.  So your proposed fix
to insert at head isn't valid because you're still implicitly allocating
more than one bio from the bioset which could cause deadlock in a low
memory situation.

I had to deal with a comparable issue with DM core not too long ago, see
this commit:

commit ee1dfad5325ff1cfb2239e564cd411b3bfe8667a
Author: Mike Snitzer [off-list ref]
Date:   Mon Sep 14 13:04:19 2020 -0400

    dm: fix bio splitting and its bio completion order for regular IO

    dm_queue_split() is removed because __split_and_process_bio() _must_
    handle splitting bios to ensure proper bio submission and completion
    ordering as a bio is split.

    Otherwise, multiple recursive calls to ->submit_bio will cause multiple
    split bios to be allocated from the same ->bio_split mempool at the same
    time. This would result in deadlock in low memory conditions because no
    progress could be made (only one bio is available in ->bio_split
    mempool).

    This fix has been verified to still fix the loss of performance, due
    to excess splitting, that commit 120c9257f5f1 provided.

    Fixes: 120c9257f5f1 ("Revert "dm: always call blk_queue_split() in dm_process_bio()"")
    Cc: stable@vger.kernel.org # 5.0+, requires custom backport due to 5.9 changes
    Reported-by: Ming Lei [off-list ref]
    Signed-off-by: Mike Snitzer [off-list ref]

Basically you cannot split the same bio more than once without
recursing.  Your elaborate documentation shows things going wrong quite
early in step 3.  That additional split and recursing back to MD
shouldn't happen before the first bio split completes.

Seems the proper fix is to disallow max_sectors_kb to be imposed, via
blk_queue_split(), if MD has further splitting constraints, via
chunk_sectors, that negate max_sectors_kb anyway.

Mike