[PATCH 0/3] COPY/CLONE pagecache invalidation

[PATCH 0/3] COPY/CLONE pagecache invalidation

[Benjamin Coddington]

I found a small issue on the client with generic/432 when testing a server
that supports COPY/CLONE but doesn't have the open file descriptor cache.
Our current knfsd server's cache causes the server to not immediately
hand out a read delegation after a COPY/COMMIT, CLOSE, OPEN, so I suspect
our normal testing didn't catch this issue.

The client bug can be exposed by adding 5 second delays after xfs_io
commands in generic/432, which gives the server enough time to clean up the
cache and give the delegation.

Benjamin Coddington (3):
  NFSv42: Fix pagecache invalidation after COPY/CLONE
  NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation
  NFS: Add a tracepoint to show the results of nfs_set_cache_invalid()

 fs/nfs/inode.c     | 5 ++++-
 fs/nfs/nfs42proc.c | 8 ++++++--
 fs/nfs/nfstrace.h  | 1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.31.1

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

[Benjamin Coddington]

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);
+	WARN_ON_ONCE(error);
+
 	spin_lock(&inode->i_lock);
 	if (newsize > i_size_read(inode))
 		i_size_write(inode, newsize);
-- 
2.31.1

[PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Benjamin Coddington]

The nfs_set_cache_invalid() helper drops NFS_INO_INVALID_CHANGE if we hold
a delegation, but after a copy or clone the change attribute can be updated
on the server.  After commit b6f80a2ebb97 "NFS: Fix open coded versions of
nfs_set_cache_invalid() in NFSv4", the client stopped updating the change
attribute after copy or clone while holding a read delegation.

We can use NFS_INO_REVAL_PAGECACHE to help nfs_set_cache_invalid() know
when we really want to keep NFS_INO_INVALID_CHANGE, even if the client
holds a delegation.

Fixes: b6f80a2ebb97 ("NFS: Fix open coded versions of nfs_set_cache_invalid() in NFSv4")
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/inode.c     | 4 +++-
 fs/nfs/nfs42proc.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 853213b3a209..296ed8ea3273 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -202,7 +202,9 @@ void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
 			flags &= ~(NFS_INO_INVALID_MODE |
 				   NFS_INO_INVALID_OTHER |
 				   NFS_INO_INVALID_XATTR);
-		flags &= ~(NFS_INO_INVALID_CHANGE | NFS_INO_INVALID_SIZE);
+		if (!(flags & NFS_INO_REVAL_PAGECACHE))
+			flags &= ~NFS_INO_INVALID_CHANGE;
+		flags &= ~NFS_INO_INVALID_SIZE;
 	} else if (flags & NFS_INO_REVAL_PAGECACHE)
 		flags |= NFS_INO_INVALID_CHANGE | NFS_INO_INVALID_SIZE;
 
diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index bbcd4c80c5a6..fc3c36e1f656 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -292,7 +292,8 @@ static void nfs42_copy_dest_done(struct inode *inode, loff_t pos, loff_t len)
 	spin_lock(&inode->i_lock);
 	if (newsize > i_size_read(inode))
 		i_size_write(inode, newsize);
-	nfs_set_cache_invalid(inode, NFS_INO_INVALID_CHANGE |
+	nfs_set_cache_invalid(inode, NFS_INO_REVAL_PAGECACHE |
+					     NFS_INO_INVALID_CHANGE |
 					     NFS_INO_INVALID_CTIME |
 					     NFS_INO_INVALID_MTIME |
 					     NFS_INO_INVALID_BLOCKS);
-- 
2.31.1

[PATCH 3/3] NFS: Add a tracepoint to show the results of nfs_set_cache_invalid()

[Benjamin Coddington]

This provides some insight into the client's invalidation behavior to show
both when the client uses the helper, and the results of calling the
helper which can vary depending on how the helper is called.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/inode.c    | 1 +
 fs/nfs/nfstrace.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 296ed8ea3273..e4b092e40178 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -216,6 +216,7 @@ void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
 		flags &= ~(NFS_INO_INVALID_DATA|NFS_INO_DATA_INVAL_DEFER);
 	flags &= ~(NFS_INO_REVAL_PAGECACHE | NFS_INO_REVAL_FORCED);
 	nfsi->cache_validity |= flags;
+	trace_nfs_set_cache_invalid(inode, 0);
 }
 EXPORT_SYMBOL_GPL(nfs_set_cache_invalid);
 
diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index 8a224871be74..76bea172dce0 100644
--- a/fs/nfs/nfstrace.h
+++ b/fs/nfs/nfstrace.h
@@ -209,6 +209,7 @@ DEFINE_NFS_INODE_EVENT_DONE(nfs_writeback_inode_exit);
 DEFINE_NFS_INODE_EVENT(nfs_fsync_enter);
 DEFINE_NFS_INODE_EVENT_DONE(nfs_fsync_exit);
 DEFINE_NFS_INODE_EVENT(nfs_access_enter);
+DEFINE_NFS_INODE_EVENT_DONE(nfs_set_cache_invalid);
 
 TRACE_EVENT(nfs_access_exit,
 		TP_PROTO(
-- 
2.31.1

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

[Trond Myklebust]

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);

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

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

[Benjamin Coddington]

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

Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Trond Myklebust]

On Tue, 2021-11-16 at 08:49 -0500, Benjamin Coddington wrote:
> The nfs_set_cache_invalid() helper drops NFS_INO_INVALID_CHANGE if we
> hold
> a delegation, but after a copy or clone the change attribute can be
> updated
> on the server.  After commit b6f80a2ebb97 "NFS: Fix open coded
> versions of
> nfs_set_cache_invalid() in NFSv4", the client stopped updating the
> change
> attribute after copy or clone while holding a read delegation.
> 
> We can use NFS_INO_REVAL_PAGECACHE to help nfs_set_cache_invalid()
> know
> when we really want to keep NFS_INO_INVALID_CHANGE, even if the
> client
> holds a delegation.

No, we really shouldn't need to care what the server thinks or does.
The client is authoritative for the change attribute while it holds a
delegation, not the server.

> 
> Fixes: b6f80a2ebb97 ("NFS: Fix open coded versions of
> nfs_set_cache_invalid() in NFSv4")
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/inode.c     | 4 +++-
>  fs/nfs/nfs42proc.c | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 853213b3a209..296ed8ea3273 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -202,7 +202,9 @@ void nfs_set_cache_invalid(struct inode *inode,
> unsigned long flags)
>                         flags &= ~(NFS_INO_INVALID_MODE |
>                                    NFS_INO_INVALID_OTHER |
>                                    NFS_INO_INVALID_XATTR);
> -               flags &= ~(NFS_INO_INVALID_CHANGE |
> NFS_INO_INVALID_SIZE);
> +               if (!(flags & NFS_INO_REVAL_PAGECACHE))
> +                       flags &= ~NFS_INO_INVALID_CHANGE;
> +               flags &= ~NFS_INO_INVALID_SIZE;
>         } else if (flags & NFS_INO_REVAL_PAGECACHE)
>                 flags |= NFS_INO_INVALID_CHANGE |
> NFS_INO_INVALID_SIZE;
>  
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index bbcd4c80c5a6..fc3c36e1f656 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -292,7 +292,8 @@ static void nfs42_copy_dest_done(struct inode
> *inode, loff_t pos, loff_t len)
>         spin_lock(&inode->i_lock);
>         if (newsize > i_size_read(inode))
>                 i_size_write(inode, newsize);
> -       nfs_set_cache_invalid(inode, NFS_INO_INVALID_CHANGE |
> +       nfs_set_cache_invalid(inode, NFS_INO_REVAL_PAGECACHE |
> +                                            NFS_INO_INVALID_CHANGE |
>                                              NFS_INO_INVALID_CTIME |
>                                              NFS_INO_INVALID_MTIME |
>                                              NFS_INO_INVALID_BLOCKS);

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

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

[Benjamin Coddington]

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

Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Benjamin Coddington]

On 16 Nov 2021, at 9:03, Trond Myklebust wrote:
> No, we really shouldn't need to care what the server thinks or does.
> The client is authoritative for the change attribute while it holds a
> delegation, not the server.

My understanding of the intention of the code (which I had to sort of put
together from historical patches in this area) is that we want to see ctime,
mtime, and block size updates after copy/clone even if we hold a delegation,
but without NFS_INO_INVALID_CHANGE, the client won't update those
attributes.

