Hello,
On Sun, Jan 29, 2023 at 09:26:57PM +0000, Matthew Wilcox wrote:
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c index e78be66bbf01..a4e76f96f291 100644 --- a/fs/crypto/crypto.c +++ b/fs/crypto/crypto.c @@ -205,6 +205,9 @@ struct page *fscrypt_encrypt_pagecache_blocks(struct page *page, } SetPagePrivate(ciphertext_page); set_page_private(ciphertext_page, (unsigned long)page); +#ifdef CONFIG_MEMCG
- ciphertext_page->memcg_data = page->memcg_data;
+#endif return ciphertext_page; }
Nothing outside mm/ and include/linux/memcontrol.h does anything with memcg_data directly. Are you sure this is the right thing to do here?
Nope ;-) Happy to hear from people who know more about cgroups than I do. Adding some more ccs.
Also, this patch causes the following:
Oops. Clearly memcg_data needs to be set to NULL before we free it.
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(). 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.
Thanks.