On Mon, 2020-02-03 at 16:18 -0500, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 20:31 +0000, Schumaker, Anna wrote:
> > 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():
>
> Since this needs to be a stable patch, it would be preferable to apply
> them in the opposite order to avoid an extra dependency on Dai's patch.
That makes sense.
>
> That said, since the nfsi->page_index is not a per-page variable, there
> is no need to put it under the page lock.
Got it. I'll swap the order of everything, and put the page_index outside of the
page lock when resolving the conflict.
Thanks!
Anna
>
>
> > @@ -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);
