mbind() fails to fail with EIO

mbind() fails to fail with EIO

[Cyril Hrubis]

Hi!
I've started to write tests for mbind() and found out that mbind() does
not work as described in manual page in a case that page has been
faulted on different node that we are asking it to bind to. Looks like
this is working fine on older kernels. On my testing machine with 3.0
mbind() fails correctly with EIO but succeeds unexpectedly on newer
kernels such as 4.12.

What the test does is:

* mmap() private mapping
* fault it
* find out on which node it is faulted on
* mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and
  expects to get EIO

The test code can be seen and compiled from:

https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] mbind() fails to fail with EIO

[Cyril Hrubis]

Hi!
I've started to write tests for mbind() and found out that mbind() does
not work as described in manual page in a case that page has been
faulted on different node that we are asking it to bind to. Looks like
this is working fine on older kernels. On my testing machine with 3.0
mbind() fails correctly with EIO but succeeds unexpectedly on newer
kernels such as 4.12.

What the test does is:

* mmap() private mapping
* fault it
* find out on which node it is faulted on
* mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and
  expects to get EIO

The test code can be seen and compiled from:

https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c

-- 
Cyril Hrubis
chrubis@suse.cz

Re: mbind() fails to fail with EIO

[Qian Cai]

On Fri, 2019-03-15 at 17:01 +0100, Cyril Hrubis wrote:
> Hi!
> I've started to write tests for mbind() and found out that mbind() does
> not work as described in manual page in a case that page has been
> faulted on different node that we are asking it to bind to. Looks like
> this is working fine on older kernels. On my testing machine with 3.0
> mbind() fails correctly with EIO but succeeds unexpectedly on newer
> kernels such as 4.12.
> 
> What the test does is:
> 
> * mmap() private mapping
> * fault it
> * find out on which node it is faulted on
> * mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and
>   expects to get EIO
> 
> The test code can be seen and compiled from:
> 
> https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/m
> bind02.c
> 

I am too lazy to checkout the repository and compile the whole thing just to be
able to reproduce. If you can make it a standalone program without LTP markups,
I'd be happy to take a look.

[LTP] mbind() fails to fail with EIO

[Qian Cai]

On Fri, 2019-03-15 at 17:01 +0100, Cyril Hrubis wrote:
> Hi!
> I've started to write tests for mbind() and found out that mbind() does
> not work as described in manual page in a case that page has been
> faulted on different node that we are asking it to bind to. Looks like
> this is working fine on older kernels. On my testing machine with 3.0
> mbind() fails correctly with EIO but succeeds unexpectedly on newer
> kernels such as 4.12.
> 
> What the test does is:
> 
> * mmap() private mapping
> * fault it
> * find out on which node it is faulted on
> * mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and
>   expects to get EIO
> 
> The test code can be seen and compiled from:
> 
> https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/m
> bind02.c
> 

I am too lazy to checkout the repository and compile the whole thing just to be
able to reproduce. If you can make it a standalone program without LTP markups,
I'd be happy to take a look.

Re: mbind() fails to fail with EIO

[Yang Shi]

On Fri, Mar 15, 2019 at 9:02 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> I've started to write tests for mbind() and found out that mbind() does
> not work as described in manual page in a case that page has been
> faulted on different node that we are asking it to bind to. Looks like
> this is working fine on older kernels. On my testing machine with 3.0
> mbind() fails correctly with EIO but succeeds unexpectedly on newer
> kernels such as 4.12.
>
> What the test does is:
>
> * mmap() private mapping
> * fault it
> * find out on which node it is faulted on
> * mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and
>   expects to get EIO

It looks the behavior was changed since v4.0 by the below commit:

6f4576e3687b mempolicy: apply page table walker on queue_pages_range()

The new queue_pages_to_pte_range() doesn't return -EIO anymore. Could
you please try the below patch (5.1-rc1 based)?

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index abe7a67..6ba45aa 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }

 static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,


Yang





>
> The test code can be seen and compiled from:
>
> https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>

[LTP] mbind() fails to fail with EIO

[Yang Shi]

