[2/4] NFS: Directory page cache pages need to be locked when read
diff mbox series

Message ID 20200202225356.995080-3-trond.myklebust@hammerspace.com
State New
Headers show
Series
  • Readdir fixes
Related show

Commit Message

Trond Myklebust Feb. 2, 2020, 10:53 p.m. UTC
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(-)

Comments

Benjamin Coddington Feb. 3, 2020, 2:21 p.m. UTC | #1
On 2 Feb 2020, at 17:53, 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>

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben
Anna Schumaker Feb. 3, 2020, 8:31 p.m. UTC | #2
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);
Trond Myklebust Feb. 3, 2020, 9:18 p.m. UTC | #3
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@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():

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 said, since the nfsi->page_index is not a per-page variable, there
is no need to put it under the page lock.


> @@ -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);
Anna Schumaker Feb. 3, 2020, 9:21 p.m. UTC | #4
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@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():
> 
> 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);
Trond Myklebust Feb. 3, 2020, 10:45 p.m. UTC | #5
On Mon, 2020-02-03 at 21:21 +0000, Schumaker, Anna wrote:
> 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@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():
> > 
> > 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.
> 

Oops... Actually Dai's code needs to go in the 'return 0' path above
(i.e. after a successful call to nfs_readdir_search_array()).
It shouldn't go in the error path.
Trond Myklebust Feb. 3, 2020, 10:50 p.m. UTC | #6
On Mon, 2020-02-03 at 22:45 +0000, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 21:21 +0000, Schumaker, Anna wrote:
> > 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@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():
> > > 
> > > 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.
> > 
> 
> Oops... Actually Dai's code needs to go in the 'return 0' path above
> (i.e. after a successful call to nfs_readdir_search_array()).
> It shouldn't go in the error path.


While moving the code, could you also add in a small micro-
optimisation? If we use file_inode(desc->file) instead of
d_inode(file_dentry(desc->file)) then we avoid at least one pointer
indirection.
Trond Myklebust Feb. 3, 2020, 10:55 p.m. UTC | #7
On Mon, 2020-02-03 at 22:50 +0000, Trond Myklebust wrote:
> On Mon, 2020-02-03 at 22:45 +0000, Trond Myklebust wrote:
> > On Mon, 2020-02-03 at 21:21 +0000, Schumaker, Anna wrote:
> > > 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@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():
> > > > 
> > > > 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.
> > > 
> > 
> > Oops... Actually Dai's code needs to go in the 'return 0' path
> > above
> > (i.e. after a successful call to nfs_readdir_search_array()).
> > It shouldn't go in the error path.
> 
> While moving the code, could you also add in a small micro-
> optimisation? If we use file_inode(desc->file) instead of
> d_inode(file_dentry(desc->file)) then we avoid at least one pointer
> indirection.
> 

Oh, and please remove the call to nfs_zap_mapping(dir, dir->i_mapping)
in nfs_for_use_readdirplus(). We don't need that when we have the call
to invalidate_mapping_pages().
Dai Ngo Feb. 5, 2020, 12:44 a.m. UTC | #8
On 2/3/20 1:18 PM, 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@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():
> 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.

Hi Trond, does this mean my patch won't make it to the stable backport
this time? This patch is not just a performance enhancement, but fixes
real bug, which in some cases can cause readdir to take very long time
to complete.

Thanks,
-Dai

>
> That said, since the nfsi->page_index is not a per-page variable, there
> is no need to put it under the page lock.
>
>
>> @@ -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);

Patch
diff mbox series

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;
 }
 
@@ -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);