Hello,
On Tue, Jan 31, 2023 at 10:31:31PM -0800, Eric Biggers wrote:
These can usually be handled by explicitly associating the bio's to the desired cgroups using one of bio_associate_blkg*() or bio_clone_blkg_association().
Here that already happens in wbc_init_bio(), called from io_submit_init_bio() in fs/ext4/page-io.c.
Yeah, without bouncing, that's usually how writeback IOs are associated with their cgroups.
It is possible to go through memcg ownership too using set_active_memcg() so that the page is owned by the target cgroup; however, the page ownership doesn't directly map to IO ownership as the relationship depends on the type of the page (e.g. IO ownership for pagecache writeback is determined per-inode, not per-page). If the in-flight pages are limited, it probably is better to set bio association directly.
ext4 also calls wbc_account_cgroup_owner() for each pagecache page that's written out. It seems this is for a different purpose -- it looks like the fs-writeback code is trying to figure out which cgroup "owns" the inode based on which cgroup "owns" most of the pagecache pages?
Yeah, there's a difference between how memory and IO track cgroup ownership. Memory ownership is per-page but IO ownership is per-inode. This is because splitting writeback IOs of the same inode can perform really badly, so we try to find the majority dirty page owner cgroup of a given inode and associate the whole inode to that cgroup.
So, something like md / dm, which gets a bio from filesystem and then bounces it to another bio, would use either bio_clone_blkg_association() to copy the association of the original bio (which probably is set through wbc_init_bio()) or determine the cgroup the bio should belong to somehow and set it explicitly with bio_associate_blkg(). However, here, as the filesystem is the one bouncing I guess it can be simpler.
The bug we're discussing here is that when ext4 writes out a pagecache page in an encrypted file, it first encrypts the data into a bounce page, then passes the bounce page (which don't have a memcg) to wbc_account_cgroup_owner(). Maybe the proper fix is to just pass the pagecache page to wbc_account_cgroup_owner() instead? See below for ext4 (a separate patch would be needed for f2fs):
Yeah, this makes sense to me and is the right thing to do no matter what. wbc_account_cgroup_owner() should be fed the origin page so that the IO can be blamed on the owner of that page.
Thanks.