All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] btrfs: implement migratepage callback
@ 2020-03-04 19:50 Roman Gushchin
  2020-03-04 19:53 ` Chris Mason
  2020-03-05  0:22 ` kbuild test robot
  0 siblings, 2 replies; 4+ messages in thread
From: Roman Gushchin @ 2020-03-04 19:50 UTC (permalink / raw)
  To: Josef Bacik, Chris Mason, David Sterba
  Cc: linux-btrfs, linux-kernel, kernel-team, Rik van Riel, Roman Gushchin

Currently btrfs doesn't provide a migratepage callback. It means that
fallback_migrate_page()	is used to migrate btrfs pages.

fallback_migrate_page() cannot move dirty pages, instead it tries to
flush them (in sync mode) or just fails (in async mode).

In the sync mode pages which are scheduled to be processed by
btrfs_writepage_fixup_worker() can't be effectively flushed by the
migration code, because there is no established way to wait for the
completion of the delayed work.

It all leads to page migration failures.

To fix it the patch implements a btrs-specific migratepage callback,
which is similar to iomap_migrate_page() used by some other fs, except
it does take care of the PagePrivate2 flag which is used for data
ordering purposes.

Signed-off-by: Roman Gushchin <guro@fb.com>
Reviewed-by: Chris Mason <clm@fb.com>
---
 fs/btrfs/inode.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7735ce6127c3..f23230b3cbda 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -28,6 +28,7 @@
 #include <linux/magic.h>
 #include <linux/iversion.h>
 #include <linux/swap.h>
+#include <linux/migrate.h>
 #include <linux/sched/mm.h>
 #include <asm/unaligned.h>
 #include "misc.h"
@@ -8323,6 +8324,37 @@ static int btrfs_releasepage(struct page *page, gfp_t gfp_flags)
 	return __btrfs_releasepage(page, gfp_flags);
 }
 
