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.
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
