write_cache_pages fixes

OK, here is a submission for the bugs and small improvements I saw for write_cache_pages. This does not include any work on the "livelock" issue (which Mikulas and myself have different approaches to fix), but I think these patches should go in first because they're fixing actual hard bugs (and just a couple of minor improvements).

I've given it some testing, but there are so many combinations of behaviours here, that I'd like some fsdevel people to look over it (eg. I didn't test with an AOP_WRITEPAGE_TRUNCATE filesystem, or a range_cyclic one).

In write_cache_pages, scanned == 1 is supposed to mean that cyclic writeback has circled through zero, thus we should not circle again. However it gets set to 1 after the first successful pagevec lookup. This leads to cases where not enough data gets written.

Counterexample: file with first 10 pages dirty, writeback_index == 5, nr_to_write == 10. Then the 5 last pages will be found, and scanned will be set to 1, after writing those out, we will not cycle back to get the first 5.

Rework this logic, now we'll always cycle unless we started off from index 0. When cycling, only write out as far as 1 page before the start page from the first cycle (so we don't write parts of the file twice).

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -872,9 +872,10 @@ int write_cache_pages(struct address_spa
 	int done = 0;
 	struct pagevec pvec;
 	int nr_pages;
+	pgoff_t writeback_index;
 	pgoff_t index;
 	pgoff_t end;		/* Inclusive */
-	int scanned = 0;
+	int cycled;
 	int range_whole = 0;
 
 	if (wbc->nonblocking && bdi_write_congested(bdi)) {

@@ -884,14 +885,19 @@ int write_cache_pages(struct address_spa
 
 	pagevec_init(&pvec, 0);
 	if (wbc->range_cyclic) {
-		index = mapping->writeback_index; /* Start from prev offset */
+		writeback_index = mapping->writeback_index; /* prev offset */
+		index = writeback_index;
+		if (index == 0)
+			cycled = 1;
+		else
+			cycled = 0;
 		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;
+		cycled = 1; /* ignore range_cyclic tests */
 	}
 retry:
 	while (!done && (index <= end) &&

@@ -900,7 +906,6 @@ retry:
 					      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];
 

@@ -918,7 +923,8 @@ retry:
 				continue;
 			}
 
-			if (!wbc->range_cyclic && page->index > end) {
+			if (page->index > end) {
+				/* Can't be range_cyclic: end == -1 there */
 				done = 1;
 				unlock_page(page);
 				continue;

@@ -949,13 +955,15 @@ retry:
 		pagevec_release(&pvec);
 		cond_resched();
 	}
-	if (!scanned && !done) {
+	if (!cycled) {
 		/*
+		 * range_cyclic:
 		 * We hit the last page and there is more work to be done: wrap
 		 * back to the start of the file
 		 */
-		scanned = 1;
+		cycled = 1;
 		index = 0;
+		end = writeback_index - 1;
 		goto retry;
 	}
 	if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0))

In write_cache_pages, if AOP_WRITEPAGE_ACTIVATE is returned, the filesystem is calling on us to drop the page lock and retry, however the existing code would just skip that page regardless of whether or not it was a data interity operation. Change this to always retry such a result.

This is a data interity bug.

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -916,6 +916,7 @@ retry:
 			 * swizzled back from swapper_space to tmpfs file
 			 * mapping
 			 */
+again:
 			lock_page(page);
 
 			if (unlikely(page->mapping != mapping)) {

@@ -940,10 +941,11 @@ retry:
 			}
 
 			ret = (*writepage)(page, wbc, data);
-
 			if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
+				/* Must retry the write */
 				unlock_page(page);
 				ret = 0;
+				goto again;
 			}
 			if (ret || (--(wbc->nr_to_write) <= 0))
 				done = 1;

Are you sure? It's not what fs.h says. I think this return value is related to reclaim (and only used by shmfs), and retrying is not the right thing in that case.

Miklos

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Only used by shmfs nowadays, yes; it means go away for now, don't keep on spamming me with this, but try it again later on.

Though I didn't invent it, it's very much my fault that it still exists: I've had a patch to remove it (setting PageActive instead, ending that horrid "but in this case, return with the page still locked") for about a year, but still hadn't got around to verifying that it really does what's intended, before the more interesting split-lru changes reached -mm, and I thought it polite to hold off for now (though in fact there's almost no conflict). I'll get there...

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oh, you're absolutely right about that. Sorry, I confused it with another AOP flag :( Thanks...

