Message-ID: <3d8a093794e943bd5b6b53b58654fd263329c611.camel@kernel.org>
Subject: Re: [PATCH 3/3] nfsd: simplify write verifier handling
From:   Jeff Layton <jlayton@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Trond Myklebust <trondmy@kernel.org>,
        Rick Macklem <rick.macklem@gmail.com>,
        Matthew Wilcox <willy@infradead.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Tue, 14 Feb 2023 10:01:57 -0500
In-Reply-To: <2EB94DDA-0894-40C7-925B-C0068DEA577C@oracle.com>
References: <20230213211345.385005-1-jlayton@kernel.org>
         <20230213211345.385005-4-jlayton@kernel.org>
         <CAM5tNy56at7gvUBbc1T7ay=NZQ08qOaAxdux2ZB70sZdb3L0xw@mail.gmail.com>
         <5e19458b1eba1dc4c187d14ec0c74547acb6a2a2.camel@kernel.org>
         <bd23b795b30c5640a8c7ebbe98cee048cc4022be.camel@kernel.org>
         <2EB94DDA-0894-40C7-925B-C0068DEA577C@oracle.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-02-14 at 14:58 +0000, Chuck Lever III wrote:
>=20
> > On Feb 14, 2023, at 8:53 AM, Jeff Layton <jlayton@kernel.org> wrote:
> >=20
> > On Mon, 2023-02-13 at 22:28 -0500, Trond Myklebust wrote:
> > > On Mon, 2023-02-13 at 16:49 -0800, Rick Macklem wrote:
> > > > On Mon, Feb 13, 2023 at 1:14 PM Jeff Layton <jlayton@kernel.org>
> > > > wrote:
> > > > >=20
> > > > > CAUTION: This email originated from outside of the University of
> > > > > Guelph. Do not click links or open attachments unless you recogni=
ze
> > > > > the sender and know the content is safe. If in doubt, forward
> > > > > suspicious emails to IThelp@uoguelph.ca
> > > > >=20
> > > > >=20
> > > > > The write verifier exists to tell the client when the server may
> > > > > have
> > > > > forgotten some unstable writes. The typical way that this happens
> > > > > is if
> > > > > the server crashes, but we've also extended nfsd to change it whe=
n
> > > > > there
> > > > > are writeback errors as well.
> > > > >=20
> > > > > The way it works today though, we call something like vfs_fsync
> > > > > (e.g.
> > > > > for a COMMIT call) and if we get back an error, we'll reset the
> > > > > write
> > > > > verifier.
> > > > >=20
> > > > > This is non-optimal for a couple of reasons:
> > > > >=20
> > > > > 1/ There could be significant delay between an error being
> > > > > recorded and the reset. It would be ideal if the write verifier
> > > > > were to
> > > > > change as soon as the error was recorded.
> > > > >=20
> > > > > 2/ It's a bit of a waste, in that if we get a writeback error on =
a
> > > > > single inode, we'll end up resetting the write verifier for
> > > > > everything,
> > > > > even on inodes that may be fine (e.g. on a completely separate fs=
).
> > > > >=20
> > > > Here's the snippet from RFC8881:
> > > >    The final portion of the result is the field writeverf.  This
> > > > field
> > > >    is the write verifier and is a cookie that the client can use to
> > > >    determine whether a server has changed instance state (e.g.,
> > > > server
> > > >    restart) between a call to WRITE and a subsequent call to either
> > > >    WRITE or COMMIT.  This cookie MUST be unchanged during a single
> > > >    instance of the NFSv4.1 server and MUST be unique between
> > > > instances
> > > >    of the NFSv4.1 server.  If the cookie changes, then the client
> > > > MUST
> > > >    assume that any data written with an UNSTABLE4 value for committ=
ed
> > > >    and an old writeverf in the reply has been lost and will need to
> > > > be
> > > >    recovered.
> > > >=20
> > > > I've always interpreted the writeverf as "per-server" and not  "per=
-
> > > > file".
> > > > Although I'll admit the above does not make that crystal clear, it
> > > > does make
> > > > it clear that the writeverf applies to a "server instance" and not =
a
> > > > file or
> > > > file system on the server.
> > > >=20
> > > > The FreeBSD client assumes it is "per-server" and re-writes all
> > > > uncommitted
> > > > writes for the server, not just ones for the file (or file system)
> > > > the
> > > > writeverf is
> > > > replied with.  (I vaguely recall Solaris does the same?)
> > > >=20
> > > > At the very least, I think you should run this past the IETF workin=
g
> > > > group
> > > > (nfsv4@ietf.org) to see what they say w.r.t. the writeverf being
> > > > "per-file" vs
> > > > "per-server".
> > > >=20
> > >=20
> > > As I recall, we've already had this discussion on the IETF NFSv4
> > > working group mailing list:
> > > https://mailarchive.ietf.org/arch/msg/nfsv4/99Ow2muMylXKWd9lzi9_BX2LJ=
DY/
> > >=20
> > >=20
> > > That's why I kept it a global in the first place.
> > >=20
> > > Now note that RFC8881 does also clarify in Section 18.3.3 that:
> > >=20
> > >=20
> > >   The server must vary the value of the write
> > >   verifier at each server event or instantiation that may lead to a
> > >   loss of uncommitted data.  Most commonly this occurs when the serve=
r
> > >   is restarted; however, other events at the server may result in
> > >   uncommitted data loss as well.
> > >=20
> > > So I feel it is quite OK to use the verifier the way we do now in ord=
er
> > > to signify that a fatal write error has occurred and that clients mus=
t
> > > resend any data that was uncommitted.
> > >=20
> >=20
> > Thanks, I missed that discussion. I think you guys have convinced me
> > that we have to keep this per-server. I won't bother starting a new
> > thread on it.
> >=20
> > It's a pity. It would have been a lot more elegant as a per-inode thing=
!
> >=20
> > Chuck, I think that means we'll just want to keep patch #1 in this=20
> > series?
>=20
> Regarding patch 1/3:
>=20
> "sizeof(verf)" works as well as "sizeof(*verf) * 2" and is a little
> more clear to boot. You can redrive a v2 of your patch or I can make
> one. Up to you.
>=20

That sounds fine to me. Go for it!
--=20
Jeff Layton <jlayton@kernel.org>