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? I wonder what the typical errors are, anyway. --b.
