Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <11c799a6cb0bf073dda77f592d70d809fca9b030.camel@kernel.org>
Subject: Re: [PATCH v2 0/2] nfsd: sanely handle inabilty to fetch pre/post
 attributes
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>
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
Date:   Fri, 21 Jul 2023 08:48:16 -0400
In-Reply-To: <168988936713.11078.5407820394334916284@noble.neil.brown.name>
References: <20230720-bz2223560-v2-0-070aaf2660b7@kernel.org>
         <168988936713.11078.5407820394334916284@noble.neil.brown.name>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-07-21 at 07:42 +1000, NeilBrown wrote:
> On Fri, 21 Jul 2023, Jeff Layton wrote:
> > Boyang reported tripping the BUG_ON in set_change_info. While we
> > couldn't confirm it, one way this could happen would be for nfsd_lookup
> > to succeed and then for fh_fill_both_attrs to fail.
> >=20
> > This patchset attempts to (sanely) fix this, usually by aborting the
> > operation if fetching the pre attributes fails. Post-op attribute fetch
> > handling is more difficult to deal with however since we've already don=
e
> > the operation, so this has it just fudge the change_info4 if that
> > occurs.
>=20
> 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.
>=20

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;
};

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).

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.

--=20
Jeff Layton <jlayton@kernel.org>