Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Jeff Layton" <jlayton@kernel.org>
Cc:     "Chuck Lever" <chuck.lever@oracle.com>,
        "Olga Kornievskaia" <kolga@netapp.com>,
        "Dai Ngo" <Dai.Ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>,
        "Boyang Xue" <bxue@redhat.com>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post attributes
In-reply-to: <11c799a6cb0bf073dda77f592d70d809fca9b030.camel@kernel.org>
References: <20230720-bz2223560-v2-0-070aaf2660b7@kernel.org>,
 <168988936713.11078.5407820394334916284@noble.neil.brown.name>,
 <11c799a6cb0bf073dda77f592d70d809fca9b030.camel@kernel.org>
Date:   Sat, 22 Jul 2023 10:34:01 +1000
Message-id: <168998604179.11078.18238251274062077853@noble.neil.brown.name>
Precedence: bulk

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


> 
> We could (in principle) allow the operation to proceed on v3 even if
> fh_fill_pre_attrs fails, but I don't think we can do the same thing with
> v4. That said, if getattr is failing then it's somewhat likely that
> other operations will fail too, so aborting the operation in this
> situation doesn't seem too onerous.
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
>