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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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.
> > 
> 
> 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
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx






[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux