On Fri, Feb 03 2023 at  6:50P -0500,
Benjamin Marzinski [off-list ref] wrote:

On Fri, Feb 03, 2023 at 11:39:36AM -0500, Mike Snitzer wrote:

quoted

On Fri, Feb 03 2023 at 10:00P -0500,
Christoph Hellwig [off-list ref] wrote:

quoted

On Thu, Feb 02, 2023 at 10:16:53PM -0500, Mike Snitzer wrote:

quoted

quoted

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).

The confidence is about how it is used.  And that's up to the driver
authors, not helped by them not having any guidelines.  And while
I've touched this code a lot, the split between the two levels of API
long predates me.

quoted

quoted

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.

The explanation was supposed to be in the Lines above.  Now accounting
is skipped if in a ->submit_bio recursion.  Before that it dependent
on drivers calling either submit_bio or submit_bio_noacct, for which
there was no clear guideline and drivers have been a bit sloppy about.

OK, but afaict the code is identical after your refactoring.
Side-effect is drivers that were double accounting will now be fixed.

Doesn't this open dm up to double accounting? Take dm-delay for
instance. If the delay is 0, then submit_bio() for the underlying device
will be called recursively and will skip the accounting.  If the delay
isn't 0, then submit_bio() for the underlying device will be called
later from a workqueue and the accounting will happen again, even though
the same amount of IO happens in either case. Or am I missing something
here?

-Ben 

Nope, you aren't missing anything, good catch!

Callers of dm_submit_bio_remap() are the poster-child for submitting
bios from a different task.

So yeah...

Nacked-by: Mike Snitzer [off-list ref]

(Could be that many submit_bio_noacct callers can be converted but we
really do need to keep the submit_bio_noacct interface)