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@xxxxxxxxxxxxxxx # v2.6.37+
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> ---
> 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);
