Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1368846 > unrolled thread

UBIFS and page migration (take 2)

Back to article view | Back to linux.kernel

Contents

  UBIFS and page migration (take 2) Richard Weinberger <richard@nod.at> - 2016-04-01 00:00 +0200
    [PATCH 2/2] UBIFS: Implement ->migratepage() Richard Weinberger <richard@nod.at> - 2016-04-01 00:00 +0200
      Re: [PATCH 2/2] UBIFS: Implement ->migratepage() Vlastimil Babka <vbabka@suse.cz> - 2016-04-01 12:20 +0200
        Re: [PATCH 2/2] UBIFS: Implement ->migratepage() Richard Weinberger <richard@nod.at> - 2016-04-01 13:30 +0200
    [PATCH 1/2] mm: Export migrate_page_move_mapping and migrate_page_copy Richard Weinberger <richard@nod.at> - 2016-04-01 00:10 +0200
    Re: UBIFS and page migration (take 2) Vlastimil Babka <vbabka@suse.cz> - 2016-04-01 12:20 +0200

#1368846 — UBIFS and page migration (take 2)

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.
UBIFS is using this flag among with PageChecked to account free space.
One possible solution is implementing a ->migratepage() function in UBIFS
which does more or less the same as fallback_migrate_page() but also
copies PagePrivate. I'm not at all sure whether this is they way to go.
IMHO either page migration should not happen if ->migratepage() is not implement
or fallback_migrate_page() has to work for all filesystems.

Comments? Flames? :-)

Thanks,
//richard

[PATCH 1/2] mm: Export migrate_page_move_mapping and
[PATCH 2/2] UBIFS: Implement ->migratepage()

[toc] | [next] | [standalone]

#1368847 — [PATCH 2/2] UBIFS: Implement ->migratepage()

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

During page migrations UBIFS might get confused
and the following assert triggers:
UBIFS assert failed in ubifs_set_page_dirty at 1451 (pid 436)

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]
Signed-off-by: Richard Weinberger <richard@nod.at>

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/file.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 065c88f..5eea5f5 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,24 @@ static int ubifs_set_page_dirty(struct page *page)
 	return ret;
 }
 
+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;
+}
+
 static int ubifs_releasepage(struct page *page, gfp_t unused_gfp_flags)
 {
 	/*
@@ -1591,6 +1610,7 @@ const struct address_space_operations ubifs_file_address_operations = {
 	.write_end      = ubifs_write_end,
 	.invalidatepage = ubifs_invalidatepage,
 	.set_page_dirty = ubifs_set_page_dirty,
+	.migratepage	= ubifs_migrate_page,
 	.releasepage    = ubifs_releasepage,
 };
 
-- 
1.8.4.5

[toc] | [prev] | [next] | [standalone]

#1369155 — Re: [PATCH 2/2] UBIFS: Implement ->migratepage()

On 03/31/2016 11:58 PM, Richard Weinberger wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> During page migrations UBIFS might get confused
> and the following assert triggers:
> UBIFS assert failed in ubifs_set_page_dirty at 1451 (pid 436)

It would be useful to have the full trace in changelog.

> 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]
> Signed-off-by: Richard Weinberger <richard@nod.at>

Stable?

> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>   fs/ubifs/file.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 065c88f..5eea5f5 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,24 @@ static int ubifs_set_page_dirty(struct page *page)
>   	return ret;
>   }
>
> +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;
> +}
> +
>   static int ubifs_releasepage(struct page *page, gfp_t unused_gfp_flags)
>   {
>   	/*
> @@ -1591,6 +1610,7 @@ const struct address_space_operations ubifs_file_address_operations = {
>   	.write_end      = ubifs_write_end,
>   	.invalidatepage = ubifs_invalidatepage,
>   	.set_page_dirty = ubifs_set_page_dirty,
> +	.migratepage	= ubifs_migrate_page,
>   	.releasepage    = ubifs_releasepage,
>   };
>
>

[toc] | [prev] | [next] | [standalone]

#1369232 — Re: [PATCH 2/2] UBIFS: Implement ->migratepage()

Am 01.04.2016 um 12:14 schrieb Vlastimil Babka:
> On 03/31/2016 11:58 PM, Richard Weinberger wrote:
>> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>>
>> During page migrations UBIFS might get confused
>> and the following assert triggers:
>> UBIFS assert failed in ubifs_set_page_dirty at 1451 (pid 436)
> 
> It would be useful to have the full trace in changelog.

Oh. Yes.

>> 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]
>> Signed-off-by: Richard Weinberger <richard@nod.at>
> 
> Stable?

Yep. But first I'd like to clarify if this approach is really the way to go.
It is also not clear to me whether this issue was always the case or if
a recently introduced change in MM uncovered it...
Blindly applying to all stable versions is risky.

Thanks,
//richard

[toc] | [prev] | [next] | [standalone]

#1368849 — [PATCH 1/2] mm: Export migrate_page_move_mapping and migrate_page_copy

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

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

diff --git a/mm/migrate.c b/mm/migrate.c
index 6c822a7..6bc1035 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
-- 
1.8.4.5

[toc] | [prev] | [next] | [standalone]

#1369161

On 03/31/2016 11:58 PM, Richard Weinberger wrote:
> 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.
> UBIFS is using this flag among with PageChecked to account free space.
> One possible solution is implementing a ->migratepage() function in UBIFS
> which does more or less the same as fallback_migrate_page() but also
> copies PagePrivate. I'm not at all sure whether this is they way to go.
> IMHO either page migration should not happen if ->migratepage() is not implement
> or fallback_migrate_page() has to work for all filesystems.

Yes, we could document more thoroughly the expectations of 
fallback_migrate_page() and audit the existing users, but still relying on every 
new address_space_operations instance to verify them isn't without risk. And I 
doubt there can be a default fallback that's guaranteed safe for all filesystems.

> Comments? Flames? :-)
>
> Thanks,
> //richard
>
> [PATCH 1/2] mm: Export migrate_page_move_mapping and
> [PATCH 2/2] UBIFS: Implement ->migratepage()
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web