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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
[email protected]
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.
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
[email protected]
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.
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
[email protected]
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