2019-07-03 00:53:54

by Andrew Morton

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

On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <[email protected]> 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, ...)?

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

2019-07-03 05:56:54

by Vitaly Wool

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

On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <[email protected]> wrote:
>
> On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <[email protected]> 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

2019-07-03 17:31:54

by Henry Burns

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

On Tue, Jul 2, 2019 at 10:54 PM Vitaly Wool <[email protected]> wrote:
>
> On Wed, Jul 3, 2019 at 12:24 AM Andrew Morton <[email protected]> wrote:
> >
> > On Tue, 2 Jul 2019 15:17:47 -0700 Henry Burns <[email protected]> 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.