No big deal.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

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.

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -941,13 +941,17 @@ again:
 			}
 
 			ret = (*writepage)(page, wbc, data);
-			if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
-				/* Must retry the write */
-				unlock_page(page);
-				ret = 0;
-				goto again;
+			if (unlikely(ret)) {
+				if (ret == AOP_WRITEPAGE_ACTIVATE) {
+					/* Must retry the write */
+					unlock_page(page);
+					ret = 0;
+					goto again;
+				}
+				done = 1;
+				break;
 			}
-			if (ret || (--(wbc->nr_to_write) <= 0))
+			if (--(wbc->nr_to_write) <= 0)
 				done = 1;
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {
 				wbc->encountered_congestion = 1;

In the range_cont case, range_start is set to index << PAGE_CACHE_SHIFT, but index is a pgoff_t and range_start is loff_t, so we can get truncation of the value on 32-bit platforms. Fix this by adding the standard loff_t cast.

This is a data interity bug (depending on how range_cont is used).

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -976,7 +976,7 @@ again:
 		mapping->writeback_index = index;
 
 	if (wbc->range_cont)
-		wbc->range_start = index << PAGE_CACHE_SHIFT;
+		wbc->range_start = (loff_t)index << PAGE_CACHE_SHIFT;
 	return ret;
 }
 EXPORT_SYMBOL(write_cache_pages);

Aneesh has a patch to kill the range_cont flag, which is queued up for 2.6.28.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

OK, great. I guess actually this patch out of all of them could go into 2.6.27 and previous stable kernels because it is obviously correct and could not really cause a regression.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Which tree is this queued up in? It's not in ext4 or the mm tree...

- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Oh, it' not queued up yet? It's part of the patch that switches ext4 to it's own copy of write_cache_pages to fix the buffer write issues.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I held off queing it up since the version Aneesh did created ext4's own copy of write_cache_pages, and given that Nick has a bunch of fixes and improvements for write_cache_pages, it confirmed my fears that queueing a patch which copied ~100 lines of code into ext4 was probably not the best way to go.

That being said, I would dearly love to see the 10x improvement in streaming write speed for ext4 make the 2.6.28 merge window, since people will start benchmarking ext4 much more seriously in the near future. So, I was tempted to queue Aneesh's original version of the fix --- but if we're going to get a version of the change which gets the required changes into to the generic write_cache_pages(), I'm not sure we have the time to coordinate ext4's and other filesystems' needs into Nick's RFC patchset.

It wouldn't be hard to create a version of Aneesh's patch which makes the change to the original write_cache_pages(), instead of creating our own copy of ext4_write_cache_pages(), but it hasn't been done yet, so it hasn't been queued. We can probably do *that* pretty quickly, and send it out for review, but it would almost certainly conflict with Nick's patchset --- and I had assumed that Nick might be interested in pushing this during this merge window given his concern about data integrity correctness issues.

So I think the main issue here is coordinating planned changes to core code....

- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hi,

I've not looked at ext4's copy of write_cache_pages, but there is also a copy in GFS2. Its used only for journaled data, and it is pretty much a direct copy of write_cache_pages except that its split into two so that a transaction can be opened in the "middle".

Perhaps it would be possible to make some changes so that we can both share the "core" version of write_cache_pages. My plan was to wait until Nick's patches have made it to Linus, and then to look into what might be done,

Steve.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

To be clear; ext4 doesn't have its own copy of write_cache_pages (at least not yet); there is a patch that creates our own copy, mainly to disable updates to writeback_index, range_start, and nr_to_write in the wbc structure.

