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@xxxxxxxxxx>
