Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629543 > unrolled thread

[PATCH v3 00/20] fs: introduce new writeback error reporting and convert existing API as a wrapper around it

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/20] fs: introduce new writeback error reporting and convert existing API as a wrapper around it Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 09/20] 9p: set mapping error when writeback fails in launder_page Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 09/20] 9p: set mapping error when writeback fails in  launder_page Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 09/20] 9p: set mapping error when writeback fails in  launder_page Jan Kara <jack@suse.cz> - 2017-04-24 18:00 +0200
    [PATCH v3 08/20] mm: ensure that we set mapping error if writeout() fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 08/20] mm: ensure that we set mapping error if  writeout() fails Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 08/20] mm: ensure that we set mapping error if  writeout() fails Jan Kara <jack@suse.cz> - 2017-04-24 18:00 +0200
    [PATCH v3 04/20] fs: check for writeback errors after syncing out buffers in generic_file_fsync Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 04/20] fs: check for writeback errors after syncing  out buffers in generic_file_fsync Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 04/20] fs: check for writeback errors after syncing  out buffers in generic_file_fsync Jan Kara <jack@suse.cz> - 2017-04-24 17:50 +0200
    [PATCH v3 06/20] dax: set errors in mapping when writeback fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
      Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails Jan Kara <jack@suse.cz> - 2017-04-24 18:00 +0200
      Re: [PATCH v3 06/20] dax: set errors in mapping when writeback fails Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-04-24 21:20 +0200
    [PATCH v3 12/20] lib: add errseq_t type and infrastructure for handling it Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 03/20] buffer: use mapping_set_error instead of setting the flag Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jan Kara <jack@suse.cz> - 2017-04-24 18:10 +0200
        Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-24 19:20 +0200
          Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jan Kara <jack@suse.cz> - 2017-04-25 11:50 +0200
            Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-25 12:40 +0200
              Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jan Kara <jack@suse.cz> - 2017-04-25 13:30 +0200
                Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked  when it fails Jeff Layton <jlayton@redhat.com> - 2017-04-25 18:50 +0200
    [PATCH v3 01/20] mm: drop "wait" parameter from write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 01/20] mm: drop "wait" parameter from write_one_page Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
    [PATCH v3 15/20] mm: remove AS_EIO and AS_ENOSPC flags Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 18/20] mm: clean up error handling in write_one_page Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 16/20] mm: don't TestClearPageError in __filemap_fdatawait_range Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 20/20] gfs2: clean up some filemap_* calls Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Bob Peterson <rpeterso@redhat.com> - 2017-04-24 16:20 +0200
        Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Jeff Layton <jlayton@redhat.com> - 2017-04-24 19:00 +0200
          Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Bob Peterson <rpeterso@redhat.com> - 2017-04-24 19:50 +0200
            Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls Jeff Layton <jlayton@redhat.com> - 2017-04-24 20:00 +0200
    [PATCH v3 07/20] nilfs2: set the mapping error when calling SetPageError on writeback Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
      Re: [PATCH v3 07/20] nilfs2: set the mapping error when calling  SetPageError on writeback Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
    [PATCH v3 13/20] fs: new infrastructure for writeback error handling and reporting Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 17/20] cifs: cleanup writeback handling errors and comments Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 19/20] jbd2: don't reset error in journal_finish_inode_data_buffers Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:30 +0200
    [PATCH v3 02/20] mm: fix mapping_set_error call in me_pagecache_dirty Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
    [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait from fsync Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
      Re: [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait  from fsync Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
        Re: [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait from fsync Mike Marshall <hubcap@omnibond.com> - 2017-04-24 20:30 +0200
    [PATCH v3 14/20] fs: retrofit old error reporting API onto new infrastructure Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
    [PATCH v3 11/20] cifs: set mapping error when page writeback fails in writepage or launder_pages Jeff Layton <jlayton@redhat.com> - 2017-04-24 15:40 +0200
      Re: [PATCH v3 11/20] cifs: set mapping error when page writeback  fails in writepage or launder_pages Christoph Hellwig <hch@infradead.org> - 2017-04-24 17:30 +0200
        Re: [PATCH v3 11/20] cifs: set mapping error when page writeback  fails in writepage or launder_pages Jeff Layton <jlayton@redhat.com> - 2017-04-24 19:20 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1630384 — Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

On Tue, 2017-04-25 at 10:17 +0200, Jan Kara wrote:
> On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> > On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > > This ensures that we see errors on fsync when writeback fails.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > 
> > > Hum, but do we really want to clobber mapping errors with temporary stuff
> > > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > > 
> > 
> > Right now we don't really have such a thing as temporary errors in the
> > writeback codepath. If you return an error here, the data doesn't stay
> > dirty or anything, and I think we want to ensure that that gets reported
> > via fsync.
> > 
> > I'd like to see us add better handling for retryable errors for stuff
> > like ENOMEM or EAGAIN. I think this is the first step toward that
> > though. Once we have more consistent handling of writeback errors in
> > general, then we can start doing more interesting things with retryable
> > errors.
> > 
> > So yeah, I this is the right thing to do for now.
> 
> OK, fair enough. And question number 2):
> 
> Who is actually responsible for setting the error in the mapping when error
> happens inside ->writepage()? Is it the ->writepage() callback or the
> caller of ->writepage()? Or something else? Currently it seems to be a
> strange mix (e.g. mm/page-writeback.c: __writepage() calls
> mapping_set_error() when ->writepage() returns error) so I'd like to
> understand what's the plan and have that recorded in the changelogs.
> 

That's an excellent question.

I think we probably want the writepage/launder_page operations to call
mapping_set_error. That makes it possible for filesystems (e.g. NFS) to
handle their own error tracking and reporting without using the new
infrastructure. If they never call mapping_set_error then we'll always
just return whatever their ->fsync operation returns on an fsync.

I'll make another pass through the tree and see whether we have some
mapping_set_error calls that should be removed, and will flesh out
vfs.txt to state this. Maybe that file needs a whole section on
writeback error reporting? Hmmm...

That probably also means that I should drop patch 8 from this series
(mm: ensure that we set mapping error if writeout fails), since that
should be happening in writepage already.

> > 
> > > 
> > > > ---
> > > >  fs/fuse/file.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > > > index ec238fb5a584..07d0efcb050c 100644
> > > > --- a/fs/fuse/file.c
> > > > +++ b/fs/fuse/file.c
> > > > @@ -1669,6 +1669,7 @@ static int fuse_writepage_locked(struct page *page)
> > > >  err_free:
> > > >  	fuse_request_free(req);
> > > >  err:
> > > > +	mapping_set_error(page->mapping, error);
> > > >  	end_page_writeback(page);
> > > >  	return error;
> > > >  }
> > > > -- 
> > > > 2.9.3
> > > > 
> > > > 
> > 
> > -- 
> > Jeff Layton <jlayton@redhat.com>

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1630419 — Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

On Tue 25-04-17 06:35:13, Jeff Layton wrote:
> On Tue, 2017-04-25 at 10:17 +0200, Jan Kara wrote:
> > On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> > > On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > > > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > > > This ensures that we see errors on fsync when writeback fails.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > 
> > > > Hum, but do we really want to clobber mapping errors with temporary stuff
> > > > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > > > 
> > > 
> > > Right now we don't really have such a thing as temporary errors in the
> > > writeback codepath. If you return an error here, the data doesn't stay
> > > dirty or anything, and I think we want to ensure that that gets reported
> > > via fsync.
> > > 
> > > I'd like to see us add better handling for retryable errors for stuff
> > > like ENOMEM or EAGAIN. I think this is the first step toward that
> > > though. Once we have more consistent handling of writeback errors in
> > > general, then we can start doing more interesting things with retryable
> > > errors.
> > > 
> > > So yeah, I this is the right thing to do for now.
> > 
> > OK, fair enough. And question number 2):
> > 
> > Who is actually responsible for setting the error in the mapping when error
> > happens inside ->writepage()? Is it the ->writepage() callback or the
> > caller of ->writepage()? Or something else? Currently it seems to be a
> > strange mix (e.g. mm/page-writeback.c: __writepage() calls
> > mapping_set_error() when ->writepage() returns error) so I'd like to
> > understand what's the plan and have that recorded in the changelogs.
> > 
> 
> That's an excellent question.
> 
> I think we probably want the writepage/launder_page operations to call
> mapping_set_error. That makes it possible for filesystems (e.g. NFS) to
> handle their own error tracking and reporting without using the new
> infrastructure. If they never call mapping_set_error then we'll always
> just return whatever their ->fsync operation returns on an fsync.