Christoph has suggested a patch which modifies write_cache_pages() so that a filesystem could set a flag which disables those updates, instead of just making a copy of write_cache_pages. (Maybe eventually we would get rid of those updates unconditionally; it's not clear to me though that this makes sense for all filesystems.)

So we have three choices as far as getting the 10x for (large) streaming write performance into 2.6.28:

1) Aneesh's first patch, which called write_cache_pages() and then undid the effects of the updates to the relevant wbc fields.
(http://lkml.org/lkml/2008/10/6/61)

2) Aneesh's second version of the patch, which copied write_cache_pages() into ext4_write_cache_pages() and then removed the updates; resulting in a large patch, but one that might be easier to understand, although harder to maintain in the long term.
(http://lkml.org/lkml/2008/10/7/52)

3) A version which (optionally via a flag in the wbc structure) instructs write_cache_pages() to not pursue those updates. This has not been written yet.

For why we need to do this, see Aneesh's explanation here:

    http://lkml.org/lkml/2008/10/7/78

If we don't think the Nick's patches are going to be stable enough for merging in time for 2.6.28, it's possible we could pursue (1) or (2), and if there's -mm concurrence, even (3). (1) might be the best if the goal is to wait for Nick's patches to hit mainline first, and then we can try to sort and merge our per-filesystems unique hacks (or copies of write_cache_pages or whatever) back to the upstream version.

All of this being said, I'll confess that I have *not* had time to look deeply at Nick's full patchset yet. Which has been another reason why I haven't queued up any of Aneesh's patches in this area yet; all of this hit just right before the merge window opened up, and I've been insanely busy as of late.

- Ted

This one sounds best to me (although we'd have to actualy see it..)

something like the below ?

index bd91987..7599af2 100644
@@ -63,6 +63,8 @@ struct writeback_control {
 	unsigned for_writepages:1;	/* This is a writepages() call */
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 	unsigned more_io:1;		/* more io to be dispatched */
+	/* flags which control the write_cache_pages behaviour */
+	int writeback_flags;
 };
 
 /*

index 718efa6..c198ead 100644
@@ -876,11 +876,18 @@ int write_cache_pages(struct address_space *mapping,
 	pgoff_t end;		/* Inclusive */
 	int scanned = 0;
 	int range_whole = 0;
+	int flags = wbc->writeback_flags;
+	long *nr_to_write, count;
 
 	if (wbc->nonblocking && bdi_write_congested(bdi)) {
 		wbc->encountered_congestion = 1;
 		return 0;
 	}
+	if (flags & WB_NO_NRWRITE_UPDATE) {
+		count  = wbc->nr_to_write;
+		nr_to_write = &count;
+	} else
+		nr_to_write = &wbc->nr_to_write;
 
 	pagevec_init(&pvec, 0);
 	if (wbc->range_cyclic) {

@@ -939,7 +946,7 @@ int write_cache_pages(struct address_space *mapping,
 				unlock_page(page);
 				ret = 0;
 			}
-			if (ret || (--(wbc->nr_to_write) <= 0))
+			if (ret || (--(*nr_to_write) <= 0))
 				done = 1;
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {
 				wbc->encountered_congestion = 1;

@@ -958,8 +965,11 @@ int write_cache_pages(struct address_space *mapping,
 		index = 0;
 		goto retry;
 	}
-	if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0))
+	if ((wbc->range_cyclic ||
+			(range_whole && wbc->nr_to_write > 0)) && 
+			(flags & ~WB_NO_INDEX_UPDATE)) {
 		mapping->writeback_index = index;
+	}
 
 	return ret;
 }

What I was hoping for when I suggested copying it in was a larger move of logic from the ext4 writepages code into the ext4 write_cache_pages. The idea was to see what ext4 really needed write_cache_pages to do, but Aneesh knows much better than I do there.

But, given that the change was only a few lines, I think it makes sense to fold it back into write_cache_pages.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

