Re: [dm-devel] block: remove submit_bio_noacct

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Feb 02 2023 at  9:00P -0500,
Mike Snitzer <snitzer@kernel.org> wrote:

> On Thu, Feb 02 2023 at  1:14P -0500,
> Christoph Hellwig <hch@lst.de> 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).
> 
> > 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.
> 

...

> > @@ -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?

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel