Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210723080000.93953-1-ying.huang@intel.com> <24187e5e-069-9f3f-cefe-39ac70783753@google.com>
In-Reply-To: <24187e5e-069-9f3f-cefe-39ac70783753@google.com>
From:   huang ying <huang.ying.caritas@gmail.com>
Date:   Wed, 28 Jul 2021 21:03:50 +0800
Message-ID: <CAC=cRTNby4GkSJ-pjs6utgHtrQYEdy3XZQ06Qsxgyf1MJSBjrw@mail.gmail.com>
Subject: Re: [PATCH] mm,shmem: Fix a typo in shmem_swapin_page()
To:     Hugh Dickins <hughd@google.com>
Cc:     Huang Ying <ying.huang@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Yang Shi <shy828301@gmail.com>, Linux-MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@suse.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Matthew Wilcox <willy@infradead.org>,
        Minchan Kim <minchan@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi, Hugh,

Thanks for your comments.

On Sat, Jul 24, 2021 at 4:23 AM Hugh Dickins <hughd@google.com> wrote:
>
> On Fri, 23 Jul 2021, Huang Ying wrote:
>
> > "-" is missing before "EINVAL".
> >
> > Fixes: 2efa33fc7f6e ("mm/shmem: fix shmem_swapin() race with swapoff")
> > Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> > Cc: Miaohe Lin <linmiaohe@huawei.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/shmem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 9af4b2173fe9..e201a3ba12fa 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1708,7 +1708,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
> >       /* Prevent swapoff from happening to us. */
> >       si = get_swap_device(swap);
> >       if (!si) {
> > -             error = EINVAL;
> > +             error = -EINVAL;
> >               goto failed;
> >       }
> >       /* Look it up and read it in.. */
> > --
> > 2.30.2
>
> Thanks for catching that; and as David says, it's worse than a typo.
>
> But this is not the right fix:
> 2efa33fc7f6e ("mm/shmem: fix shmem_swapin() race with swapoff")
> needs to be reverted.
>
> It's been on my pile to look at for weeks: now I look at it and see
> it's just a bad patch.  Over-enthusiastic stablehands already rushed
> it out, I was wary, and reverts are already in -rc for 5.13 and 5.10,
> phew, but 5.12.19 EOL is stuck with it unfortunately, oh well.
>
> I was wary because, if the (never observed) race to be fixed is in
> swap_cluster_readahead(), why was shmem_swapin_page() being patched?

When we get a swap entry from the page table or shmem xarray, and no
necessary lock is held to prevent the swap device to be swapoff (e.g.
page table lock, page lock, etc.), it's possible that the swap device
has been swapoff when we operate on the swap entry (e.g. swapin).  So
we need to find a way to prevent the swap device to be swapoff,
get_swap_device() based on percpu_ref is used for that.  To avoid to
call get_swap_device() here and there (e.g. now it is called in many
different places), I think it's better to call get_swap_device() when
we just get a swap entry without holding the necessary lock, that is,
in do_swap_page() and shmem_swapin_page(), etc.  So that we can delete
the get_swap_device() call in lookup_swap_cache(),
__read_swap_cache_async(), etc.  This will make it easier to
understand when to use get_swap_device() and clean up the code.  Do
you agree?

> Not explained in its commit message, probably a misunderstanding of
> how mm/shmem.c already manages races (and prefers not to be involved
> in swap_info_struct stuff).

Yes.  The commit message isn't clean enough about why we do that.

> But why do I now say it's bad?  Because even if you correct the EINVAL
> to -EINVAL, that's an unexpected error: -EEXIST is common, -ENOMEM is
> not surprising, -ENOSPC can need consideration, but -EIO and anything
> else just end up as SIGBUS when faulting (or as error from syscall).

Yes.  -EINVAL isn't a good choice.  If it's the swapoff race, then
retrying can fix the race, so -EAGAIN may be a choice.  But if the
swap entry is really invalid (almost impossible in theory), we may
need something else, for example, WARN_ON_ONCE() and SIGBUS?  This
reminds me that we may need to distinguish the two possibilities in
get_swap_device()?

Best Regards,
Huang, Ying