In write_cache_pages, nr_to_write is heeded even for data-integrity syncs, so the function will return success after writing out nr_to_write pages, even if that was not sufficient to guarantee data integrity.

The callers tend to set it to values that could break data interity semantics easily in practice. For example, nr_to_write can be set to mapping->nr_pages * 2, however if a file has a single, dirty page, then fsync is called, subsequent pages might be concurrently added and dirtied, then write_cache_pages might writeout two of these newly dirty pages, while not writing out the old page that should have been written out.

Fix this by ignoring nr_to_write if it is a data integrity sync.

This is a data interity bug.

---
The reason this has been done in the past is to avoid stalling sync operations behind page dirtiers.

"If a file has one dirty page at offset 1000000000000000 then someone
does an fsync() and someone else gets in first and starts madly writing pages at offset 0, we want to write that page at 1000000000000000. Somehow."

What we to today is return success after an arbitrary amount of pages are written, whether or not we have provided the data-integrity semantics that the caller has asked for. Even this doesn't actually fix all stall cases
completely: in the above situation, if the file has a huge number of pages in pagecache (but not dirty), then mapping->nrpages is going to be huge, even if pages are being dirtied.

This change does indeed make the possibility of long stalls lager, and that's not a good thing, but lying about data integrity is even worse. We have to either perform the sync, or return -ELINUXISLAME so at least the caller knows what has happened.

There are subsequent competing approaches in the works to solve the stall problems properly, without compromising data integrity.

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -951,8 +951,10 @@ again:
 				done = 1;
 				break;
 			}
