[PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Henry Burns]

__SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
lock the page. Following zsmalloc.c's example we call trylock_page() and
unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
passed in locked, as documentation.

Signed-off-by: Henry Burns <henryburns@google.com>
Suggested-by: Vitaly Wool <vitalywool@gmail.com>
---
 Changelog since v1:
 - Added an if statement around WARN_ON(trylock_page(page)) to avoid
   unlocking a page locked by a someone else.

 mm/z3fold.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/z3fold.c b/mm/z3fold.c
index e174d1549734..6341435b9610 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
 		set_bit(PAGE_HEADLESS, &page->private);
 		goto headless;
 	}
-	__SetPageMovable(page, pool->inode->i_mapping);
+	if (!WARN_ON(!trylock_page(page))) {
+		__SetPageMovable(page, pool->inode->i_mapping);
+		unlock_page(page);
+	}
 	z3fold_page_lock(zhdr);
 
 found:
@@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
 
 	VM_BUG_ON_PAGE(!PageMovable(page), page);
 	VM_BUG_ON_PAGE(!PageIsolated(page), page);
+	VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
 
 	zhdr = page_address(page);
 	pool = zhdr_to_pool(zhdr);
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Shakeel Butt]

On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote:
>
> __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> lock the page. Following zsmalloc.c's example we call trylock_page() and
> unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> passed in locked, as documentation.
>
> Signed-off-by: Henry Burns <henryburns@google.com>
> Suggested-by: Vitaly Wool <vitalywool@gmail.com>
> ---
>  Changelog since v1:
>  - Added an if statement around WARN_ON(trylock_page(page)) to avoid
>    unlocking a page locked by a someone else.
>
>  mm/z3fold.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e174d1549734..6341435b9610 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
>                 set_bit(PAGE_HEADLESS, &page->private);
>                 goto headless;
>         }
> -       __SetPageMovable(page, pool->inode->i_mapping);
> +       if (!WARN_ON(!trylock_page(page))) {
> +               __SetPageMovable(page, pool->inode->i_mapping);
> +               unlock_page(page);
> +       }

Can you please comment why lock_page() is not used here?

>         z3fold_page_lock(zhdr);
>
>  found:
> @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
>
>         VM_BUG_ON_PAGE(!PageMovable(page), page);
>         VM_BUG_ON_PAGE(!PageIsolated(page), page);
> +       VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
>
>         zhdr = page_address(page);
>         pool = zhdr_to_pool(zhdr);
> --
> 2.22.0.410.gd8fdbe21b5-goog
>

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Henry Burns]

On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote:
> >
> > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> > lock the page. Following zsmalloc.c's example we call trylock_page() and
> > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> > passed in locked, as documentation.
> >
> > Signed-off-by: Henry Burns <henryburns@google.com>
> > Suggested-by: Vitaly Wool <vitalywool@gmail.com>
> > ---
> >  Changelog since v1:
> >  - Added an if statement around WARN_ON(trylock_page(page)) to avoid
> >    unlocking a page locked by a someone else.
> >
> >  mm/z3fold.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > index e174d1549734..6341435b9610 100644
> > --- a/mm/z3fold.c
> > +++ b/mm/z3fold.c
> > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> >                 set_bit(PAGE_HEADLESS, &page->private);
> >                 goto headless;
> >         }
> > -       __SetPageMovable(page, pool->inode->i_mapping);
> > +       if (!WARN_ON(!trylock_page(page))) {
> > +               __SetPageMovable(page, pool->inode->i_mapping);
> > +               unlock_page(page);
> > +       }
>
> Can you please comment why lock_page() is not used here?
Since z3fold_alloc can be called in atomic or non atomic context,
calling lock_page() could trigger a number of
warnings about might_sleep() being called in atomic context. WARN_ON
should avoid the problem described
above as well, and in any weird condition where someone else has the
page lock, we can avoid calling
__SetPageMovable().
>
> >         z3fold_page_lock(zhdr);
> >
> >  found:
> > @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
> >
> >         VM_BUG_ON_PAGE(!PageMovable(page), page);
> >         VM_BUG_ON_PAGE(!PageIsolated(page), page);
> > +       VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
> >
> >         zhdr = page_address(page);
> >         pool = zhdr_to_pool(zhdr);
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Vitaly Wool]

