On Fri 19-05-17 15:20:52, Jeff Layton wrote: > On Mon, 2017-05-15 at 12:42 +0200, Jan Kara wrote: > > On Tue 09-05-17 11:49:18, Jeff Layton wrote: > > > Now that we have a better way to store and report errors that occur > > > during writeback, we need to convert the existing codebase to use it. We > > > could just adapt all of the filesystem code and related infrastructure > > > to the new API, but that's a lot of churn. > > > > > > When it comes to setting errors in the mapping, filemap_set_wb_error is > > > a drop-in replacement for mapping_set_error. Turn that function into a > > > simple wrapper around the new one. > > > > > > Because we want to ensure that writeback errors are always reported at > > > fsync time, inject filemap_report_wb_error calls much closer to the > > > syscall boundary, in call_fsync. > > > > > > For fsync calls (and things like the nfsd equivalent), we either return > > > the error that the fsync operation returns, or the one returned by > > > filemap_report_wb_error. In both cases, we advance the file->f_wb_err to > > > the latest value. This allows us to provide new fsync semantics that > > > will return errors that may have occurred previously and been viewed > > > via other file descriptors. > > > > > > The final piece of the puzzle is what to do about filemap_check_errors > > > calls that are being called directly or via filemap_* functions. Here, > > > we must take a little "creative license". > > > > > > Since we now handle advancing the file->f_wb_err value at the generic > > > filesystem layer, we no longer need those callers to clear errors out > > > of the mapping or advance an errseq_t. > > > > > > A lot of the existing codebase relies on being getting an error back > > > from those functions when there is a writeback problem, so we do still > > > want to have them report writeback errors somehow. > > > > > > When reporting writeback errors, we will always report errors that have > > > occurred since a particular point in time. With the old writeback error > > > reporting, the time we used was "since it was last tested/cleared" which > > > is entirely arbitrary and potentially racy. Now, we can at least report > > > the latest error that has occurred since an arbitrary point in time > > > (represented as a sampled errseq_t value). > > > > > > In the case where we don't have a struct file to work with, this patch > > > just has the wrappers sample the current mapping->wb_err value, and use > > > that as an arbitrary point from which to check for errors. > > > > I think this is really dangerous and we shouldn't do this. You are quite > > likely to lose IO errors in such calls because you will ignore all errors > > that happened during previous background writeback or even for IO that > > managed to complete before we called filemap_fdatawait(). Maybe we need to > > keep the original set-clear-bit IO error reporting for these cases, until > > we can convert them to fdatawait_range_since()? > > > > > That's probably not "correct" in all cases, particularly in the case of > > > something like filemap_fdatawait, but I'm not sure it's any worse than > > > what we already have, and this gives us a basis from which to work. > > > > > > A lot of those callers will likely want to change to a model where they > > > sample the errseq_t much earlier (perhaps when starting a transaction), > > > store it in an appropriate place and then use that value later when > > > checking to see if an error occurred. > > > > > > That will almost certainly take some involvement from other subsystem > > > maintainers. I'm quite open to adding new API functions to help enable > > > this if that would be helpful, but I don't really want to do that until > > > I better understand what's needed. > > > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > > > ... > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > index 5f7317875a67..7ce13281925f 100644 > > > --- a/fs/f2fs/file.c > > > +++ b/fs/f2fs/file.c > > > @@ -187,6 +187,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > > > .nr_to_write = LONG_MAX, > > > .for_reclaim = 0, > > > }; > > > + errseq_t since = READ_ONCE(file->f_wb_err); > > > > > > if (unlikely(f2fs_readonly(inode->i_sb))) > > > return 0; > > > @@ -265,6 +266,8 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > > > } > > > > > > ret = wait_on_node_pages_writeback(sbi, ino); > > > + if (ret == 0) > > > + ret = filemap_check_wb_error(NODE_MAPPING(sbi), since); > > > if (ret) > > > goto out; > > > > So this conversion looks wrong and actually points to a larger issue with > > the scheme. The problem is there are two mappings that come into play here > > - file_inode(file)->i_mapping which is the data mapping and > > NODE_MAPPING(sbi) which is the metadata mapping (and this is not a problem > > specific to f2fs. For example ext2 also uses this scheme where block > > devices' mapping is the metadata mapping). And we need to merge error > > information from these two mappings so for the stamping scheme to work, > > we'd need two stamps stored in struct file. One for data mapping and one > > for metadata mapping. Or maybe there's some more clever scheme but for now > > I don't see one... > > > > Honza > > In the case of something like ext2, could we instead get away with just > marking the data mapping of the inode with an error if the metadata > writeout fails? > > Then we could just have write_inode operations call mapping_set_error on > inode->i_mapping when they're going to return an error. That should be > functionally equivalent, I'd think. > > The catch there is that that requires a 1:1 data:metadata mapping, and > I'm not sure that that is the case (or will always be, even if it is > now). So for ext2 / ext4 in nojournal mode this should work - we track all relevant metadata in mapping->private_list. But I cannot really comment on other filesystems like f2fs... Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
