On Mon, 2017-04-03 at 10:12 +0300, Nikolay Borisov wrote: > > On 31.03.2017 22:26, Jeff Layton wrote: > > Most filesystems currently use mapping_set_error and > > filemap_check_errors for setting and reporting/clearing writeback errors > > at the mapping level. filemap_check_errors is indirectly called from > > most of the filemap_fdatawait_* functions and from > > filemap_write_and_wait*. These functions are called from all sorts of > > contexts to wait on writeback to finish -- e.g. mostly in fsync, but > > also in truncate calls, getattr, etc. > > > > It's those non-fsync callers that are problematic. We should be > > reporting writeback errors during fsync, but many places in the code > > clear out errors before they can be properly reported, or report errors > > at nonsensical times. If I get -EIO on a stat() call, how do I know that > > was because writeback failed? > > > > This patch adds a small bit of new infrastructure for setting and > > reporting errors during pagecache writeback. While the above was my > > original impetus for adding this, I think it's also the case that > > current fsync semantics are just problematic for userland. Most > > applications that call fsync do so to ensure that the data they wrote > > has hit the backing store. > > > > In the case where there are multiple writers to the file at the same > > time, this is really hard to determine. The first one to call fsync will > > see any stored error, and the rest get back 0. The processes with open > > fd may not be associated with one another in any way. They could even be > > in different containers, so ensuring coordination between all fsync > > callers is not really an option. > > > > One way to remedy this would be to track what file descriptor was used > > to dirty the file, but that's rather cumbersome and would likely be > > slow. However, there is a simpler way to improve the semantics here > > without incurring too much overhead. > > > > This set adds a wb_error field and a sequence counter to the > > address_space, and a corresponding sequence counter in the struct file. > > When errors are reported during writeback, we set the error field in the > > mapping and increment the sequence counter. > > > > When fsync or flush is called, we check the sequence in the file vs. the > > one in the mapping. If the file's counter is behind the one in the > > mapping, then we update the sequence counter in the file to the value of > > the one in the mapping and report the error. If the file is "caught up" > > then we just report 0. > > > > This changes the semantics of fsync such that applications can now use > > it to determine whether there were any writeback errors since fsync(fd) > > was last called (or since the file was opened in the case of fsync > > having never been called). > > > > Note that those writeback errors may have occurred when writing data > > that was dirtied via an entirely different fd, but that's the case now > > with the current mapping_set_error/filemap_check_error infrastructure. > > This will at least prevent you from getting a false report of success. > > > > The basic idea here is for filesystems to use filemap_set_wb_error to > > set the error in the mapping when there are writeback errors, and then > > have the fsync and flush operations call filemap_report_wb_error just > > before returning to ensure that those errors get reported properly. > > > > Eventually, it may make sense to move the reporting into the generic > > vfs_fsync_range helper, but doing it this way for now makes it simpler > > to convert filesystems to the new API individually. > > There is already a mapping_set_error API which sets flags in > mapping->flags (AS_EIO/AS_ENOSPC). Aren't you essentially duplicating > some of the semantics of that API ? Yes, more or less for now. The arguments of mapping_set_error and filemap_set_wb_error are the same, but they do different things with the error. The plan is eventually to eliminate mapping_set_error and convert everything over to use the new infrastructure I'm adding. That's difficult to do all at once however, so for now some duplication is necessary. -- Jeff Layton <jlayton@xxxxxxxxxx>
