Message ID | 1458168919-11597-1-git-send-email-richard@nod.at (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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
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
+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
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 --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, };