Re: [PATCH RFC] nfsd: set missing after_change as before_change + 1

[Chuck Lever III]

> On Jul 24, 2023, at 12:21 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> 
> On Mon, 2023-07-24 at 11:20 -0400, Chuck Lever wrote:
>> On Mon, Jul 24, 2023 at 10:53:39AM -0400, Jeff Layton wrote:
>>> In the event that we can't fetch post_op_attr attributes, we still need
>>> to set a value for the after_change. The operation has already happened,
>>> so we're not able to return an error at that point, but we do want to
>>> ensure that the client knows that its cache should be invalidated.
>>> 
>>> If we weren't able to fetch post-op attrs, then just set the
>>> after_change to before_change + 1. The atomic flag should already be
>>> clear in this case.
>>> 
>>> Suggested-by: Neil Brown <neilb@xxxxxxx>
>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>> ---
>>> fs/nfsd/nfs4proc.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> I'm not sure this change makes any difference. The client would
>> possibly see the change value move forward then back. I'd think a
>> false "atomic" field and using the /same/ pre- and post-change would
>> be safer...?
>> 
>> But I'm intrigued enough to apply this to nfsd-next provisionally,
>> at least for testing and further review. It will appear a little
>> later today.
>> 
>> 
> 
> Thanks. I think there really are no great choices here.
> 
> This is a rather unlikely error case that should only come into play
> when there are problems with the underlying filesystem, but only when
> fetching the post-op attrs.
> 
> We don't have a way to just opt out of providing a post-op attribute and
> I think this is probably the least bad option of what to put in there.

No argument, it's a rock-and-hard-place thing.

There doesn't seem to be a way of testing this except
with fault injection.

Any client implementer that has an opinion about our
choice of post-change value (zero versus pre-change
versus pre-change-plus-one), please chime in.


>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 3f6710c9c5c9..f0f318e78630 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -411,7 +411,7 @@ set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp)
>>> if (WARN_ON_ONCE(!fhp->fh_pre_saved))
>>> cinfo->before_change = 0;
>>> if (!fhp->fh_post_saved)
>>> - cinfo->after_change = 0;
>>> + cinfo->after_change = cinfo->before_change + 1;
>>> }
>>> 
>>> static __be32
>>> 
>>> ---
>>> base-commit: 97a5d0146ef443df148805a4e9c3c44111f14ab1
>>> change-id: 20230724-bz2223560-5ed6bc3a5db7
>>> 
>>> Best regards,
>>> -- 
>>> Jeff Layton <jlayton@xxxxxxxxxx>
>>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>


--
Chuck Lever