-			if (--(wbc->nr_to_write) <= 0)
-				done = 1;
+			if (wbc->sync_mode == WB_SYNC_NONE) {
+				if (--(wbc->nr_to_write) <= 0)
+					done = 1;
+			}
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {
 				wbc->encountered_congestion = 1;
 				done = 1;

Index: linux-2.6/mm/filemap.c
===================================================================
@@ -209,7 +209,7 @@ int __filemap_fdatawrite_range(struct ad
 	int ret;
 	struct writeback_control wbc = {
 		.sync_mode = sync_mode,
-		.nr_to_write = mapping->nrpages * 2,
+		.nr_to_write = LONG_MAX,
 		.range_start = start,
 		.range_end = end,
 	};

Thanks for working on these.

We should have a wbc->integrity flag because WB_SYNC_NONE is somewhat over used, and it is often used in data integrity syncs.

See fs/sync.c:do_sync_mapping_range()

There are many valid uses where we don't want to wait on pages that are already writeback but we do want to write everything else.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No problem. Actually I feel I would be negligent for knowingly shipping a kernel with these bugs :( So I don't have much choice...

Oh great, more data integrity bugs.

I've always disliked the sync_file_range API ;) it seems over complex and introduces the concept of writeout to userspace that seems questionable to me. I should add SYNC_FILE_RANGE_ASYNC and SYNC_FILE_RANGE_SYNC for people who already know POSIX and just want to convert existing fsync or msync to a file and range based API, and also get a proper async operation that isn't bound to kick off writeback for every page but could hand off page cleaning to another thread...

Anyway, quick fix says we have to change that WB_SYNC_NONE into WB_SYNC_ALL. WB_SYNC_NONE is all over the kernel. So do_sync_mapping_range is broken as-is.

Hmm. For this strange "synchronously start writeback but don't have to wait on it" operation, maybe. But that's not a great async operation because for big ranges or under any sort of load, it's going to block on congested block devices anyway. Are there any other uses than this one?

However I don't have any problem with improving it if it is useful. But for now...
---

Chris Mason notices do_sync_mapping_range didn't actually ask for data integrity writeout. Oops.

---

Index: linux-2.6/fs/sync.c
===================================================================
@@ -269,7 +269,7 @@ int do_sync_mapping_range(struct address
 
 	if (flags & SYNC_FILE_RANGE_WRITE) {
 		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
-						WB_SYNC_NONE);
+						WB_SYNC_ALL);
 		if (ret < 0)
 			goto out;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I don't think do_sync_mapping_range is broken as is. It simply splits the operations into different parts. The caller can request that we wait for pending IO first.

WB_SYNC_NONE none just means don't wait for IO in flight, and there are valid uses for it that will slow down if you switch them all to WB_SYNC_ALL.

The problem is that we have a few flags and callers that mean almost but not quite the same thing. Some people confuse WB_SYNC_NONE with wbc->nonblocking.

I'd leave WB_SYNC_NONE alone and set wbc->nr_to_write to the max int, or just make a new flag that says write every dirty page in this range.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

It is. Not because of it's whacky API, but because it uses WB_SYNC_NONE.

To write_cache_pages it means that, but further down the chain (eg. block_write_full_page) it also means not to wait on other stuff.

It has broadly meant "don't worry about data integirty" for a long time AFAIKS.

Well it wouldn't hurt to clarify things. I'd rather introduce a new WB_SYNC_WRITEBACK or something for that guy. But anyway, as I said, it's just a minimal fix.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sadly it has broadly meant different things to different people ;) You're right, block_write_full_page is broken.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Well, I really just think it is do_sync_mapping_range that is broken. Because __sync_single_inode treats WB_SYNC_NONE as a general "nowait", so does __writeback_single_inode. Weakest semantics define the API :)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Unfortunately these things are using the flag differently __sync_single_inode and __writeback_single_inode do different things with the flag than people that end up directly calling the writepages methods.

At the write_cache_pages level, WB_SYNC_NONE should only change the waiting for IO in flight.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sure, but it's the "nowait" semantics that they want, right? And *they* eventually call into writepages. So they want similar semantics from writepages, presumably.

The comment in WB_SYNC_NONE definition kind of suggests it meant don't wait for anything when it was written...

Aside from do_sync_mapping_range, what are other reasons to enforce the same thing all up and down the writeout stack? If there are good reasons, let's add WB_SYNC_WRITEBACK?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

That seems to have turned into wbc->nonblocking. WB_SYNC_NONE doesn't stop other blocking inside the FS (delalloc and other fun).

Your change to skip writeback pages that aren't dirty makes WB_SYNC_ALL almost the same as WB_SYNC_WRITEBACK. With that in place we're pretty deep into grey areas where people may not want to go around rewriting pages that were dirtied after their sync began.

At least that's what I think the idea behind do_sync_mapping_range using WB_SYNC_NONE was.

-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Right, and neither does nonblocking in all cases ;)

Anyway, I completely agree that it is unclear at best and could really use a spring clean of the fields and their semantics.

Yeah, they definitely do, though that's a slightly different problem. At the moment, any dirty pages found in a data integrity operation *must* be written out. Because we have no idea when they were dirtied. This is how sync can get stuck behind write(2) for a long time (and this is why our sync has traditionally bailed out after ->nrpages*2).

I have further patches to add a new tag to the radix-tree to mark all the pages to sync up-front to solve this nicely. Mikulas has a different approach to instead throttle the dirtiers. Whichever approach is favoured should be the next step after this round of patches.

do_sync_mapping_range indeed can ignore dirty,writeback pages, because its data integrity operation would wait for writeback, then write dirty, then wait for writeback again. This is quite a corner-case, for its unusual semantics though. You may just as well not wait for writeback to start with, but wait for them in the writeout pass (and only if they are dirty):
that will likely be as fast or faster anyway.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Get rid of some complex expressions from flow control statements, add a comment, remove some duplicate code.

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -900,11 +900,14 @@ int write_cache_pages(struct address_spa
 		cycled = 1; /* ignore range_cyclic tests */
 	}
 retry:
