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
