diff mbox

UBIFS: Implement ->migratepage()

Message ID 1458168919-11597-1-git-send-email-richard@nod.at (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Weinberger March 16, 2016, 10:55 p.m. UTC
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

When using CMA 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.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
[rw: Massaged changelog]
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/file.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

kernel test robot March 16, 2016, 11:12 p.m. UTC | #1
Hi Kirill,

[auto build test ERROR on v4.5-rc7]
[also build test ERROR on next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Richard-Weinberger/UBIFS-Implement-migratepage/20160317-065742
config: sh-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   fs/ubifs/file.c: In function 'ubifs_migrate_page':
>> fs/ubifs/file.c:1461:2: error: implicit declaration of function 'migrate_page_move_mapping' [-Werror=implicit-function-declaration]
   cc1: some warnings being treated as errors

vim +/migrate_page_move_mapping +1461 fs/ubifs/file.c

  1455	
  1456	static int ubifs_migrate_page(struct address_space *mapping,
  1457			struct page *newpage, struct page *page, enum migrate_mode mode)
  1458	{
  1459		int rc;
  1460	
> 1461		rc = migrate_page_move_mapping(mapping, newpage, page, NULL, mode, 0);
  1462		if (rc != MIGRATEPAGE_SUCCESS)
  1463			return rc;
  1464	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot March 17, 2016, 4:39 a.m. UTC | #2
Hi Kirill,

[auto build test ERROR on v4.5-rc7]
[also build test ERROR on next-20160316]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Richard-Weinberger/UBIFS-Implement-migratepage/20160317-065742
config: x86_64-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "migrate_page_move_mapping" [fs/ubifs/ubifs.ko] undefined!
>> ERROR: "migrate_page_copy" [fs/ubifs/ubifs.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Richard Weinberger March 17, 2016, 8:09 a.m. UTC | #3
Am 17.03.2016 um 05:39 schrieb kbuild test robot:
> Hi Kirill,
> 
> [auto build test ERROR on v4.5-rc7]
> [also build test ERROR on next-20160316]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Richard-Weinberger/UBIFS-Implement-migratepage/20160317-065742
> config: x86_64-allmodconfig (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>>> ERROR: "migrate_page_move_mapping" [fs/ubifs/ubifs.ko] undefined!
>>> ERROR: "migrate_page_copy" [fs/ubifs/ubifs.ko] undefined!

Meh. Just noticted that these functions are not exported and therefore not
usable in modules.
So, this patch is not really the solution although it makes the problem go away.

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlastimil Babka March 17, 2016, 9:57 a.m. UTC | #4
+CC Hugh, Mel

On 03/16/2016 11:55 PM, Richard Weinberger wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> When using CMA during page migrations UBIFS might get confused

It shouldn't be CMA specific, the same code runs from compaction, 
autonuma balancing...

> 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.

Lack of PagePrivate() migration is surely a bug, but at a glance of how 
UBIFS uses the flag, it's more about accounting, it shouldn't prevent a 
page from being marked PageDirty()?
I suspect your initial bug (which is IIUC the fact that there's a dirty 
pte, but PageDirty(page) is false) comes from the generic 
fallback_migrate_page() which does:

         if (PageDirty(page)) {
                 /* Only writeback pages in full synchronous migration */
                 if (mode != MIGRATE_SYNC)
                         return -EBUSY;
                 return writeout(mapping, page);
         }

And writeout() seems to Clear PageDirty() through 
clear_page_dirty_for_io() but I'm not so sure about the pte (or pte's in 
all rmaps). But this comment in the latter function:

                  * Yes, Virginia, this is indeed insane.

scared me enough to not investigate further. Hopefully the people I CC'd 
understand more about page migration than me. I'm just an user :)

In any case, this patch would solve both lack of PageDirty() transfer, 
and avoid the path leading from fallback_migrate_page() to writeout(). 
But I'm not confident enough here to ack it.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [rw: Massaged changelog]
> 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 0edc128..48b2944 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,
>   };
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Weinberger March 25, 2016, 10:53 p.m. UTC | #5
Am 17.03.2016 um 10:57 schrieb Vlastimil Babka:
> +CC Hugh, Mel
> 
> On 03/16/2016 11:55 PM, Richard Weinberger wrote:
>> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>>
>> When using CMA during page migrations UBIFS might get confused
> 
> It shouldn't be CMA specific, the same code runs from compaction, autonuma balancing...
> 
>> 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.
> 
> Lack of PagePrivate() migration is surely a bug, but at a glance of how UBIFS uses the flag, it's more about accounting, it shouldn't prevent a page from being marked PageDirty()?
> I suspect your initial bug (which is IIUC the fact that there's a dirty pte, but PageDirty(page) is false) comes from the generic fallback_migrate_page() which does:
> 
>         if (PageDirty(page)) {
>                 /* Only writeback pages in full synchronous migration */
>                 if (mode != MIGRATE_SYNC)
>                         return -EBUSY;
>                 return writeout(mapping, page);
>         }
> 
> And writeout() seems to Clear PageDirty() through clear_page_dirty_for_io() but I'm not so sure about the pte (or pte's in all rmaps). But this comment in the latter function:
> 
>                  * Yes, Virginia, this is indeed insane.
> 
> scared me enough to not investigate further. Hopefully the people I CC'd understand more about page migration than me. I'm just an user :)
> 
> In any case, this patch would solve both lack of PageDirty() transfer, and avoid the path leading from fallback_migrate_page() to writeout(). But I'm not confident enough here to
> ack it.

Hugh? Mel? Anyone? :-)

It is still not clear to me whether this needs fixing in MM or UBIFS.

Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 0edc128..48b2944 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,
 };