2021-11-16 13:49:36

by Benjamin Coddington

[permalink] [raw]
Subject: [PATCH 1/3] NFSv42: Fix pagecache invalidation after COPY/CLONE

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



2021-11-16 13:57:21

by Trond Myklebust

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

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]


2021-11-16 14:02:40

by Benjamin Coddington

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

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


2021-11-16 14:06:42

by Benjamin Coddington

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

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


2021-11-16 15:27:02

by Trond Myklebust

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

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]