OK, makes sense. It is also in line with what you did for DAX, 9p, or here
for FUSE. So feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

for this patch but please also add a sentense that ->writepage() is
responsible for calling mapping_set_error() if it fails and page is not
redirtied to the changelogs of patches changing writepage handlers.

> I'll make another pass through the tree and see whether we have some
> mapping_set_error calls that should be removed, and will flesh out
> vfs.txt to state this. Maybe that file needs a whole section on
> writeback error reporting? Hmmm...

I think it would be nice to have all the logic described in one place. So
+1 from me.

> That probably also means that I should drop patch 8 from this series
> (mm: ensure that we set mapping error if writeout fails), since that
> should be happening in writepage already.

Yes.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1630779 — Re: [PATCH v3 10/20] fuse: set mapping error in writepage_locked when it fails

On Tue, 2017-04-25 at 13:19 +0200, Jan Kara wrote:
> On Tue 25-04-17 06:35:13, Jeff Layton wrote:
> > On Tue, 2017-04-25 at 10:17 +0200, Jan Kara wrote:
> > > On Mon 24-04-17 13:14:36, Jeff Layton wrote:
> > > > On Mon, 2017-04-24 at 18:04 +0200, Jan Kara wrote:
> > > > > On Mon 24-04-17 09:22:49, Jeff Layton wrote:
> > > > > > This ensures that we see errors on fsync when writeback fails.
> > > > > > 
> > > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > 
> > > > > Hum, but do we really want to clobber mapping errors with temporary stuff
> > > > > like ENOMEM? Or do you want to handle that in mapping_set_error?
> > > > > 
> > > > 
> > > > Right now we don't really have such a thing as temporary errors in the
> > > > writeback codepath. If you return an error here, the data doesn't stay
> > > > dirty or anything, and I think we want to ensure that that gets reported
> > > > via fsync.
> > > > 
> > > > I'd like to see us add better handling for retryable errors for stuff
> > > > like ENOMEM or EAGAIN. I think this is the first step toward that
> > > > though. Once we have more consistent handling of writeback errors in
> > > > general, then we can start doing more interesting things with retryable
> > > > errors.
> > > > 
> > > > So yeah, I this is the right thing to do for now.
> > > 
> > > OK, fair enough. And question number 2):
> > > 
> > > Who is actually responsible for setting the error in the mapping when error
> > > happens inside ->writepage()? Is it the ->writepage() callback or the
> > > caller of ->writepage()? Or something else? Currently it seems to be a
> > > strange mix (e.g. mm/page-writeback.c: __writepage() calls
> > > mapping_set_error() when ->writepage() returns error) so I'd like to
> > > understand what's the plan and have that recorded in the changelogs.
> > > 
> > 
> > That's an excellent question.
> > 
> > I think we probably want the writepage/launder_page operations to call
> > mapping_set_error. That makes it possible for filesystems (e.g. NFS) to
> > handle their own error tracking and reporting without using the new
> > infrastructure. If they never call mapping_set_error then we'll always
> > just return whatever their ->fsync operation returns on an fsync.
> 
> OK, makes sense. It is also in line with what you did for DAX, 9p, or here
> for FUSE. So feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> for this patch but please also add a sentense that ->writepage() is
> responsible for calling mapping_set_error() if it fails and page is not
> redirtied to the changelogs of patches changing writepage handlers.
> 
> > I'll make another pass through the tree and see whether we have some
> > mapping_set_error calls that should be removed, and will flesh out
> > vfs.txt to state this. Maybe that file needs a whole section on
> > writeback error reporting? Hmmm...
> 
> I think it would be nice to have all the logic described in one place. So
> +1 from me.
> 
> > That probably also means that I should drop patch 8 from this series
> > (mm: ensure that we set mapping error if writeout fails), since that
> > should be happening in writepage already.
> 
> Yes.
> 
> 								Honza

(cc'ing Jon since I'm proposing a doc update)

Here's what I'm thinking for a vfs.txt update after this series. The
section on writeback_control could probably be more specific.

----------------------8<-------------------------

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 94dd27ef4a76..aa912b65792a 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -576,7 +576,23 @@ should clear PG_Dirty and set PG_Writeback.  It can be actually
 written at any point after PG_Dirty is clear.  Once it is known to be
 safe, PG_Writeback is cleared.
 
