On Thu, Feb 24, 2022 at 5:53 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > On Thu, 2022-02-24 at 15:14 +0000, Chuck Lever III wrote: > > > > > On Feb 24, 2022, at 6:07 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > > > On Thu, 2022-02-24 at 12:13 +0200, Amir Goldstein wrote: > > > > Hi Jeff, > > > > > > > > I got some reports from customers about failure to allocate the > > > > nfsd_file_hashtbl on nfs server restart on a long running system, > > > > probably due to memory fragmentation. > > > > > > > > A search in Google for this error message yields several results of > > > > similar reports [1][2]. > > > > > > > > My question is, does nfsd_file_cache_init() have to be done on server > > > > startup? > > > > > > > > Doesn't it make more sense to allocate all the memory pools and > > > > hash table on init_nfsd()? > > > > > > > > Thanks, > > > > Amir. > > > > > > > > [1] https://unix.stackexchange.com/questions/640534/nfs-cannot-allocate-memory > > > > [2] https://askubuntu.com/questions/1365821/nfs-crashing-on-ubuntu-server-20-04 > > > > > > That is a big allocation. On my box, nfsd_fcache_bucket is 80 bytes, so > > > we end up needing 80 contiguous pages to allocate the whole table. It > > > doesn't surprise me that it fails sometimes. > > > > > > We could just allocate it on init_nfsd, but that happens when the module > > > is plugged in and we'll lose 80 pages when people plug it in (or build > > > it in) and don't actually use nfsd. > > > > Reducing the bucket count might also help, especially if nfb_head > > were to be replaced with an rb_tree to allow more items in each > > bucket. > > > > > > I don't think you can do RCU traversal of an rbtree (can you?), and > you'd lose that ability if you switch to one. OTOH, maybe it doesn't buy > much, if you're looking to redesign that table for other reasons. > > > > The other option might be to just use kvcalloc? It's not a frequent > > > allocation, so I don't think performance would be an issue. We had > > > similar reports several years ago with nfsd_reply_cache_init, and using > > > kvzalloc ended up taking care of it. > > > > A better long-term solution would be to not require a large > > allocation at all. Maybe we could consider some alternative > > data structures. > > > > Sure, you could build it up from pages or something. That's a lot more > hassle though. > > I don't see a problem with vmalloc here. This allocation only happens > when the nfs server is started, which is an infrequent occurrence. A > modest performance hit at that time to fix up the kernel pagetables > doesn't seem too awful. > > This is almost exactly the same problem that 8f97514b423a (nfsd: more > robust allocation failure handling in nfsd_reply_cache_init) was > intended to fix, and that was suggested by the head penguin himself. > > Here's what I'd suggest, but I haven't had time to test it out: You raced me to sending a patch, but my patch is broken (forgot to change to kvfree :-/) Thanks, Amir. > > ---------------------8<-------------------- > > [RFC PATCH] nfsd: use kvcalloc to allocate nfsd_file_hashtbl > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > --- > fs/nfsd/filecache.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index 8bc807c5fea4..cc2831cec669 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -632,7 +632,7 @@ nfsd_file_cache_init(void) > if (!nfsd_filecache_wq) > goto out; > > - nfsd_file_hashtbl = kcalloc(NFSD_FILE_HASH_SIZE, > + nfsd_file_hashtbl = kvcalloc(NFSD_FILE_HASH_SIZE, > sizeof(*nfsd_file_hashtbl), GFP_KERNEL); > if (!nfsd_file_hashtbl) { > pr_err("nfsd: unable to allocate nfsd_file_hashtbl\n"); > @@ -700,7 +700,7 @@ nfsd_file_cache_init(void) > nfsd_file_slab = NULL; > kmem_cache_destroy(nfsd_file_mark_slab); > nfsd_file_mark_slab = NULL; > - kfree(nfsd_file_hashtbl); > + kvfree(nfsd_file_hashtbl); > nfsd_file_hashtbl = NULL; > destroy_workqueue(nfsd_filecache_wq); > nfsd_filecache_wq = NULL; > @@ -811,7 +811,7 @@ nfsd_file_cache_shutdown(void) > fsnotify_wait_marks_destroyed(); > kmem_cache_destroy(nfsd_file_mark_slab); > nfsd_file_mark_slab = NULL; > - kfree(nfsd_file_hashtbl); > + kvfree(nfsd_file_hashtbl); > nfsd_file_hashtbl = NULL; > destroy_workqueue(nfsd_filecache_wq); > nfsd_filecache_wq = NULL; > -- > 2.35.1 > >
