Re: [PATCH 1/3] NFSv42: Fix pagecache invalidation after COPY/CLONE

[Trond Myklebust <trondmy@hammerspace.com>]

On Tue, 2021-11-16 at 09:06 -0500, Benjamin Coddington wrote:
> On 16 Nov 2021, at 9:01, Benjamin Coddington wrote:
> 
> > On 16 Nov 2021, at 8:57, Trond Myklebust wrote:
> > 
> > > On Tue, 2021-11-16 at 08:49 -0500, Benjamin Coddington wrote:
> > > > The mechanism in use to allow the client to see the results of
> > > > COPY/CLONE
> > > > is to drop those pages from the pagecache.  This forces the
> > > > client 
> > > > to
> > > > read
> > > > those pages once more from the server.  However,
> > > > truncate_pagecache_range()
> > > > zeros out partial pages instead of dropping them.  Let us
> > > > instead 
> > > > use
> > > > invalidate_inode_pages2_range() with full-page offsets to
> > > > ensure the
> > > > client
> > > > properly sees the results of COPY/CLONE operations.
> > > > 
> > > > Cc: <stable@vger.kernel.org> # v4.7+
> > > > Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
> > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > > > ---
> > > >  fs/nfs/nfs42proc.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > index a24349512ffe..bbcd4c80c5a6 100644
> > > > --- a/fs/nfs/nfs42proc.c
> > > > +++ b/fs/nfs/nfs42proc.c
> > > > @@ -285,7 +285,10 @@ static void nfs42_copy_dest_done(struct
> > > > inode
> > > > *inode, loff_t pos, loff_t len)
> > > >         loff_t newsize = pos + len;
> > > >         loff_t end = newsize - 1;
> > > >  
> > > > -       truncate_pagecache_range(inode, pos, end);
> > > > +       int error = 
> > > > invalidate_inode_pages2_range(inode->i_mapping,
> > > > +                               pos 
> > > > > > PAGE_SHIFT, end >>
> > > > PAGE_SHIFT);
> > > 
> > > Shouldn't that be "(end + PAGE_SIZE - 1) >> PAGE_SHIFT" in order
> > > to
> > > align to the set of pages that fully contains the byte range from
> > > pos
> > > to end?
> > 
> > It's embarrassing that I've messed that up, I will resend it.
> 
> I've had it sitting around a bit too long -- on a second look no, it 
> should
> be right because invalidate_inode_pages2_range()'s index arguments
> are
> inclusive.
> 

OK, but can you resend without the unnecessary attribute declaration?
WARN_ON_ONCE(invalidate_inode_pages(...)) should be good enough.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com