[RFC] some page can't be migrated

[RFC] some page can't be migrated

[Shaohua Li]

Anonymous page might have fs-private metadata, the page is truncated. As
the page hasn't mapping, page migration refuse to migrate the page. It
appears the page is only freed in page reclaim and if zone watermark is
low, the page is never freed, as a result migration always fail. I
thought we could free the metadata so such page can be freed in
migration and make migration more reliable?

Thanks,
Shaohua

diff --git a/mm/migrate.c b/mm/migrate.c
index 6a207e8..6bc38f7 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -633,6 +633,17 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 			goto unlock;
 		wait_on_page_writeback(page);
 	}
+
+	/*
+	 * See truncate_complete_page(). Anonymous page might have
+	 * fs-private metadata, the page is truncated. Such page can't be
+	 * migrated. Try to free metadata, so the page can be freed.
+	 */
+	if (!page->mapping && !PageAnon(page) && PagePrivate(page)) {
+		try_to_release_page(page, GFP_KERNEL);
+		goto unlock;
+	}
+
 	/*
 	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
 	 * we cannot notice that anon_vma is freed while we migrates a page.

Re: [RFC] some page can't be migrated

[Nick Piggin]

On Wednesday 23 January 2008 17:22, Shaohua Li wrote:
> Anonymous page might have fs-private metadata, the page is truncated. As
> the page hasn't mapping, page migration refuse to migrate the page. It
> appears the page is only freed in page reclaim and if zone watermark is
> low, the page is never freed, as a result migration always fail. I
> thought we could free the metadata so such page can be freed in
> migration and make migration more reliable?

Anonymous pages should not have fs-private metadata.

Orphaned pages I guess you mean? They should not be accessable via
the pagecache or the page tables, so how do they keep tangling up
migration? Where/how is migration finding these pages?!


>
> Thanks,
> Shaohua
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6a207e8..6bc38f7 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -633,6 +633,17 @@ static int unmap_and_move(new_page_t get_new_page,
> unsigned long private, goto unlock;
>  		wait_on_page_writeback(page);
>  	}
> +
> +	/*
> +	 * See truncate_complete_page(). Anonymous page might have
> +	 * fs-private metadata, the page is truncated. Such page can't be
> +	 * migrated. Try to free metadata, so the page can be freed.
> +	 */
> +	if (!page->mapping && !PageAnon(page) && PagePrivate(page)) {
> +		try_to_release_page(page, GFP_KERNEL);
> +		goto unlock;
> +	}
> +
>  	/*
>  	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
>  	 * we cannot notice that anon_vma is freed while we migrates a page.
>
>
> --

Re: [RFC] some page can't be migrated

[Christoph Lameter]

On Fri, 25 Jan 2008, Nick Piggin wrote:

> On Wednesday 23 January 2008 17:22, Shaohua Li wrote:
> > Anonymous page might have fs-private metadata, the page is truncated. As
> > the page hasn't mapping, page migration refuse to migrate the page. It
> > appears the page is only freed in page reclaim and if zone watermark is
> > low, the page is never freed, as a result migration always fail. I
> > thought we could free the metadata so such page can be freed in
> > migration and make migration more reliable?
> 
> Anonymous pages should not have fs-private metadata.
> 
> Orphaned pages I guess you mean? They should not be accessable via
> the pagecache or the page tables, so how do they keep tangling up
> migration? Where/how is migration finding these pages?!

Is this maybe related to memory unplug or some such project?

Re: [RFC] some page can't be migrated

[Shaohua Li]

On Fri, 2008-01-25 at 14:03 +1100, Nick Piggin wrote:
> On Wednesday 23 January 2008 17:22, Shaohua Li wrote:
> > Anonymous page might have fs-private metadata, the page is truncated. As
> > the page hasn't mapping, page migration refuse to migrate the page. It
> > appears the page is only freed in page reclaim and if zone watermark is
> > low, the page is never freed, as a result migration always fail. I
> > thought we could free the metadata so such page can be freed in
> > migration and make migration more reliable?
> 
> Anonymous pages should not have fs-private metadata.
> 
> Orphaned pages I guess you mean?
yes, maybe, but the comments in truncate_complete_page called the page
anonymous.
> They should not be accessable via
> the pagecache or the page tables, so how do they keep tangling up
> migration? Where/how is migration finding these pages?!
the page is still in lru list. Memory hot remove will try to migrate the
page.

Thanks,
Shaohua

> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 6a207e8..6bc38f7 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -633,6 +633,17 @@ static int unmap_and_move(new_page_t get_new_page,
> > unsigned long private, goto unlock;
> >  		wait_on_page_writeback(page);
> >  	}
> > +
> > +	/*
> > +	 * See truncate_complete_page(). Anonymous page might have
> > +	 * fs-private metadata, the page is truncated. Such page can't be
> > +	 * migrated. Try to free metadata, so the page can be freed.
> > +	 */
> > +	if (!page->mapping && !PageAnon(page) && PagePrivate(page)) {
> > +		try_to_release_page(page, GFP_KERNEL);
> > +		goto unlock;
> > +	}
> > +
> >  	/*
> >  	 * By try_to_unmap(), page->mapcount goes down to 0 here. In this case,
> >  	 * we cannot notice that anon_vma is freed while we migrates a page.
> >
> >
> > --

Re: [RFC] some page can't be migrated

[Christoph Lameter]

On Fri, 25 Jan 2008, Shaohua Li wrote:

> the page is still in lru list. Memory hot remove will try to migrate the
> page.

So this is an abandoned page with a refcount that only exists because of 
fs private data? Truncate race?

Re: [RFC] some page can't be migrated

[Shaohua Li]

On Thu, 2008-01-24 at 19:12 -0800, Christoph Lameter wrote:
> On Fri, 25 Jan 2008, Shaohua Li wrote:
> 
> > the page is still in lru list. Memory hot remove will try to migrate the
> > page.
> 
> So this is an abandoned page with a refcount that only exists because of 
> fs private data?
Yes, this is what I observed.

> Truncate race?
truncate_complete_page could fail to remove fs private data, this is the
comments say.

Thanks,
Shaohua

Re: [RFC] some page can't be migrated

[Nick Piggin]

On Friday 25 January 2008 14:09, Shaohua Li wrote:
> On Fri, 2008-01-25 at 14:03 +1100, Nick Piggin wrote:
> > On Wednesday 23 January 2008 17:22, Shaohua Li wrote:
> > > Anonymous page might have fs-private metadata, the page is truncated.
> > > As the page hasn't mapping, page migration refuse to migrate the page.
> > > It appears the page is only freed in page reclaim and if zone watermark
> > > is low, the page is never freed, as a result migration always fail. I
> > > thought we could free the metadata so such page can be freed in
> > > migration and make migration more reliable?
> >
> > Anonymous pages should not have fs-private metadata.
> >
> > Orphaned pages I guess you mean?
>
> yes, maybe, but the comments in truncate_complete_page called the page
> anonymous.

Ah, I see. I think we should use orphaned (or anything except
anonymous) to describe these pages.

Re: [RFC] some page can't be migrated

[Christoph Lameter]

On Wed, 23 Jan 2008, Shaohua Li wrote:

> +
> +	/*
> +	 * See truncate_complete_page(). Anonymous page might have
> +	 * fs-private metadata, the page is truncated. Such page can't be
> +	 * migrated. Try to free metadata, so the page can be freed.
> +	 */

