On Tue, Nov 9, 2021 at 3:56 PM Mina Almasry [off-list ref] wrote:

On Mon, Nov 8, 2021 at 5:18 PM Dave Chinner [off-list ref] wrote:

quoted

On Mon, Nov 08, 2021 at 11:41:51PM +0000, Matthew Wilcox wrote:

quoted

On Tue, Nov 09, 2021 at 09:10:47AM +1100, Dave Chinner wrote:

quoted

quoted

+ rcu_read_lock();
+ memcg = rcu_dereference(mapping->host->i_sb->s_memcg_to_charge);

Anything doing pointer chasing to obtain static, unchanging
superblock state is poorly implemented. The s_memcg_to_charge value never
changes, so this code should associate the memcg to charge directly
on the mapping when the mapping is first initialised by the
filesystem. We already do this with things like attaching address
space ops and mapping specific gfp masks (i.e
mapping_set_gfp_mask()), so this association should be set up that
way, too (e.g. mapping_set_memcg_to_charge()).

I'm not a fan of enlarging struct address_space with another pointer
unless it's going to be used by all/most filesystems.  If this is
destined to be a shmem-only feature, then it should be in the
shmem_inode instead of the mapping.

Neither am I, but I'm also not a fan of the filemap code still
having to drill through the mapping to the host inode just to check
if it needs to do special stuff for shmem inodes on every call that
adds a page to the page cache. This is just as messy and intrusive
and the memcg code really has no business digging about in the
filesystem specific details of the inode behind the mapping.

Hmmm. The mem_cgroup_charge() call in filemap_add_folio() passes a
null mm context, so deep in the guts it ends getting the memcg from
active_memcg() in get_mem_cgroup_from_mm(). That ends up using
current->active_memcg, so maybe a better approach here is to have
shmem override current->active_memcg via set_active_memcg() before
it enters the generic fs paths and restore it on return...

current_fsmemcg()?

Thank you for providing a detailed alternative. To be honest it seems
a bit brittle to me, as in folks can easily add calls to generic fs
paths forgetting to override the active_memcg and having memory
charged incorrectly, but if there is no other option and we want to
make this a shmem-only feature, I can do this anyway.

quoted

quoted

If we are to have this for all filesystems, then let's do that properly
and make it generic functionality from its introduction.

Fully agree.

So the tmpfs feature addresses the first 2 usecases I mention in the
cover letter. For the 3rd usecase I will likely need to extend this
support to 1 disk-based filesystem, and I'm not sure which at this
point. It also looks like Roman has in mind 1 or more use cases and
may extend it to other filesystems as a result. I'm hoping that I can
provide the generic implementation and the tmpfs support and in follow
up patches folks can extend this to other file systems by providing
the fs-specific changes needed for that filesystem.

AFAICT with this patch the work to extend to another file system is to
parse the memcg= option in that filesystem, set the s_memcg_to_charge
on the superblock (or mapping) of that filesystem, and to charge
s_memcg_to_charge in fs specific code paths, so all are fs-specific
changes.

Based on this, it seems to me the suggestion is to hang the
memcg_to_charge off the mapping? I'm not sure if *most/all*
filesystems will eventually support it, but likely more than just
tmpfs.

Greg actually points out to me off list that the patches - as written
- supports remounting the tmpfs with a different memcg= option, where
future charges will be directed to the new memcg provided by the
option, so s_memcg_to_charge is more of a property of the super_block.

We could explicitly disable remounting with a different memcg=, but
I'm hoping to preserve that support if possible. The only way to
preserve it I think and avoid the pointer chasing in fs generic paths
is for shmem to set_active_memcg() before calling into the generic fs
code, but all other fs that apply this feature will need to do the
same. I'm not sure if that's the better option. Let me know what you
think please. Thanks!

quoted

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com