bug in migrate page

bug in migrate page

[Shaohua Li]

commit dc386d4d1e98bb39fb967ee156cd456c802fc692 adds rcu_read_lock, but
some routines in the lock range might sleep (like lock_buffer,
aops->writepage), I saw a 'sleep in atomic' warning. It appears the
patch has several versions before. Doing rcu_read_lock in PageAnon
sounds break the case of PageAnon(page) && PageSwapCache(page),
as .writepage might be called. The dummy anon patch maybe is ok.

Thanks,
Shaohua

Re: bug in migrate page

[Shaohua Li]

On Wed, 2007-08-22 at 11:52 +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 22 Aug 2007 10:08:09 +0800
> Shaohua Li <shaohua.li@intel.com> wrote:
> 
> > commit dc386d4d1e98bb39fb967ee156cd456c802fc692 adds rcu_read_lock, but
> > some routines in the lock range might sleep (like lock_buffer,
> > aops->writepage), I saw a 'sleep in atomic' warning. It appears the
> > patch has several versions before. Doing rcu_read_lock in PageAnon
> > sounds break the case of PageAnon(page) && PageSwapCache(page),
> > as .writepage might be called. The dummy anon patch maybe is ok.
> > 
> 
> Thank you for catching.
> 
> Maybe you're correct.
> 
> BTW, in PageAnon(page) && PageSwapCache(page) case, I can't find when
> .writepage is called. Could you explain ?
> 
> In my understanding,
> 
> rcu_read_lock()
> 	-> try_to_unmap()
> 	-> move_to_new_page()
> 		-> migrate_page() // swap has .migratepage member.
> 			-> migrate_page_move_mapping().
> 			-> migrate_page_copy().
> 		-> remove_migration_ptes().
> 
> 
> At quick glance, above path has no writepage() ops.
> just replace swap's radix tree entry.
I missed swap has .migratepage and thought fallback_migrate_page is
used, then I thought doing rcu lock in PageAnon case is ok.

Thanks,
Shaohua

Re: bug in migrate page

[KAMEZAWA Hiroyuki]

On Wed, 22 Aug 2007 10:08:09 +0800
Shaohua Li <shaohua.li@intel.com> wrote:

> commit dc386d4d1e98bb39fb967ee156cd456c802fc692 adds rcu_read_lock, but
> some routines in the lock range might sleep (like lock_buffer,
> aops->writepage), I saw a 'sleep in atomic' warning. It appears the
> patch has several versions before. Doing rcu_read_lock in PageAnon
> sounds break the case of PageAnon(page) && PageSwapCache(page),
> as .writepage might be called. The dummy anon patch maybe is ok.
> 

Thank you for catching.

Maybe you're correct.

BTW, in PageAnon(page) && PageSwapCache(page) case, I can't find when
.writepage is called. Could you explain ?

In my understanding,

rcu_read_lock()
	-> try_to_unmap()
	-> move_to_new_page()
		-> migrate_page() // swap has .migratepage member.
			-> migrate_page_move_mapping().
			-> migrate_page_copy().
		-> remove_migration_ptes().


At quick glance, above path has no writepage() ops.
just replace swap's radix tree entry.

Thanks,
-Kame

Re: bug in migrate page

[KAMEZAWA Hiroyuki]

On Wed, 22 Aug 2007 10:50:53 +0800
Shaohua Li <shaohua.li@intel.com> wrote:

> > At quick glance, above path has no writepage() ops.
> > just replace swap's radix tree entry.
> I missed swap has .migratepage and thought fallback_migrate_page is
> used, then I thought doing rcu lock in PageAnon case is ok.
> 
Thank you, I'll write a patch.

Regards,
-Kame

[RFC][PATCH][BUGFIX] fix rcu_read_lock in page migraton

[KAMEZAWA Hiroyuki]

This is a patch against the problme Shaohua rported.
Just an idea for fix the problem.
How do you think ? dummy vma is better ? (I don't like dummy vma.)

-Kame
==
In migration fallback path, write_page() or lock_page() will be called.
This causes sleep with holding rcu_read_lock().
For avoding that, just do rcu_lock if the page is Anon.(this is enough.)

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


---
 mm/migrate.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Index: linux-2.6.23-rc2-mm2/mm/migrate.c
===================================================================
--- linux-2.6.23-rc2-mm2.orig/mm/migrate.c
+++ linux-2.6.23-rc2-mm2/mm/migrate.c
@@ -611,6 +611,7 @@ static int unmap_and_move(new_page_t get
 	int rc = 0;
 	int *result = NULL;
 	struct page *newpage = get_new_page(page, private, &result);
+	int rcu_locked = 0;
 
 	if (!newpage)
 		return -ENOMEM;
@@ -636,8 +637,13 @@ static int unmap_and_move(new_page_t get
 	 * we cannot notice that anon_vma is freed while we migrates a page.
 	 * This rcu_read_lock() delays freeing anon_vma pointer until the end
 	 * of migration. File cache pages are no problem because of page_lock()
+	 * File Caches may use write_page() or lock_page() in migration, then,
+	 * just care Anon page here.
 	 */
-	rcu_read_lock();
+	if (PageAnon(page)) {
+		rcu_read_lock();
+		rcu_locked = 1;
+	}
 	/*
 	 * This is a corner case handling.
 	 * When a new swap-cache is read into, it is linked to LRU
@@ -656,7 +662,8 @@ static int unmap_and_move(new_page_t get
 	if (rc)
 		remove_migration_ptes(page, page);
 rcu_unlock:
-	rcu_read_unlock();
+	if (rcu_locked)
+		rcu_read_unlock();
 
 unlock:
 

Re: bug in migrate page

[Christoph Lameter]

On Wed, 22 Aug 2007, Shaohua Li wrote:

> commit dc386d4d1e98bb39fb967ee156cd456c802fc692 adds rcu_read_lock, but
> some routines in the lock range might sleep (like lock_buffer,
> aops->writepage), I saw a 'sleep in atomic' warning. It appears the
> patch has several versions before. Doing rcu_read_lock in PageAnon
> sounds break the case of PageAnon(page) && PageSwapCache(page),
> as .writepage might be called. The dummy anon patch maybe is ok.

Ok so we agree that .writepage is not used. Anonymous pages also do not
not have buffers. Only taking the rcu lock on page_anon() would suffice 
to fix this right?

Re: [RFC][PATCH][BUGFIX] fix rcu_read_lock in page migraton

[Christoph Lameter]

On Wed, 22 Aug 2007, KAMEZAWA Hiroyuki wrote:

> This is a patch against the problme Shaohua rported.
> Just an idea for fix the problem.
> How do you think ? dummy vma is better ? (I don't like dummy vma.)

Yup but we have to do it.

Acked-by: Christoph Lameter <clameter@sgi.com>