Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2023-07-22 at 10:34 +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > On Fri, 2023-07-21 at 07:42 +1000, NeilBrown wrote:
> > > 
> > > I think both v3 and v4 allow a reply that says "the operation was a
> > > success but there are no post-op attrs".  With v4 you can say "there is
> > > no change-attr, but here are some other attrs".  I think.
> > > 
> > 
> > v3 has this ability:
> > 
> >       union pre_op_attr switch (bool attributes_follow) {
> >       case TRUE:
> >            wcc_attr  attributes;
> >       case FALSE:
> >            void;
> >       };
> > 
> > ...we can just set the attributes_follow flag to false there in that
> > case.
> > 
> > That's not possible with v4, AFAICT. Several of the *4resok structures
> > contain a change_info4, which just looks like this:
> > 
> > struct change_info4 {
> >         bool            atomic;
> >         changeid4       before;
> >         changeid4       after;
> > };
> 
> Yes...  I was thinking of GETATTR which reports a bitmap of all the
> attributes that it can return.  Though I'm not sure if the server is
> "allowed" to not return something that it has said is "supported".  And
> I think changeid has to be "supported".  I'm not sure.
>
> But anyway, that doesn't help change_info4 which comes with
> directory-modifying operation.
> 
> > 
> > We can set "atomic" to false (and this patch does that in this
> > situation), but I don't believe there is any alternative to the change
> > attribute. If the underlying fs doesn't support native change attrs, the
> > server is expected to fake one up somehow (usually from the ctime).
> 
> I had a look again at the current code and your patch, and I think that
> if the "post' vfs_getattr() fails, then the operation succeeds, the
> change_info is marked non-atomic (as you say) and the "after" changeid is
> set to an uninitialised value.  Is that right?  Did I miss something?
> Maybe we should set it to the pre value plus 1.
> 
> It probably doesn't matter at all in practice, but if I'm right and it
> is using an uninitialized value, we should at least fix that.
> 
> Thanks - your v3 patch looks good in general.  I like the must_check and
> the goto structure.
> 
> Thanks,
> NeilBrown
> 
> 


The current patch sets the missing pre/post values to 0. I'm happy to
change that to pre-value+1 though if you think that'd be more correct.
The client already fudges the changeid like that in the CB_GETATTR case,
so I doubt that would break anything.
-- 
Jeff Layton <jlayton@kernel.org>