On Mon, 2017-03-06 at 14:06 +1100, NeilBrown wrote: > On Sun, Mar 05 2017, Jeff Layton wrote: > > > I recently did some work to wire up -ENOSPC handling in ceph, and found > > I could get back -EIO errors in some cases when I should have instead > > gotten -ENOSPC. The problem was that the ceph writeback code would set > > PG_error on a writeback error, and that error would clobber the mapping > > error. > > > > While I fixed that problem by simply not setting that bit on errors, > > that led me down a rabbit hole of looking at how PG_error is being > > handled in the kernel. > > Speaking of rabbit holes... I thought to wonder how IO error propagate > up from NFS. > It doesn't use SetPageError or mapping_set_error() for files (except in > one case that looks a bit odd). > It has an "nfs_open_context" and store the latest error in ctx->error. > > So when you get around to documenting how this is supposed to work, it > would be worth while describing the required observable behaviour, and > note that while filesystems can use mapping_set_error() to achieve this, > they don't have to. > > I notice that > drivers/staging/lustre/lustre/llite/rw.c > fs/afs/write.c > fs/btrfs/extent_io.c > fs/cifs/file.c > fs/jffs2/file.c > fs/jfs/jfs_metapage.c > fs/ntfs/aops.c > > (and possible others) all have SetPageError() calls that seem to be > in response to a write error to a file, but don't appear to have > matching mapping_set_error() calls. Did you look at these? Did I miss > something? > Those are all in writepage implementations, and the callers of writepage all call mapping_set_error if it returns error. The exception is write_one_page, which is typically used for writing out dir info and such, and so it's not really necessary there. Now that I look though, I think I may have gotten the page migration codepath wrong. I had convinced myself it was ok before, but looking again, I think we probably need to add a mapping_set_error call to writeout() as well. I'll go over that more carefully in a little while. > > > > This patch series is a few fixes for things that I 100% noticed by > > inspection. I don't have a great way to test these since they involve > > error handling. I can certainly doctor up a kernel to inject errors > > in this code and test by hand however if these look plausible up front. > > > > Jeff Layton (3): > > nilfs2: set the mapping error when calling SetPageError on writeback > > mm: don't TestClearPageError in __filemap_fdatawait_range > > mm: set mapping error when launder_pages fails > > > > fs/nilfs2/segment.c | 1 + > > mm/filemap.c | 19 ++++--------------- > > mm/truncate.c | 6 +++++- > > 3 files changed, 10 insertions(+), 16 deletions(-) > > > > -- > > 2.9.3 -- Jeff Layton <jlayton@xxxxxxxxxx>
