Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <b6c881-c352-bb91-85a8-febeb09dfd71@google.com>
In-Reply-To: <b6c881-c352-bb91-85a8-febeb09dfd71@google.com>
From:   David Stevens <stevensd@chromium.org>
Date:   Mon, 24 Apr 2023 10:53:23 +0900
Message-ID: <CAD=HUj5NSXX8a5shf-GVYqkMFC-o26vuyo26hMPpPCnuSD683A@mail.gmail.com>
Subject: Re: [PATCH next] mm/khugepaged: fix conflicting mods to collapse_file()
To:     Hugh Dickins <hughd@google.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Ivan Orlov <ivan.orlov0322@gmail.com>,
        Jiaqi Yan <jiaqiyan@google.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Apr 23, 2023 at 1:47=E2=80=AFPM Hugh Dickins <hughd@google.com> wro=
te:
>
> Inserting Ivan Orlov's syzbot fix commit 2ce0bdfebc74
> ("mm: khugepaged: fix kernel BUG in hpage_collapse_scan_file()")
> ahead of Jiaqi Yan's and David Stevens's commits
> 12904d953364 ("mm/khugepaged: recover from poisoned file-backed memory")
> cae106dd67b9 ("mm/khugepaged: refactor collapse_file control flow")
> ac492b9c70ca ("mm/khugepaged: skip shmem with userfaultfd")
> (all of which restructure collapse_file()) did not work out well.
>
> xfstests generic/086 on huge tmpfs (with accelerated khugepaged) freezes
> (if not on the first attempt, then the 2nd or 3rd) in find_lock_entries()
> while doing drop_caches: the file's xarray seems to have been corrupted,
> with find_get_entry() returning nonsense which makes no progress.
>
> Bisection led to ac492b9c70ca; and diff against earlier working linux-nex=
t
> suggested that it's probably down to an errant xas_store(), which does no=
t
> belong with the later changes (and nor does the positioning of warnings).
> The later changes look as if they fix the syzbot issue independently.
>
> Remove most of what's left of 2ce0bdfebc74: just leave one WARN_ON_ONCE
> (xas_error) after the final xas_store() of the multi-index entry.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
>
>  mm/khugepaged.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
>
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1941,16 +1941,6 @@ static int collapse_file(struct mm_struct *mm, uns=
igned long addr,
>                                         result =3D SCAN_FAIL;
>                                         goto xa_locked;
>                                 }
> -                               xas_store(&xas, hpage);
> -                               if (xas_error(&xas)) {
> -                                       /* revert shmem_charge performed
> -                                        * in the previous condition
> -                                        */
> -                                       mapping->nrpages--;
> -                                       shmem_uncharge(mapping->host, 1);
> -                                       result =3D SCAN_STORE_FAILED;

With this being removed, SCAN_STORE_FAILED should also be removed from
the scan_result enum and trace event definitions.

-David

> -                                       goto xa_locked;
> -                               }
>                                 nr_none++;
>                                 continue;
>                         }
> @@ -2105,13 +2095,6 @@ static int collapse_file(struct mm_struct *mm, uns=
igned long addr,
>                  * Accumulate the pages that are being collapsed.
>                  */
>                 list_add_tail(&page->lru, &pagelist);
> -
> -               /*
> -                * We can't get an ENOMEM here (because the allocation ha=
ppened
> -                * before) but let's check for errors (XArray implementat=
ion
> -                * can be changed in the future)
> -                */
> -               WARN_ON_ONCE(xas_error(&xas));
>                 continue;
>  out_unlock:
>                 unlock_page(page);
> @@ -2134,11 +2117,6 @@ static int collapse_file(struct mm_struct *mm, uns=
igned long addr,
>                 }
>         }
>
> -       /* Here we can't get an ENOMEM (because entries were
> -        * previously allocated) But let's check for errors
> -        * (XArray implementation can be changed in the future)
> -        */
> -       WARN_ON_ONCE(xas_error(&xas));
>  xa_locked:
>         xas_unlock_irq(&xas);
>  xa_unlocked:
> @@ -2259,6 +2237,7 @@ static int collapse_file(struct mm_struct *mm, unsi=
gned long addr,
>         /* Join all the small entries into a single multi-index entry. */
>         xas_set_order(&xas, start, HPAGE_PMD_ORDER);
>         xas_store(&xas, hpage);
> +       WARN_ON_ONCE(xas_error(&xas));
>         xas_unlock_irq(&xas);
>
>         /*