If that's not necessary, we can drop this patch.

Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Trond Myklebust]

On Tue, 2021-11-16 at 09:12 -0500, Benjamin Coddington wrote:
> On 16 Nov 2021, at 9:03, Trond Myklebust wrote:
> > No, we really shouldn't need to care what the server thinks or
> > does.
> > The client is authoritative for the change attribute while it holds
> > a
> > delegation, not the server.
> 
> My understanding of the intention of the code (which I had to sort of
> put
> together from historical patches in this area) is that we want to see
> ctime,
> mtime, and block size updates after copy/clone even if we hold a
> delegation,
> but without NFS_INO_INVALID_CHANGE, the client won't update those
> attributes.
> 
> If that's not necessary, we can drop this patch.
> 

We will still see the ctime/mtime/block size updates even if
NFS_INO_INVALID_CHANGE is not set. Those attributes' cache status are
tracked separately through their own NFS_INO_INVALID_* bits.

That said, there really is no reason why we shouldn't treat the copy
and clone code exactly the same way we would treat a regular write.
Perhaps we can fix up the arguments of nfs_writeback_update_inode() so
that it can be called here?

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

Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Benjamin Coddington]

On 16 Nov 2021, at 9:17, Trond Myklebust wrote:

> On Tue, 2021-11-16 at 09:12 -0500, Benjamin Coddington wrote:
>> On 16 Nov 2021, at 9:03, Trond Myklebust wrote:
>>> No, we really shouldn't need to care what the server thinks or
>>> does.
>>> The client is authoritative for the change attribute while it holds
>>> a
>>> delegation, not the server.
>>
>> My understanding of the intention of the code (which I had to sort of
>> put
>> together from historical patches in this area) is that we want to see
>> ctime,
>> mtime, and block size updates after copy/clone even if we hold a
>> delegation,
>> but without NFS_INO_INVALID_CHANGE, the client won't update those
>> attributes.
>>
>> If that's not necessary, we can drop this patch.
>>
>
> We will still see the ctime/mtime/block size updates even if
> NFS_INO_INVALID_CHANGE is not set. Those attributes' cache status are
> tracked separately through their own NFS_INO_INVALID_* bits.
>
> That said, there really is no reason why we shouldn't treat the copy
> and clone code exactly the same way we would treat a regular write.
> Perhaps we can fix up the arguments of nfs_writeback_update_inode() so
> that it can be called here?

I wonder if that just kicks the problem down the road to when we return the
delegation, and we'll see cases where ctime/mtime move backward.  And I
don't think we can ever be authoritative for space_used, but maybe it doesn't
matter if we're within the attribute timeouts.

Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Trond Myklebust]

On Tue, 2021-11-16 at 09:34 -0500, Benjamin Coddington wrote:
> On 16 Nov 2021, at 9:17, Trond Myklebust wrote:
> 
> > On Tue, 2021-11-16 at 09:12 -0500, Benjamin Coddington wrote:
> > > On 16 Nov 2021, at 9:03, Trond Myklebust wrote:
> > > > No, we really shouldn't need to care what the server thinks or
> > > > does.
> > > > The client is authoritative for the change attribute while it
> > > > holds
> > > > a
> > > > delegation, not the server.
> > > 
> > > My understanding of the intention of the code (which I had to
> > > sort of
> > > put
> > > together from historical patches in this area) is that we want to
> > > see
> > > ctime,
> > > mtime, and block size updates after copy/clone even if we hold a
> > > delegation,
> > > but without NFS_INO_INVALID_CHANGE, the client won't update those
> > > attributes.
> > > 
> > > If that's not necessary, we can drop this patch.
> > > 
> > 
> > We will still see the ctime/mtime/block size updates even if
> > NFS_INO_INVALID_CHANGE is not set. Those attributes' cache status
> > are
> > tracked separately through their own NFS_INO_INVALID_* bits.
> > 
> > That said, there really is no reason why we shouldn't treat the
> > copy
> > and clone code exactly the same way we would treat a regular write.
> > Perhaps we can fix up the arguments of nfs_writeback_update_inode()
> > so
> > that it can be called here?
> 
> I wonder if that just kicks the problem down the road to when we
> return the
> delegation, and we'll see cases where ctime/mtime move backward.  And
> I
> don't think we can ever be authoritative for space_used, but maybe it
> doesn't
> matter if we're within the attribute timeouts.
> 

