On Fri, Feb 03 2023 at 6:50P -0500, Benjamin Marzinski <bmarzins@xxxxxxxxxx> 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@xxxxxx> 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@xxxxxxxxxx> (Could be that many submit_bio_noacct callers can be converted but we really do need to keep the submit_bio_noacct interface)
