| Message ID | YlRmhlL8TtQow0W0@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: remove redundant blk-cgroup init from __bio_clone | expand |
On Mon, Apr 11, 2022 at 01:33:58PM -0400, Mike Snitzer wrote: > When bio_{alloc,init}_clone are passed a bdev, bio_init() will call > bio_associate_blkg() so the __bio_clone() work to initialize blkcg > isn't needed. No, unfortunately it isn't as simple as that. There are bios that do not use the default cgroup and thus blkg, e.g. those that come from cgroup writeback.
On Mon, Apr 11, 2022 at 10:27:54PM -0700, Christoph Hellwig wrote: > On Mon, Apr 11, 2022 at 01:33:58PM -0400, Mike Snitzer wrote: > > When bio_{alloc,init}_clone are passed a bdev, bio_init() will call > > bio_associate_blkg() so the __bio_clone() work to initialize blkcg > > isn't needed. > > No, unfortunately it isn't as simple as that. There are bios that do > not use the default cgroup and thus blkg, e.g. those that come from > cgroup writeback. Yeah I wasn't quite right earlier. But, the new api isn't in line with the original semantics. Cloning the blkg preserves the original bios request_queue which likely differs from the bdev passed into clone. This means an IO might be charged to the wrong device. So, the blkg combines the who, blkcg, and the where, the corresponding request_queue. Before bios were inited in 2 phases: bio_alloc(); bio_set_dev(); This meant at clone time, we didn't have the where, but the who was encased in the blkg. So, after bio_clone_blkg_association() expected a bio_set_dev() call which called bio_associate_blkg(). When the bio already has a blkg, it attempts to reuse the blkcg while using the new bdev to find the correct blkg. The tricky part seems to be how to seamlessly expose the appropriate blkcg without being intrusive to bio_alloc*() apis. Regarding the NULL bdev, I think that works as long as we keep the bio_clone_blkg_association() call to carry the correct blkcg to the bio_set_dev() call. Thanks, Dennis
So while I'm still diving into the somewhat invasive changes to optimize some of the cloning all we might relaly need for your use case should be this: http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/blk-clone-no-bdev Can you check if this helps you?
On Sat, Apr 23 2022 at 12:55P -0400, Christoph Hellwig <hch@infradead.org> wrote: > So while I'm still diving into the somewhat invasive changes to > optimize some of the cloning all we might relaly need for your > use case should be this: > > http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/blk-clone-no-bdev > > Can you check if this helps you? Passing NULL will be useful for targets that do call bio_set_dev() but there are some that inherit the original bdev. I'll audit all targets and sort out how best to handle the conditional initialization to avoid needless work. This change is useful and a requirement for relevant follow-on DM changes: Reviewed-by: Mike Snitzer <snitzer@kernel.org> Thanks.
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) &&
When bio_{alloc,init}_clone are passed a bdev, bio_init() will call bio_associate_blkg() so the __bio_clone() work to initialize blkcg isn't needed. Signed-off-by: Mike Snitzer <snitzer@kernel.org> --- block/bio.c | 3 --- 1 file changed, 3 deletions(-)