On Thu, Feb 02 2023 at 9:00P -0500,
Mike Snitzer <snitzer@xxxxxxxxxx> wrote:
> On Thu, Feb 02 2023 at 1:14P -0500,
> Christoph Hellwig <hch@xxxxxx> 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(¤t->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?
