writeback data integrity and other fixes

In write_cache_pages, if ret signals a real error, but we still have some pages left in the pagevec, done would be set to 1, but the remaining pages would continue to be processed and ret will be overwritten in the process. It could easily be overwritten with success, and thus success will be returned even if there is an error. Thus the caller is told all writes succeeded, wheras in reality some did not.

Fix this by bailing immediately if there is an error, and retaining the first error code.

This is a data interity bug.

Signed-off-by Nick Piggin <[email protected]>

---
Index: linux-2.6/mm/page-writeback.c
===================================================================

mm/page-writeback.c +10 -6

@@ -931,12 +931,16 @@ retry:
 			}
 
 			ret = (*writepage)(page, wbc, data);
-
-			if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
-				unlock_page(page);
-				ret = 0;
-			}
-			if (ret || (--nr_to_write <= 0))
+			if (unlikely(ret)) {
+				if (ret == AOP_WRITEPAGE_ACTIVATE) {
+					unlock_page(page);
+					ret = 0;
+				} else {
+					done = 1;
+					break;
+				}
+ 			}
+			if (--nr_to_write <= 0)
 				done = 1;
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {
 				wbc->encountered_congestion = 1;

Signature · 1 line

-- 

Review retry:

Quoted 1 line · Andrew Morton

(quoted diff) +0 -0

@@ -916,8 +915,8 @@ retry:

                      ^^^^^^

Hey, look everyone, our tools are completely shit.

mm/page-writeback.c retry: · line 919

Quoted 6 lines

mm/page-writeback.c renamed from mm/page-writeback.c~mm-write_cache_pages-writepage-error-fix +1 -1

 			}
 
 			if (!wbc->range_cyclic && page->index > end) {
-				done = 1;
 				unlock_page(page);
+				goto bale;

I think you mean "bail". Unless you mean making hay, or the 1913 Webster definition:
1. Misery; calamity; misfortune; sorrow.
which would be kind of appropriate, but I think you mean bail out.

Quoted 43 lines · On Tue, 21 Oct 2008 19:09:48 +1100 [email protected] wrote:

In write_cache_pages, if ret signals a real error, but we still have some pages left in the pagevec, done would be set to 1, but the remaining pages would continue to be processed and ret will be overwritten in the process. It could easily be overwritten with success, and thus success will be returned even if there is an error. Thus the caller is told all writes succeeded, wheras in reality some did not.

Fix this by bailing immediately if there is an error, and retaining the first error code.

This is a data interity bug.

Signed-off-by Nick Piggin <[email protected]>

---
Index: linux-2.6/mm/page-writeback.c
===================================================================

mm/page-writeback.c +10 -6

@@ -931,12 +931,16 @@ retry:
 			}
 
 			ret = (*writepage)(page, wbc, data);
-
-			if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
-				unlock_page(page);
-				ret = 0;
-			}
-			if (ret || (--nr_to_write <= 0))
+			if (unlikely(ret)) {
+				if (ret == AOP_WRITEPAGE_ACTIVATE) {
+					unlock_page(page);
+					ret = 0;
+				} else {
+					done = 1;
+					break;
+				}
+ 			}
+			if (--nr_to_write <= 0)
 				done = 1;
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {
 				wbc->encountered_congestion = 1;

Signature · 1 line

-- 

I don't think I like the implementation much.

In all cases in that function where we set done=1, we want to bale out right now at this page, rather than processing the remaining pages in the pagevec.

So it would be better to implement a bit of code which releases the pagevec pages and then breaks out of the loop. Then this bug automatically gets fixed.

Like this:

mm/page-writeback.c renamed from mm/page-writeback.c~mm-write_cache_pages-writepage-error-fix +14 -11

@@ -865,7 +865,6 @@ int write_cache_pages(struct address_spa
 {
 	struct backing_dev_info *bdi = mapping->backing_dev_info;
 	int ret = 0;
-	int done = 0;
 	struct pagevec pvec;
 	int nr_pages;
 	pgoff_t index;

@@ -891,10 +890,10 @@ int write_cache_pages(struct address_spa
 		scanned = 1;
 	}
 retry:
-	while (!done && (index <= end) &&
+	while ((index <= end) &&
 	       (nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
-					      PAGECACHE_TAG_DIRTY,
-					      min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1))) {
+				PAGECACHE_TAG_DIRTY,
+				min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1))) {
 		unsigned i;
 
 		scanned = 1;

@@ -903,9 +902,9 @@ retry:
 
 			/*
 			 * At this point we hold neither mapping->tree_lock nor
-			 * lock on the page itself: the page may be truncated or
-			 * invalidated (changing page->mapping to NULL), or even
-			 * swizzled back from swapper_space to tmpfs file
+			 * lock on the page itself: the page may be truncated
+			 * or invalidated (changing page->mapping to NULL), or
+			 * even swizzled back from swapper_space to tmpfs file
 			 * mapping
 			 */
 			lock_page(page);

@@ -916,8 +915,8 @@ retry:
 			}
 
 			if (!wbc->range_cyclic && page->index > end) {
-				done = 1;
 				unlock_page(page);
+				goto bale;
 				continue;
 			}
 

@@ -937,16 +936,16 @@ retry:
 				ret = 0;
 			}
 			if (ret || (--nr_to_write <= 0))
