Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231119194740.94101-1-ryncsn@gmail.com> <20231119194740.94101-20-ryncsn@gmail.com>
 <CAF8kJuMJMBjn2VGq8aUzv_t4+k0zPW8WvJaraYe-8GRbX0q1fg@mail.gmail.com>
In-Reply-To: <CAF8kJuMJMBjn2VGq8aUzv_t4+k0zPW8WvJaraYe-8GRbX0q1fg@mail.gmail.com>
From:   Kairui Song <ryncsn@gmail.com>
Date:   Mon, 20 Nov 2023 19:17:24 +0800
Message-ID: <CAMgjq7DCq1mXwKW8o2nyqb2VA0RV0LP1L9D6to48Qww8VRkAOw@mail.gmail.com>
Subject: Re: [PATCH 19/24] shmem, swap: refactor error check on OOM or race
To:     Chris Li <chrisl@kernel.org>
Cc:     linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        "Huang, Ying" <ying.huang@intel.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Matthew Wilcox <willy@infradead.org>,
        Michal Hocko <mhocko@suse.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Chris Li <chrisl@kernel.org> =E4=BA=8E2023=E5=B9=B411=E6=9C=8820=E6=97=A5=
=E5=91=A8=E4=B8=80 15:12=E5=86=99=E9=81=93=EF=BC=9A
>
> Hi Kairui,
>
> On Sun, Nov 19, 2023 at 11:49=E2=80=AFAM Kairui Song <ryncsn@gmail.com> w=
rote:
> >
> > From: Kairui Song <kasong@tencent.com>
> >
> > It should always check if a swap entry is already swaped in on error,
> > fix potential false error issue.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >  mm/shmem.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 81d129aa66d1..6154b5b68b8f 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1857,13 +1857,11 @@ static int shmem_swapin_folio(struct inode *ino=
de, pgoff_t index,
> >         page =3D swapin_page_non_fault(swap, gfp, mpol, ilx, fault_mm, =
&result);
> >         mpol_cond_put(mpol);
> >
> > -       if (PTR_ERR(page) =3D=3D -EBUSY) {
> > -               if (!shmem_confirm_swap(mapping, index, swap))
> > -                       return -EEXIST;
>
> Do you intentionally remove checking shmem_confirm_swap()?
> I am not sure I am following.

Hi, Chris, thanks for the review.

This was also called under failed: label so I think there is no need
to keep it here.

>
> > +       if (IS_ERR_OR_NULL(page)) {
> > +               if (!page)
> > +                       error =3D -ENOMEM;
> >                 else
> > -                       return -EINVAL;
> > -       } else if (!page) {
> > -               error =3D -ENOMEM;
> > +                       error =3D -EINVAL;
>
> The resulting code is a bit hard to read in diff. In plan source it is li=
ke:
>
>         if (IS_ERR_OR_NULL(page)) {
>                 if (!page)
>                         error =3D -ENOMEM;
>                 else
>                         error =3D -EINVAL;
>                 goto failed;
>         } else {
>                 folio =3D page_folio(page);
>                 if (fault_type && result !=3D SWAP_CACHE_HIT) {
>                         *fault_type |=3D VM_FAULT_MAJOR;
>                         count_vm_event(PGMAJFAULT);
>                         count_memcg_event_mm(fault_mm, PGMAJFAULT);
>                 }
>         }
>
> First of all, if the first always "goto failed", the second else is not n=
eeded.
> The whole else block can be flatten one indentation.

Yes, thanks for the suggestion.

>
>  if (IS_ERR_OR_NULL(page)) {
>                 if (!page)
>                         error =3D -ENOMEM;
>                 else
>                         error =3D -EINVAL;
>                 goto failed;
>   } else {
>
> Can be rewrite as following with less indentation:
>
> if (!page) {
>      error =3D -ENOMEM;
>      goto failed;
> }
> if (IS_ERR(page)) {
>      error =3D -EINVAL;
>      goto failed;
> }
> /* else block */
>
> Am I missing something and misreading your code?

Your code looks cleaner :)

This patch is trying to clean up the error path after the helper
refactor, and your suggestions are very helpful, thanks!

>
> Chris