On Mon, 2019-08-26 at 20:48 -0400, bfields@xxxxxxxxxxxx wrote: > On Mon, Aug 26, 2019 at 09:02:31PM +0000, Trond Myklebust wrote: > > On Mon, 2019-08-26 at 16:51 -0400, J. Bruce Fields wrote: > > > On Mon, Aug 26, 2019 at 12:50:18PM -0400, Trond Myklebust wrote: > > > > Recently, a number of changes went into the kernel to try to > > > > ensure that I/O errors (specifically write errors) are reported > > > > to > > > > the application once and only once. The vehicle for ensuring > > > > the > > > > errors are reported is the struct file, which uses the > > > > 'f_wb_err' > > > > field to track which errors have been reported. > > > > > > > > The problem is that errors are mainly intended to be reported > > > > through fsync(). If the client is doing synchronous writes, > > > > then > > > > all is well, but if it is doing unstable writes, then the > > > > errors > > > > may not be reported until the client calls COMMIT. If the file > > > > cache has thrown out the struct file, due to memory pressure, > > > > or > > > > just because the client took a long while between the last > > > > WRITE > > > > and the COMMIT, then the error report may be lost, and the > > > > client > > > > may just think its data is safely stored. > > > > > > These were lost before the file caching patches as well, > > > right? Or > > > is there some regression? > > > > Correct. This is not a regression, but an attempt to fix a problem > > that has existed for some time now. > > > > > > Note that the problem is compounded by the fact that NFSv3 is > > > > stateless, so the server never knows that the client may have > > > > rebooted, so there can be no guarantee that a COMMIT will ever > > > > be > > > > sent. > > > > > > > > The following patch set attempts to remedy the situation using > > > > 2 > > > > strategies: > > > > > > > > 1) If the inode is dirty, then avoid garbage collecting the > > > > file > > > > from the file cache. 2) If the file is closed, and we see that > > > > it > > > > would have reported an error to COMMIT, then we bump the boot > > > > verifier in order to ensure the client retransmits all its > > > > writes. > > > > > > Sounds sensible to me. > > > > > > > Note that if multiple clients were writing to the same file, > > > > then > > > > we probably want to bump the boot verifier anyway, since only > > > > one > > > > COMMIT will see the error report (because the cached file is > > > > also > > > > shared). > > > > > > I'm confused by the "probably should". So that's future work? I > > > guess it'd mean some additional work to identify that case. You > > > can't really even distinguish clients in the NFSv3 case, but I > > > suppose you could use IP address or TCP connection as an > > > approximation. > > > > I'm suggesting we should do this too, but I haven't done so yet in > > these patches. I'd like to hear other opinions (particularly from > > you, > > Chuck and Jeff). > > Does this process actually converge, or do we end up with all the > clients retrying the writes and, again, only one of them getting the > error? The client that gets the error should stop retrying if the error is fatal. > I wonder what the typical errors are, anyway. I would expect ENOSPC, and EIO to be the most common. The former if delayed allocation and/or snapshots result in writes failing after writing to the page cache. The latter if we hit a disk outage or other such problem. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