-				done = 1;
+				goto bale;
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {
 				wbc->encountered_congestion = 1;
-				done = 1;
+				goto bale;
 			}
 		}
 		pagevec_release(&pvec);
 		cond_resched();
 	}
-	if (!scanned && !done) {
+	if (!scanned) {
 		/*
 		 * We hit the last page and there is more work to be done: wrap
 		 * back to the start of the file

@@ -961,7 +960,11 @@ retry:
 		wbc->nr_to_write = nr_to_write;
 	}
 
+out:
 	return ret;
+bale:
+	pagevec_release(&pvec);
+	goto out;
 }
 EXPORT_SYMBOL(write_cache_pages);
 

_

Which yields this:

int write_cache_pages(struct address_space *mapping,
struct writeback_control *wbc, writepage_t writepage, void *data)

{
	struct backing_dev_info *bdi = mapping->backing_dev_info;
	int ret = 0;
	struct pagevec pvec;
	int nr_pages;
	pgoff_t index;
	pgoff_t end;		/* Inclusive */
	int scanned = 0;
	int range_whole = 0;
	long nr_to_write = wbc->nr_to_write;

	if (wbc->nonblocking && bdi_write_congested(bdi)) {
		wbc->encountered_congestion = 1;
		return 0;
	}

	pagevec_init(&pvec, 0);
	if (wbc->range_cyclic) {
		index = mapping->writeback_index; /* Start from prev offset */
		end = -1;
	} else {
		index = wbc->range_start >> PAGE_CACHE_SHIFT;
		end = wbc->range_end >> PAGE_CACHE_SHIFT;
		if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX)
			range_whole = 1;
		scanned = 1;
	}
retry:
	while ((index <= end) &&
	       (nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
				PAGECACHE_TAG_DIRTY,
				min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1))) {
		unsigned i;

		scanned = 1;
		for (i = 0; i < nr_pages; i++) {
			struct page *page = pvec.pages[i];

			/*
			 * At this point we hold neither mapping->tree_lock nor
			 * lock on the page itself: the page may be truncated
			 * or invalidated (changing page->mapping to NULL), or
			 * even swizzled back from swapper_space to tmpfs file
			 * mapping
			 */
			lock_page(page);

			if (unlikely(page->mapping != mapping)) {
				unlock_page(page);
				continue;
			}

			if (!wbc->range_cyclic && page->index > end) {
				unlock_page(page);
				goto bale;
				continue;
			}

			if (wbc->sync_mode != WB_SYNC_NONE)
				wait_on_page_writeback(page);

			if (PageWriteback(page) ||
			    !clear_page_dirty_for_io(page)) {
				unlock_page(page);
				continue;
			}

			ret = (*writepage)(page, wbc, data);

			if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
				unlock_page(page);
				ret = 0;
			}
			if (ret || (--nr_to_write <= 0))
				goto bale;
			if (wbc->nonblocking && bdi_write_congested(bdi)) {
				wbc->encountered_congestion = 1;
				goto bale;
			}
		}
		pagevec_release(&pvec);
		cond_resched();
	}
	if (!scanned) {
		/*
		 * We hit the last page and there is more work to be done: wrap
		 * back to the start of the file
		 */
		scanned = 1;
		index = 0;
		goto retry;
	}
	if (!wbc->no_nrwrite_index_update) {
		if (wbc->range_cyclic || (range_whole && nr_to_write > 0))
			mapping->writeback_index = index;
		wbc->nr_to_write = nr_to_write;
	}

out:

	return ret;
bale:
	pagevec_release(&pvec);
	goto out;
}

Quoted 6 lines

Quoted 3 lines

wbc->encountered_congestion = 1;

I don't think I like the implementation much.

Quoted 7 lines

In all cases in that function where we set done=1, we want to bale out right now at this page, rather than processing the remaining pages in the pagevec.

So it would be better to implement a bit of code which releases the pagevec pages and then breaks out of the loop. Then this bug automatically gets fixed.

I had almost exactly that at some point, then I rethought it because it was a bigger behaviour change than the bugfix-only.

I thought the !done thing must be just for batching and to process the pages already looked up in the pagevec.

I'll need to backport at least some of these to stable -- any chance of making the change you suggest toward the end of the patchset?

Let me
know if you want a patch or will do one yourself.

Quoted 14 lines · On Wed, 22 Oct 2008 11:36:32 +0200 Nick Piggin <[email protected]> wrote:

Quoted 6 lines

Quoted 3 lines

wbc->encountered_congestion = 1;

I don't think I like the implementation much.

Quoted 7 lines

In all cases in that function where we set done=1, we want to bale out right now at this page, rather than processing the remaining pages in the pagevec.

So it would be better to implement a bit of code which releases the pagevec pages and then breaks out of the loop. Then this bug automatically gets fixed.

Quoted 8 lines

I had almost exactly that at some point, then I rethought it because it was a bigger behaviour change than the bugfix-only.

I thought the !done thing must be just for batching and to process the pages already looked up in the pagevec.

I'll need to backport at least some of these to stable -- any chance of making the change you suggest toward the end of the patchset?

That works.

Quoted 2 lines

Let me
know if you want a patch or will do one yourself.

I couldn't hope to match your level of testing ;)