Skip to contentSkip to conversation

netfs: Fix missing alloc tagging of direct mempool allocations

PATCH Applied Hao Ge 4 participants 4 messages Sep 23 – Sep 28

Subsystems (5)
lore
Share & tools
lore

Hao Ge

author
#0 lore raw

PATCH v2 1/1 netfs: Fix missing alloc tagging of direct mempool allocations

Commit 1d78d56c43ef ("netfs: Fix folio_queue ENOMEM in writeback by adding a mempool") added a mempool for the folio_queues and made the request, subrequest and folio_queue allocations distinguish between writeback and everything else. Writeback is part of memory reclaim and must not fail due to ENOMEM, so it allocates under GFP_NOFS through mempool_alloc(), which may dip into the pool's reserve and, if that runs empty, wait for elements to be returned. The GFP_KERNEL paths, which can return -ENOMEM to their callers, invoke the pool's ->alloc() callback directly instead.

The direct call, however, skips the alloc_hooks() wrapper that the mempool_alloc() macro provides. The pool callbacks, mempool_alloc_slab() and mempool_kmalloc(), call kmem_cache_alloc_noprof() and kmalloc_noprof() and rely on current->alloc_tag having been set by the caller. With CONFIG_MEM_ALLOC_PROFILING_DEBUG=y this leads to

current->alloc_tag not set
WARNING: ./include/linux/alloc_tag.h:161 at __alloc_tagging_slab_alloc_hook alloc_tag was not set
WARNING: ./include/linux/alloc_tag.h:166 at __alloc_tagging_slab_free_hook

at allocation and free time respectively, as reported when reading files on a CIFS mount. The allocations are also missing from /proc/allocinfo.

Wrap the direct ->alloc() invocations in alloc_hooks() with a new mempool_alloc_noreserve() helper in include/linux/mempool.h, next to the other alloc_hooks()-wrapped macros such as mempool_alloc(). The GFP_KERNEL paths keep their failable allocation semantics, they just get tagged now.

Fixes 1d78d56c43ef ("netfs: Fix folio_queue ENOMEM in writeback by adding a mempool")

---
Changes in v2:
- Move the alloc_hooks() wrapper from fs/netfs/internal.h into include/linux/mempool.h as mempool_alloc_noreserve(), per Suren's suggestion, so it is not netfs-specific.

v1 Link: https://lore.kernel.org/all/[email protected]/

---
 fs/netfs/objects.c        | 4 ++--
 fs/netfs/rolling_buffer.c | 2 +-
 include/linux/mempool.h   | 7 +++++++
 3 files changed, 10 insertions(+), 3 deletions(-)
fs/netfs/objects.c +2 -2
index 7f6a3e912602..ad549daa9c79 100644
@@ -34,7 +34,7 @@ struct netfs_io_request *netfs_alloc_request(struct address_space *mapping,
 
 		rreq = mempool_alloc(mempool, gfp);
 	} else {
-		rreq = mempool->alloc(gfp, mempool->pool_data);
+		rreq = mempool_alloc_noreserve(mempool, gfp);
 		if (!rreq)
 			return ERR_PTR(-ENOMEM);
 	}
