On Tue, 2017-03-07 at 11:26 +0100, Jan Kara wrote:
> On Mon 06-03-17 16:08:01, Ross Zwisler wrote:
> > On Sun, Mar 05, 2017 at 09:40:54AM -0500, Jeff Layton wrote:
> > > On Sun, 2017-03-05 at 08:35 -0500, 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.
> > > >
> > >
> > > I should also note that relying on PG_error to report writeback errors
> > > is inherently unreliable as well. If someone calls sync() before your
> > > fsync gets in there, then you'll likely lose it anyway.
> > >
> > > filemap_fdatawait_keep_errors will preserve the error in the mapping,
> > > but not the individual PG_error flags, so I think we do want to ensure
> > > that the mapping error is set when there is a writeback error and not
> > > rely on PG_error bit for that.
> > >
> > > > 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.
> > > >
> > > > 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(-)
> > > >
> > >
> > > (cc'ing Ross...)
> > >
> > > Just when I thought that only NILFS2 needed a little work here, I see
> > > another spot...
> > >
> > > I think that we should also need to fix dax_writeback_mapping_range to
> > > set a mapping error on writeback as well. It looks like that's not
> > > happening today. Something like the patch below (obviously untested).
> > >
> > > I'll also plan to follow up with a patch to vfs.txt to outline how
> > > writeback errors should be handled by filesystems, assuming that this
> > > patchset isn't completely off base.
> > >
> > > -------------------8<-----------------------
> > >
> > > [PATCH] dax: set error in mapping when writeback fails
> > >
> > > In order to get proper error codes from fsync, we must set an error in
> > > the mapping range when writeback fails.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > > fs/dax.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/dax.c b/fs/dax.c
> > > index c45598b912e1..9005d90deeda 100644
> > > --- a/fs/dax.c
> > > +++ b/fs/dax.c
> > > @@ -888,8 +888,10 @@ int dax_writeback_mapping_range(struct address_space *mapping,
> > >
> > > ret = dax_writeback_one(bdev, mapping, indices[i],
> > > pvec.pages[i]);
> > > - if (ret < 0)
> > > + if (ret < 0) {
> > > + mapping_set_error(mapping, ret);
> > > return ret;
> > > + }
> >
> > (Adding Jan)
> >
> > I tested this a bit, and for the DAX case at least I don't think this does
> > what you want. The current code already returns -EIO if dax_writeback_one()
> > hits an error, which bubbles up through the call stack and makes the fsync()
> > call in userspace fail with EIO, as we want. With both ext4 and xfs this
> > patch (applied to v4.10) makes it so that we fail the current fsync() due to
> > the return value of -EIO, then we fail the next fsync() as well because only
> > then do we actually process the AS_EIO flag inside of filemap_check_errors().
> >
> > I think maybe the missing piece is that our normal DAX fsync call stack
> > doesn't include a call to filemap_check_errors() if we return -EIO. Here's
> > our stack in xfs:
> >
> > dax_writeback_mapping_range+0x32/0x70
> > xfs_vm_writepages+0x8c/0xf0
> > do_writepages+0x21/0x30
> > __filemap_fdatawrite_range+0xc6/0x100
> > filemap_write_and_wait_range+0x44/0x90
> > xfs_file_fsync+0x7a/0x2c0
> > vfs_fsync_range+0x4b/0xb0
> > ? trace_hardirqs_on_caller+0xf5/0x1b0
> > do_fsync+0x3d/0x70
> > SyS_fsync+0x10/0x20
> > entry_SYSCALL_64_fastpath+0x1f/0xc2
> >
> > On the subsequent fsync() call we *do* end up calling filemap_check_errors()
> > via filemap_fdatawrite_range(), which tests & clears the AS_EIO flag in the
> > mapping:
> >
> > filemap_fdatawait_range+0x3b/0x80
> > filemap_write_and_wait_range+0x5a/0x90
> > xfs_file_fsync+0x7a/0x2c0
> > vfs_fsync_range+0x4b/0xb0
> > ? trace_hardirqs_on_caller+0xf5/0x1b0
> > do_fsync+0x3d/0x70
> > SyS_fsync+0x10/0x20
> > entry_SYSCALL_64_fastpath+0x1f/0xc2
> >
> > Was your concern just that you didn't think that fsync() was properly
> > returning an error when dax_writeback_one() hit an error? Or is there another
> > path by which we need to report the error, where it is actually important that
> > we set AS_EIO? If it's the latter, then I think we need to rework the fsync
> > call path so that we both generate and consume AS_EIO on the same call,
> > probably in filemap_write_and_wait_range().
>
> So I believe this is due to the special handling of EIO inside
> filemap_write_and_wait(). Normally, filemap_check_errors() happens inside
> filemap_fdatawait() there however not for EIO returned from
> filemap_fdatawrite(). In that case we bail out immediately. So I think
> Jeff's patch is correct but we need to change filemap_write_and_wait() to
> call also filemap_check_errors() directly on EIO from filemap_fdatawrite().
>
Yes that makes total sense. I've got a filemap_write_and_wait patch in
my pile now that does this. I'll run what I have through an xfstests
run, and see how it does, and will plan to post a v2 set once I do.
> On a more general note (DAX is actually fine here), I find the current
> practice of clearing page dirty bits on error and reporting it just once
> problematic. It keeps the system running but data is lost and possibly
> without getting the error anywhere where it is useful. We get away with
> this because it is a rare event but it seems like a problematic behavior.
> But this is more for the discussion at LSF.
>
That really is the crux of the matter. Unfortunately, that's sort of how
the POSIX write/fsync model is designed. If we want to change that, then
I think that we have to consider what a new interface for this would
look like. Maybe we can do something there with new sync_file_range
flags?
I think this probably also dovetails with Kevin Wolf's proposed LSF
topic too, so maybe we can discuss all of this together there.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
