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

Commit Message

Jeff Layton July 24, 2023, 2:53 p.m. UTC

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 <[e-mail]>
Signed-off-by: Jeff Layton <[e-mail]>
---
 fs/nfsd/nfs4proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 97a5d0146ef443df148805a4e9c3c44111f14ab1
change-id: 20230724-bz2223560-5ed6bc3a5db7

Best regards,

Comments

Chuck Lever July 24, 2023, 3:20 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Jeff Layton <[e-mail]>
> ---
>  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.


> 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 <[e-mail]>
>

Jeff Layton July 24, 2023, 4:21 p.m. UTC | #2

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 <[e-mail]>
> > Signed-off-by: Jeff Layton <[e-mail]>
> > ---
> >  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.

> > 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 <[e-mail]>
> > 
>

Chuck Lever July 24, 2023, 4:44 p.m. UTC | #3

> On Jul 24, 2023, at 12:21 PM, Jeff Layton <[e-mail]> 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 <[e-mail]>
>>> Signed-off-by: Jeff Layton <[e-mail]>
>>> ---
>>> 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 <[e-mail]>
>>> 
>> 
> 
> -- 
> Jeff Layton <[e-mail]>


--
Chuck Lever

NeilBrown July 24, 2023, 11:15 p.m. UTC | #4

On Tue, 25 Jul 2023, 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 <[e-mail]>
> Signed-off-by: Jeff Layton <[e-mail]>
> ---
>  fs/nfsd/nfs4proc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;
>  }

Thanks for this Jeff.
Only problem is that the comment above this code is now even more wrong
than it was before :-)

I cannot convincingly argue that having the "+1" is better than not (as
Chuck wondered about), but I think "0" is completely arbitrary, while
->before_change+1 is the sort of value we might reasonably expect.

Thanks,
NeilBrown


>  
>  static __be32
> 
> ---
> base-commit: 97a5d0146ef443df148805a4e9c3c44111f14ab1
> change-id: 20230724-bz2223560-5ed6bc3a5db7
> 
> Best regards,
> -- 
> Jeff Layton <[e-mail]>
> 
>

Patch

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