From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 647A2C4338F for ; Wed, 28 Jul 2021 13:04:18 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 13D3960F9E for ; Wed, 28 Jul 2021 13:04:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 13D3960F9E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 83CAC6B0036; Wed, 28 Jul 2021 09:04:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7ED396B006C; Wed, 28 Jul 2021 09:04:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6DB8A8D0002; Wed, 28 Jul 2021 09:04:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0060.hostedemail.com [216.40.44.60]) by kanga.kvack.org (Postfix) with SMTP id 50EF96B0036 for ; Wed, 28 Jul 2021 09:04:17 -0400 (EDT) Received: from smtpin32.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 25D588249980 for ; Wed, 28 Jul 2021 13:04:03 +0000 (UTC) X-FDA: 78412014366.32.CD8045E Received: from mail-io1-f41.google.com (mail-io1-f41.google.com [209.85.166.41]) by imf17.hostedemail.com (Postfix) with ESMTP id B561AF003230 for ; Wed, 28 Jul 2021 13:04:02 +0000 (UTC) Received: by mail-io1-f41.google.com with SMTP id j21so2893627ioo.6 for ; Wed, 28 Jul 2021 06:04:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=CDwDfx19j8IGibNTUHdU1OYEGVTNVsxhXq9vsnGephU=; b=T9Kn0NABhI9/vFnTqlnUzxPVoF4Eg+BM4JaLDCYMJ4enYtWQ+1AQwKYiVIAiAkduJy 9xc8EdyrObSuET7Ru9C48g2wGdErdmKWoGVz6ZsP9mYxH10aH6KuAPuM2g3UljUT8iqx VSP88IUJgbfm7dlS2xzHmQDd16b6Xm1EaB2ZHdWM1WPbX6IaTBganzyEd5mIBa7nNxMk LR9kEcP1H+tYeq2Dp1fcO0VvmpsrzAYS4KxTDVcgEigyrDR4j8dCdiftqdG4N+IJRfA7 kL1ESIbuQ1YlFx19YsgDLs4QcZL4PI+cE0dPbOPvfEbeN+7S/QXqSL1UhgKqrosr73X/ xjXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=CDwDfx19j8IGibNTUHdU1OYEGVTNVsxhXq9vsnGephU=; b=iPsqVLMznrKoxqX4k4YVjXoggCtcLszg7zpOKU/uJpmrOkLGhVa6hNqKFNzyog1w0G odzMx4sznrzmN/es7bmRXxa3Ib9c92ne1ZfJ2MFagGjB3o3QPkX2L5ftrnK8y1J8PNRA 8aXJQj4dP/7Bzwl7b8OFgJymnt69Xyqs2w1sZIbxRy6pK1l383Ka+Z7uwxykVHsj14OL 79ZhqrLfit7CtwAPjPJEKXnLd3ijjG0gKssPaJUiFwjCfWzV8PqJfqnF3XHKXZfCiJ/M XSTpTLYflvdI8SzNwiXDqZHEqV5kNktDlNGpn1nWapXOnPqUiRHqDIjpA+cyJ6BO+bi4 l0Jw== X-Gm-Message-State: AOAM530wEWlNwB1dvpnpsUbGbC/R/lEE8dfQ/WMe8td+Vs88R58kxn51 3Ro6NsuOdRL/bHBQ9vPAkNf4CNH1hS2WunQ8dYE= X-Google-Smtp-Source: ABdhPJy/duLqQ6muZtfL+logI9fB+6Ffci+lLijgEoR2IX9pEEZu1ZmBjYloZz5eSitgIJKup9OkrIqjh3ehtIdYpGw= X-Received: by 2002:a6b:fe06:: with SMTP id x6mr23706657ioh.38.1627477442031; Wed, 28 Jul 2021 06:04:02 -0700 (PDT) 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 Date: Wed, 28 Jul 2021 21:03:50 +0800 Message-ID: Subject: Re: [PATCH] mm,shmem: Fix a typo in shmem_swapin_page() To: Hugh Dickins Cc: Huang Ying , Andrew Morton , David Hildenbrand , Yang Shi , Linux-MM , LKML , Miaohe Lin , Johannes Weiner , Michal Hocko , Joonsoo Kim , Matthew Wilcox , Minchan Kim Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: B561AF003230 Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=T9Kn0NAB; spf=pass (imf17.hostedemail.com: domain of huangyingcaritas@gmail.com designates 209.85.166.41 as permitted sender) smtp.mailfrom=huangyingcaritas@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Stat-Signature: ad5xfi8snj41i7wj7zbb3u3sr9epz7c4 X-HE-Tag: 1627477442-237642 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi, Hugh, Thanks for your comments. On Sat, Jul 24, 2021 at 4:23 AM Hugh Dickins 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" > > Cc: Miaohe Lin > > Cc: Hugh Dickins > > Cc: Johannes Weiner > > Cc: Michal Hocko > > Cc: Joonsoo Kim > > Cc: Matthew Wilcox > > Cc: Minchan Kim > > --- > > 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