| Message ID | 20191212181934.GA33645@dennisz-mbp.dhcp.thefacebook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Thu, Dec 12, 2019 at 10:19:34AM -0800, Dennis Zhou wrote: > From a0569aebde08e31e994c92d0b70befb84f7f5563 Mon Sep 17 00:00:00 2001 > From: Dennis Zhou <dennis@kernel.org> > Date: Wed, 11 Dec 2019 15:20:15 -0800 > > Bio attribution is handled at bio_set_dev() as once we have a device, we > have a corresponding request_queue and then can derive the current css. > In special cases, we want to attribute to bio to someone else. This can > be done by calling bio_associate_blkg_from_css() or > kthread_associate_blkcg() depending on the scenario. Btrfs does this for > compressed writeback as they are handled by kworkers, so the latter can > be done here. > > Commit 1a41802701ec ("btrfs: drop bio_set_dev where not needed") removes > early bio_set_dev() calls prior to submit_stripe_bio(). This breaks the > above assumption that we'll have a request_queue when we are doing > association. To fix this, switch to using kthread_associate_blkcg(). Can be kthread_associate_blkcg used also for submit_extent_page that calls bio_associate_blkg_from_css indirectly when initializing wbc? 2996 bio_set_dev(bio, bdev); 2997 wbc_init_bio(wbc, bio); 2998 wbc_account_cgroup_owner(wbc, page, page_size); wbc_init_bio: if (wbc) bio_associate_blkg_from_css();
On Fri, Dec 13, 2019 at 01:24:01PM +0100, David Sterba wrote: > On Thu, Dec 12, 2019 at 10:19:34AM -0800, Dennis Zhou wrote: > > From a0569aebde08e31e994c92d0b70befb84f7f5563 Mon Sep 17 00:00:00 2001 > > From: Dennis Zhou <dennis@kernel.org> > > Date: Wed, 11 Dec 2019 15:20:15 -0800 > > > > Bio attribution is handled at bio_set_dev() as once we have a device, we > > have a corresponding request_queue and then can derive the current css. > > In special cases, we want to attribute to bio to someone else. This can > > be done by calling bio_associate_blkg_from_css() or > > kthread_associate_blkcg() depending on the scenario. Btrfs does this for > > compressed writeback as they are handled by kworkers, so the latter can > > be done here. > > > > Commit 1a41802701ec ("btrfs: drop bio_set_dev where not needed") removes > > early bio_set_dev() calls prior to submit_stripe_bio(). This breaks the > > above assumption that we'll have a request_queue when we are doing > > association. To fix this, switch to using kthread_associate_blkcg(). > > Can be kthread_associate_blkcg used also for submit_extent_page that > calls bio_associate_blkg_from_css indirectly when initializing wbc? > > 2996 bio_set_dev(bio, bdev); > 2997 wbc_init_bio(wbc, bio); > 2998 wbc_account_cgroup_owner(wbc, page, page_size); > > wbc_init_bio: > > if (wbc) > bio_associate_blkg_from_css(); Correct me if I'm wrong, but I don't think submit_extent_page() is only called from kthread contexts. So, we wouldn't be able to rely on kthread_associate_blkcg(). I can think about how to make wbc better for association in general, but it's a percpu decrement and increment so it shouldn't really be much in overhead. Thanks, Dennis
On Fri, Dec 13, 2019 at 02:21:49PM -0800, Dennis Zhou wrote: > On Fri, Dec 13, 2019 at 01:24:01PM +0100, David Sterba wrote: > > On Thu, Dec 12, 2019 at 10:19:34AM -0800, Dennis Zhou wrote: > > > From a0569aebde08e31e994c92d0b70befb84f7f5563 Mon Sep 17 00:00:00 2001 > > > From: Dennis Zhou <dennis@kernel.org> > > > Date: Wed, 11 Dec 2019 15:20:15 -0800 > > > > > > Bio attribution is handled at bio_set_dev() as once we have a device, we > > > have a corresponding request_queue and then can derive the current css. > > > In special cases, we want to attribute to bio to someone else. This can > > > be done by calling bio_associate_blkg_from_css() or > > > kthread_associate_blkcg() depending on the scenario. Btrfs does this for > > > compressed writeback as they are handled by kworkers, so the latter can > > > be done here. > > > > > > Commit 1a41802701ec ("btrfs: drop bio_set_dev where not needed") removes > > > early bio_set_dev() calls prior to submit_stripe_bio(). This breaks the > > > above assumption that we'll have a request_queue when we are doing > > > association. To fix this, switch to using kthread_associate_blkcg(). > > > > Can be kthread_associate_blkcg used also for submit_extent_page that > > calls bio_associate_blkg_from_css indirectly when initializing wbc? > > > > 2996 bio_set_dev(bio, bdev); > > 2997 wbc_init_bio(wbc, bio); > > 2998 wbc_account_cgroup_owner(wbc, page, page_size); > > > > wbc_init_bio: > > > > if (wbc) > > bio_associate_blkg_from_css(); > > Correct me if I'm wrong, but I don't think submit_extent_page() is only > called from kthread contexts. So, we wouldn't be able to rely on > kthread_associate_blkcg(). Yeah, the kthread is not guaranteed here. > I can think about how to make wbc better for association in general, but > it's a percpu decrement and increment so it shouldn't really be much in > overhead. Performance is not my concern here, the addition of bios and blkcg association is new and there were some integration bugs where I independently removed early bdev association while the blkg relied on that. I'm looking for ways to make it less error prone and the kthread association looks exactly like that so I was curious if it's possible to use it everywhere. If not, the bdev needs to be found from other available data.
On Tue, Dec 17, 2019 at 04:05:48PM +0100, David Sterba wrote: > On Fri, Dec 13, 2019 at 02:21:49PM -0800, Dennis Zhou wrote: > > On Fri, Dec 13, 2019 at 01:24:01PM +0100, David Sterba wrote: > > > On Thu, Dec 12, 2019 at 10:19:34AM -0800, Dennis Zhou wrote: > > > > From a0569aebde08e31e994c92d0b70befb84f7f5563 Mon Sep 17 00:00:00 2001 > > > > From: Dennis Zhou <dennis@kernel.org> > > > > Date: Wed, 11 Dec 2019 15:20:15 -0800 > > > > > > > > Bio attribution is handled at bio_set_dev() as once we have a device, we > > > > have a corresponding request_queue and then can derive the current css. > > > > In special cases, we want to attribute to bio to someone else. This can > > > > be done by calling bio_associate_blkg_from_css() or > > > > kthread_associate_blkcg() depending on the scenario. Btrfs does this for > > > > compressed writeback as they are handled by kworkers, so the latter can > > > > be done here. > > > > > > > > Commit 1a41802701ec ("btrfs: drop bio_set_dev where not needed") removes > > > > early bio_set_dev() calls prior to submit_stripe_bio(). This breaks the > > > > above assumption that we'll have a request_queue when we are doing > > > > association. To fix this, switch to using kthread_associate_blkcg(). > > > > > > Can be kthread_associate_blkcg used also for submit_extent_page that > > > calls bio_associate_blkg_from_css indirectly when initializing wbc? > > > > > > 2996 bio_set_dev(bio, bdev); > > > 2997 wbc_init_bio(wbc, bio); > > > 2998 wbc_account_cgroup_owner(wbc, page, page_size); > > > > > > wbc_init_bio: > > > > > > if (wbc) > > > bio_associate_blkg_from_css(); > > > > Correct me if I'm wrong, but I don't think submit_extent_page() is only > > called from kthread contexts. So, we wouldn't be able to rely on > > kthread_associate_blkcg(). > > Yeah, the kthread is not guaranteed here. > > > I can think about how to make wbc better for association in general, but > > it's a percpu decrement and increment so it shouldn't really be much in > > overhead. > > Performance is not my concern here, the addition of bios and blkcg > association is new and there were some integration bugs where I > independently removed early bdev association while the blkg relied on > that. I'm looking for ways to make it less error prone and the kthread > association looks exactly like that so I was curious if it's possible to > use it everywhere. If not, the bdev needs to be found from other > available data. Yeah. At the time, going through bio_set_dev() was a way to guarantee we weren't missing an association with a blk-cgroup. This simplified auditing and prevented newer use cases from missing it. However, I do agree it's quite error prone.. I'll put it on my list and see if I can come up with something better. Thanks, Dennis
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 4ce81571f0cd..de95ad27722f 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -447,7 +447,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start, if (blkcg_css) { bio->bi_opf |= REQ_CGROUP_PUNT; - bio_associate_blkg_from_css(bio, blkcg_css); + kthread_associate_blkcg(blkcg_css); } refcount_set(&cb->pending_bios, 1); @@ -491,10 +491,8 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start, bio->bi_opf = REQ_OP_WRITE | write_flags; bio->bi_private = cb; bio->bi_end_io = end_compressed_bio_write; - if (blkcg_css) { + if (blkcg_css) bio->bi_opf |= REQ_CGROUP_PUNT; - bio_associate_blkg_from_css(bio, blkcg_css); - } bio_add_page(bio, page, PAGE_SIZE, 0); } if (bytes_left < PAGE_SIZE) { @@ -521,6 +519,9 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start, bio_endio(bio); } + if (blkcg_css) + kthread_associate_blkcg(NULL); + return 0; }