Well maybe you should change the comment to refer to an orphaned page. 
That is what Nick used. Also change the comment in truncate_complete_page. 
Anonymous page is confusing here because you check that it is *not* an 
anonymous page.

> +	if (!page->mapping && !PageAnon(page) && PagePrivate(page)) {
> +		try_to_release_page(page, GFP_KERNEL);
> +		goto unlock;
> +	}
> +


Could you move that into the corner case handling that follows?

So it would be something like

if (!page->mapping) {
	if (!PageAnon(page) && PagePrivate(page))
		try_to_relase_page(page, GFP_KERNEL);
	goto rcu_unlock;
}

?

Re: [RFC] some page can't be migrated

[Shaohua Li]

On Thu, 2008-01-24 at 19:37 -0800, Christoph Lameter wrote:
> On Wed, 23 Jan 2008, Shaohua Li wrote:
> 
> > +
> > +	/*
> > +	 * See truncate_complete_page(). Anonymous page might have
> > +	 * fs-private metadata, the page is truncated. Such page can't be
> > +	 * migrated. Try to free metadata, so the page can be freed.
> > +	 */
> 
> Well maybe you should change the comment to refer to an orphaned page. 
> That is what Nick used. Also change the comment in truncate_complete_page. 
> Anonymous page is confusing here because you check that it is *not* an 
> anonymous page.
> 
> > +	if (!page->mapping && !PageAnon(page) && PagePrivate(page)) {
> > +		try_to_release_page(page, GFP_KERNEL);
> > +		goto unlock;
> > +	}
> > +
> 
> 
> Could you move that into the corner case handling that follows?
> 
> So it would be something like
> 
> if (!page->mapping) {
> 	if (!PageAnon(page) && PagePrivate(page))
> 		try_to_relase_page(page, GFP_KERNEL);
> 	goto rcu_unlock;
> }
Ok, changed.

Orphaned page might have fs-private metadata, the page is truncated. As
the page hasn't mapping, page migration refuse to migrate the page. It
appears the page is only freed in page reclaim and if zone watermark is
low, the page is never freed, as a result migration always fail. I
thought we could free the metadata so such page can be freed in
migration and make migration more reliable.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/mm/migrate.c b/mm/migrate.c
index 6a207e8..e82acc9 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -652,8 +652,16 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 	 * Calling try_to_unmap() against a page->mapping==NULL page is
 	 * BUG. So handle it here.
 	 */
-	if (!page->mapping)
+	if (!page->mapping) {
+		/*
+		 * See truncate_complete_page(). Orphaned page might have
+		 * fs-private metadata, the page is truncated. Such page can't
+		 * be migrated. Try to free metadata, so the page can be freed.
+		 */
+		if (!PageAnon(page) && PagePrivate(page))
+			try_to_release_page(page, GFP_KERNEL);
 		goto rcu_unlock;
+	}
 	/* Establish migration ptes or remove ptes */
 	try_to_unmap(page, 1);
 
diff --git a/mm/truncate.c b/mm/truncate.c
index cadc156..62fd8cd 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -84,7 +84,7 @@ EXPORT_SYMBOL(cancel_dirty_page);
 
 /*
  * If truncate cannot remove the fs-private metadata from the page, the page
- * becomes anonymous.  It will be left on the LRU and may even be mapped into
+ * becomes orphaned.  It will be left on the LRU and may even be mapped into
  * user pagetables if we're racing with filemap_fault().
  *
  * We need to bale out if page->mapping is no longer equal to the original

Re: [RFC] some page can't be migrated

[Christoph Lameter]

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

Nick? Ok with you too?

Re: [RFC] some page can't be migrated

[Nick Piggin]

On Friday 25 January 2008 15:01, Christoph Lameter wrote:
> Acked-by: Christoph Lameter <clameter@sgi.com>
>
> Nick? Ok with you too?

Yeah, for memory hot remove that makes sense. A comment
might be in order, at least a reference to the orphaned
page code in vmscan.c.

Otherwise, it is OK by me.

Acked-by: Nick Piggin <npiggin@suse.de>

Re: [RFC] some page can't be migrated

[Christoph Lameter]

On Fri, 25 Jan 2008, Nick Piggin wrote:

> Yeah, for memory hot remove that makes sense. A comment
> might be in order, at least a reference to the orphaned
> page code in vmscan.c.

Right. The surrounding comments in mm/migrate.c also need to be made 
consistent. The comment before is now slightly off. Could you do another 
patch Shaohua with better comments?

Re: [RFC] some page can't be migrated

[Shaohua Li]

On Fri, 2008-01-25 at 15:17 +1100, Nick Piggin wrote:
> On Friday 25 January 2008 15:01, Christoph Lameter wrote:
> > Acked-by: Christoph Lameter <clameter@sgi.com>
> >
> > Nick? Ok with you too?
> 
> Yeah, for memory hot remove that makes sense. A comment
> might be in order, at least a reference to the orphaned
> page code in vmscan.c.
> 
> Otherwise, it is OK by me.
Orphaned page might have fs-private metadata, the page is truncated. As
the page hasn't mapping, page migration refuse to migrate the page. It
appears the page is only freed in page reclaim and if zone watermark is
low, the page is never freed, as a result migration always fail. I
thought we could free the metadata so such page can be freed in
migration and make migration more reliable.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Acked-by: Nick Piggin <npiggin@suse.de>
Acked-by: Christoph Lameter <clameter@sgi.com>

diff --git a/mm/migrate.c b/mm/migrate.c
index 6a207e8..2c1a48a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -646,14 +646,22 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private,
 		rcu_locked = 1;
 	}
 	/*
-	 * This is a corner case handling.
-	 * When a new swap-cache is read into, it is linked to LRU
+	 * There are corner cases handling.
+	 * 1. When a new swap-cache is read into, it is linked to LRU
 	 * and treated as swapcache but has no rmap yet.
 	 * Calling try_to_unmap() against a page->mapping==NULL page is
 	 * BUG. So handle it here.
+	 * 2. Orphaned page (see truncate_complete_page) might have
+	 * fs-private metadata, the page is truncated. The page can be picked
+	 * up due to memory offlineing. Everywhere else except page reclaim,
+	 * the page is invisible to the vm, so the page can't be migrated. Try
+	 * to free metadata, so the page can be freed.
 	 */
-	if (!page->mapping)
+	if (!page->mapping) {
+		if (!PageAnon(page) && PagePrivate(page))
+			try_to_release_page(page, GFP_KERNEL);
 		goto rcu_unlock;
+	}
 	/* Establish migration ptes or remove ptes */
 	try_to_unmap(page, 1);
 
diff --git a/mm/truncate.c b/mm/truncate.c
index cadc156..62fd8cd 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -84,7 +84,7 @@ EXPORT_SYMBOL(cancel_dirty_page);
 
 /*
  * If truncate cannot remove the fs-private metadata from the page, the page
- * becomes anonymous.  It will be left on the LRU and may even be mapped into
+ * becomes orphaned.  It will be left on the LRU and may even be mapped into
  * user pagetables if we're racing with filemap_fault().
  *
  * We need to bale out if page->mapping is no longer equal to the original

Re: [RFC] some page can't be migrated

[Andrew Morton]

> On Fri, 25 Jan 2008 14:03:25 +0800 Shaohua Li <shaohua.li@intel.com> wrote:
> 
> -	if (!page->mapping)
> +	if (!page->mapping) {
> +		if (!PageAnon(page) && PagePrivate(page))
> +			try_to_release_page(page, GFP_KERNEL);
>  		goto rcu_unlock;
> +	}

We call something(GFP_KERNEL) under rcu_read_lock()?  I've lost track of
the myriad flavours of rcu which we purport to support, but I don't think
they'll all like us blocking under rcu_read_lock().

We _won't_ block, because try_to_release_page() will see the NULL ->mapping
and will call the non-blocking try_to_free_buffers().  But still, it looks
bad, and will cause problems if someone decides to add a might_sleep_if()
to try_to_release_page().

So...  I'd suggest that it would be better to add an apologetic comment and
call direct into try_to_free_buffers().

> -	 * This is a corner case handling.
> -	 * When a new swap-cache is read into, it is linked to LRU
> +	 * There are corner cases handling.
> +	 * 1. When a new swap-cache is read into, it is linked to LRU

hm, that didn't improve the rather flakey grammar in there.  Oh well.

> Acked-by: Nick Piggin <npiggin@suse.de>
> Acked-by: Christoph Lameter <clameter@sgi.com>

hm.



Please review and test....

--- a/mm/migrate.c~page-migraton-handle-orphaned-pages-fix
+++ a/mm/migrate.c
@@ -650,20 +650,28 @@ static int unmap_and_move(new_page_t get
 	}
 
 	/*
-	 * There are corner cases handling.
-	 * 1. When a new swap-cache is read into, it is linked to LRU
-	 * and treated as swapcache but has no rmap yet.
-	 * Calling try_to_unmap() against a page->mapping==NULL page is
-	 * BUG. So handle it here.
-	 * 2. Orphaned page (see truncate_complete_page) might have
-	 * fs-private metadata, the page is truncated. The page can be picked
-	 * up due to memory offlineing. Everywhere else except page reclaim,
-	 * the page is invisible to the vm, so the page can't be migrated. Try
-	 * to free metadata, so the page can be freed.
+	 * Corner case handling:
+	 * 1. When a new swap-cache page is read into, it is added to the LRU
+	 * and treated as swapcache but it has no rmap yet.
+	 * Calling try_to_unmap() against a page->mapping==NULL page will
+	 * trigger a BUG.  So handle it here.
+	 * 2. An orphaned page (see truncate_complete_page) might have
+	 * fs-private metadata. The page can be picked up due to memory
+	 * offlining.  Everywhere else except page reclaim, the page is
+	 * invisible to the vm, so the page can not be migrated.  So try to
+	 * free the metadata, so the page can be freed.
 	 */
 	if (!page->mapping) {
-		if (!PageAnon(page) && PagePrivate(page))
-			try_to_release_page(page, GFP_KERNEL);
+		if (!PageAnon(page) && PagePrivate(page)) {
+			/*
+			 * Go direct to try_to_free_buffers() here because
+			 * a) that's what try_to_release_page() would do anyway
+			 * b) we may be under rcu_read_lock() here, so we can't
+			 *    use GFP_KERNEL which is what try_to_release_page()
+			 *    needs to be effective.
+			 */
+			try_to_release_buffers(page);
+		}
 		goto rcu_unlock;
 	}
 

Re: [RFC] some page can't be migrated

[Nick Piggin]

On Sunday 27 January 2008 17:03, Andrew Morton wrote:
> > On Fri, 25 Jan 2008 14:03:25 +0800 Shaohua Li <shaohua.li@intel.com>
> > wrote:
> >
> > -	if (!page->mapping)
> > +	if (!page->mapping) {
> > +		if (!PageAnon(page) && PagePrivate(page))
> > +			try_to_release_page(page, GFP_KERNEL);
> >  		goto rcu_unlock;
> > +	}
>
> We call something(GFP_KERNEL) under rcu_read_lock()?  I've lost track of
> the myriad flavours of rcu which we purport to support, but I don't think
> they'll all like us blocking under rcu_read_lock().
>
> We _won't_ block, because try_to_release_page() will see the NULL ->mapping
> and will call the non-blocking try_to_free_buffers().  But still, it looks
> bad, and will cause problems if someone decides to add a might_sleep_if()
> to try_to_release_page().
>
> So...  I'd suggest that it would be better to add an apologetic comment and
> call direct into try_to_free_buffers().

You're right, but can't we just rcu_read_unlock() before try_to_release_page?

Re: [RFC] some page can't be migrated

[Shaohua Li]

On Mon, 2008-01-28 at 12:43 +1100, Nick Piggin wrote:
> On Sunday 27 January 2008 17:03, Andrew Morton wrote:
> > > On Fri, 25 Jan 2008 14:03:25 +0800 Shaohua Li <shaohua.li@intel.com>
> > > wrote:
> > >
> > > -	if (!page->mapping)
> > > +	if (!page->mapping) {
> > > +		if (!PageAnon(page) && PagePrivate(page))
> > > +			try_to_release_page(page, GFP_KERNEL);
> > >  		goto rcu_unlock;
> > > +	}
> >
> > We call something(GFP_KERNEL) under rcu_read_lock()?  I've lost track of
> > the myriad flavours of rcu which we purport to support, but I don't think
> > they'll all like us blocking under rcu_read_lock().
> >
> > We _won't_ block, because try_to_release_page() will see the NULL ->mapping
> > and will call the non-blocking try_to_free_buffers().  But still, it looks
> > bad, and will cause problems if someone decides to add a might_sleep_if()
> > to try_to_release_page().
> >
> > So...  I'd suggest that it would be better to add an apologetic comment and
> > call direct into try_to_free_buffers().
> 
> You're right, but can't we just rcu_read_unlock() before try_to_release_page?
or we could move the code above before doing rcu_read_lock()?

Re: [RFC] some page can't be migrated

[Christoph Lameter]

On Sat, 26 Jan 2008, Andrew Morton wrote:

> We call something(GFP_KERNEL) under rcu_read_lock()?  I've lost track of
> the myriad flavours of rcu which we purport to support, but I don't think
> they'll all like us blocking under rcu_read_lock().
> 
> We _won't_ block, because try_to_release_page() will see the NULL ->mapping
> and will call the non-blocking try_to_free_buffers().  But still, it looks
> bad, and will cause problems if someone decides to add a might_sleep_if()
> to try_to_release_page().
> 
> So...  I'd suggest that it would be better to add an apologetic comment and
> call direct into try_to_free_buffers().

Right. Looks good. PageWriteback cannot be set if we do not have a 
mappig...

Re: [RFC] some page can't be migrated

[Christoph Lameter]

On Sat, 26 Jan 2008, Andrew Morton wrote:

> > 
> > -	if (!page->mapping)
> > +	if (!page->mapping) {
> > +		if (!PageAnon(page) && PagePrivate(page))
> > +			try_to_release_page(page, GFP_KERNEL);
> >  		goto rcu_unlock;
> > +	}
> 
> We call something(GFP_KERNEL) under rcu_read_lock()?  I've lost track of
> the myriad flavours of rcu which we purport to support, but I don't think
> they'll all like us blocking under rcu_read_lock().

I still think your solution is good but even with the original patch we do 
not call try_to_release_page() with GFP_KERNEL under rcu.

   if (PageAnon(page)) {
                rcu_read_lock();
                rcu_locked = 1;
        }


rcu is only active if we have an anonymous page and in that case we do not
call try_to_release_page(). Just to make sure that Nick and I do not get 
dinged needlessly....