From: Trond Myklebust <trondmy@hammerspace.com>
To: "Anna.Schumaker@netapp.com" <Anna.Schumaker@netapp.com>
Cc: "dai.ngo@oracle.com" <dai.ngo@oracle.com>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"bcodding@redhat.com" <bcodding@redhat.com>
Subject: Re: [PATCH 2/4] NFS: Directory page cache pages need to be locked when read
Date: Mon, 3 Feb 2020 22:55:55 +0000 [thread overview]
Message-ID: <af9f123727b738150c598c0b35107f6081703e68.camel@hammerspace.com> (raw)
In-Reply-To: <fdf116b70a147d169a4b9b6800a34682eba582b6.camel@hammerspace.com>
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().
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
next prev parent reply other threads:[~2020-02-03 22:56 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-02 22:53 [PATCH 0/4] Readdir fixes Trond Myklebust
2020-02-02 22:53 ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Trond Myklebust
2020-02-02 22:53 ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Trond Myklebust
2020-02-02 22:53 ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Trond Myklebust
2020-02-02 22:53 ` [PATCH 4/4] NFS: Switch readdir to using iterate_shared() Trond Myklebust
2020-02-03 14:41 ` Benjamin Coddington
2020-02-03 14:22 ` [PATCH 3/4] NFS: Use kmemdup_nul() in nfs_readdir_make_qstr() Benjamin Coddington
2020-02-03 14:21 ` [PATCH 2/4] NFS: Directory page cache pages need to be locked when read Benjamin Coddington
2020-02-03 20:31 ` Schumaker, Anna
2020-02-03 21:18 ` Trond Myklebust
2020-02-03 21:21 ` Schumaker, Anna
2020-02-03 22:45 ` Trond Myklebust
2020-02-03 22:50 ` Trond Myklebust
2020-02-03 22:55 ` Trond Myklebust [this message]
2020-02-05 0:44 ` Dai Ngo
2020-02-03 13:44 ` [PATCH 1/4] NFS: Fix memory leaks and corruption in readdir Benjamin Coddington
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=af9f123727b738150c598c0b35107f6081703e68.camel@hammerspace.com \
--to=trondmy@hammerspace.com \
--cc=Anna.Schumaker@netapp.com \
--cc=bcodding@redhat.com \
--cc=dai.ngo@oracle.com \
--cc=linux-nfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).