On Fri, Mar 15, 2019 at 9:02 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> I've started to write tests for mbind() and found out that mbind() does
> not work as described in manual page in a case that page has been
> faulted on different node that we are asking it to bind to. Looks like
> this is working fine on older kernels. On my testing machine with 3.0
> mbind() fails correctly with EIO but succeeds unexpectedly on newer
> kernels such as 4.12.
>
> What the test does is:
>
> * mmap() private mapping
> * fault it
> * find out on which node it is faulted on
> * mbind() it to a different node with MPOL_BIND and MPOL_MF_STRICT and
>   expects to get EIO

It looks the behavior was changed since v4.0 by the below commit:

6f4576e3687b mempolicy: apply page table walker on queue_pages_range()

The new queue_pages_to_pte_range() doesn't return -EIO anymore. Could
you please try the below patch (5.1-rc1 based)?

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index abe7a67..6ba45aa 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }

 static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,


Yang





>
> The test code can be seen and compiled from:
>
> https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>

Re: mbind() fails to fail with EIO

[Cyril Hrubis]

Hi!
> I am too lazy to checkout the repository and compile the whole thing
> just to be able to reproduce. If you can make it a standalone program
> without LTP markups, I'd be happy to take a look.

Sigh, I've spend last few years so working on the testsuite so that you
can compile and run a single test from the checked out repository, we
even have howto for that in the README.md.

https://github.com/linux-test-project/ltp/#quick-guide-to-running-the-tests

It shuld be really easy, so can you please give it a try?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] mbind() fails to fail with EIO

[Cyril Hrubis]

Hi!
> I am too lazy to checkout the repository and compile the whole thing
> just to be able to reproduce. If you can make it a standalone program
> without LTP markups, I'd be happy to take a look.

Sigh, I've spend last few years so working on the testsuite so that you
can compile and run a single test from the checked out repository, we
even have howto for that in the README.md.

https://github.com/linux-test-project/ltp/#quick-guide-to-running-the-tests

It shuld be really easy, so can you please give it a try?

-- 
Cyril Hrubis
chrubis@suse.cz

Re: mbind() fails to fail with EIO

[Oscar Salvador]

+CC Kirill

On Mon, Mar 18, 2019 at 11:12:19AM -0700, Yang Shi wrote:
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index abe7a67..6ba45aa 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
> unsigned long addr,
>                         continue;
>                 if (!queue_pages_required(page, qp))
>                         continue;
> -               migrate_page_add(page, qp->pagelist, flags);
> +               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> +                       migrate_page_add(page, qp->pagelist, flags);
> +               else
> +                       break;
>         }
>         pte_unmap_unlock(pte - 1, ptl);
>         cond_resched();
> -       return 0;
> +       return addr != end ? -EIO : 0;
>  }
> 
>  static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,

This alone is not going to help.

The problem is that we do skip the vma early in queue_pages_test_walk() in
case MPOL_MF_MOVE and MPOL_MF_MOVE_ALL are not set.

walk_page_range
 walk_page_test
  queue_pages_test_walk

	...
 	...
	/* queue pages from current vma */
	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
		return 0;
	return 1;

So, we skip the vma and keep going.

Before ("77bf45e78050: mempolicy: do not try to queue pages from !vma_migratable()"),
queue_pages_test_walk() would not have skipped the vma in case we had MPOL_MF_STRICT
or MPOL_MF_MOVE | MPOL_MF_MOVE_ALL.

I did not give it a lot of thought, but it seems to me that we might need to reach
queue_pages_to_pte_range() in order to see whether the page is in the required node
or not by calling queue_pages_required(), and if it is not, check for
MPOL_MF_MOVE | MPOL_MF_MOVE_ALL like the above patch does, so we would be able to
return -EIO.
That would imply that we would need to re-add MPOL_MF_STRICT in queue_pages_test_walk().

-- 
Oscar Salvador
SUSE L3

[LTP] mbind() fails to fail with EIO

[Oscar Salvador]

+CC Kirill

