mm/page-writeback.c: don't break integrity writeback on ->writepage() error

write_cache_pages() is used in both background and integrity writeback
scenarios by various filesystems.  Background writeback is mostly
concerned with cleaning a certain number of dirty pages based on various
mm heuristics.  It may not write the full set of dirty pages or wait for
I/O to complete.  Integrity writeback is responsible for persisting a set
of dirty pages before the writeback job completes.  For example, an
fsync() call must perform integrity writeback to ensure data is on disk
before the call returns.

write_cache_pages() unconditionally breaks out of its processing loop in
the event of a ->writepage() error.  This is fine for background
writeback, which had no strict requirements and will eventually come
around again.  This can cause problems for integrity writeback on
filesystems that might need to clean up state associated with failed page
writeouts.  For example, XFS performs internal delayed allocation
accounting before returning a ->writepage() error, where applicable.  If
the current writeback happens to be associated with an unmount and
write_cache_pages() completes the writeback prematurely due to error, the
filesystem is unmounted in an inconsistent state if dirty+delalloc pages
still exist.

To handle this problem, update write_cache_pages() to always process the
full set of pages for integrity writeback regardless of ->writepage()
errors.  Save the first encountered error and return it to the caller once
complete.  This facilitates XFS (or any other fs that expects integrity
writeback to process the entire set of dirty pages) to clean up its
internal state completely in the event of persistent mapping errors.
Background writeback continues to exit on the first error encountered.

[akpm@linux-foundation.org: fix typo in comment]
Link: http://lkml.kernel.org/r/20181116134304.32440-1-bfoster@redhat.com
Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 3f690bae6b780304ad31ea04e999a3f377c35991..7d1010453fb95a26c13e9004999d028659815987 100644 (file)
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2154,6 +2154,7 @@ int write_cache_pages(struct address_space *mapping,
 {
        int ret = 0;
        int done = 0;
+       int error;
        struct pagevec pvec;
        int nr_pages;
        pgoff_t uninitialized_var(writeback_index);
@@ -2227,25 +2228,31 @@ continue_unlock:
                                goto continue_unlock;
 
                        trace_wbc_writepage(wbc, inode_to_bdi(mapping->host));
-                       ret = (*writepage)(page, wbc, data);
-                       if (unlikely(ret)) {
-                               if (ret == AOP_WRITEPAGE_ACTIVATE) {
+                       error = (*writepage)(page, wbc, data);
+                       if (unlikely(error)) {
+                               /*
+                                * Handle errors according to the type of
+                                * writeback. There's no need to continue for
+                                * background writeback. Just push done_index
+                                * past this page so media errors won't choke
+                                * writeout for the entire file. For integrity
+                                * writeback, we must process the entire dirty
+                                * set regardless of errors because the fs may
+                                * still have state to clear for each page. In
+                                * that case we continue processing and return
+                                * the first error.
+                                */
+                               if (error == AOP_WRITEPAGE_ACTIVATE) {
                                        unlock_page(page);
-                                       ret = 0;
-                               } else {
-                                       /*
-                                        * done_index is set past this page,
-                                        * so media errors will not choke
-                                        * background writeout for the entire
-                                        * file. This has consequences for
-                                        * range_cyclic semantics (ie. it may
-                                        * not be suitable for data integrity
-                                        * writeout).
-                                        */
+                                       error = 0;
+                               } else if (wbc->sync_mode != WB_SYNC_ALL) {
+                                       ret = error;
                                        done_index = page->index + 1;
                                        done = 1;
                                        break;
                                }
+                               if (!ret)
+                                       ret = error;
                        }
 
                        /*