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.
> > >
> > > How have you tested this patch? Seems like I should throw all the lvm
> > > and DM tests at it.
>
> blktests and the mdadm tests (at least as far as they got upstream, md
> or it's tests always seem somewhat broken). dmtests is something
> I've never managed to get to actually run due it's insistence on
> using not packaged ruby stuff.
Yeah, device-mapper-test-suite (dmts) is a PITA due to ruby dep. And
the tests have gotten a bit stale relative to recent kernels. I'm
aware of this and also about how others would like to see more DM
coverage in blktests. We'll be looking to improve DM testing but it
does always tend to get put on back-burner (but I'll be getting some
help from Ben Marzinski to assess what tests we do have, see where we
have gaps and also put effort to making DM testing part of blktests).
I'm actually now pretty interested to see which (if any) DM tests
would have caught the missing bio checks issue in your initial patch.
> > > 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.
>
> Yes, the branch needs to be later as in this version below.
Thanks.
> > Unless I'm missing something, this seems like it needs proper
> > justification and a lot of review and testing.
> >
> > So why do this change?
>
> Because I once again got a question from an auther of a driver that
> is planned to be upstreamed on which one to use. And the answer
> was it's complicated, and you really should not have to think about
> it, let me dig out my old patch so that driver authors don't have
> to care.
That's fair, and as I tried to say in my first reply: I agree it does
clear up that confusion nicely.
Please fold this incremental patch in, with that you can add:
Reviewed-by: Mike Snitzer <snitzer@xxxxxxxxxx>
Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
(not sure if my Signed-off-by needed but there you have it if so).
diff --git a/block/bio.c b/block/bio.c
index ea143fd825d7..aa0586012b0d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -475,7 +475,7 @@ static struct bio *bio_alloc_percpu_cache(struct block_device *bdev,
*
* Note that when running under submit_bio() (i.e. any block driver),
* bios are not submitted until after you return - see the code in
- * submit_bio() that converts recursion into iteration, to prevent
+ * submit_bio_nocheck() that converts recursion into iteration, to prevent
* stack overflows.
*
* This would normally mean allocating multiple bios under submit_bio()
diff --git a/block/blk-core.c b/block/blk-core.c
index f755ac1a2931..c41f086cb183 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -687,7 +687,7 @@ void submit_bio_nocheck(struct bio *bio)
/*
* 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
+ * to collect a list of requests submitted by a ->submit_bio method while
* it is active, and then process them after it returned.
*/
if (current->bio_list)
@@ -720,13 +720,13 @@ void submit_bio(struct bio *bio)
might_sleep();
- if (blkcg_punt_bio_submit(bio))
- return;
-
/*
* Do not double account bios that are remapped and resubmitted.
*/
if (!current->bio_list) {
+ if (blkcg_punt_bio_submit(bio))
+ return;
+
if (bio_op(bio) == REQ_OP_READ) {
task_io_account_read(bio->bi_iter.bi_size);
count_vm_events(PGPGIN, bio_sectors(bio));
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