On Mon, Mar 18, 2019 at 11:12:19AM -0700, Yang Shi wrote:
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index abe7a67..6ba45aa 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
> unsigned long addr,
>                         continue;
>                 if (!queue_pages_required(page, qp))
>                         continue;
> -               migrate_page_add(page, qp->pagelist, flags);
> +               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> +                       migrate_page_add(page, qp->pagelist, flags);
> +               else
> +                       break;
>         }
>         pte_unmap_unlock(pte - 1, ptl);
>         cond_resched();
> -       return 0;
> +       return addr != end ? -EIO : 0;
>  }
> 
>  static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,

This alone is not going to help.

The problem is that we do skip the vma early in queue_pages_test_walk() in
case MPOL_MF_MOVE and MPOL_MF_MOVE_ALL are not set.

walk_page_range
 walk_page_test
  queue_pages_test_walk

	...
 	...
	/* queue pages from current vma */
	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
		return 0;
	return 1;

So, we skip the vma and keep going.

Before ("77bf45e78050: mempolicy: do not try to queue pages from !vma_migratable()"),
queue_pages_test_walk() would not have skipped the vma in case we had MPOL_MF_STRICT
or MPOL_MF_MOVE | MPOL_MF_MOVE_ALL.

I did not give it a lot of thought, but it seems to me that we might need to reach
queue_pages_to_pte_range() in order to see whether the page is in the required node
or not by calling queue_pages_required(), and if it is not, check for
MPOL_MF_MOVE | MPOL_MF_MOVE_ALL like the above patch does, so we would be able to
return -EIO.
That would imply that we would need to re-add MPOL_MF_STRICT in queue_pages_test_walk().

-- 
Oscar Salvador
SUSE L3

Re: mbind() fails to fail with EIO

[Kirill A. Shutemov]

On Tue, Mar 19, 2019 at 02:27:33PM +0100, Oscar Salvador wrote:
> +CC Kirill
> 
> On Mon, Mar 18, 2019 at 11:12:19AM -0700, Yang Shi wrote:
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index abe7a67..6ba45aa 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
> > unsigned long addr,
> >                         continue;
> >                 if (!queue_pages_required(page, qp))
> >                         continue;
> > -               migrate_page_add(page, qp->pagelist, flags);
> > +               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> > +                       migrate_page_add(page, qp->pagelist, flags);
> > +               else
> > +                       break;
> >         }
> >         pte_unmap_unlock(pte - 1, ptl);
> >         cond_resched();
> > -       return 0;
> > +       return addr != end ? -EIO : 0;
> >  }
> > 
> >  static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,
> 
> This alone is not going to help.
> 
> The problem is that we do skip the vma early in queue_pages_test_walk() in
> case MPOL_MF_MOVE and MPOL_MF_MOVE_ALL are not set.
> 
> walk_page_range
>  walk_page_test
>   queue_pages_test_walk
> 
> 	...
>  	...
> 	/* queue pages from current vma */
> 	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> 		return 0;
> 	return 1;
> 
> So, we skip the vma and keep going.
> 
> Before ("77bf45e78050: mempolicy: do not try to queue pages from !vma_migratable()"),
> queue_pages_test_walk() would not have skipped the vma in case we had MPOL_MF_STRICT
> or MPOL_MF_MOVE | MPOL_MF_MOVE_ALL.
> 
> I did not give it a lot of thought, but it seems to me that we might need to reach
> queue_pages_to_pte_range() in order to see whether the page is in the required node
> or not by calling queue_pages_required(), and if it is not, check for
> MPOL_MF_MOVE | MPOL_MF_MOVE_ALL like the above patch does, so we would be able to
> return -EIO.
> That would imply that we would need to re-add MPOL_MF_STRICT in queue_pages_test_walk().

That's all sounds reasonable.

We only need to make sure the bug fixed by 77bf45e78050 will not be
re-introduced.

-- 
 Kirill A. Shutemov

[LTP] mbind() fails to fail with EIO

[Kirill A. Shutemov]

