Re: block: remove submit_bio_noacct

[Mike Snitzer <snitzer@kernel.org>]

On Fri, Feb 03 2023 at  6:50P -0500,
Benjamin Marzinski <bmarzins@redhat.com> wrote:

> On Fri, Feb 03, 2023 at 11:39:36AM -0500, Mike Snitzer wrote:
> > On Fri, Feb 03 2023 at 10:00P -0500,
> > Christoph Hellwig <hch@lst.de> wrote:
> > 
> > > On Thu, Feb 02, 2023 at 10:16:53PM -0500, Mike Snitzer wrote:
> > > > > > 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.
> > > 
> > > > > > 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 <snitzer@kernel.org>

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