Re: [PATCH] Fix do_move_pages_to_node() error handling

[Michal Hocko <mhocko@suse.com>]

On Tue 13-11-18 16:40:59, p.jaroszynski@gmail.com wrote:
> From: Piotr Jaroszynski <pjaroszynski@nvidia.com>
> 
> migrate_pages() can return the number of pages that failed to migrate
> instead of 0 or an error code. If that happens, the positive return is
> treated as an error all the way up through the stack leading to the
> move_pages() syscall returning a positive number. I believe this
> regressed with commit a49bd4d71637 ("mm, numa: rework do_pages_move")
> that refactored a lot of this code.

Yes this is correct.

> Fix this by treating positive returns as success in
> do_move_pages_to_node() as that seems to most closely follow the
> previous code. This still leaves the question whether silently
> considering this case a success is the right thing to do as even the
> status of the pages will be set as if they were successfully migrated,
> but that seems to have been the case before as well.

Yes, I believe the previous semantic was just wrong and we want to fix
it. Jan has already brought this up [1]. I believe we want to update the
documentation rather than restore the previous hazy semantic.

Just wondering, how have you found out? Is there any real application
failing because of the change or this is a result of some test?

[1] http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com
> 
> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
> Signed-off-by: Piotr Jaroszynski <pjaroszynski@nvidia.com>
> ---
>  mm/migrate.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8baeb7ff2f6d..b42efef780d6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1461,6 +1461,7 @@ static int store_status(int __user *status, int start, int value, int nr)
>  	return 0;
>  }
>  
> +/* Returns 0 or an error code. */
>  static int do_move_pages_to_node(struct mm_struct *mm,
>  		struct list_head *pagelist, int node)
>  {
> @@ -1473,6 +1474,15 @@ static int do_move_pages_to_node(struct mm_struct *mm,
>  			MIGRATE_SYNC, MR_SYSCALL);
>  	if (err)
>  		putback_movable_pages(pagelist);
> +
> +	/*
> +	 * migrate_pages() can return the number of not migrated pages, but the
> +	 * callers of do_move_pages_to_node() only care about and handle hard
> +	 * failures.
> +	 */
> +	if (err > 0)
> +		err = 0;
> +
>  	return err;
>  }
>  
> -- 
> 2.11.0.262.g4b0a5b2.dirty
> 

-- 
Michal Hocko
SUSE Labs