On Tue, Mar 19, 2019 at 02:27:33PM +0100, Oscar Salvador wrote:
> +CC Kirill
> 
> On Mon, Mar 18, 2019 at 11:12:19AM -0700, Yang Shi wrote:
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index abe7a67..6ba45aa 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -521,11 +521,14 @@ static int queue_pages_pte_range(pmd_t *pmd,
> > unsigned long addr,
> >                         continue;
> >                 if (!queue_pages_required(page, qp))
> >                         continue;
> > -               migrate_page_add(page, qp->pagelist, flags);
> > +               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> > +                       migrate_page_add(page, qp->pagelist, flags);
> > +               else
> > +                       break;
> >         }
> >         pte_unmap_unlock(pte - 1, ptl);
> >         cond_resched();
> > -       return 0;
> > +       return addr != end ? -EIO : 0;
> >  }
> > 
> >  static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,
> 
> This alone is not going to help.
> 
> The problem is that we do skip the vma early in queue_pages_test_walk() in
> case MPOL_MF_MOVE and MPOL_MF_MOVE_ALL are not set.
> 
> walk_page_range
>  walk_page_test
>   queue_pages_test_walk
> 
> 	...
>  	...
> 	/* queue pages from current vma */
> 	if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
> 		return 0;
> 	return 1;
> 
> So, we skip the vma and keep going.
> 
> Before ("77bf45e78050: mempolicy: do not try to queue pages from !vma_migratable()"),
> queue_pages_test_walk() would not have skipped the vma in case we had MPOL_MF_STRICT
> or MPOL_MF_MOVE | MPOL_MF_MOVE_ALL.
> 
> I did not give it a lot of thought, but it seems to me that we might need to reach
> queue_pages_to_pte_range() in order to see whether the page is in the required node
> or not by calling queue_pages_required(), and if it is not, check for
> MPOL_MF_MOVE | MPOL_MF_MOVE_ALL like the above patch does, so we would be able to
> return -EIO.
> That would imply that we would need to re-add MPOL_MF_STRICT in queue_pages_test_walk().

That's all sounds reasonable.

We only need to make sure the bug fixed by 77bf45e78050 will not be
re-introduced.

-- 
 Kirill A. Shutemov

Re: mbind() fails to fail with EIO

[Cyril Hrubis]

Hi!
> That's all sounds reasonable.
> 
> We only need to make sure the bug fixed by 77bf45e78050 will not be
> re-introduced.

Well, we can turn the reproducer from that commit into an automated
test, that's the only way to make sure bugs are not reintroduced
anyways...

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] mbind() fails to fail with EIO

[Cyril Hrubis]

Hi!
> That's all sounds reasonable.
> 
> We only need to make sure the bug fixed by 77bf45e78050 will not be
> re-introduced.

Well, we can turn the reproducer from that commit into an automated
test, that's the only way to make sure bugs are not reintroduced
anyways...

-- 
Cyril Hrubis
chrubis@suse.cz

Re: mbind() fails to fail with EIO

[Oscar Salvador]

On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> That's all sounds reasonable.
> 
> We only need to make sure the bug fixed by 77bf45e78050 will not be
> re-introduced.

I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO
when running the ltp test [1].
So unless I am missing something, it should be enough.

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index af171ccb56a2..b192b13460f0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -502,11 +508,16 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }

 static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,
@@ -603,7 +614,8 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
        }

        /* queue pages from current vma */