-Writeback makes use of a writeback_control structure...
+Writeback makes use of a writeback_control structure to direct the
+operations. This tells the writepage and writepages operations something
+about the nature of and reason for the writeback request, and the
+constraints under which it is being done. It is also used to track state
+between successive writeback requests.
+
+When there is an error during writeback, then an error should be
+reported to fsync on all file descriptors that were open at the time of
+the error. This is typically done by setting the wb_err value in the
+address_space via mapping_set_error when writeback errors occur. The
+vfs-layer fsync code will then report the errors on a per-fd basis.
+
+Filesystems are free to track errors internally if they choose, but they
+should aim to provide the same semantics for error reporting when there
+are multiple writers. Filesystems that track their own errors should
+avoid calling mapping_set_error in order to ensure that errors stored in
+the mapping aren't improperly reported by the generic filesystem code.
 
 struct address_space_operations
 -------------------------------
@@ -888,7 +904,9 @@ otherwise noted.
 
   release: called when the last reference to an open file is closed
 
-  fsync: called by the fsync(2) system call
+  fsync: called by the fsync(2) system call. Errors that were previously
+	 recorded using mapping_set_error will automatically be returned to
+	 the application and the file's error sequence advanced.
 
   fasync: called by the fcntl(2) system call when asynchronous
 	(non-blocking) mode is enabled for a file

[toc] | [prev] | [next] | [standalone]

#1629553 — [PATCH v3 01/20] mm: drop "wait" parameter from write_one_page

The callers all set it to 1.

Also, make it clear that this function will not set any sort of AS_*
error, and that the caller must do so if necessary. No existing caller
uses this on normal files, so none of them need it.

Also, add __must_check here since, in general, the callers need to
handle an error here in some fashion.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 fs/exofs/dir.c        |  2 +-
 fs/ext2/dir.c         |  2 +-
 fs/jfs/jfs_metapage.c |  4 ++--
 fs/minix/dir.c        |  2 +-
 fs/sysv/dir.c         |  2 +-
 fs/ufs/dir.c          |  2 +-
 include/linux/mm.h    |  2 +-
 mm/page-writeback.c   | 14 +++++++-------
 8 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
index 42f9a0a0c4ca..e163ed980c20 100644
--- a/fs/exofs/dir.c
+++ b/fs/exofs/dir.c
@@ -72,7 +72,7 @@ static int exofs_commit_chunk(struct page *page, loff_t pos, unsigned len)
 	set_page_dirty(page);
 
 	if (IS_DIRSYNC(dir))
-		err = write_one_page(page, 1);
+		err = write_one_page(page);
 	else
 		unlock_page(page);
 
diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index d9650c9508e4..e2709695b177 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -100,7 +100,7 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)
 	}
 
 	if (IS_DIRSYNC(dir)) {
-		err = write_one_page(page, 1);
+		err = write_one_page(page);
 		if (!err)
 			err = sync_inode_metadata(dir, 1);
 	} else {
diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
index 489aaa1403e5..744fa3c079e6 100644
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -711,7 +711,7 @@ void force_metapage(struct metapage *mp)
 	get_page(page);
 	lock_page(page);
 	set_page_dirty(page);
-	write_one_page(page, 1);
+	write_one_page(page);
 	clear_bit(META_forcewrite, &mp->flag);
 	put_page(page);
 }
@@ -756,7 +756,7 @@ void release_metapage(struct metapage * mp)
 		set_page_dirty(page);
 		if (test_bit(META_sync, &mp->flag)) {
 			clear_bit(META_sync, &mp->flag);
-			write_one_page(page, 1);
+			write_one_page(page);
 			lock_page(page); /* write_one_page unlocks the page */
 		}
 	} else if (mp->lsn)	/* discard_metapage doesn't remove it */
diff --git a/fs/minix/dir.c b/fs/minix/dir.c
index 7edc9b395700..baa9721f1299 100644
--- a/fs/minix/dir.c
+++ b/fs/minix/dir.c
@@ -57,7 +57,7 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
 		mark_inode_dirty(dir);
 	}
 	if (IS_DIRSYNC(dir))
-		err = write_one_page(page, 1);
+		err = write_one_page(page);
 	else
 		unlock_page(page);
 	return err;
diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c
index 5bdae85ceef7..f5191cb2c947 100644
--- a/fs/sysv/dir.c
+++ b/fs/sysv/dir.c
@@ -45,7 +45,7 @@ static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
 		mark_inode_dirty(dir);
 	}
 	if (IS_DIRSYNC(dir))
-		err = write_one_page(page, 1);
+		err = write_one_page(page);
 	else
 		unlock_page(page);
 	return err;
diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
index de01b8f2aa78..48609f1d9580 100644
--- a/fs/ufs/dir.c
+++ b/fs/ufs/dir.c
@@ -53,7 +53,7 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len)
 		mark_inode_dirty(dir);
 	}
 	if (IS_DIRSYNC(dir))
-		err = write_one_page(page, 1);
+		err = write_one_page(page);
 	else
 		unlock_page(page);
 	return err;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 00a8fa7e366a..403f78afee20 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2187,7 +2187,7 @@ extern void filemap_map_pages(struct vm_fault *vmf,
 extern int filemap_page_mkwrite(struct vm_fault *vmf);
 
 /* mm/page-writeback.c */
-int write_one_page(struct page *page, int wait);
+int __must_check write_one_page(struct page *page);
 void task_dirty_inc(struct task_struct *tsk);
 
 /* readahead.c */
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index d8ac2a7fb9e7..de0dbf12e2c1 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2361,15 +2361,16 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 }
 
 /**
- * write_one_page - write out a single page and optionally wait on I/O
+ * write_one_page - write out a single page and wait on I/O
  * @page: the page to write
- * @wait: if true, wait on writeout
  *
  * The page must be locked by the caller and will be unlocked upon return.
  *
- * write_one_page() returns a negative error code if I/O failed.
+ * write_one_page() returns a negative error code if I/O failed. Note that
+ * the address_space is not marked for error. The caller must do this if
+ * needed.
  */
