On Mon, 2017-05-22 at 19:53 +0200, Jan Kara wrote: > On Mon 22-05-17 09:53:21, Jeff Layton wrote: > > On Mon, 2017-05-22 at 15:38 +0200, Jan Kara wrote: > > > 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... > > > > > > > Actually, I think that may be problematic... > > > > We could end up calling ext2_write_inode with sync_mode != WB_SYNC_ALL, > > which just dirties the buffer without starting writeback. Then, have VM > > subsystem write back the buffer due to memory pressure and have that > > fail. Trying to set the error in write_inode would miss that situation. > > Two notes here: > > 1) Inode is a bad example because there isn't 1:1 mapping between buffers > containing inodes and mappings - one buffer contains several inodes. > I wanted to add that for inodes specifically it does not matter as they get > special handling but actually fsync seems to be currently unreliable for > them - if we first wrote them in WB_SYNC_NONE mode, they will be just > written in bdev's page cache, but following fsync(2) will do nothing as > they will be clean. Anyway, this is unrelated problem. > Yes, that's what I was trying to articulate above. I'm not sure it's unrelated though. Moving to errseq_t based handling there based on the blockdev mapping seems like it'd solve that. That does require an extra errseq_t though. (I assume that on ext2 inode writeback, bh->b_page->mapping->host points to the bdev inode?) > 2) For metadata like indirect blocks where you indeed have 1:1 mapping, you > can do the error setting in ->end_io handler based on bh->b_assoc_map and > that should do what you need, shouldn't it? > That would probably work, and I think the mark_buffer_write_io_error function that I was adding should already be doing the right thing there. > If I'm indeed right, then for buffers which have 1:1 mapping we are fine > and if we find a solution for inodes, we could avoid the second errseq_t. > Yeah, I'm just still not seeing a good way to track error in inode metadata writeback without an extra errseq_t though. I don't suppose that a buffer holding inode metadata has a list of those inodes, does it? Then we could walk the list and flag each one with the error. Without something like that, I think we're stuck with an extra errseq_t. BTW, thanks for the help so far. I haven't spent much time in local fs metadata handling up until now, so this has been very helpful. -- Jeff Layton <jlayton@xxxxxxxxxx>