On Tue, Jul 2, 2019 at 3:51 AM Henry Burns <henryburns@google.com> wrote:
>
> __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> lock the page. Following zsmalloc.c's example we call trylock_page() and
> unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> passed in locked, as documentation.
>
> Signed-off-by: Henry Burns <henryburns@google.com>
> Suggested-by: Vitaly Wool <vitalywool@gmail.com>

Acked-by: Vitaly Wool <vitalywool@gmail.com>

Thanks!

> ---
>  Changelog since v1:
>  - Added an if statement around WARN_ON(trylock_page(page)) to avoid
>    unlocking a page locked by a someone else.
>
>  mm/z3fold.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e174d1549734..6341435b9610 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
>                 set_bit(PAGE_HEADLESS, &page->private);
>                 goto headless;
>         }
> -       __SetPageMovable(page, pool->inode->i_mapping);
> +       if (!WARN_ON(!trylock_page(page))) {
> +               __SetPageMovable(page, pool->inode->i_mapping);
> +               unlock_page(page);
> +       }
>         z3fold_page_lock(zhdr);
>
>  found:
> @@ -1325,6 +1328,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
>
>         VM_BUG_ON_PAGE(!PageMovable(page), page);
>         VM_BUG_ON_PAGE(!PageIsolated(page), page);
> +       VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
>
>         zhdr = page_address(page);
>         pool = zhdr_to_pool(zhdr);
> --
> 2.22.0.410.gd8fdbe21b5-goog
>

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[David Rientjes]

On Mon, 1 Jul 2019, Henry Burns wrote:

> __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> lock the page. Following zsmalloc.c's example we call trylock_page() and
> unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> passed in locked, as documentation.
> 
> Signed-off-by: Henry Burns <henryburns@google.com>
> Suggested-by: Vitaly Wool <vitalywool@gmail.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Andrew Morton]

On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote:

> Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com>

Are these the same person?

> Subject: Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
> Date: Mon, 1 Jul 2019 18:16:30 -0700
> 
> On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote:
> >
> > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote:
> > >
> > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> > > lock the page. Following zsmalloc.c's example we call trylock_page() and
> > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> > > passed in locked, as documentation.

The changelog still doesn't mention that this bug triggers a
VM_BUG_ON_PAGE().  It should do so.  I did this:

: __SetPageMovable() expects its page to be locked, but z3fold.c doesn't
: lock the page.  This triggers the VM_BUG_ON_PAGE(!PageLocked(page), page)
: in __SetPageMovable().
:
: Following zsmalloc.c's example we call trylock_page() and unlock_page(). 
: Also make z3fold_page_migrate() assert that newpage is passed in locked,
: as per the documentation.

I'll add a cc:stable to this fix.

> > > Signed-off-by: Henry Burns <henryburns@google.com>
> > > Suggested-by: Vitaly Wool <vitalywool@gmail.com>
> > > ---
> > >  Changelog since v1:
> > >  - Added an if statement around WARN_ON(trylock_page(page)) to avoid
> > >    unlocking a page locked by a someone else.
> > >
> > >  mm/z3fold.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > > index e174d1549734..6341435b9610 100644
> > > --- a/mm/z3fold.c
> > > +++ b/mm/z3fold.c
> > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> > >                 set_bit(PAGE_HEADLESS, &page->private);
> > >                 goto headless;
> > >         }
> > > -       __SetPageMovable(page, pool->inode->i_mapping);
> > > +       if (!WARN_ON(!trylock_page(page))) {
> > > +               __SetPageMovable(page, pool->inode->i_mapping);
> > > +               unlock_page(page);
> > > +       }
> >
> > Can you please comment why lock_page() is not used here?

Shakeel asked "please comment" (ie, please add a code comment), not
"please comment on".  Subtle ;)

> Since z3fold_alloc can be called in atomic or non atomic context,
> calling lock_page() could trigger a number of
> warnings about might_sleep() being called in atomic context. WARN_ON
> should avoid the problem described
> above as well, and in any weird condition where someone else has the
> page lock, we can avoid calling
> __SetPageMovable().

I think this will suffice:

--- a/mm/z3fold.c~mm-z3foldc-lock-z3fold-page-before-__setpagemovable-fix
+++ a/mm/z3fold.c
@@ -919,6 +919,9 @@ retry:
 		set_bit(PAGE_HEADLESS, &page->private);
 		goto headless;
 	}
