> 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
