Re: can we reduce bio_set_dev overhead due to bio_associate_blkg?

From: Mike Snitzer <snitzer@redhat.com>
To: Mike Snitzer <snitzer@kernel.org>
Cc: Christoph Hellwig <hch@infradead.org>,
	Dennis Zhou <dennis@kernel.org>,
	tj@kernel.org, axboe@kernel.dk, linux-block@vger.kernel.org,
	dm-devel@redhat.com
Subject: Re: can we reduce bio_set_dev overhead due to bio_associate_blkg?
Date: Mon, 11 Apr 2022 13:16:01 -0400	[thread overview]
Message-ID: <YlRiUVFK+a0DwQhu@redhat.com> (raw)
In-Reply-To: <YlReKjjWhvTZjfg/@redhat.com>

On Mon, Apr 11 2022 at 12:58P -0400,
Mike Snitzer <snitzer@kernel.org> wrote:

> On Sat, Apr 09 2022 at  1:15P -0400,
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Fri, Apr 08, 2022 at 11:42:51AM -0400, Mike Snitzer wrote:
> > > I think we can achieve the goal of efficient cloning/remapping for
> > > both usecases simply by splitting out the bio_set_dev() and leaving it
> > > to the caller to pick which interface to use (e.g. clone vs
> > > clone_and_remap).
> > 
> > You can just pass a NULL bdev to bio_alloc_clone/bio_init_clone.
> > I've been hoping to get rid of that, but if we have a clear use case
> > it will have to stay.
> 
> DM core is just using bio_alloc_clone. And bio_alloc_bioset() allows
> bdev to be NULL -- so you're likely referring to that (which will skip
> bio_init's bio_associate_blkg).
...

> diff --git a/block/bio.c b/block/bio.c
> index 7892f1108ca6..0340acc283a0 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -772,14 +772,16 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
> 	bio_set_flag(bio, BIO_CLONED);
> 	if (bio_flagged(bio_src, BIO_THROTTLED))
> 		bio_set_flag(bio, BIO_THROTTLED);
> -	if (bio->bi_bdev == bio_src->bi_bdev &&
> -	    bio_flagged(bio_src, BIO_REMAPPED))
> -		bio_set_flag(bio, BIO_REMAPPED);
> 	bio->bi_ioprio = bio_src->bi_ioprio;
> 	bio->bi_iter = bio_src->bi_iter;
> 
> -	bio_clone_blkg_association(bio, bio_src);
> -	blkcg_bio_issue_init(bio);
> +	if (bio->bi_bdev == bio_src->bi_bdev) {
> +		if (bio_flagged(bio_src, BIO_REMAPPED))
> +			bio_set_flag(bio, BIO_REMAPPED);
> +
> +		bio_clone_blkg_association(bio, bio_src);
> +		blkcg_bio_issue_init(bio);
> +	}
> 
> 	if (bio_crypt_clone(bio, bio_src, gfp) < 0)
> 		return -ENOMEM;
> 
> Think this will fix some of the performance penalty of redundant blkcg
> initialization that I reported (though like was also discussed: more
> work likely needed to further optimize bio_associate_blkg).

Looking closer at the case where bio_{alloc,init}_clone are passed a
bdev, bio_init() will call bio_associate_blkg() so the __bio_clone()
work to do anything with blkbg isn't needed at all. So this patch is
best:

diff --git a/block/bio.c b/block/bio.c
index 7892f1108ca6..6980f1b4b0f4 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -778,9 +778,6 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
	bio->bi_ioprio = bio_src->bi_ioprio;
	bio->bi_iter = bio_src->bi_iter;

-	bio_clone_blkg_association(bio, bio_src);
-	blkcg_bio_issue_init(bio);
-
	if (bio_crypt_clone(bio, bio_src, gfp) < 0)
		return -ENOMEM;
	if (bio_integrity(bio_src) &&

