netfs: Fix missing alloc tagging of direct mempool allocations
Subsystems (5)
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.
---
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
@@ -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 *rreqstruct 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
@@ -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
@@ -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
+Cc Christoph
Quoted 31 lines · Hao Ge
Commit
1d78d56c43ef("netfs: Fixfolio_queueENOMEM in writeback by adding a mempool") added a mempool for thefolio_queuesand made the request, subrequest andfolio_queueallocations distinguish between writeback and everything else. Writeback is part of memory reclaim and must not fail due to ENOMEM, so it allocates underGFP_NOFSthroughmempool_alloc(), which may dip into the pool's reserve and, if that runs empty, wait for elements to be returned. TheGFP_KERNELpaths, which can return-ENOMEMto their callers, invoke the pool's ->alloc() callback directly instead.The direct call, however, skips the
alloc_hooks()wrapper that themempool_alloc()macro provides. The pool callbacks,mempool_alloc_slab()andmempool_kmalloc(), callkmem_cache_alloc_noprof()andkmalloc_noprof()and rely oncurrent->alloc_taghaving been set by the caller. WithCONFIG_MEM_ALLOC_PROFILING_DEBUG=ythis leads to
current->alloc_tagnot set
WARNING:./include/linux/alloc_tag.h:161at__alloc_tagging_slab_alloc_hookalloc_tagwas not set
WARNING:./include/linux/alloc_tag.h:166at__alloc_tagging_slab_free_hookat 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 newmempool_alloc_noreserve()helper ininclude/linux/mempool.h, next to the other alloc_hooks()-wrapped macros such asmempool_alloc(). TheGFP_KERNELpaths 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")Reported-by Erhard Furtner <[email protected]>Tested-by Erhard Furtner <[email protected]>Suggested-by Suren Baghdasaryan <[email protected]>Signed-off-by Hao Ge <[email protected]>
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 thealloc_hooks()wrapper fromfs/netfs/internal.hintoinclude/linux/mempool.hasmempool_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@@ -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 *rreqstruct 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@@ -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@@ -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,
Quoted 11 lines · Vlastimil Babka (SUSE)
Quoted 5 lines
Tested-by Erhard Furtner <[email protected]>Suggested-by Suren Baghdasaryan <[email protected]>Signed-off-by Hao Ge <[email protected]>Seems ok enough as a hotfix. The naming perhaps isn't ideal, we have
mempool_alloc_preallocated()already for the opposite case. Maybemempool_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.
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