@@ -214,7 +214,7 @@ struct netfs_io_subrequest *netfs_alloc_subrequest(struct netfs_io_request *rreq
 	struct kmem_cache *cache = mempool->pool_data;
 
 	if (rreq->gfp == GFP_KERNEL)
-		subreq = mempool->alloc(rreq->gfp, mempool->pool_data);
+		subreq = mempool_alloc_noreserve(mempool, rreq->gfp);
 	else
 		subreq = mempool_alloc(mempool, rreq->gfp);
 	if (!subreq)
fs/netfs/rolling_buffer.c +1 -1
index 424e77a9a109..d30d5ef6d86e 100644
@@ -29,7 +29,7 @@ struct folio_queue *netfs_folioq_alloc(unsigned int rreq_id, gfp_t gfp,
 	struct folio_queue *fq;
 
 	if (gfp == GFP_KERNEL)
-		fq = netfs_folioq_pool.alloc(gfp, netfs_folioq_pool.pool_data);
+		fq = mempool_alloc_noreserve(&netfs_folioq_pool, gfp);
 	else
 		fq = mempool_alloc(&netfs_folioq_pool, gfp);
 	if (fq) {
include/linux/mempool.h +7 -0
index a0fa6d43e0dc..6da502aef2f7 100644
@@ -70,6 +70,13 @@ int mempool_alloc_bulk_noprof(struct mempool *pool, void **elem,
 #define mempool_alloc_bulk(...)						\
 	alloc_hooks(mempool_alloc_bulk_noprof(__VA_ARGS__))
 
+/*
+ * Allocate a new element without dipping into the pool's reserves or
+ * waiting.  Returns NULL on failure.
+ */
+#define mempool_alloc_noreserve(_pool, _gfp)				\
+	alloc_hooks((_pool)->alloc(_gfp, (_pool)->pool_data))
+
 void *mempool_alloc_preallocated(struct mempool *pool) __malloc;
 void mempool_free(void *element, struct mempool *pool);
 unsigned int mempool_free_bulk(struct mempool *pool, void **elem,
Signature · 1 line
2.25.1
maintainer Acked-by on v2 1/1
#1 lore raw

+Cc Christoph

Quoted 31 lines · Hao Ge

Commit 1d78d56c43ef ("netfs: Fix folio_queue ENOMEM in writeback by adding a mempool") added a mempool for the folio_queues and made the request, subrequest and folio_queue allocations distinguish between writeback and everything else. Writeback is part of memory reclaim and must not fail due to ENOMEM, so it allocates under GFP_NOFS through mempool_alloc(), which may dip into the pool's reserve and, if that runs empty, wait for elements to be returned. The GFP_KERNEL paths, which can return -ENOMEM to their callers, invoke the pool's ->alloc() callback directly instead.

The direct call, however, skips the alloc_hooks() wrapper that the mempool_alloc() macro provides. The pool callbacks, mempool_alloc_slab() and mempool_kmalloc(), call kmem_cache_alloc_noprof() and kmalloc_noprof() and rely on current->alloc_tag having been set by the caller. With CONFIG_MEM_ALLOC_PROFILING_DEBUG=y this leads to

current->alloc_tag not set
WARNING: ./include/linux/alloc_tag.h:161 at __alloc_tagging_slab_alloc_hook alloc_tag was not set
WARNING: ./include/linux/alloc_tag.h:166 at __alloc_tagging_slab_free_hook

at allocation and free time respectively, as reported when reading files on a CIFS mount. The allocations are also missing from /proc/allocinfo.

Wrap the direct ->alloc() invocations in alloc_hooks() with a new mempool_alloc_noreserve() helper in include/linux/mempool.h, next to the other alloc_hooks()-wrapped macros such as mempool_alloc(). The GFP_KERNEL paths keep their failable allocation semantics, they just get tagged now.

Quoted 7 lines
Fixes 1d78d56c43ef ("netfs: Fix folio_queue ENOMEM in writeback by adding a mempool")

Seems ok enough as a hotfix. The naming perhaps isn't ideal, we have mempool_alloc_preallocated() already for the opposite case. Maybe mempool_alloc_no_preallocated() to complement it? Rather long though.

I guess long-term it would be neater to have a mempool_alloc() variant with extra parameter that says if preallocated pool is allowed or not. The "gfp == GFP_KERNEL" check is probably not an universal one. mempool_alloc() is actually rather aggressive in getting objects from the pool before even attempting reclaim. Makes sense for I/O usecases but maybe not for this one?

Quoted 66 lines

---
Changes in v2:
- Move the alloc_hooks() wrapper from fs/netfs/internal.h into include/linux/mempool.h as mempool_alloc_noreserve(), per Suren's suggestion, so it is not netfs-specific.

v1 Link: https://lore.kernel.org/all/[email protected]/

---
 fs/netfs/objects.c        | 4 ++--
 fs/netfs/rolling_buffer.c | 2 +-
 include/linux/mempool.h   | 7 +++++++
 3 files changed, 10 insertions(+), 3 deletions(-)
fs/netfs/objects.c +2 -2
index 7f6a3e912602..ad549daa9c79 100644
@@ -34,7 +34,7 @@ struct netfs_io_request *netfs_alloc_request(struct address_space *mapping,
 
 		rreq = mempool_alloc(mempool, gfp);
 	} else {
-		rreq = mempool->alloc(gfp, mempool->pool_data);
+		rreq = mempool_alloc_noreserve(mempool, gfp);
 		if (!rreq)
 			return ERR_PTR(-ENOMEM);
 	}
@@ -214,7 +214,7 @@ struct netfs_io_subrequest *netfs_alloc_subrequest(struct netfs_io_request *rreq
 	struct kmem_cache *cache = mempool->pool_data;
 
 	if (rreq->gfp == GFP_KERNEL)
-		subreq = mempool->alloc(rreq->gfp, mempool->pool_data);
+		subreq = mempool_alloc_noreserve(mempool, rreq->gfp);
 	else
 		subreq = mempool_alloc(mempool, rreq->gfp);
 	if (!subreq)
fs/netfs/rolling_buffer.c +1 -1
index 424e77a9a109..d30d5ef6d86e 100644
@@ -29,7 +29,7 @@ struct folio_queue *netfs_folioq_alloc(unsigned int rreq_id, gfp_t gfp,
 	struct folio_queue *fq;
 
 	if (gfp == GFP_KERNEL)
-		fq = netfs_folioq_pool.alloc(gfp, netfs_folioq_pool.pool_data);
+		fq = mempool_alloc_noreserve(&netfs_folioq_pool, gfp);
 	else
 		fq = mempool_alloc(&netfs_folioq_pool, gfp);
 	if (fq) {
include/linux/mempool.h +7 -0
index a0fa6d43e0dc..6da502aef2f7 100644
@@ -70,6 +70,13 @@ int mempool_alloc_bulk_noprof(struct mempool *pool, void **elem,
 #define mempool_alloc_bulk(...)						\
 	alloc_hooks(mempool_alloc_bulk_noprof(__VA_ARGS__))
 
+/*
+ * Allocate a new element without dipping into the pool's reserves or
+ * waiting.  Returns NULL on failure.
+ */
+#define mempool_alloc_noreserve(_pool, _gfp)				\
+	alloc_hooks((_pool)->alloc(_gfp, (_pool)->pool_data))
+
 void *mempool_alloc_preallocated(struct mempool *pool) __malloc;
 void mempool_free(void *element, struct mempool *pool);
 unsigned int mempool_free_bulk(struct mempool *pool, void **elem,
#2 lore raw
Quoted 11 lines · Vlastimil Babka (SUSE)
Quoted 5 lines

Seems ok enough as a hotfix. The naming perhaps isn't ideal, we have mempool_alloc_preallocated() already for the opposite case. Maybe mempool_alloc_no_preallocated() to complement it? Rather long though.

None of this is ok. Directly alling into mempool->alloc, using a wrapper and not is stupid and broken. Just switch the code to use mempool_alloc undconditionally or to use a manually kmalloc/kmem_cache_allow if the subsystem has a really good reason to not use a mempool for non-writeback requests, but it'd better be a really good and well-documented reason.

maintainer Applied
#3 lore raw
In reply to Hao Ge
Quoted 1 line · Hao Ge

netfs: Fix missing alloc tagging of direct mempool allocations

Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] netfs: Fix missing alloc tagging of direct mempool allocations

      https://git.kernel.org/vfs/vfs/c/b78b728e21c3