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

[Christoph Hellwig <hch@infradead.org>]

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