I don't understand your point. Mine is that we _should_ be setting the
NFS_INO_INVALID_MTIME, NFS_INO_INVALID_CTIME, NFS_INO_INVALID_BLOCKS...
flags here, and as far as I can tell, we are indeed doing so in
nfs42_copy_dest_done().

At least for clone(), we're then calling nfs_post_op_update_inode(),
which updates the attributes with whatever was retrieved in the CLONE
call. That will usually contain new values for mtime, ctime and blocks,
and so the cache is refreshed.

If the client failed to retrieve attributes as part of the CLONE or
COPY call, then we are indeed kicking the can of revalidating the
attributes down the road, but only as far as until the next call to
nfs_getattr(), or the delegation return, whichever comes first. The
fact that we do set the NFS_INO_INVALID_MTIME, ... means that we will
update those cached values before replying to a stat() or statx() call.

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

Re: [PATCH 2/3] NFSv42: Don't drop NFS_INO_INVALID_CHANGE if we hold a delegation

[Benjamin Coddington]

On 16 Nov 2021, at 9:49, Trond Myklebust wrote:

> On Tue, 2021-11-16 at 09:34 -0500, Benjamin Coddington wrote:
>> On 16 Nov 2021, at 9:17, Trond Myklebust wrote:
>>
>>> On Tue, 2021-11-16 at 09:12 -0500, Benjamin Coddington wrote:
>>>> On 16 Nov 2021, at 9:03, Trond Myklebust wrote:
>>>>> No, we really shouldn't need to care what the server thinks or
>>>>> does.
>>>>> The client is authoritative for the change attribute while it
>>>>> holds
>>>>> a
>>>>> delegation, not the server.
>>>>
>>>> My understanding of the intention of the code (which I had to
>>>> sort of
>>>> put
>>>> together from historical patches in this area) is that we want to
>>>> see
>>>> ctime,
>>>> mtime, and block size updates after copy/clone even if we hold a
>>>> delegation,
>>>> but without NFS_INO_INVALID_CHANGE, the client won't update those
>>>> attributes.
>>>>
>>>> If that's not necessary, we can drop this patch.
>>>>
>>>
>>> We will still see the ctime/mtime/block size updates even if
>>> NFS_INO_INVALID_CHANGE is not set. Those attributes' cache status
>>> are
>>> tracked separately through their own NFS_INO_INVALID_* bits.
>>>
>>> That said, there really is no reason why we shouldn't treat the
>>> copy
>>> and clone code exactly the same way we would treat a regular write.
>>> Perhaps we can fix up the arguments of nfs_writeback_update_inode()
>>> so
>>> that it can be called here?
>>
>> I wonder if that just kicks the problem down the road to when we
>> return the
>> delegation, and we'll see cases where ctime/mtime move backward.  And
>> I
>> don't think we can ever be authoritative for space_used, but maybe it
>> doesn't
>> matter if we're within the attribute timeouts.
>>
>
> I don't understand your point. Mine is that we _should_ be setting the
> NFS_INO_INVALID_MTIME, NFS_INO_INVALID_CTIME, NFS_INO_INVALID_BLOCKS...
> flags here, and as far as I can tell, we are indeed doing so in
> nfs42_copy_dest_done().

Yes, -- I misunderstood your suggestion to use nfs_writeback_update_inode()
as a way to fill in our cached attributes with values we'd expect from the
result of the copy.

> At least for clone(), we're then calling nfs_post_op_update_inode(),
> which updates the attributes with whatever was retrieved in the CLONE
> call. That will usually contain new values for mtime, ctime and blocks,
> and so the cache is refreshed.
>
> If the client failed to retrieve attributes as part of the CLONE or
> COPY call, then we are indeed kicking the can of revalidating the
> attributes down the road, but only as far as until the next call to
> nfs_getattr(), or the delegation return, whichever comes first. The
> fact that we do set the NFS_INO_INVALID_MTIME, ... means that we will
> update those cached values before replying to a stat() or statx() call.

Ok, thanks for the looks at this, you've convinced me that this patch is
unnecessary.  I still need the first one, let me know if you want me to
resend it as a stand-alone fix.

Ben

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

[Trond Myklebust]

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