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@xxxxxxxxxxxxxxx> # v4.7+
> > > > Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
> > > > Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx
