Remove page migration fallback (was: UBIFS and page migration)

Remove page migration fallback (was: UBIFS and page migration)

[Richard Weinberger]

During page migrations UBIFS gets confused. We triggered this by using CMA
on two different targets.
It turned out that fallback_migrate_page() is not suitable for UBIFS as it
does not copy the PagePrivate flag. Non-trivial block based filesystems
do not notice since they can use buffer_migrate_page().
UBIFS is using this flag among with PageChecked to account free space.

In order to address this issue implement a convenient ->migratepage()
function for UBIFS and disable the automatic assignment of
fallback_migrate_page(). Filesystems maintains should decide themselves
whether they have to implement ->migratepage() or can use the generic function.

Another interesting topic is testing ->migratepage(). So far the only reliable
test to trigger the UBIFS issue we have is real hardware and CMA.
I was able to trigger it a few times in KVM using the migrate_pages() system call.
But not reliable at all.

Thanks,
//richard

[PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()
[PATCH 2/3] mm: Export migrate_page_move_mapping and
[PATCH 3/3] UBIFS: Implement ->migratepage()

[PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Richard Weinberger]

While block oriented filesystems use buffer_migrate_page()
as page migration function other filesystems which don't
implement ->migratepage() will automatically get fallback_migrate_page()
assigned. fallback_migrate_page() is not as generic as is should
be. Page migration is filesystem specific and a one-fits-all function
is hard to achieve. UBIFS leaned this lection the hard way.
It uses various page flags and fallback_migrate_page() does not
handle these flags as UBIFS expected.

To make sure that no further filesystem will get confused by
fallback_migrate_page() disable the automatic assignment and
allow filesystems to use this function explicitly if it is
really suitable.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/linux/migrate.h |  9 +++++++++
 mm/migrate.c            | 16 ++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 9b50325..aba86d4 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -47,6 +47,9 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
 		struct page *newpage, struct page *page,
 		struct buffer_head *head, enum migrate_mode mode,
 		int extra_count);
+extern int generic_migrate_page(struct address_space *mapping,
+				struct page *newpage, struct page *page,
+				enum migrate_mode mode);
 #else
 
 static inline void putback_movable_pages(struct list_head *l) {}
@@ -67,6 +70,12 @@ static inline int migrate_huge_page_move_mapping(struct address_space *mapping,
 	return -ENOSYS;
 }
 
+static inline int generic_migrate_page(struct address_space *mapping,
+				       struct page *newpage, struct page *page,
+				       enum migrate_mode mode)
+{
+	return -ENOSYS;
+}
 #endif /* CONFIG_MIGRATION */
 
 #ifdef CONFIG_NUMA_BALANCING
diff --git a/mm/migrate.c b/mm/migrate.c
index 9baf41c..5129143 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -719,8 +719,9 @@ static int writeout(struct address_space *mapping, struct page *page)
 /*
  * Default handling if a filesystem does not provide a migration function.
  */
-static int fallback_migrate_page(struct address_space *mapping,
-	struct page *newpage, struct page *page, enum migrate_mode mode)
+int generic_migrate_page(struct address_space *mapping,
+			 struct page *newpage, struct page *page,
+			 enum migrate_mode mode)
 {
 	if (PageDirty(page)) {
 		/* Only writeback pages in full synchronous migration */
@@ -771,8 +772,15 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 		 * is the most common path for page migration.
 		 */
 		rc = mapping->a_ops->migratepage(mapping, newpage, page, mode);
-	else
-		rc = fallback_migrate_page(mapping, newpage, page, mode);
+	else {
+		/*
+		 * Dear filesystem maintainer, please verify whether
+		 * generic_migrate_page() is suitable for your
+		 * filesystem, especially wrt. page flag handling.
+		 */
+		WARN_ON_ONCE(1);
+		rc = -EINVAL;
+	}
 
 	/*
 	 * When successful, old pagecache page->mapping must be cleared before
-- 
2.7.3

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Andrew Morton]

On Thu, 16 Jun 2016 23:26:13 +0200 Richard Weinberger <richard@nod.at> wrote:

> While block oriented filesystems use buffer_migrate_page()
> as page migration function other filesystems which don't
> implement ->migratepage() will automatically get fallback_migrate_page()
> assigned. fallback_migrate_page() is not as generic as is should
> be. Page migration is filesystem specific and a one-fits-all function
> is hard to achieve. UBIFS leaned this lection the hard way.
> It uses various page flags and fallback_migrate_page() does not
> handle these flags as UBIFS expected.
> 
> To make sure that no further filesystem will get confused by
> fallback_migrate_page() disable the automatic assignment and
> allow filesystems to use this function explicitly if it is
> really suitable.

hm, is there really much point in doing this?  I assume it doesn't
actually affect any current filesystems?

[2/3] is of course OK - please add it to the UBIFS tree.

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Richard Weinberger]

Andrew,

Am 17.06.2016 um 01:11 schrieb Andrew Morton:
> On Thu, 16 Jun 2016 23:26:13 +0200 Richard Weinberger <richard@nod.at> wrote:
> 
>> While block oriented filesystems use buffer_migrate_page()
>> as page migration function other filesystems which don't
>> implement ->migratepage() will automatically get fallback_migrate_page()
>> assigned. fallback_migrate_page() is not as generic as is should
>> be. Page migration is filesystem specific and a one-fits-all function
>> is hard to achieve. UBIFS leaned this lection the hard way.
>> It uses various page flags and fallback_migrate_page() does not
>> handle these flags as UBIFS expected.
>>
>> To make sure that no further filesystem will get confused by
>> fallback_migrate_page() disable the automatic assignment and
>> allow filesystems to use this function explicitly if it is
>> really suitable.
> 
> hm, is there really much point in doing this?  I assume it doesn't
> actually affect any current filesystems?

Well, we simply don't know which filesystems are affected by similar issues.
JFFS2 is maybe also affected since it is not block based.
For UBIFS it also worked many years.

> [2/3] is of course OK - please add it to the UBIFS tree.

Can I add your Acked-by?

Thanks,
//richard

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Michal Hocko]

On Fri 17-06-16 09:41:38, Richard Weinberger wrote:
> Andrew,
> 
> Am 17.06.2016 um 01:11 schrieb Andrew Morton:
> > On Thu, 16 Jun 2016 23:26:13 +0200 Richard Weinberger <richard@nod.at> wrote:
> > 
> >> While block oriented filesystems use buffer_migrate_page()
> >> as page migration function other filesystems which don't
> >> implement ->migratepage() will automatically get fallback_migrate_page()
> >> assigned. fallback_migrate_page() is not as generic as is should
> >> be. Page migration is filesystem specific and a one-fits-all function
> >> is hard to achieve. UBIFS leaned this lection the hard way.
> >> It uses various page flags and fallback_migrate_page() does not
> >> handle these flags as UBIFS expected.
> >>
> >> To make sure that no further filesystem will get confused by
> >> fallback_migrate_page() disable the automatic assignment and
> >> allow filesystems to use this function explicitly if it is
> >> really suitable.
> > 
> > hm, is there really much point in doing this?  I assume it doesn't
> > actually affect any current filesystems?
> 
> Well, we simply don't know which filesystems are affected by similar issues.

But doesn't this disable the page migration and so potentially reduce
the compaction success rate for the large pile of filesystems? Without
any hint about that?

$ git grep "\.migratepage[[:space:]]*=" -- fs | wc -l
16
out of
$ git grep "struct address_space_operations[[:space:]]*[a-zA-Z0-9_]*[[:space:]]*=" -- fs | wc -l
87

That just seems to be too conservative for something that even not might
be a problem, especially when considering the fallback migration code is
there for many years with only UBIFS seeing a problem.

Wouldn't it be safer to contact FS developers who might have have
similar issue and work with them to use a proper migration code?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Richard Weinberger]

Am 17.06.2016 um 18:28 schrieb Michal Hocko:
> But doesn't this disable the page migration and so potentially reduce
> the compaction success rate for the large pile of filesystems? Without
> any hint about that?

The WARN_ON_ONCE() is the hint. ;)
But I can understand your point we'd have to communicate that change better.

> $ git grep "\.migratepage[[:space:]]*=" -- fs | wc -l
> 16
> out of
> $ git grep "struct address_space_operations[[:space:]]*[a-zA-Z0-9_]*[[:space:]]*=" -- fs | wc -l
> 87
> 
> That just seems to be too conservative for something that even not might
> be a problem, especially when considering the fallback migration code is
> there for many years with only UBIFS seeing a problem.

UBIFS is also there for many years.
It just shows that the issue is hard to hit but at least for UBIFS it is real.

> Wouldn't it be safer to contact FS developers who might have have
> similar issue and work with them to use a proper migration code?

That was the goal of this patch. Forcing the filesystem developers
to look as the WARN_ON_ONCE() triggered.

I fear just sending a mail to linux-fsdevel@vger is not enough.

Thanks,
//richard

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Michal Hocko]

On Fri 17-06-16 18:55:45, Richard Weinberger wrote:
> Am 17.06.2016 um 18:28 schrieb Michal Hocko:
> > But doesn't this disable the page migration and so potentially reduce
> > the compaction success rate for the large pile of filesystems? Without
> > any hint about that?
> 
> The WARN_ON_ONCE() is the hint. ;)

Right. My reply turned a different way than I meant... I meant to say
that there might be different regressions caused by this change without much
hint that a particular warning would be the smoking gun... 
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Richard Weinberger]

Am 17.06.2016 um 20:27 schrieb Michal Hocko:
> On Fri 17-06-16 18:55:45, Richard Weinberger wrote:
>> Am 17.06.2016 um 18:28 schrieb Michal Hocko:
>>> But doesn't this disable the page migration and so potentially reduce
>>> the compaction success rate for the large pile of filesystems? Without
>>> any hint about that?
>>
>> The WARN_ON_ONCE() is the hint. ;)
> 
> Right. My reply turned a different way than I meant... I meant to say
> that there might be different regressions caused by this change without much
> hint that a particular warning would be the smoking gun... 
> 

Okay, what about something like that?
That way everything works as before and we don't have regressions
but FS maintainers will notice the WARN_ON_ONCE() and hopefully review
whether generic_migrate_page() is really suitable.
If so, they can set their a_ops->migratepage to generic_migrate_page().

@@ -771,8 +773,15 @@ static int move_to_new_page(struct page *newpage, struct page *page,
                 * is the most common path for page migration.
                 */
                rc = mapping->a_ops->migratepage(mapping, newpage, page, mode);
-       else
-               rc = fallback_migrate_page(mapping, newpage, page, mode);
+       else {
+               /*
+                * Dear filesystem maintainer, please verify whether
+                * generic_migrate_page() is suitable for your
+                * filesystem, especially wrt. page flag handling.
+                */
+               WARN_ON_ONCE(1);
+               rc = generic_migrate_page(mapping, newpage, page, mode);
+       }

        /*
         * When successful, old pagecache page->mapping must be cleared before

Thanks,
//richard

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Michal Hocko]

On Fri 17-06-16 21:36:30, Richard Weinberger wrote:
> 
> 
> Am 17.06.2016 um 20:27 schrieb Michal Hocko:
> > On Fri 17-06-16 18:55:45, Richard Weinberger wrote:
> >> Am 17.06.2016 um 18:28 schrieb Michal Hocko:
> >>> But doesn't this disable the page migration and so potentially reduce
> >>> the compaction success rate for the large pile of filesystems? Without
> >>> any hint about that?
> >>
> >> The WARN_ON_ONCE() is the hint. ;)
> > 
> > Right. My reply turned a different way than I meant... I meant to say
> > that there might be different regressions caused by this change without much
> > hint that a particular warning would be the smoking gun... 
> > 
> 
> Okay, what about something like that?
> That way everything works as before and we don't have regressions
> but FS maintainers will notice the WARN_ON_ONCE() and hopefully review
> whether generic_migrate_page() is really suitable.
> If so, they can set their a_ops->migratepage to generic_migrate_page().

Yes this sounds better to me. I would just be more verbose about which
a_ops is missing the migratepage callback. The WARN_ON_ONCE will not
tell us which fs is the culprit. I am not even sure the calltrace is
really helpful and maybe printk_once would be more appropriate.

	printk_once(KERN_INFO "%ps is missing migratepage callback. Please report to the respective filesystem maintainers.\n",
			mapping->a_ops);

Or print once per a_ops would be even better but that sounds like an
over engineering...
 
> @@ -771,8 +773,15 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>                  * is the most common path for page migration.
>                  */
>                 rc = mapping->a_ops->migratepage(mapping, newpage, page, mode);
> -       else
> -               rc = fallback_migrate_page(mapping, newpage, page, mode);
> +       else {
> +               /*
> +                * Dear filesystem maintainer, please verify whether
> +                * generic_migrate_page() is suitable for your
> +                * filesystem, especially wrt. page flag handling.
> +                */
> +               WARN_ON_ONCE(1);
> +               rc = generic_migrate_page(mapping, newpage, page, mode);
> +       }
> 
>         /*
>          * When successful, old pagecache page->mapping must be cleared before
> 
> Thanks,
> //richard

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] mm: Don't blindly assign fallback_migrate_page()

[Richard Weinberger]

Am 17.06.2016 um 01:11 schrieb Andrew Morton:
> On Thu, 16 Jun 2016 23:26:13 +0200 Richard Weinberger <richard@nod.at> wrote:
> 
>> While block oriented filesystems use buffer_migrate_page()
>> as page migration function other filesystems which don't
>> implement ->migratepage() will automatically get fallback_migrate_page()
>> assigned. fallback_migrate_page() is not as generic as is should
>> be. Page migration is filesystem specific and a one-fits-all function
>> is hard to achieve. UBIFS leaned this lection the hard way.
>> It uses various page flags and fallback_migrate_page() does not
>> handle these flags as UBIFS expected.
>>
>> To make sure that no further filesystem will get confused by
>> fallback_migrate_page() disable the automatic assignment and
>> allow filesystems to use this function explicitly if it is
>> really suitable.
> 
> hm, is there really much point in doing this?  I assume it doesn't
> actually affect any current filesystems?
> 
> [2/3] is of course OK - please add it to the UBIFS tree.

Pushed 2/3 and 3/3 into UBIFS next tree.

Thanks,
//richard

[PATCH 2/3] mm: Export migrate_page_move_mapping and migrate_page_copy

[Richard Weinberger]

Export these symbols such that UBIFS can implement
->migratepage.

Signed-off-by: Richard Weinberger <richard@nod.at>
Acked-by: Christoph Hellwig <hch@lst.de>
---
 mm/migrate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/migrate.c b/mm/migrate.c
index 5129143..0fcdd86 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -431,6 +431,7 @@ int migrate_page_move_mapping(struct address_space *mapping,
 
 	return MIGRATEPAGE_SUCCESS;
 }
+EXPORT_SYMBOL(migrate_page_move_mapping);
 
 /*
  * The expected number of remaining references is the same as that
@@ -586,6 +587,7 @@ void migrate_page_copy(struct page *newpage, struct page *page)
 
 	mem_cgroup_migrate(page, newpage);
 }
+EXPORT_SYMBOL(migrate_page_copy);
 
 /************************************************************
  *                    Migration functions
-- 
2.7.3

[PATCH 3/3] UBIFS: Implement ->migratepage()

[Richard Weinberger]

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

During page migrations UBIFS might get confused
and the following assert triggers:
[  213.480000] UBIFS assert failed in ubifs_set_page_dirty at 1451 (pid 436)
[  213.490000] CPU: 0 PID: 436 Comm: drm-stress-test Not tainted 4.4.4-00176-geaa802524636-dirty #1008
[  213.490000] Hardware name: Allwinner sun4i/sun5i Families
[  213.490000] [<c0015e70>] (unwind_backtrace) from [<c0012cdc>] (show_stack+0x10/0x14)
[  213.490000] [<c0012cdc>] (show_stack) from [<c02ad834>] (dump_stack+0x8c/0xa0)
[  213.490000] [<c02ad834>] (dump_stack) from [<c0236ee8>] (ubifs_set_page_dirty+0x44/0x50)
[  213.490000] [<c0236ee8>] (ubifs_set_page_dirty) from [<c00fa0bc>] (try_to_unmap_one+0x10c/0x3a8)
[  213.490000] [<c00fa0bc>] (try_to_unmap_one) from [<c00fadb4>] (rmap_walk+0xb4/0x290)
[  213.490000] [<c00fadb4>] (rmap_walk) from [<c00fb1bc>] (try_to_unmap+0x64/0x80)
[  213.490000] [<c00fb1bc>] (try_to_unmap) from [<c010dc28>] (migrate_pages+0x328/0x7a0)
[  213.490000] [<c010dc28>] (migrate_pages) from [<c00d0cb0>] (alloc_contig_range+0x168/0x2f4)
[  213.490000] [<c00d0cb0>] (alloc_contig_range) from [<c010ec00>] (cma_alloc+0x170/0x2c0)
[  213.490000] [<c010ec00>] (cma_alloc) from [<c001a958>] (__alloc_from_contiguous+0x38/0xd8)
[  213.490000] [<c001a958>] (__alloc_from_contiguous) from [<c001ad44>] (__dma_alloc+0x23c/0x274)
[  213.490000] [<c001ad44>] (__dma_alloc) from [<c001ae08>] (arm_dma_alloc+0x54/0x5c)
[  213.490000] [<c001ae08>] (arm_dma_alloc) from [<c035cecc>] (drm_gem_cma_create+0xb8/0xf0)
[  213.490000] [<c035cecc>] (drm_gem_cma_create) from [<c035cf20>] (drm_gem_cma_create_with_handle+0x1c/0xe8)
[  213.490000] [<c035cf20>] (drm_gem_cma_create_with_handle) from [<c035d088>] (drm_gem_cma_dumb_create+0x3c/0x48)
[  213.490000] [<c035d088>] (drm_gem_cma_dumb_create) from [<c0341ed8>] (drm_ioctl+0x12c/0x444)
[  213.490000] [<c0341ed8>] (drm_ioctl) from [<c0121adc>] (do_vfs_ioctl+0x3f4/0x614)
[  213.490000] [<c0121adc>] (do_vfs_ioctl) from [<c0121d30>] (SyS_ioctl+0x34/0x5c)
[  213.490000] [<c0121d30>] (SyS_ioctl) from [<c000f2c0>] (ret_fast_syscall+0x0/0x34)

UBIFS is using PagePrivate() which can have different meanings across
filesystems. Therefore the generic page migration code cannot handle this
case correctly.
We have to implement our own migration function which basically does a
plain copy but also duplicates the page private flag.
UBIFS is not a block device filesystem and cannot use buffer_migrate_page().

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
[rw: Massaged changelog, build fixes, etc...]
Signed-off-by: Richard Weinberger <richard@nod.at>
Acked-by: Christoph Hellwig <hch@lst.de>

---
 fs/ubifs/file.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 0831697..7bbf420 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -52,6 +52,7 @@
 #include "ubifs.h"
 #include <linux/mount.h>
 #include <linux/slab.h>
+#include <linux/migrate.h>
 
 static int read_block(struct inode *inode, void *addr, unsigned int block,
 		      struct ubifs_data_node *dn)
@@ -1452,6 +1453,26 @@ static int ubifs_set_page_dirty(struct page *page)
 	return ret;
 }
 
+#ifdef CONFIG_MIGRATION
+static int ubifs_migrate_page(struct address_space *mapping,
+		struct page *newpage, struct page *page, enum migrate_mode mode)
+{
+	int rc;
+
+	rc = migrate_page_move_mapping(mapping, newpage, page, NULL, mode, 0);
+	if (rc != MIGRATEPAGE_SUCCESS)
+		return rc;
+
+	if (PagePrivate(page)) {
+		ClearPagePrivate(page);
+		SetPagePrivate(newpage);
+	}
+
+	migrate_page_copy(newpage, page);
+	return MIGRATEPAGE_SUCCESS;
+}
+#endif
+
 static int ubifs_releasepage(struct page *page, gfp_t unused_gfp_flags)
 {
 	/*
@@ -1591,6 +1612,9 @@ const struct address_space_operations ubifs_file_address_operations = {
 	.write_end      = ubifs_write_end,
 	.invalidatepage = ubifs_invalidatepage,
 	.set_page_dirty = ubifs_set_page_dirty,
+#ifdef CONFIG_MIGRATION
+	.migratepage	= ubifs_migrate_page,
+#endif
 	.releasepage    = ubifs_releasepage,
 };
 
-- 
2.7.3