-int write_one_page(struct page *page, int wait)
+int write_one_page(struct page *page)
 {
 	struct address_space *mapping = page->mapping;
 	int ret = 0;
@@ -2380,13 +2381,12 @@ int write_one_page(struct page *page, int wait)
 
 	BUG_ON(!PageLocked(page));
 
-	if (wait)
-		wait_on_page_writeback(page);
+	wait_on_page_writeback(page);
 
 	if (clear_page_dirty_for_io(page)) {
 		get_page(page);
 		ret = mapping->a_ops->writepage(page, &wbc);
-		if (ret == 0 && wait) {
+		if (ret == 0) {
 			wait_on_page_writeback(page);
 			if (PageError(page))
 				ret = -EIO;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629670 — Re: [PATCH v3 01/20] mm: drop "wait" parameter from write_one_page

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1629555 — [PATCH v3 15/20] mm: remove AS_EIO and AS_ENOSPC flags

They're no longer used.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/pagemap.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 32512ffc15fa..9593eac41499 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -20,13 +20,11 @@
  * Bits in mapping->flags.
  */
 enum mapping_flags {
-	AS_EIO		= 0,	/* IO error on async write */
-	AS_ENOSPC	= 1,	/* ENOSPC on async write */
-	AS_MM_ALL_LOCKS	= 2,	/* under mm_take_all_locks() */
-	AS_UNEVICTABLE	= 3,	/* e.g., ramdisk, SHM_LOCK */
-	AS_EXITING	= 4, 	/* final truncate in progress */
+	AS_MM_ALL_LOCKS	= 0,	/* under mm_take_all_locks() */
+	AS_UNEVICTABLE	= 1,	/* e.g., ramdisk, SHM_LOCK */
+	AS_EXITING	= 2, 	/* final truncate in progress */
 	/* writeback related tags are not used */
-	AS_NO_WRITEBACK_TAGS = 5,
+	AS_NO_WRITEBACK_TAGS = 3,
 };
 
 static inline void mapping_set_error(struct address_space *mapping, int error)
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629556 — [PATCH v3 18/20] mm: clean up error handling in write_one_page

Don't try to check PageError since that's potentially racy and not
necessarily going to be set after writepage errors out.

Instead, sample the mapping error early on, and use that value to tell
us whether we got a writeback error since then.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 mm/page-writeback.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index de0dbf12e2c1..1643456881b4 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2373,11 +2373,12 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 int write_one_page(struct page *page)
 {
 	struct address_space *mapping = page->mapping;
-	int ret = 0;
+	int ret = 0, ret2;
 	struct writeback_control wbc = {
 		.sync_mode = WB_SYNC_ALL,
 		.nr_to_write = 1,
 	};
+	errseq_t since = filemap_sample_wb_error(mapping);
 
 	BUG_ON(!PageLocked(page));
 
@@ -2386,16 +2387,14 @@ int write_one_page(struct page *page)
 	if (clear_page_dirty_for_io(page)) {
 		get_page(page);
 		ret = mapping->a_ops->writepage(page, &wbc);
-		if (ret == 0) {
+		if (ret == 0)
 			wait_on_page_writeback(page);
-			if (PageError(page))
-				ret = -EIO;
-		}
 		put_page(page);
 	} else {
 		unlock_page(page);
 	}
-	return ret;
+	ret2 = filemap_check_wb_error(mapping, since);
+	return ret ? : ret2;
 }
 EXPORT_SYMBOL(write_one_page);
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629557 — [PATCH v3 16/20] mm: don't TestClearPageError in __filemap_fdatawait_range

The -EIO returned here can end up overriding whatever error is marked in
the address space, and be returned at fsync time, even when there is a
more appropriate error stored in the mapping.

Read errors are also sometimes tracked on a per-page level using
PG_error. Suppose we have a read error on a page, and then that page is
subsequently dirtied by overwriting the whole page. Writeback doesn't
clear PG_error, so we can then end up successfully writing back that
page and still return -EIO on fsync.

Worse yet, PG_error is cleared during a sync() syscall, but the -EIO
return from that is silently discarded. Any subsystem that is relying on
PG_error to report errors during fsync can easily lose writeback errors
due to this. All you need is a stray sync() call on the box at the wrong
time and you've lost the error.

Since the handling of the PG_error flag is somewhat inconsistent across
subsystems, let's just rely on marking the address space when there are
writeback errors. Change the TestClearPageError call to ClearPageError,
and make __filemap_fdatawait_range a void return function.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 mm/filemap.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index d94a76d4e023..47e7f50fb830 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -363,17 +363,16 @@ int filemap_flush(struct address_space *mapping)
 }
 EXPORT_SYMBOL(filemap_flush);
 
-static int __filemap_fdatawait_range(struct address_space *mapping,
+static void __filemap_fdatawait_range(struct address_space *mapping,
 				     loff_t start_byte, loff_t end_byte)
 {
 	pgoff_t index = start_byte >> PAGE_SHIFT;
 	pgoff_t end = end_byte >> PAGE_SHIFT;
 	struct pagevec pvec;
 	int nr_pages;
-	int ret = 0;
 
 	if (end_byte < start_byte)
-		goto out;
+		return;
 
 	pagevec_init(&pvec, 0);
 	while ((index <= end) &&
@@ -390,14 +389,11 @@ static int __filemap_fdatawait_range(struct address_space *mapping,
 				continue;
 
 			wait_on_page_writeback(page);
-			if (TestClearPageError(page))
-				ret = -EIO;
+			ClearPageError(page);
 		}
 		pagevec_release(&pvec);
 		cond_resched();
 	}
-out:
-	return ret;
 }
 
 /**
@@ -417,15 +413,10 @@ static int __filemap_fdatawait_range(struct address_space *mapping,
 int filemap_fdatawait_range(struct address_space *mapping, loff_t start_byte,
 			    loff_t end_byte)
 {
-	int ret, ret2;
 	errseq_t since = filemap_sample_wb_error(mapping);
 
-	ret = __filemap_fdatawait_range(mapping, start_byte, end_byte);
-	ret2 = filemap_check_wb_error(mapping, since);
-	if (!ret)
-		ret = ret2;
-
-	return ret;
+	__filemap_fdatawait_range(mapping, start_byte, end_byte);
+	return filemap_check_wb_error(mapping, since);
 }
 EXPORT_SYMBOL(filemap_fdatawait_range);
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629560 — [PATCH v3 20/20] gfs2: clean up some filemap_* calls

In some places, it's trying to reset the mapping error after calling
filemap_fdatawait. That's no longer required. Also, turn several
filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
That will at least return writeback errors that occur during the write
phase.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/gfs2/glops.c | 12 ++++--------
 fs/gfs2/lops.c  |  4 +---
 fs/gfs2/super.c |  6 ++----
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 5db59d444838..7362d19fdc4c 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -158,9 +158,7 @@ static void rgrp_go_sync(struct gfs2_glock *gl)
 	GLOCK_BUG_ON(gl, gl->gl_state != LM_ST_EXCLUSIVE);
 
 	gfs2_log_flush(sdp, gl, NORMAL_FLUSH);
-	filemap_fdatawrite_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
-	error = filemap_fdatawait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
-	mapping_set_error(mapping, error);
+	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
 	gfs2_ail_empty_gl(gl);
 
 	spin_lock(&gl->gl_lockref.lock);
@@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
 	filemap_fdatawrite(metamapping);
 	if (ip) {
 		struct address_space *mapping = ip->i_inode.i_mapping;
-		filemap_fdatawrite(mapping);
-		error = filemap_fdatawait(mapping);
-		mapping_set_error(mapping, error);
+		filemap_write_and_wait(mapping);
+	} else {
+		filemap_fdatawait(metamapping);
 	}
-	error = filemap_fdatawait(metamapping);
-	mapping_set_error(metamapping, error);
 	gfs2_ail_empty_gl(gl);
 	/*
 	 * Writeback of the data mapping may cause the dirty flag to be set
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index b1f9144b42c7..565ce33dec9b 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -586,9 +586,7 @@ static void gfs2_meta_sync(struct gfs2_glock *gl)
 	if (mapping == NULL)
 		mapping = &sdp->sd_aspace;
 
-	filemap_fdatawrite(mapping);
-	error = filemap_fdatawait(mapping);
-
+	error = filemap_write_and_wait(mapping);
 	if (error)
 		gfs2_io_error(gl->gl_name.ln_sbd);
 }
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 361796a84fce..675c39566ea1 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1593,10 +1593,8 @@ static void gfs2_evict_inode(struct inode *inode)
 out_truncate:
 	gfs2_log_flush(sdp, ip->i_gl, NORMAL_FLUSH);
 	metamapping = gfs2_glock2aspace(ip->i_gl);
-	if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags)) {
-		filemap_fdatawrite(metamapping);
-		filemap_fdatawait(metamapping);
-	}
+	if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags))
+		filemap_write_and_wait(metamapping);
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);
 
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629614 — Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls

----- Original Message -----
| In some places, it's trying to reset the mapping error after calling
| filemap_fdatawait. That's no longer required. Also, turn several
| filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
| That will at least return writeback errors that occur during the write
| phase.
| 
| Signed-off-by: Jeff Layton <jlayton@redhat.com>
| ---
|  fs/gfs2/glops.c | 12 ++++--------
|  fs/gfs2/lops.c  |  4 +---
|  fs/gfs2/super.c |  6 ++----
|  3 files changed, 7 insertions(+), 15 deletions(-)
| 
| diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
| index 5db59d444838..7362d19fdc4c 100644
| --- a/fs/gfs2/glops.c
| +++ b/fs/gfs2/glops.c
| @@ -158,9 +158,7 @@ static void rgrp_go_sync(struct gfs2_glock *gl)
|  	GLOCK_BUG_ON(gl, gl->gl_state != LM_ST_EXCLUSIVE);
|  
|  	gfs2_log_flush(sdp, gl, NORMAL_FLUSH);
| -	filemap_fdatawrite_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
| -	error = filemap_fdatawait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
| -	mapping_set_error(mapping, error);
| +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);

This should probably have "error = ", no?

|  	gfs2_ail_empty_gl(gl);
|  
|  	spin_lock(&gl->gl_lockref.lock);
| @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
|  	filemap_fdatawrite(metamapping);
|  	if (ip) {
|  		struct address_space *mapping = ip->i_inode.i_mapping;
| -		filemap_fdatawrite(mapping);
| -		error = filemap_fdatawait(mapping);
| -		mapping_set_error(mapping, error);
| +		filemap_write_and_wait(mapping);
| +	} else {
| +		filemap_fdatawait(metamapping);
|  	}
| -	error = filemap_fdatawait(metamapping);
| -	mapping_set_error(metamapping, error);

This part doesn't look right at all. There's a big difference in gfs2 between
mapping and metamapping. We need to wait for metamapping regardless.

(snip)

Regards,

Bob Peterson
Red Hat File Systems

[toc] | [prev] | [next] | [standalone]

#1629786 — Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls

On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:
> ----- Original Message -----
> > In some places, it's trying to reset the mapping error after calling
> > filemap_fdatawait. That's no longer required. Also, turn several
> > filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
> > That will at least return writeback errors that occur during the write
> > phase.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/gfs2/glops.c | 12 ++++--------
> >  fs/gfs2/lops.c  |  4 +---
> >  fs/gfs2/super.c |  6 ++----
> >  3 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > index 5db59d444838..7362d19fdc4c 100644
> > --- a/fs/gfs2/glops.c
> > +++ b/fs/gfs2/glops.c
> > @@ -158,9 +158,7 @@ static void rgrp_go_sync(struct gfs2_glock *gl)
> >  	GLOCK_BUG_ON(gl, gl->gl_state != LM_ST_EXCLUSIVE);
> >  
> >  	gfs2_log_flush(sdp, gl, NORMAL_FLUSH);
> > -	filemap_fdatawrite_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> > -	error = filemap_fdatawait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> > -	mapping_set_error(mapping, error);
> > +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> 
> This should probably have "error = ", no?
> 

This error is discarded in the current code after resetting the error in
the mapping. With the earlier patches in this set we don't need to reset
the error like this anymore.

Now, if this code should doing something else with those errors, then
that's a separate problem.

> >  	gfs2_ail_empty_gl(gl);
> >  
> >  	spin_lock(&gl->gl_lockref.lock);
> > @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
> >  	filemap_fdatawrite(metamapping);
> >  	if (ip) {
> >  		struct address_space *mapping = ip->i_inode.i_mapping;
> > -		filemap_fdatawrite(mapping);
> > -		error = filemap_fdatawait(mapping);
> > -		mapping_set_error(mapping, error);
> > +		filemap_write_and_wait(mapping);
> > +	} else {
> > +		filemap_fdatawait(metamapping);
> >  	}
> > -	error = filemap_fdatawait(metamapping);
> > -	mapping_set_error(metamapping, error);
> 
> This part doesn't look right at all. There's a big difference in gfs2 between
> mapping and metamapping. We need to wait for metamapping regardless.
> 

...and this should wait. Basically, filemap_write_and_wait does
filemap_fdatawrite and then filemap_fdatawait. This is mostly just
replacing the existing code with a more concise helper.

-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1629823 — Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls

Hi,

----- Original Message -----
| On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:
| > > +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
| > 
| > This should probably have "error = ", no?
| > 
| 
| This error is discarded in the current code after resetting the error in
| the mapping. With the earlier patches in this set we don't need to reset
| the error like this anymore.
| 
| Now, if this code should doing something else with those errors, then
| that's a separate problem.

Okay, I see. My bad.
 
| > >  	gfs2_ail_empty_gl(gl);
| > >  
| > >  	spin_lock(&gl->gl_lockref.lock);
| > > @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
| > >  	filemap_fdatawrite(metamapping);
| > >  	if (ip) {
| > >  		struct address_space *mapping = ip->i_inode.i_mapping;
| > > -		filemap_fdatawrite(mapping);
| > > -		error = filemap_fdatawait(mapping);
| > > -		mapping_set_error(mapping, error);
| > > +		filemap_write_and_wait(mapping);
| > > +	} else {
| > > +		filemap_fdatawait(metamapping);
| > >  	}
| > > -	error = filemap_fdatawait(metamapping);
| > > -	mapping_set_error(metamapping, error);
| > 
| > This part doesn't look right at all. There's a big difference in gfs2
| > between
| > mapping and metamapping. We need to wait for metamapping regardless.
| > 
| 
| ...and this should wait. Basically, filemap_write_and_wait does
| filemap_fdatawrite and then filemap_fdatawait. This is mostly just
| replacing the existing code with a more concise helper.

But this isn't a simple replacement with a helper. This is two different
address spaces (mapping and metamapping) and you added an else in there.

So with this patch metamapping gets written, and if there's an ip,
mapping gets written but it doesn't wait for metamapping. Unless
I'm missing something.

You could replace both filemap_fdatawrites with the helper instead.
Today's code is structured as:

(a) write metamapping
if (ip)
    (b) write mapping
    (c) wait for mapping
(d) wait for metamapping

If you use the helper for both, it becomes, (a & d)(b & c) which is probably
acceptable. (I think we just tried to optimize what the elevator was doing).

But the way you've got it coded here still looks wrong. It looks like:
(a)
if (ip)
   (b & c)
ELSE -
   (d)

So (d) (metamapping) isn't guaranteed to be synced at the end of the function.
Of course, you know the modified helper functions better than I do.
What am I missing?

Regards,

Bob Peterson
Red Hat File Systems

[toc] | [prev] | [next] | [standalone]

#1629835 — Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls

On Mon, 2017-04-24 at 13:41 -0400, Bob Peterson wrote:
> Hi,
> 
> ----- Original Message -----
> > On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:
> > > > +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> > > 
> > > This should probably have "error = ", no?
> > > 
> > 
> > This error is discarded in the current code after resetting the error in
> > the mapping. With the earlier patches in this set we don't need to reset
> > the error like this anymore.
> > 
> > Now, if this code should doing something else with those errors, then
> > that's a separate problem.
> 
> Okay, I see. My bad.
>  
> > > >  	gfs2_ail_empty_gl(gl);
> > > >  
> > > >  	spin_lock(&gl->gl_lockref.lock);
> > > > @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
> > > >  	filemap_fdatawrite(metamapping);
> > > >  	if (ip) {
> > > >  		struct address_space *mapping = ip->i_inode.i_mapping;
> > > > -		filemap_fdatawrite(mapping);
> > > > -		error = filemap_fdatawait(mapping);
> > > > -		mapping_set_error(mapping, error);
> > > > +		filemap_write_and_wait(mapping);
> > > > +	} else {
> > > > +		filemap_fdatawait(metamapping);
> > > >  	}
> > > > -	error = filemap_fdatawait(metamapping);
> > > > -	mapping_set_error(metamapping, error);
> > > 
> > > This part doesn't look right at all. There's a big difference in gfs2
> > > between
> > > mapping and metamapping. We need to wait for metamapping regardless.
> > > 
> > 
> > ...and this should wait. Basically, filemap_write_and_wait does
> > filemap_fdatawrite and then filemap_fdatawait. This is mostly just
> > replacing the existing code with a more concise helper.
> 
> But this isn't a simple replacement with a helper. This is two different
> address spaces (mapping and metamapping) and you added an else in there.
> 
> So with this patch metamapping gets written, and if there's an ip,
> mapping gets written but it doesn't wait for metamapping. Unless
> I'm missing something.
> 
> You could replace both filemap_fdatawrites with the helper instead.
> Today's code is structured as:
> 
> (a) write metamapping
> if (ip)
>     (b) write mapping
>     (c) wait for mapping
> (d) wait for metamapping
> 
> If you use the helper for both, it becomes, (a & d)(b & c) which is probably
> acceptable. (I think we just tried to optimize what the elevator was doing).
> 
> But the way you've got it coded here still looks wrong. It looks like:
> (a)
> if (ip)
>    (b & c)
> ELSE -
>    (d)
> 
> So (d) (metamapping) isn't guaranteed to be synced at the end of the function.
> Of course, you know the modified helper functions better than I do.
> What am I missing?
> 
> 

<facepalm>
You're right of course. I'll fix that up in my tree.

Thanks!
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [next] | [standalone]

#1629562 — [PATCH v3 07/20] nilfs2: set the mapping error when calling SetPageError on writeback

In a later patch, we're going to want to make the fsync codepath not do
a TestClearPageError call as that can override the error set in the
address space. To do that though, we need to ensure that filesystems
that are relying on the PG_error bit for reporting writeback errors
also set an error in the address space.

Ensure that this is set in nilfs2.

Cc: Ryusuke Konishi <konishi.ryusuke@lab.ntt.co.jp>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nilfs2/segment.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index febed1217b3f..612d4b446793 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -1745,6 +1745,7 @@ static void nilfs_end_page_io(struct page *page, int err)
 	} else {
 		__set_page_dirty_nobuffers(page);
 		SetPageError(page);
+		mapping_set_error(page_mapping(page), err);
 	}
 
 	end_page_writeback(page);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629675 — Re: [PATCH v3 07/20] nilfs2: set the mapping error when calling SetPageError on writeback

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1629563 — [PATCH v3 13/20] fs: new infrastructure for writeback error handling and reporting

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.

The non-fsync callers are problematic. We should be reporting writeback
errors during fsync, but many places spread over the tree clear out
errors before they can be properly reported, or report errors at
nonsensical times.

If I get -EIO on a stat() call, there is no reason for me to assume that
it is because some previous writeback failed. The fact that it also
clears out the error such that a subsequent fsync returns 0 is a bug,
and a nasty one since that's potentially silent data corruption.

This patch adds a small bit of new infrastructure for setting and
reporting errors during address_space 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
fds 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 an errseq_t to struct address_space, and a corresponding
one is added to struct file. Writeback errors are recorded in the
mapping's errseq_t, and the one in struct file is used as the "since"
value.

This changes the semantics of the Linux fsync implementation 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 new behavior is still consistent with the POSIX spec, and is more
reliable for application developers. This patch just adds some basic
infrastructure for doing this. Later patches will change the existing
code to use this new infrastructure.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 Documentation/filesystems/vfs.txt | 10 +++++++++-
 fs/open.c                         |  3 +++
 include/linux/fs.h                | 24 ++++++++++++++++++++++++
 mm/filemap.c                      | 38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 94dd27ef4a76..ed06fb39822b 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -576,6 +576,11 @@ should clear PG_Dirty and set PG_Writeback.  It can be actually
 written at any point after PG_Dirty is clear.  Once it is known to be
 safe, PG_Writeback is cleared.
 
+If there is an error during writeback, then the address_space should be
+marked with an error (typically using filemap_set_wb_error), in order to
+ensure that the error can later be reported to the application when an
+fsync is issued.
+
 Writeback makes use of a writeback_control structure...
 
 struct address_space_operations
@@ -888,7 +893,10 @@ otherwise noted.
 
   release: called when the last reference to an open file is closed
 
-  fsync: called by the fsync(2) system call
+  fsync: called by the fsync(2) system call. Filesystems that use the
+	pagecache should call filemap_report_wb_error before returning
+	to ensure that any errors that occurred during writeback are
+	reported and the file's error sequence advanced.
 
   fasync: called by the fcntl(2) system call when asynchronous
 	(non-blocking) mode is enabled for a file
diff --git a/fs/open.c b/fs/open.c
index 949cef29c3bb..88bfed8d3c88 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -709,6 +709,9 @@ static int do_dentry_open(struct file *f,
 	f->f_inode = inode;
 	f->f_mapping = inode->i_mapping;
 
+	/* Ensure that we skip any errors that predate opening of the file */
+	f->f_wb_err = filemap_sample_wb_error(f->f_mapping);
+
 	if (unlikely(f->f_flags & O_PATH)) {
 		f->f_mode = FMODE_PATH;
 		f->f_op = &empty_fops;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7251f7bb45e8..69a89f667c7f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -31,6 +31,7 @@
 #include <linux/workqueue.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/delayed_call.h>
+#include <linux/errseq.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -394,6 +395,7 @@ struct address_space {
 	gfp_t			gfp_mask;	/* implicit gfp mask for allocations */
 	struct list_head	private_list;	/* ditto */
 	void			*private_data;	/* ditto */
+	errseq_t		wb_err;
 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but
@@ -846,6 +848,7 @@ struct file {
 	 * Must not be taken from IRQ context.
 	 */
 	spinlock_t		f_lock;
+	errseq_t		f_wb_err;
 	atomic_long_t		f_count;
 	unsigned int 		f_flags;
 	fmode_t			f_mode;
@@ -2521,6 +2524,27 @@ extern int __filemap_fdatawrite_range(struct address_space *mapping,
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
 extern int filemap_check_errors(struct address_space *mapping);
+extern int __must_check filemap_report_wb_error(struct file *file);
+
+/**
+ * filemap_check_wb_error - has an error occurred since the mark was sampled?
+ * @mapping: mapping to check for writeback errors
+ * @since: previously-sampled errseq_t
+ *
+ * Grab the errseq_t value from the mapping, and see if it has changed "since"
+ * the given value was sampled.
+ *
+ * If it has then report the latest error set, otherwise return 0.
+ */
+static inline int filemap_check_wb_error(struct address_space *mapping, errseq_t since)
+{
+	return errseq_check(&mapping->wb_err, since);
+}
+
+static inline errseq_t filemap_sample_wb_error(struct address_space *mapping)
+{
+	return errseq_sample(&mapping->wb_err);
+}
 
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);
diff --git a/mm/filemap.c b/mm/filemap.c
index 1694623a6289..ee1a798acfc1 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -546,6 +546,44 @@ int filemap_write_and_wait_range(struct address_space *mapping,
 EXPORT_SYMBOL(filemap_write_and_wait_range);
 
 /**
+ * filemap_report_wb_error - report wb error (if any) that was previously set
+ * @file: struct file on which the error is being reported
+ *
+ * When userland calls fsync (or something like nfsd does the equivalent), we
+ * want to report any writeback errors that occurred since the last fsync (or
+ * since the file was opened if there haven't been any).
+ *
+ * Grab the wb_err from the mapping. If it matches what we have in the file,
+ * then just quickly return 0. The file is all caught up.
+ *
+ * If it doesn't match, then take the mapping value, set the "seen" flag in
+ * it and try to swap it into place. If it works, or another task beat us
+ * to it with the new value, then update the f_wb_err and return the error
+ * portion. The error at this point must be reported via proper channels
+ * (a'la fsync, or NFS COMMIT operation, etc.).
+ *
+ * While we handle mapping->wb_err with atomic operations, the f_wb_err
+ * value is protected by the f_lock since we must ensure that it reflects
+ * the latest value swapped in for this file descriptor.
+ */
+int filemap_report_wb_error(struct file *file)
+{
+	int err = 0;
+	struct address_space *mapping = file->f_mapping;
+
+	/* Locklessly handle the common case where nothing has changed */
+	if (errseq_check(&mapping->wb_err, READ_ONCE(file->f_wb_err))) {
+		/* Something changed, must use slow path */
+		spin_lock(&file->f_lock);
+		err = errseq_check_and_advance(&mapping->wb_err,
+						&file->f_wb_err);
+		spin_unlock(&file->f_lock);
+	}
+	return err;
+}
+EXPORT_SYMBOL(filemap_report_wb_error);
+
+/**
  * replace_page_cache_page - replace a pagecache page with a new one
  * @old:	page to be replaced
  * @new:	page to replace with
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629564 — [PATCH v3 17/20] cifs: cleanup writeback handling errors and comments

Now that writeback errors are handled on a per-file basis using the new
sequence counter method at the vfs layer, we no longer need to re-set
errors in the mapping after doing writeback in non-fsync codepaths.

Also, fix up some bogus comments.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsfs.c |  4 +---
 fs/cifs/file.c   |  7 ++-----
 fs/cifs/inode.c  | 22 +++++++---------------
 3 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index dd3f5fabfdf6..017a2d1d02c7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -829,10 +829,8 @@ static loff_t cifs_llseek(struct file *file, loff_t offset, int whence)
 		if (!CIFS_CACHE_READ(CIFS_I(inode)) && inode->i_mapping &&
 		    inode->i_mapping->nrpages != 0) {
 			rc = filemap_fdatawait(inode->i_mapping);
-			if (rc) {
-				mapping_set_error(inode->i_mapping, rc);
+			if (rc)
 				return rc;
-			}
 		}
 		/*
 		 * Some applications poll for the file length in this strange
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 4b696a23b0b1..9b4f7f182add 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -722,9 +722,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
 	cinode = CIFS_I(inode);
 
 	if (can_flush) {
-		rc = filemap_write_and_wait(inode->i_mapping);
-		mapping_set_error(inode->i_mapping, rc);
-
+		filemap_write_and_wait(inode->i_mapping);
 		if (tcon->unix_ext)
 			rc = cifs_get_inode_info_unix(&inode, full_path,
 						      inode->i_sb, xid);
@@ -3906,8 +3904,7 @@ void cifs_oplock_break(struct work_struct *work)
 			break_lease(inode, O_WRONLY);
 		rc = filemap_fdatawrite(inode->i_mapping);
 		if (!CIFS_CACHE_READ(cinode)) {
-			rc = filemap_fdatawait(inode->i_mapping);
-			mapping_set_error(inode->i_mapping, rc);
+			filemap_fdatawait(inode->i_mapping);
 			cifs_zap_mapping(inode);
 		}
 		cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc);
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index b261db34103c..a58e605240fc 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2008,10 +2008,8 @@ int cifs_getattr(const struct path *path, struct kstat *stat,
 	if (!CIFS_CACHE_READ(CIFS_I(inode)) && inode->i_mapping &&
 	    inode->i_mapping->nrpages != 0) {
 		rc = filemap_fdatawait(inode->i_mapping);
-		if (rc) {
-			mapping_set_error(inode->i_mapping, rc);
+		if (rc)
 			return rc;
-		}
 	}
 
 	rc = cifs_revalidate_dentry_attr(dentry);
@@ -2171,15 +2169,12 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 	 * Attempt to flush data before changing attributes. We need to do
 	 * this for ATTR_SIZE and ATTR_MTIME for sure, and if we change the
 	 * ownership or mode then we may also need to do this. Here, we take
-	 * the safe way out and just do the flush on all setattr requests. If
-	 * the flush returns error, store it to report later and continue.
+	 * the safe way out and just do the flush on all setattr requests.
 	 *
 	 * BB: This should be smarter. Why bother flushing pages that
-	 * will be truncated anyway? Also, should we error out here if
-	 * the flush returns error?
+	 * will be truncated anyway?
 	 */
-	rc = filemap_write_and_wait(inode->i_mapping);
-	mapping_set_error(inode->i_mapping, rc);
+	filemap_write_and_wait(inode->i_mapping);
 	rc = 0;
 
 	if (attrs->ia_valid & ATTR_SIZE) {
@@ -2314,15 +2309,12 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	 * Attempt to flush data before changing attributes. We need to do
 	 * this for ATTR_SIZE and ATTR_MTIME for sure, and if we change the
 	 * ownership or mode then we may also need to do this. Here, we take
-	 * the safe way out and just do the flush on all setattr requests. If
-	 * the flush returns error, store it to report later and continue.
+	 * the safe way out and just do the flush on all setattr requests.
 	 *
 	 * BB: This should be smarter. Why bother flushing pages that
-	 * will be truncated anyway? Also, should we error out here if
-	 * the flush returns error?
+	 * will be truncated anyway?
 	 */
-	rc = filemap_write_and_wait(inode->i_mapping);
-	mapping_set_error(inode->i_mapping, rc);
+	filemap_write_and_wait(inode->i_mapping);
 	rc = 0;
 
 	if (attrs->ia_valid & ATTR_SIZE) {
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629565 — [PATCH v3 19/20] jbd2: don't reset error in journal_finish_inode_data_buffers

Now that we don't clear writeback errors after fetching them, there is
no need to reset them. This is also potentially racy.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/jbd2/commit.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index b6b194ec1b4f..4c6262652028 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -264,17 +264,8 @@ static int journal_finish_inode_data_buffers(journal_t *journal,
 		jinode->i_flags |= JI_COMMIT_RUNNING;
 		spin_unlock(&journal->j_list_lock);
 		err = filemap_fdatawait(jinode->i_vfs_inode->i_mapping);
-		if (err) {
-			/*
-			 * Because AS_EIO is cleared by
-			 * filemap_fdatawait_range(), set it again so
-			 * that user process can get -EIO from fsync().
-			 */
-			mapping_set_error(jinode->i_vfs_inode->i_mapping, -EIO);
-
-			if (!ret)
-				ret = err;
-		}
+		if (err && !ret)
+			ret = err;
 		spin_lock(&journal->j_list_lock);
 		jinode->i_flags &= ~JI_COMMIT_RUNNING;
 		smp_mb();
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629576 — [PATCH v3 02/20] mm: fix mapping_set_error call in me_pagecache_dirty

The error code should be negative. Since this ends up in the default
case anyway, this is harmless, but it's less confusing to negate it.
Also, later patches will require a negative error code here.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
---
 mm/memory-failure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 27f7210e7fab..4b56e53e5378 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -674,7 +674,7 @@ static int me_pagecache_dirty(struct page *p, unsigned long pfn)
 		 * the first EIO, but we're not worse than other parts
 		 * of the kernel.
 		 */
-		mapping_set_error(mapping, EIO);
+		mapping_set_error(mapping, -EIO);
 	}
 
 	return me_pagecache_clean(p, pfn);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1629579 — [PATCH v3 05/20] orangefs: don't call filemap_write_and_wait from fsync

Orangefs doesn't do buffered writes yet, so there's no point in
initiating and waiting for writeback.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/orangefs/file.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index e6bbc8083d77..17ab42c4db52 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -646,14 +646,11 @@ static int orangefs_fsync(struct file *file,
 		       loff_t end,
 		       int datasync)
 {
-	int ret = -EINVAL;
+	int ret;
 	struct orangefs_inode_s *orangefs_inode =
 		ORANGEFS_I(file_inode(file));
 	struct orangefs_kernel_op_s *new_op = NULL;
 
-	/* required call */
-	filemap_write_and_wait_range(file->f_mapping, start, end);
-
 	new_op = op_alloc(ORANGEFS_VFS_OP_FSYNC);
 	if (!new_op)
 		return -ENOMEM;
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web