Re: [PATCH] btrfs: root memcgroup for metadata filemap_add_folio()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/10/2 17:11, Christoph Hellwig 写道:
> On Tue, Oct 01, 2024 at 07:10:07PM +0930, Qu Wenruo wrote:
>>> This looks pretty ugly.  What speaks against a version of
>>> filemap_add_folio that doesn't charge the memcg?
>>>
>>
>> Because there is so far only one caller has such requirement.
>
> That is a good argument to review the reasons for an interface, but
> not a killer argument.
>
>> Furthermore I believe the folio API doesn't prefer too many different
>> functions doing similar things.
>>
>> E.g. the new folio interfaces only provides filemap_get_folio(),
>> filemap_lock_folio(), and the more generic __filemap_get_folio().
>>
>> Meanwhile there are tons of page based interfaces, find_get_page(),
>> find_or_create_page(), find_lock_page() and flags version etc.
>
> That's a totally different argument, tough.  Those functions were
> trivial wrappers around a more versatile low-level function.
>
> While this is about adding clearly defined functionality, and
> more importantly not exporting totally random low-level data.
>
> What I'd propose is something like the patch below, plus proper
> documentation.  Note that this now does the uncharge on the unlocked
> folio in the error case.  From a quick look that should be fine, but
> someone who actually knows the code needs to confirm that.

The interface looks good to me, especially we completely skip the
charging, which is even better than the current form.

And since Michal is also happy with this idea, I can definite go this path.

Just a little curious, would it be better to introduce a flag for
address_space to indicate whether the folio needs to be charged or not?

Thanks,
Qu
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 68a5f1ff3301c6..70da62cf32f6c3 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -1284,6 +1284,8 @@ int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
>   		pgoff_t index, gfp_t gfp);
>   int filemap_add_folio(struct address_space *mapping, struct folio *folio,
>   		pgoff_t index, gfp_t gfp);
> +int filemap_add_folio_nocharge(struct address_space *mapping,
> +		struct folio *folio, pgoff_t index, gfp_t gfp);
>   void filemap_remove_folio(struct folio *folio);
>   void __filemap_remove_folio(struct folio *folio, void *shadow);
>   void replace_page_cache_folio(struct folio *old, struct folio *new);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 36d22968be9a1e..0a1ae841e8c10f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -958,20 +958,15 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>   }
>   ALLOW_ERROR_INJECTION(__filemap_add_folio, ERRNO);
>
> -int filemap_add_folio(struct address_space *mapping, struct folio *folio,
> -				pgoff_t index, gfp_t gfp)
> +int filemap_add_folio_nocharge(struct address_space *mapping,
> +		struct folio *folio, pgoff_t index, gfp_t gfp)
>   {
>   	void *shadow = NULL;
>   	int ret;
>
> -	ret = mem_cgroup_charge(folio, NULL, gfp);
> -	if (ret)
> -		return ret;
> -
>   	__folio_set_locked(folio);
>   	ret = __filemap_add_folio(mapping, folio, index, gfp, &shadow);
>   	if (unlikely(ret)) {
> -		mem_cgroup_uncharge(folio);
>   		__folio_clear_locked(folio);
>   	} else {
>   		/*
> @@ -989,6 +984,22 @@ int filemap_add_folio(struct address_space *mapping, struct folio *folio,
>   	}
>   	return ret;
>   }
> +EXPORT_SYMBOL_GPL(filemap_add_folio_nocharge);
> +
> +int filemap_add_folio(struct address_space *mapping, struct folio *folio,
> +		pgoff_t index, gfp_t gfp)
> +{
> +	int ret;
> +
> +	ret = mem_cgroup_charge(folio, NULL, gfp);
> +	if (ret)
> +		return ret;
> +
> +	ret = filemap_add_folio_nocharge(mapping, folio, index, gfp);
> +	if (ret)
> +		mem_cgroup_uncharge(folio);
> +	return ret;
> +}
>   EXPORT_SYMBOL_GPL(filemap_add_folio);
>
>   #ifdef CONFIG_NUMA
>