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: <[email protected]> # v4.7+
Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
Signed-off-by: Benjamin Coddington <[email protected]>
---
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);
+ WARN_ON_ONCE(error);
+
spin_lock(&inode->i_lock);
if (newsize > i_size_read(inode))
i_size_write(inode, newsize);
--
2.31.1
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: <[email protected]> # v4.7+
> Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> 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);
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
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: <[email protected]> # v4.7+
>> Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
>> Signed-off-by: Benjamin Coddington <[email protected]>
>> ---
>> 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: <[email protected]> # v4.7+
>>> Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
>>> Signed-off-by: Benjamin Coddington <[email protected]>
>>> ---
>>> 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: <[email protected]> # v4.7+
> > > > Fixes: 2e72448b07dc ("NFS: Add COPY nfs operation")
> > > > Signed-off-by: Benjamin Coddington <[email protected]>
> > > > ---
> > > > 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
[email protected]