Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read

["Schumaker, Anna" <Anna.Schumaker@netapp.com>]

Hi Trond,

On Sun, 2020-02-02 at 17:53 -0500, Trond Myklebust wrote:
> When a NFS directory page cache page is removed from the page cache,
> its contents are freed through a call to nfs_readdir_clear_array().
> To prevent the removal of the page cache entry until after we've
> finished reading it, we must take the page lock.
> 
> Fixes: 11de3b11e08c ("NFS: Fix a memory leak in nfs_readdir")
> Cc: stable@vger.kernel.org # v2.6.37+
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/dir.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index ba0d55930e8a..90467b44ec13 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -705,8 +705,6 @@ int nfs_readdir_filler(void *data, struct page* page)
>  static
>  void cache_page_release(nfs_readdir_descriptor_t *desc)
>  {
> -	if (!desc->page->mapping)
> -		nfs_readdir_clear_array(desc->page);
>  	put_page(desc->page);
>  	desc->page = NULL;
>  }
> @@ -720,19 +718,28 @@ struct page *get_cache_page(nfs_readdir_descriptor_t
> *desc)
>  
>  /*
>   * Returns 0 if desc->dir_cookie was found on page desc->page_index
> + * and locks the page to prevent removal from the page cache.
>   */
>  static
> -int find_cache_page(nfs_readdir_descriptor_t *desc)
> +int find_and_lock_cache_page(nfs_readdir_descriptor_t *desc)
>  {
>  	int res;
>  
>  	desc->page = get_cache_page(desc);
>  	if (IS_ERR(desc->page))
>  		return PTR_ERR(desc->page);
> -
> -	res = nfs_readdir_search_array(desc);
> +	res = lock_page_killable(desc->page);
>  	if (res != 0)
> -		cache_page_release(desc);
> +		goto error;
> +	res = -EAGAIN;
> +	if (desc->page->mapping != NULL) {
> +		res = nfs_readdir_search_array(desc);
> +		if (res == 0)
> +			return 0;
> +	}
> +	unlock_page(desc->page);
> +error:
> +	cache_page_release(desc);
>  	return res;
>  }
>  

Can you give me some guidance on how to apply this on top of Dai's v2 patch from
January 23? Right now I have the nfsi->page_index getting set before the
unlock_page():

@@ -732,15 +733,24 @@ int find_cache_page(nfs_readdir_descriptor_t *desc)
 	if (IS_ERR(desc->page))
 		return PTR_ERR(desc->page);
 
-	res = nfs_readdir_search_array(desc);
+	res = lock_page_killable(desc->page);
 	if (res != 0)
 		cache_page_release(desc);
+		goto error;
+	res = -EAGAIN;
+	if (desc->page->mapping != NULL) {
+		res = nfs_readdir_search_array(desc);
+		if (res == 0)
+			return 0;
+	}
 
 	dentry = file_dentry(desc->file);
 	inode = d_inode(dentry);
 	nfsi = NFS_I(inode);
 	nfsi->page_index = desc->page_index;
-
+	unlock_page(desc->page);
+error:
+	cache_page_release(desc);
 	return res;
 }
 

Please let me know if there is a better way to handle the conflict!

Anna


> @@ -747,7 +754,7 @@ int readdir_search_pagecache(nfs_readdir_descriptor_t
> *desc)
>  		desc->last_cookie = 0;
>  	}
>  	do {
> -		res = find_cache_page(desc);
> +		res = find_and_lock_cache_page(desc);
>  	} while (res == -EAGAIN);
>  	return res;
>  }
> @@ -786,7 +793,6 @@ int nfs_do_filldir(nfs_readdir_descriptor_t *desc)
>  		desc->eof = true;
>  
>  	kunmap(desc->page);
> -	cache_page_release(desc);
>  	dfprintk(DIRCACHE, "NFS: nfs_do_filldir() filling ended @ cookie %Lu;
> returning = %d\n",
>  			(unsigned long long)*desc->dir_cookie, res);
>  	return res;
> @@ -832,13 +838,13 @@ int uncached_readdir(nfs_readdir_descriptor_t *desc)
>  
>  	status = nfs_do_filldir(desc);
>  
> + out_release:
> +	nfs_readdir_clear_array(desc->page);
> +	cache_page_release(desc);
>   out:
>  	dfprintk(DIRCACHE, "NFS: %s: returns %d\n",
>  			__func__, status);
>  	return status;
> - out_release:
> -	cache_page_release(desc);
> -	goto out;
>  }
>  
>  /* The file offset position represents the dirent entry number.  A
> @@ -903,6 +909,8 @@ static int nfs_readdir(struct file *file, struct
> dir_context *ctx)
>  			break;
>  
>  		res = nfs_do_filldir(desc);
> +		unlock_page(desc->page);
> +		cache_page_release(desc);
>  		if (res < 0)
>  			break;
>  	} while (!desc->eof);