On Thu, Feb 02 2023 at  9:00P -0500,
Mike Snitzer [off-list ref] wrote:

On Thu, Feb 02 2023 at  1:14P -0500,
Christoph Hellwig [off-list ref] wrote:

quoted

The current usage of submit_bio vs submit_bio_noacct which skips the
VM events and task account is a bit unclear.  It seems to be mostly
intended for sending on bios received by stacking drivers, but also
seems to be used for stacking drivers newly generated metadata
sometimes.

Your lack of confidence conveyed in the above shook me a little bit
considering so much of this code is attributed to you -- I mostly got
past that, but I am a bit concerned about one aspect of the
submit_bio() change (2nd to last comment inlined below).

quoted

Remove the separate API and just skip the accounting if submit_bio
is called recursively.  This gets us an accounting behavior that
is very similar (but not quite identical) to the current one, while
simplifying the API and code base.

Can you elaborate on the "but not quite identical"? This patch is
pretty mechanical, just folding code and renaming.. but you obviously
saw subtle differences.  Likely worth callign those out precisely.

How have you tested this patch?  Seems like I should throw all the lvm
and DM tests at it.

...

quoted

@@ -716,6 +712,27 @@ void submit_bio_noacct(struct bio *bio)
 
 	might_sleep();
 
+	/*
+	 * We only want one ->submit_bio to be active at a time, else stack
+	 * usage with stacked devices could be a problem.  Use current->bio_list
+	 * to collect a list of requests submited by a ->submit_bio method while
+	 * it is active, and then process them after it returned.
+	 */
+	if (current->bio_list) {
+		bio_list_add(&current->bio_list[0], bio);
+		return;
+	}

It seems pretty aggressive to queue the bio to current->bio_list so
early. Before this patch, that didn't happen until the very end
(meaning all the negative checks of submit_bio_noacct were done before
doing the bio_list_add() due to recursion). This is my primary concern
with this patch. Is that the biggest aspect of your "not quite
identical" comment in the patch header?

In practice this will manifest as delaying the negative checks, until
returning from active submit_bio, but they will still happen.

Actually, I don't think those checks are done at all now.

Unless I'm missing something, this seems like it needs proper
justification and a lot of review and testing.

So why do this change?