nfsd: unable to allocate nfsd_file_hashtbl

nfsd: unable to allocate nfsd_file_hashtbl

[Amir Goldstein]

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

Re: nfsd: unable to allocate nfsd_file_hashtbl

[Jeff Layton]

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.

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.

-- 
Jeff Layton <jlayton@kernel.org>

Re: nfsd: unable to allocate nfsd_file_hashtbl

[Chuck Lever III]

> On Feb 24, 2022, at 6:07 AM, Jeff Layton <jlayton@kernel.org> 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.


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


--
Chuck Lever

Re: nfsd: unable to allocate nfsd_file_hashtbl

[Jeff Layton]

On Thu, 2022-02-24 at 15:14 +0000, Chuck Lever III wrote:
> 
> > On Feb 24, 2022, at 6:07 AM, Jeff Layton <jlayton@kernel.org> 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:

---------------------8<--------------------

[RFC PATCH] nfsd: use kvcalloc to allocate nfsd_file_hashtbl

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 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

Re: nfsd: unable to allocate nfsd_file_hashtbl

[Amir Goldstein]

On Thu, Feb 24, 2022 at 5:53 PM Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@kernel.org>
> ---
>  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
>
>