| Message ID | 8b8ccdb69af2473eef4a36968894e7aee34d5851.1637069577.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | COPY/CLONE pagecache invalidation | expand |
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? > + WARN_ON_ONCE(error); > + > spin_lock(&inode->i_lock); > if (newsize > i_size_read(inode)) > i_size_write(inode, newsize);
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. Ben
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. Ben
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.
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); + WARN_ON_ONCE(error); + spin_lock(&inode->i_lock); if (newsize > i_size_read(inode)) i_size_write(inode, newsize);
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(-)