+	/*
+	 * z3fold_alloc() can be called from atomic contexts, hence the trylock
+	 */
 	if (!WARN_ON(!trylock_page(page))) {
 		__SetPageMovable(page, pool->inode->i_mapping);
 		unlock_page(page);

However this code would be more effective if z3fold_alloc() were to be
told when it is running in non-atomic context so it can perform a
sleeping lock_page() in that case.  That's an improvement to consider
for later, please.

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Henry Burns]

On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote:
>
> > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com>
>
> Are these the same person?
I Think it's the same person, but i wasn't sure which email to include
because one was
in the list of maintainers and I had contacted the other earlier.
>
> > Subject: Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()
> > Date: Mon, 1 Jul 2019 18:16:30 -0700
> >
> > On Mon, Jul 1, 2019 at 6:00 PM Shakeel Butt <shakeelb@google.com> wrote:
> > >
> > > On Mon, Jul 1, 2019 at 5:51 PM Henry Burns <henryburns@google.com> wrote:
> > > >
> > > > __SetPageMovable() expects it's page to be locked, but z3fold.c doesn't
> > > > lock the page. Following zsmalloc.c's example we call trylock_page() and
> > > > unlock_page(). Also makes z3fold_page_migrate() assert that newpage is
> > > > passed in locked, as documentation.
>
> The changelog still doesn't mention that this bug triggers a
> VM_BUG_ON_PAGE().  It should do so.  I did this:
>
> : __SetPageMovable() expects its page to be locked, but z3fold.c doesn't
> : lock the page.  This triggers the VM_BUG_ON_PAGE(!PageLocked(page), page)
> : in __SetPageMovable().
> :
> : Following zsmalloc.c's example we call trylock_page() and unlock_page().
> : Also make z3fold_page_migrate() assert that newpage is passed in locked,
> : as per the documentation.
>
> I'll add a cc:stable to this fix.
>
> > > > Signed-off-by: Henry Burns <henryburns@google.com>
> > > > Suggested-by: Vitaly Wool <vitalywool@gmail.com>
> > > > ---
> > > >  Changelog since v1:
> > > >  - Added an if statement around WARN_ON(trylock_page(page)) to avoid
> > > >    unlocking a page locked by a someone else.
> > > >
> > > >  mm/z3fold.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > > > index e174d1549734..6341435b9610 100644
> > > > --- a/mm/z3fold.c
> > > > +++ b/mm/z3fold.c
> > > > @@ -918,7 +918,10 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> > > >                 set_bit(PAGE_HEADLESS, &page->private);
> > > >                 goto headless;
> > > >         }
> > > > -       __SetPageMovable(page, pool->inode->i_mapping);
> > > > +       if (!WARN_ON(!trylock_page(page))) {
> > > > +               __SetPageMovable(page, pool->inode->i_mapping);
> > > > +               unlock_page(page);
> > > > +       }
> > >
> > > Can you please comment why lock_page() is not used here?
>
> Shakeel asked "please comment" (ie, please add a code comment), not
> "please comment on".  Subtle ;)
>
> > Since z3fold_alloc can be called in atomic or non atomic context,
> > calling lock_page() could trigger a number of
> > warnings about might_sleep() being called in atomic context. WARN_ON
> > should avoid the problem described
> > above as well, and in any weird condition where someone else has the
> > page lock, we can avoid calling
> > __SetPageMovable().
>
> I think this will suffice:
>
> --- a/mm/z3fold.c~mm-z3foldc-lock-z3fold-page-before-__setpagemovable-fix
> +++ a/mm/z3fold.c
> @@ -919,6 +919,9 @@ retry:
>                 set_bit(PAGE_HEADLESS, &page->private);
>                 goto headless;
>         }
> +       /*
> +        * z3fold_alloc() can be called from atomic contexts, hence the trylock
> +        */
>         if (!WARN_ON(!trylock_page(page))) {
>                 __SetPageMovable(page, pool->inode->i_mapping);
>                 unlock_page(page);
>
> However this code would be more effective if z3fold_alloc() were to be
> told when it is running in non-atomic context so it can perform a
> sleeping lock_page() in that case.  That's an improvement to consider
> for later, please.
>

z3fold_alloc() can tell when its called in atomic context, new patch incoming!
I'm thinking something like this:

> > > > +       if (can_sleep) {
> > > > +               lock_page(page);
> > > > +               __SetPageMovable(page, pool->inode->i_mapping);
> > > > +               unlock_page(page);
> > > > +       } else {
> > > > +               if (!WARN_ON(!trylock_page(page))) {
> > > > +                       __SetPageMovable(page, pool->inode->i_mapping);
> > > > +                       unlock_page(page);
> > > > +               } else {
> > > > +                       pr_err("Newly allocated z3fold page is locked\n");
> > > > +                       WARN_ON(1);
> > > > +               }
> > > > +       }

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Andrew Morton]

