Re: [PATCH] mm: fix kswap excessive pressure after wrong condition transfer

[Greg Thelen <gthelen@google.com>]

On Thu, May 31, 2018 at 12:34 PM Ivan Kalvachev <ikalvachev@gmail.com> wrote:
>
> Fixes commit 69d763fc6d3aee787a3e8c8c35092b4f4960fa5d
> (mm: pin address_space before dereferencing it while isolating an LRU page)
>
> working code:
>
>     mapping = page_mapping(page);
>     if (mapping && !mapping->a_ops->migratepage)
>         return ret;
>
> buggy code:
>
>     if (!trylock_page(page))
>         return ret;
>
>     mapping = page_mapping(page);
>     migrate_dirty = mapping && mapping->a_ops->migratepage;
>     unlock_page(page);
>     if (!migrate_dirty)
>         return ret;
>
> The problem is that !(a && b) = (!a || !b) while the old code was (a && !b).
> The commit message of the buggy commit explains the need for locking/unlocking
> around the check but does not give any reason for the change of the condition.
> It seems to be an unintended change.
>
> The result of that change is noticeable under swap pressure.
> Big memory consumers like browsers would have a lot of pages swapped out,
> even pages that are been used actively, causing the process to repeatedly
> block for second or longer. At the same time there would be gigabytes of
> unused free memory (sometimes half of the total RAM).
> The buffers/cache would also be at minimum size.
>
> Fixes: 69d763fc6d3a ("mm: pin address_space before dereferencing it while isolating an LRU page")
> Signed-off-by: Ivan Kalvachev <ikalvachev@gmail.com>
> ---
>  mm/vmscan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 9b697323a88c..83df26078d13 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1418,9 +1418,9 @@ int __isolate_lru_page(struct page *page, isolate_mode_t mode)
>                                 return ret;
>
>                         mapping = page_mapping(page);
> -                       migrate_dirty = mapping && mapping->a_ops->migratepage;
> +                       migrate_dirty = mapping && !mapping->a_ops->migratepage;
>                         unlock_page(page);
> -                       if (!migrate_dirty)
> +                       if (migrate_dirty)
>                                 return ret;
>                 }
>         }
> --
> 2.17.1

This looks like yesterday's https://lkml.org/lkml/2018/5/30/1158