-       if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+       if ((flags & MPOL_MF_STRICT) ||
+               (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
                return 0;
  

[1] https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c

-- 
Oscar Salvador
SUSE L3

[LTP] mbind() fails to fail with EIO

[Oscar Salvador]

On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> That's all sounds reasonable.
> 
> We only need to make sure the bug fixed by 77bf45e78050 will not be
> re-introduced.

I gave it a spin with the below patch.
Your testcase works (so the bug is not re-introduced), and we get -EIO
when running the ltp test [1].
So unless I am missing something, it should be enough.

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index af171ccb56a2..b192b13460f0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -502,11 +508,16 @@ static int queue_pages_pte_range(pmd_t *pmd, unsigned long addr,
                        continue;
                if (!queue_pages_required(page, qp))
                        continue;
-               migrate_page_add(page, qp->pagelist, flags);
+               if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+                       migrate_page_add(page, qp->pagelist, flags);
+               else
+                       break;
        }
        pte_unmap_unlock(pte - 1, ptl);
        cond_resched();
-       return 0;
+       return addr != end ? -EIO : 0;
 }

 static int queue_pages_hugetlb(pte_t *pte, unsigned long hmask,
@@ -603,7 +614,8 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end,
        }

        /* queue pages from current vma */
-       if (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
+       if ((flags & MPOL_MF_STRICT) ||
+               (flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL))
                return 0;
  

[1] https://github.com/metan-ucw/ltp/blob/master/testcases/kernel/syscalls/mbind/mbind02.c

-- 
Oscar Salvador
SUSE L3

Re: mbind() fails to fail with EIO

[Kirill A. Shutemov]

On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > That's all sounds reasonable.
> > 
> > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > re-introduced.
> 
> I gave it a spin with the below patch.
> Your testcase works (so the bug is not re-introduced), and we get -EIO
> when running the ltp test [1].
> So unless I am missing something, it should be enough.

Don't we need to bypass !vma_migratable(vma) check in
queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
to check if all pages are on the right not even the vma is not migratable.

-- 
 Kirill A. Shutemov

[LTP] mbind() fails to fail with EIO

[Kirill A. Shutemov]

On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > That's all sounds reasonable.
> > 
> > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > re-introduced.
> 
> I gave it a spin with the below patch.
> Your testcase works (so the bug is not re-introduced), and we get -EIO
> when running the ltp test [1].
> So unless I am missing something, it should be enough.

Don't we need to bypass !vma_migratable(vma) check in
queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
to check if all pages are on the right not even the vma is not migratable.

-- 
 Kirill A. Shutemov

Re: mbind() fails to fail with EIO

[Oscar Salvador]

On Tue, Mar 19, 2019 at 05:52:33PM +0300, Kirill A. Shutemov wrote:
> On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> > On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > > That's all sounds reasonable.
> > > 
> > > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > > re-introduced.
> > 
> > I gave it a spin with the below patch.
> > Your testcase works (so the bug is not re-introduced), and we get -EIO
> > when running the ltp test [1].
> > So unless I am missing something, it should be enough.
> 
> Don't we need to bypass !vma_migratable(vma) check in
> queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
> to check if all pages are on the right not even the vma is not migratable.

Yeah, I missed that.
Then, I guess that we have to put the check into queue_pages_pte_range as well,
and place it right before migrate_page_add().
So, if it is not placed in the node and is not migreatable, we return -EIO.

-- 
Oscar Salvador
SUSE L3

[LTP] mbind() fails to fail with EIO

[Oscar Salvador]

On Tue, Mar 19, 2019 at 05:52:33PM +0300, Kirill A. Shutemov wrote:
> On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> > On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > > That's all sounds reasonable.
> > > 
> > > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > > re-introduced.
> > 
> > I gave it a spin with the below patch.
> > Your testcase works (so the bug is not re-introduced), and we get -EIO
> > when running the ltp test [1].
> > So unless I am missing something, it should be enough.
> 
> Don't we need to bypass !vma_migratable(vma) check in
> queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
> to check if all pages are on the right not even the vma is not migratable.

Yeah, I missed that.
Then, I guess that we have to put the check into queue_pages_pte_range as well,
and place it right before migrate_page_add().
So, if it is not placed in the node and is not migreatable, we return -EIO.

-- 
Oscar Salvador
SUSE L3

Re: mbind() fails to fail with EIO

[Yang Shi]

On Tue, Mar 19, 2019 at 7:52 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> > On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > > That's all sounds reasonable.
> > >
> > > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > > re-introduced.
> >
> > I gave it a spin with the below patch.
> > Your testcase works (so the bug is not re-introduced), and we get -EIO
> > when running the ltp test [1].
> > So unless I am missing something, it should be enough.

Thanks for adding the missing part.

>
> Don't we need to bypass !vma_migratable(vma) check in
> queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
> to check if all pages are on the right not even the vma is not migratable.

I think we need. As long as there is "existing page was already on a
node that does not follow the policy" with MPOL_MF_STRICT, it should
return -EIO. So, even though the vma is not migratable it should check
if the above condition is true or not.

I will wrap all the stuff into a formal patch.

Thanks,
Yang

>
> --
>  Kirill A. Shutemov

[LTP] mbind() fails to fail with EIO

[Yang Shi]

On Tue, Mar 19, 2019 at 7:52 AM Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> > On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > > That's all sounds reasonable.
> > >
> > > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > > re-introduced.
> >
> > I gave it a spin with the below patch.
> > Your testcase works (so the bug is not re-introduced), and we get -EIO
> > when running the ltp test [1].
> > So unless I am missing something, it should be enough.

Thanks for adding the missing part.

>
> Don't we need to bypass !vma_migratable(vma) check in
> queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
> to check if all pages are on the right not even the vma is not migratable.

I think we need. As long as there is "existing page was already on a
node that does not follow the policy" with MPOL_MF_STRICT, it should
return -EIO. So, even though the vma is not migratable it should check
if the above condition is true or not.

I will wrap all the stuff into a formal patch.

Thanks,
Yang

>
> --
>  Kirill A. Shutemov

Re: mbind() fails to fail with EIO

[Yang Shi]

On Tue, Mar 19, 2019 at 8:10 AM Oscar Salvador <osalvador@suse.de> wrote:
>
> On Tue, Mar 19, 2019 at 05:52:33PM +0300, Kirill A. Shutemov wrote:
> > On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> > > On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > > > That's all sounds reasonable.
> > > >
> > > > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > > > re-introduced.
> > >
> > > I gave it a spin with the below patch.
> > > Your testcase works (so the bug is not re-introduced), and we get -EIO
> > > when running the ltp test [1].
> > > So unless I am missing something, it should be enough.
> >
> > Don't we need to bypass !vma_migratable(vma) check in
> > queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
> > to check if all pages are on the right not even the vma is not migratable.
>
> Yeah, I missed that.
> Then, I guess that we have to put the check into queue_pages_pte_range as well,
> and place it right before migrate_page_add().
> So, if it is not placed in the node and is not migreatable, we return -EIO.

Sorry, I didn't see this reply before I replied Kirill's email
earlier. Yes, I agree, it should return -EIO too.

Thanks,
Yang

>
> --
> Oscar Salvador
> SUSE L3

[LTP] mbind() fails to fail with EIO

[Yang Shi]

On Tue, Mar 19, 2019 at 8:10 AM Oscar Salvador <osalvador@suse.de> wrote:
>
> On Tue, Mar 19, 2019 at 05:52:33PM +0300, Kirill A. Shutemov wrote:
> > On Tue, Mar 19, 2019 at 03:41:33PM +0100, Oscar Salvador wrote:
> > > On Tue, Mar 19, 2019 at 05:26:39PM +0300, Kirill A. Shutemov wrote:
> > > > That's all sounds reasonable.
> > > >
> > > > We only need to make sure the bug fixed by 77bf45e78050 will not be
> > > > re-introduced.
> > >
> > > I gave it a spin with the below patch.
> > > Your testcase works (so the bug is not re-introduced), and we get -EIO
> > > when running the ltp test [1].
> > > So unless I am missing something, it should be enough.
> >
> > Don't we need to bypass !vma_migratable(vma) check in
> > queue_pages_test_walk() for MPOL_MF_STRICT? I mean user still might want
> > to check if all pages are on the right not even the vma is not migratable.
>
> Yeah, I missed that.
> Then, I guess that we have to put the check into queue_pages_pte_range as well,
> and place it right before migrate_page_add().
> So, if it is not placed in the node and is not migreatable, we return -EIO.

Sorry, I didn't see this reply before I replied Kirill's email
earlier. Yes, I agree, it should return -EIO too.

Thanks,
Yang

>
> --
> Oscar Salvador
> SUSE L3