Re: shmem_getpage_locked() / swapin_readahead() race in 2.4.4-pre3

[Christoph Rohland <cr@sap.com>]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

Hi,

On Sat, 14 Apr 2001, Marcelo Tosatti wrote:
> There is a nasty race between shmem_getpage_locked() and
> swapin_readahead() with the new shmem code (introduced in 2.4.3-ac3
> and merged in the main tree in 2.4.4-pre3):
> 
> shmem_getpage_locked() finds a page in the swapcache and moves it to
> the pagecache as an shmem page, freeing the swapcache and the swap
> map entry for this page. (which causes a BUG() in mm/shmem.c:353
> since the swap map entry is being used)
> 
> In the meanwhile, swapin_readahead() is allocating a page and adding
> it to the swapcache.

Oh, I was just chasing this also. 

> I don't see any clean fix for this one.

I think the actual check for swap_count is not necessary: If
swapin_readahead allocates a new swap_cache page for the entry, that's
not a real bug. On memory pressure this page will be reclaimed.

Actually we have to make shmem much more unfriendly to the swap cache
to make it correct: I think we have to drop the whole drop swap cache
pages on truncate logic since it uses lookup_swap_cache and
delete_from_swap_cache which both lock the page, while holding a
spinlock :-(

The appended patch implements both changes and relies on the page
stealer to shrink the swap cache. 

It also integrates fixes which Marcelo did send earlier.

Greetings
		Christoph


[-- Attachment #2: patch-2.4.4-tmpfs-fixes --]
[-- Type: text/plain, Size: 1190 bytes --]

--- 2.4.4-pre3/mm/shmem.c	Sat Apr 14 11:12:54 2001
+++ u2.4.3/mm/shmem.c	Sun Apr 15 13:45:58 2001
@@ -123,10 +123,19 @@
 		entry = *ptr;
 		*ptr = (swp_entry_t){0};
 		freed++;
+#if 0
+                /*
+		 * This does not work since it may sleep while holding
+		 * a spinlock 
+		 *
+		 * We rely on the page stealer to free up the
+		 * allocated swap space later
+		 */
 		if ((page = lookup_swap_cache(entry)) != NULL) {
 			delete_from_swap_cache(page);
 			page_cache_release(page);	
 		}
+#endif
 		swap_free (entry);
 	}
 	return freed;
@@ -236,8 +245,10 @@
 	
 	/* Only move to the swap cache if there are no other users of
 	 * the page. */
-	if (atomic_read(&page->count) > 2)
-		goto out;
+	if (atomic_read(&page->count) > 2){
+		set_page_dirty(page);
+ 		goto out;
+	}
 	
 	inode = page->mapping->host;
 	info = &inode->u.shmem_i;
@@ -348,9 +359,6 @@
 		if (TryLockPage(page)) 
 			goto wait_retry;
 
-		if (swap_count(page) > 2)
-			BUG();
-		
 		swap_free(*entry);
 		*entry = (swp_entry_t) {0};
 		delete_from_swap_cache_nolock(page);
@@ -432,6 +440,7 @@
 		*ptr = NOPAGE_SIGBUS;
 	return error;
 sigbus:
+	up (&inode->i_sem);
 	*ptr = NOPAGE_SIGBUS;
 	return -EFAULT;
 }