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

[Michal Hocko <mhocko@suse.com>]

On Wed 02-10-24 00:41:29, Christoph Hellwig wrote:
[...]
> 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.

yes, this is a much cleaner solution. filemap_add_folio_nocharge would
need documentation explaining when this is supposed to be used.

> 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

-- 
Michal Hocko
SUSE Labs