[PATCH] nfsd: more robust allocation failure handling in nfsd_file_cache_init

[PATCH] nfsd: more robust allocation failure handling in nfsd_file_cache_init

[Amir Goldstein]

The nfsd file cache table can be pretty large and its allocation
may require as many as 80 contigious pages.

Employ the same fix that was employed for similar issue that was
reported for the reply cache hash table allocation several years ago
by commit 8f97514b423a ("nfsd: more robust allocation failure handling
in nfsd_reply_cache_init").

Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility to nfsd")
Link: https://lore.kernel.org/linux-nfs/e3cdaeec85a6cfec980e87fc294327c0381c1778.camel@kernel.org/
Suggested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/nfsd/filecache.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 8bc807c5fea4..b75cd6d1e331 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -60,6 +60,9 @@ static struct fsnotify_group		*nfsd_file_fsnotify_group;
 static atomic_long_t			nfsd_filecache_count;
 static struct delayed_work		nfsd_filecache_laundrette;
 
+#define NFSD_FILE_HASHTBL_SIZE \
+	array_size(NFSD_FILE_HASH_SIZE, sizeof(*nfsd_file_hashtbl))
+
 static void nfsd_file_gc(void);
 
 static void
@@ -632,8 +635,7 @@ nfsd_file_cache_init(void)
 	if (!nfsd_filecache_wq)
 		goto out;
 
-	nfsd_file_hashtbl = kcalloc(NFSD_FILE_HASH_SIZE,
-				sizeof(*nfsd_file_hashtbl), GFP_KERNEL);
+	nfsd_file_hashtbl = kvzalloc(NFSD_FILE_HASHTBL_SIZE, GFP_KERNEL);
 	if (!nfsd_file_hashtbl) {
 		pr_err("nfsd: unable to allocate nfsd_file_hashtbl\n");
 		goto out_err;
-- 
2.25.1

Re: [PATCH] nfsd: more robust allocation failure handling in nfsd_file_cache_init

[Jeff Layton]

On Thu, 2022-02-24 at 18:01 +0200, Amir Goldstein wrote:
> The nfsd file cache table can be pretty large and its allocation
> may require as many as 80 contigious pages.
> 
> Employ the same fix that was employed for similar issue that was
> reported for the reply cache hash table allocation several years ago
> by commit 8f97514b423a ("nfsd: more robust allocation failure handling
> in nfsd_reply_cache_init").
> 
> Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility to nfsd")
> Link: https://lore.kernel.org/linux-nfs/e3cdaeec85a6cfec980e87fc294327c0381c1778.camel@kernel.org/
> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/nfsd/filecache.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 8bc807c5fea4..b75cd6d1e331 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -60,6 +60,9 @@ static struct fsnotify_group		*nfsd_file_fsnotify_group;
>  static atomic_long_t			nfsd_filecache_count;
>  static struct delayed_work		nfsd_filecache_laundrette;
>  
> +#define NFSD_FILE_HASHTBL_SIZE \
> +	array_size(NFSD_FILE_HASH_SIZE, sizeof(*nfsd_file_hashtbl))
> +
>  static void nfsd_file_gc(void);
>  
>  static void
> @@ -632,8 +635,7 @@ nfsd_file_cache_init(void)
>  	if (!nfsd_filecache_wq)
>  		goto out;
>  
> -	nfsd_file_hashtbl = kcalloc(NFSD_FILE_HASH_SIZE,
> -				sizeof(*nfsd_file_hashtbl), GFP_KERNEL);
> +	nfsd_file_hashtbl = kvzalloc(NFSD_FILE_HASHTBL_SIZE, GFP_KERNEL);
>  	if (!nfsd_file_hashtbl) {
>  		pr_err("nfsd: unable to allocate nfsd_file_hashtbl\n");
>  		goto out_err;

You'd also need to change to kvfree(), in case it does end up being
vmalloc'ed. Also, I don't see that the new constant adds anything -- you
could just use kvcalloc().

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: more robust allocation failure handling in nfsd_file_cache_init

[Chuck Lever III]

Hi Jeff, Amir -

> On Feb 24, 2022, at 11:01 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> The nfsd file cache table can be pretty large and its allocation
> may require as many as 80 contigious pages.
> 
> Employ the same fix that was employed for similar issue that was
> reported for the reply cache hash table allocation several years ago
> by commit 8f97514b423a ("nfsd: more robust allocation failure handling
> in nfsd_reply_cache_init").
> 
> Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility to nfsd")
> Link: https://lore.kernel.org/linux-nfs/e3cdaeec85a6cfec980e87fc294327c0381c1778.camel@kernel.org/
> Suggested-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

IMO, this approach is a band-aid, as was 8f97514b423a. I note
that not as a rejection, but rather to point out that more
work is needed in this area. Big hash tables are inflexible.

I'll go with this one because you provided a patch description ;-)
But, as you noted, you need to fix up the kfree() paths too.


> ---
> fs/nfsd/filecache.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 8bc807c5fea4..b75cd6d1e331 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -60,6 +60,9 @@ static struct fsnotify_group		*nfsd_file_fsnotify_group;
> static atomic_long_t			nfsd_filecache_count;
> static struct delayed_work		nfsd_filecache_laundrette;
> 
> +#define NFSD_FILE_HASHTBL_SIZE \
> +	array_size(NFSD_FILE_HASH_SIZE, sizeof(*nfsd_file_hashtbl))
> +
> static void nfsd_file_gc(void);
> 
> static void
> @@ -632,8 +635,7 @@ nfsd_file_cache_init(void)
> 	if (!nfsd_filecache_wq)
> 		goto out;
> 
> -	nfsd_file_hashtbl = kcalloc(NFSD_FILE_HASH_SIZE,
> -				sizeof(*nfsd_file_hashtbl), GFP_KERNEL);
> +	nfsd_file_hashtbl = kvzalloc(NFSD_FILE_HASHTBL_SIZE, GFP_KERNEL);
> 	if (!nfsd_file_hashtbl) {
> 		pr_err("nfsd: unable to allocate nfsd_file_hashtbl\n");
> 		goto out_err;
> -- 
> 2.25.1
> 

--
Chuck Lever