On Tue, 2023-02-14 at 14:58 +0000, Chuck Lever III wrote: > > > On Feb 14, 2023, at 8:53 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > 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@xxxxxxxxxx> > > > > wrote: > > > > > > > > > > CAUTION: This email originated from outside of the University of > > > > > Guelph. Do not click links or open attachments unless you recognize > > > > > the sender and know the content is safe. If in doubt, forward > > > > > suspicious emails to IThelp@xxxxxxxxxxx > > > > > > > > > > > > > > > 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 when > > > > > there > > > > > are writeback errors as well. > > > > > > > > > > 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. > > > > > > > > > > This is non-optimal for a couple of reasons: > > > > > > > > > > 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. > > > > > > > > > > 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). > > > > > > > > > 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 committed > > > > and an old writeverf in the reply has been lost and will need to > > > > be > > > > recovered. > > > > > > > > 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. > > > > > > > > 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?) > > > > > > > > At the very least, I think you should run this past the IETF working > > > > group > > > > (nfsv4@xxxxxxxx) to see what they say w.r.t. the writeverf being > > > > "per-file" vs > > > > "per-server". > > > > > > > > > > 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_BX2LJDY/ > > > > > > > > > That's why I kept it a global in the first place. > > > > > > Now note that RFC8881 does also clarify in Section 18.3.3 that: > > > > > > > > > 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 server > > > is restarted; however, other events at the server may result in > > > uncommitted data loss as well. > > > > > > So I feel it is quite OK to use the verifier the way we do now in order > > > to signify that a fatal write error has occurred and that clients must > > > resend any data that was uncommitted. > > > > > > > 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. > > > > It's a pity. It would have been a lot more elegant as a per-inode thing! > > > > Chuck, I think that means we'll just want to keep patch #1 in this > > series? > > Regarding patch 1/3: > > "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. > That sounds fine to me. Go for it! -- Jeff Layton <jlayton@xxxxxxxxxx>
