Re: page_launder() bug

[David S. Miller]

Marcelo Tosatti writes:
 > My point is that its _ok_ for us to check if the page is a dead swap cache
 > page _without_ the lock since writepage() will recheck again with the page
 > _locked_. Quoting you two messages back: 
 > 
 > "But it is important to re-calculate the deadness after getting the lock.
 > Before, it was just an informed guess. After the lock, it is knowledge."
 > 
 > See ? 

In fact my patch isn't changing writepage behavior wrt. that page, it
is changing behavior with respect to laundering policy for that page.

Here, let's talk code a little bit so there are no misunderstandings,
I really want to put this to rest:

+		int dead_swap_page;
+
 		page = list_entry(page_lru, struct page, lru);
 
+		dead_swap_page =
+			(PageSwapCache(page) &&
+			 page_count(page) == (1 + !!page->buffers));
+

Calculate dead_swap_page outside of lock.

 		/* Page is or was in use?  Move it to the active list. */
-		if (PageTestandClearReferenced(page) || page->age > 0 ||
-				(!page->buffers && page_count(page) > 1) ||
-				page_ramdisk(page)) {
+		if (!dead_swap_page &&
+		    (PageTestandClearReferenced(page) || page->age > 0 ||
+		     (!page->buffers && page_count(page) > 1) ||
+		     page_ramdisk(page))) {
 			del_page_from_inactive_dirty_list(page);
 			add_page_to_active_list(page);
 			continue;

If dead_swap_page, ignore referenced bit heuristics.

-			/* First time through? Move it to the back of the list */
-			if (!launder_loop) {
+			/* First time through? Move it to the back of the list,
+			 * but not if it is a dead swap page. We want to reap
+			 * those as fast as possible.
+			 */
+			if (!launder_loop && !dead_swap_page) {
 				list_del(page_lru);
 				list_add(page_lru, &inactive_dirty_list);
 				UnlockPage(page);

If dead_swap_page, ignore launder_loop.  Again, another heuristic
test, not a "state correctness" test.  "launder_loop" is not
protecting "state correctness" of what we do to the page.

Really, what does this have to do with swap counts and page counts?

It's a heuristic. In fact it even seems stupid to me to recalculate
dead_swap_page after we get the lock just for the sake of these
heuristics.

Maybe I should have diguised this bit as:

if (dead_swap_page)
	do_writepage_first_pass = 1;

To divert people's brains to what the intent was :-)

Later,
David S. Miller
[e-mail]