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>
 <CAD=HUj5NSXX8a5shf-GVYqkMFC-o26vuyo26hMPpPCnuSD683A@mail.gmail.com> <cf24ea0-92da-cd46-a7cd-87f5449016e8@google.com>
In-Reply-To: <cf24ea0-92da-cd46-a7cd-87f5449016e8@google.com>
From:   David Stevens <stevensd@chromium.org>
Date:   Mon, 24 Apr 2023 11:36:48 +0900
Message-ID: <CAD=HUj5EzNc28MFs6m2-RpH0Ao1KNNOBUUSL1OjAj7T+cxqpUg@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 Mon, Apr 24, 2023 at 11:17=E2=80=AFAM Hugh Dickins <hughd@google.com> wr=
ote:
>
> On Mon, 24 Apr 2023, David Stevens wrote:
> > On Sun, Apr 23, 2023 at 1:47=E2=80=AFPM Hugh Dickins <hughd@google.com>=
 wrote:
> > >
> > > 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 memor=
y")
> > > 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) free=
zes
> > > (if not on the first attempt, then the 2nd or 3rd) in find_lock_entri=
es()
> > > while doing drop_caches: the file's xarray seems to have been corrupt=
ed,
> > > with find_get_entry() returning nonsense which makes no progress.
> > >
> > > Bisection led to ac492b9c70ca; and diff against earlier working linux=
-next
> > > suggested that it's probably down to an errant xas_store(), which doe=
s not
> > > belong with the later changes (and nor does the positioning of warnin=
gs).
> > > 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_ON=
CE
> > > (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,=
 unsigned long addr,
> > >                                         result =3D SCAN_FAIL;
> > >                                         goto xa_locked;
> > >                                 }
> > > -                               xas_store(&xas, hpage);
> > > -                               if (xas_error(&xas)) {
> > > -                                       /* revert shmem_charge perfor=
med
> > > -                                        * 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.
>
> Only if we also remove your use of SCAN_STORE_FAILED in ac492b9c70ca:
> what would you want that to say instead?
>
> I don't care myself for any of those "SCAN" result codes, nor whether the=
y
> are few or many: I'd rather have __LINE__ numbers for my own debugging.
>
> But if people want to remove SCAN_STORE_FAILED now, sure, send a patch;
> my intent was to unbreak the breakage.

Oh, I had forgotten that I used the code when I rebased v6 of my
patches. I have no strong opinions one way or the other. Sorry for the
noise.

-David