+static int btrfs_migratepage(struct address_space *mapping,
+			     struct page *newpage, struct page *page,
+			     enum migrate_mode mode)
+{
+	int ret;
+
+	ret = migrate_page_move_mapping(mapping, newpage, page, 0);
+	if (ret != MIGRATEPAGE_SUCCESS)
+		return ret;
+
+	if (page_has_private(page)) {
+		ClearPagePrivate(page);
+		get_page(newpage);
+		set_page_private(newpage, page_private(page));
+		set_page_private(page, 0);
+		put_page(page);
+		SetPagePrivate(newpage);
+	}
+
+	if (PagePrivate2(page)) {
+		ClearPagePrivate2(page);
+		SetPagePrivate2(newpage);
+	}
+
+	if (mode != MIGRATE_SYNC_NO_COPY)
+		migrate_page_copy(newpage, page);
+	else
+		migrate_page_states(newpage, page);
+	return MIGRATEPAGE_SUCCESS;
+}
+
 static void btrfs_invalidatepage(struct page *page, unsigned int offset,
 				 unsigned int length)
 {
@@ -10525,6 +10557,7 @@ static const struct address_space_operations btrfs_aops = {
 	.direct_IO	= btrfs_direct_IO,
 	.invalidatepage = btrfs_invalidatepage,
 	.releasepage	= btrfs_releasepage,
+	.migratepage	= btrfs_migratepage,
 	.set_page_dirty	= btrfs_set_page_dirty,
 	.error_remove_page = generic_error_remove_page,
 	.swap_activate	= btrfs_swap_activate,
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] btrfs: implement migratepage callback
  2020-03-04 19:50 [PATCH] btrfs: implement migratepage callback Roman Gushchin
@ 2020-03-04 19:53 ` Chris Mason
  2020-03-04 20:19   ` Roman Gushchin
  2020-03-05  0:22 ` kbuild test robot
  1 sibling, 1 reply; 4+ messages in thread
From: Chris Mason @ 2020-03-04 19:53 UTC (permalink / raw)
  To: Roman Gushchin
  Cc: Josef Bacik, David Sterba, linux-btrfs, linux-kernel,
	kernel-team, Rik van Riel



On 4 Mar 2020, at 14:50, Roman Gushchin wrote:

> Currently btrfs doesn't provide a migratepage callback. It means that
> fallback_migrate_page()	is used to migrate btrfs pages.
>
> fallback_migrate_page() cannot move dirty pages, instead it tries to
> flush them (in sync mode) or just fails (in async mode).
>
> In the sync mode pages which are scheduled to be processed by
> btrfs_writepage_fixup_worker() can't be effectively flushed by the
> migration code, because there is no established way to wait for the
> completion of the delayed work.
>
> It all leads to page migration failures.
>
> To fix it the patch implements a btrs-specific migratepage callback,
> which is similar to iomap_migrate_page() used by some other fs, except
> it does take care of the PagePrivate2 flag which is used for data
> ordering purposes.

Since the default migratepage didn’t copy PagePrivate2, didn’t you 
find it was also causing pages to get funneled into the fixup worker 
flow?

-chris

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] btrfs: implement migratepage callback
  2020-03-04 19:53 ` Chris Mason
@ 2020-03-04 20:19   ` Roman Gushchin
  0 siblings, 0 replies; 4+ messages in thread
From: Roman Gushchin @ 2020-03-04 20:19 UTC (permalink / raw)
  To: Chris Mason
  Cc: Josef Bacik, David Sterba, linux-btrfs, linux-kernel,
	kernel-team, Rik van Riel

On Wed, Mar 04, 2020 at 02:53:21PM -0500, Chris Mason wrote:
> 
> 
> On 4 Mar 2020, at 14:50, Roman Gushchin wrote:
> 
> > Currently btrfs doesn't provide a migratepage callback. It means that
> > fallback_migrate_page()	is used to migrate btrfs pages.
> > 
> > fallback_migrate_page() cannot move dirty pages, instead it tries to
> > flush them (in sync mode) or just fails (in async mode).
> > 
> > In the sync mode pages which are scheduled to be processed by
> > btrfs_writepage_fixup_worker() can't be effectively flushed by the
> > migration code, because there is no established way to wait for the
> > completion of the delayed work.
> > 
> > It all leads to page migration failures.
> > 
> > To fix it the patch implements a btrs-specific migratepage callback,
> > which is similar to iomap_migrate_page() used by some other fs, except
> > it does take care of the PagePrivate2 flag which is used for data
> > ordering purposes.
> 
> Since the default migratepage didn’t copy PagePrivate2, didn’t you find it
> was also causing pages to get funneled into the fixup worker flow?

A good question.

I've definitely seen a lot of fixup worker activity.

On the other hand the default (fallback) migration path is flushing
the page first (if dirty), so it should not move dirty pages.
If PagePrivate2 can outlive PageDirty, then the answer is yes.

Thanks!

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] btrfs: implement migratepage callback
  2020-03-04 19:50 [PATCH] btrfs: implement migratepage callback Roman Gushchin
  2020-03-04 19:53 ` Chris Mason
@ 2020-03-05  0:22 ` kbuild test robot
  1 sibling, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2020-03-05  0:22 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]

Hi Roman,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.6-rc4]
[also build test ERROR on next-20200304]
[cannot apply to btrfs/next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Roman-Gushchin/btrfs-implement-migratepage-callback/20200305-060949
base:    98d54f81e36ba3bf92172791eba5ca5bd813989b
config: c6x-allyesconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   fs/btrfs/inode.c: In function 'btrfs_migratepage':
>> fs/btrfs/inode.c:8280:8: error: implicit declaration of function 'migrate_page_move_mapping'; did you mean 'migrate_huge_page_move_mapping'? [-Werror=implicit-function-declaration]
     ret = migrate_page_move_mapping(mapping, newpage, page, 0);
           ^~~~~~~~~~~~~~~~~~~~~~~~~
           migrate_huge_page_move_mapping
   cc1: some warnings being treated as errors

vim +8280 fs/btrfs/inode.c

  8273	
  8274	static int btrfs_migratepage(struct address_space *mapping,
  8275				     struct page *newpage, struct page *page,
  8276				     enum migrate_mode mode)
  8277	{
  8278		int ret;
  8279	
> 8280		ret = migrate_page_move_mapping(mapping, newpage, page, 0);
  8281		if (ret != MIGRATEPAGE_SUCCESS)
  8282			return ret;
  8283	
  8284		if (page_has_private(page)) {
  8285			ClearPagePrivate(page);
  8286			get_page(newpage);
  8287			set_page_private(newpage, page_private(page));
  8288			set_page_private(page, 0);
  8289			put_page(page);
  8290			SetPagePrivate(newpage);
  8291		}
  8292	
  8293		if (PagePrivate2(page)) {
  8294			ClearPagePrivate2(page);
  8295			SetPagePrivate2(newpage);
  8296		}
  8297	
  8298		if (mode != MIGRATE_SYNC_NO_COPY)
  8299			migrate_page_copy(newpage, page);
  8300		else
  8301			migrate_page_states(newpage, page);
  8302		return MIGRATEPAGE_SUCCESS;
  8303	}
  8304	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 51588 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-03-05  0:22 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04 19:50 [PATCH] btrfs: implement migratepage callback Roman Gushchin
2020-03-04 19:53 ` Chris Mason
2020-03-04 20:19   ` Roman Gushchin
2020-03-05  0:22 ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.