On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> wrote:

> > > > > +       if (can_sleep) {
> > > > > +               lock_page(page);
> > > > > +               __SetPageMovable(page, pool->inode->i_mapping);
> > > > > +               unlock_page(page);
> > > > > +       } else {
> > > > > +               if (!WARN_ON(!trylock_page(page))) {
> > > > > +                       __SetPageMovable(page, pool->inode->i_mapping);
> > > > > +                       unlock_page(page);
> > > > > +               } else {
> > > > > +                       pr_err("Newly allocated z3fold page is locked\n");
> > > > > +                       WARN_ON(1);

The WARN_ON will have already warned in this case.

But the whole idea of warning in this case may be undesirable.  We KNOW
that the warning will sometimes trigger (yes?).  So what's the point in
scaring users?

Also, pr_err(...)+WARN_ON(1) can basically be replaced with WARN(1, ...)?

> > > > > +               }
> > > > > +       }

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Vitaly Wool]

On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> wrote:
>
> > > > > > +       if (can_sleep) {
> > > > > > +               lock_page(page);
> > > > > > +               __SetPageMovable(page, pool->inode->i_mapping);
> > > > > > +               unlock_page(page);
> > > > > > +       } else {
> > > > > > +               if (!WARN_ON(!trylock_page(page))) {
> > > > > > +                       __SetPageMovable(page, pool->inode->i_mapping);
> > > > > > +                       unlock_page(page);
> > > > > > +               } else {
> > > > > > +                       pr_err("Newly allocated z3fold page is locked\n");
> > > > > > +                       WARN_ON(1);
>
> The WARN_ON will have already warned in this case.
>
> But the whole idea of warning in this case may be undesirable.  We KNOW
> that the warning will sometimes trigger (yes?).  So what's the point in
> scaring users?

Well, normally a newly allocated page that we own should not be locked
by someone else so this is worth a warning IMO. With that said, the
else branch here appears to be redundant.

~Vitaly

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Vitaly Wool]

On Wed, Jul 3, 2019 at 12:18 AM Henry Burns <henryburns@google.com> wrote:
>
> On Tue, Jul 2, 2019 at 2:19 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Mon, 1 Jul 2019 18:16:30 -0700 Henry Burns <henryburns@google.com> wrote:
> >
> > > Cc: Vitaly Wool <vitalywool@gmail.com>, Vitaly Vul <vitaly.vul@sony.com>
> >
> > Are these the same person?
> I Think it's the same person, but i wasn't sure which email to include
> because one was
> in the list of maintainers and I had contacted the other earlier.

This is the same person, it's the transliteration done differently
that caused this :)

~Vitaly

Re: [PATCH v2] mm/z3fold.c: Lock z3fold page before __SetPageMovable()

[Henry Burns]

On Tue, Jul 2, 2019 at 10:54 PM Vitaly Wool <vitalywool@gmail.com> wrote:
>
> On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <henryburns@google.com> wrote:
> >
> > > > > > > +       if (can_sleep) {
> > > > > > > +               lock_page(page);
> > > > > > > +               __SetPageMovable(page, pool->inode->i_mapping);
> > > > > > > +               unlock_page(page);
> > > > > > > +       } else {
> > > > > > > +               if (!WARN_ON(!trylock_page(page))) {
> > > > > > > +                       __SetPageMovable(page, pool->inode->i_mapping);
> > > > > > > +                       unlock_page(page);
> > > > > > > +               } else {
> > > > > > > +                       pr_err("Newly allocated z3fold page is locked\n");
> > > > > > > +                       WARN_ON(1);
> >
> > The WARN_ON will have already warned in this case.
> >
> > But the whole idea of warning in this case may be undesirable.  We KNOW
> > that the warning will sometimes trigger (yes?).  So what's the point in
> > scaring users?
>
> Well, normally a newly allocated page that we own should not be locked
> by someone else so this is worth a warning IMO. With that said, the
> else branch here appears to be redundant.
The else branch has been removed, and I think it's possible (albeit unlikely)
that the trylock could fail due to either compaction or kstaled
(In which case the page just won't be movable).

Also Vitaly, do you have a preference between the two emails? I'm not sure which
one to include.