-	while (!done && (index <= end) &&
-	       (nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
-					      PAGECACHE_TAG_DIRTY,
-					      min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1))) {
-		unsigned i;
+	while (!done && (index <= end)) {
+		int i;
+
+		nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
+			      PAGECACHE_TAG_DIRTY,
+			      min(end - index, (pgoff_t)PAGEVEC_SIZE-1) + 1);
+		if (nr_pages == 0)
+			break;
 
 		for (i = 0; i < nr_pages; i++) {
 			struct page *page = pvec.pages[i];

@@ -919,7 +922,16 @@ retry:
 again:
 			lock_page(page);
 
+			/*
+			 * Page truncated or invalidated. We can freely skip it
+			 * then, even for data integrity operations: the page
+			 * has disappeared concurrently, so there could be no
+			 * real expectation of this data interity operation
+			 * even if there is now a new, dirty page at the same
+			 * pagecache address.
+			 */
 			if (unlikely(page->mapping != mapping)) {
+continue_unlock:
 				unlock_page(page);
 				continue;
 			}

@@ -927,18 +939,15 @@ again:
 			if (page->index > end) {
 				/* Can't be range_cyclic: end == -1 there */
 				done = 1;
-				unlock_page(page);
-				continue;
+				goto continue_unlock;
 			}
 
 			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;
-			}
+			    !clear_page_dirty_for_io(page))
+				goto continue_unlock;
 
 			ret = (*writepage)(page, wbc, data);
 			if (unlikely(ret)) {

@@ -952,7 +961,8 @@ again:
 				break;
 			}
 			if (wbc->sync_mode == WB_SYNC_NONE) {
-				if (--(wbc->nr_to_write) <= 0)
+				wbc->nr_to_write--;
+				if (wbc->nr_to_write <= 0)
 					done = 1;
 			}
 			if (wbc->nonblocking && bdi_write_congested(bdi)) {

In write_cache_pages, if we get stuck behind another process that is cleaning pages, we will be forced to wait for them to finish, then perform our own writeout (if it was redirtied during the long wait), then wait for that.

If a page under writeout is still clean, we can skip waiting for it (if we're part of a data integrity sync, we'll be waiting for all writeout pages afterwards, so we'll still be waiting for the other guy's write that's cleaned the page).

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -942,11 +942,20 @@ continue_unlock:
 				goto continue_unlock;
 			}
 
-			if (wbc->sync_mode != WB_SYNC_NONE)
-				wait_on_page_writeback(page);
+			if (!PageDirty(page)) {
+				/* someone wrote it for us */
+				goto continue_unlock;
+			}
+
+			if (PageWriteback(page)) {
+				if (wbc->sync_mode != WB_SYNC_NONE)
+					wait_on_page_writeback(page);
+				else
+					goto continue_unlock;
+			}
 
-			if (PageWriteback(page) ||
-			    !clear_page_dirty_for_io(page))
+			BUG_ON(PageWriteback(page));
+			if (!clear_page_dirty_for_io(page))
 				goto continue_unlock;
 
 			ret = (*writepage)(page, wbc, data);

Terminate the write_cache_pages loop upon encountering the first page past end, without locking the page. Pages cannot have their index change when we have a reference on them (truncate, eg truncate_inode_pages_range performs the same check without the page lock).

---

Index: linux-2.6/mm/page-writeback.c
===================================================================
@@ -913,12 +913,18 @@ retry:
 			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
+			 * At this point, the page may be truncated or
+			 * invalidated (changing page->mapping to NULL), or
+			 * even swizzled back from swapper_space to tmpfs file
+			 * mapping. However, page->index will not change
+			 * because we have a reference on the page.
 			 */
+			if (page->index > end) {
+				/* Can't be range_cyclic: end == -1 there */
+				done = 1;
+				break;
+			}
+
 again:
 			lock_page(page);
 

@@ -936,12 +942,6 @@ continue_unlock:
 				continue;
 			}
 
-			if (page->index > end) {
-				/* Can't be range_cyclic: end == -1 there */
-				done = 1;
-				goto continue_unlock;
-			}
-
 			if (!PageDirty(page)) {
 				/* someone wrote it for us */
 				goto continue_unlock;