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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD695C433EF for ; Sun, 14 Nov 2021 16:54:52 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5CC986109E for ; Sun, 14 Nov 2021 16:54:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 5CC986109E 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 6FE9D6B0074; Sun, 14 Nov 2021 11:54:51 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 685DA6B007D; Sun, 14 Nov 2021 11:54:51 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 526796B007E; Sun, 14 Nov 2021 11:54:51 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0195.hostedemail.com [216.40.44.195]) by kanga.kvack.org (Postfix) with ESMTP id 3E80B6B0074 for ; Sun, 14 Nov 2021 11:54:51 -0500 (EST) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id D967081209 for ; Sun, 14 Nov 2021 16:54:50 +0000 (UTC) X-FDA: 78808135098.27.F9B2231 Received: from mail-ed1-f41.google.com (mail-ed1-f41.google.com [209.85.208.41]) by imf08.hostedemail.com (Postfix) with ESMTP id 5819830000A8 for ; Sun, 14 Nov 2021 16:54:34 +0000 (UTC) Received: by mail-ed1-f41.google.com with SMTP id b15so60602172edd.7 for ; Sun, 14 Nov 2021 08:54:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=e4mTkMaJLBp4mSDJvC547VxkTQcqH8W2cpJ2J7gpbKs=; b=ZM48247MOH5DIfBDrKDqGsm6HKJAxRGMnc8RsyJf8mTgsiUmX0T422ZwyXnlddCOzy nbNuZhs9mxSNLUr5HG8kiyx/sXiMDUk5MuiTITOfDI96ZzrC+4/XcNBK924GzZi3FwB2 0a+0SPcQChaDN+TQxM3YZCZ38+Zyjil1ESoucM/8eHYr0vPVUX5szL8Ynryx3BH6aqbO KNB7B4F8zkPfyCFMi3T1IXIn7YeF9Z3QCxdn+X84K+EjhpbNnVAwr7xbYvldA7GpxRzW LJ1vcYcyVMEcsUK/otAwnRYDrvO9b2Y6iGNgqDiy1DxKCZ//Q3nrYcwo/deO+KaSsUw/ hwvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=e4mTkMaJLBp4mSDJvC547VxkTQcqH8W2cpJ2J7gpbKs=; b=xS/zOLtulJRhiIJ8IOzeEfrRjqbKZhjoR3gfDtnhyTRKFa66tgDxyNfUnWdT8ATjsu 9fYvfSmADMz3DLZGqKVnrYyv6aNppmVWYdwcIg6efmXyDr0f3UY9QYa/3iRVPkmF4wTQ JnB49p8SwWTBOhgFETKgrEhC1jc/jONJAWa/itOhwX8LneC0UTnJJcCdJb4iXMLBRTDs SqeTZc+zc/22iKe8XG9b4ln66xnhzVjq2tuyAjQTM46n8dvGFpftRDmcTWPR/uxQ6VQq ArGlh+PRiDAE+f7UdmiQvbfwwiQOPvvugkH7jS7bu1um3WOEuVxtAkiSFKvwxkfwZmb+ jKAw== X-Gm-Message-State: AOAM530srei5HSpVpbtS6pIzQ4KcdB707MCTFT2S/SmTY2ScPboO/sfH u44+N/arRbHWMdf4DfGHpWleUPtLfIWs3bY9Vq8= X-Google-Smtp-Source: ABdhPJz8Ve8GJIT3h/LX8SDO4o5mA9nsOg4+4AtZAoqIPF7GABCFN4uv5Yv/mZMwIyzME0u1NBgOIhHnuHGI4qRnEVo= X-Received: by 2002:a17:906:d20c:: with SMTP id w12mr40179562ejz.521.1636908888207; Sun, 14 Nov 2021 08:54:48 -0800 (PST) MIME-Version: 1.0 References: <20211114053221.315753-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Sun, 14 Nov 2021 08:54:36 -0800 Message-ID: Subject: Re: [UPDATE PATCH] mm: shmem: don't truncate page if memory failure happens To: Matthew Wilcox Cc: Linus Torvalds , Arnd Bergmann , Hugh Dickins , "Kirill A. Shutemov" , =?UTF-8?B?SE9SSUdVQ0hJIE5BT1lBKOWggOWPoyDnm7TkuZ8p?= , Oscar Salvador , Peter Xu , Ajay Garg , Muchun Song , Andrew Morton , Linux MM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 5819830000A8 X-Stat-Signature: 4bkwoj5o3pab1sf5si6wiur3hwekar46 Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ZM48247M; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf08.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.41 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-HE-Tag: 1636908874-875922 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: On Sun, Nov 14, 2021 at 7:28 AM Matthew Wilcox wrote: > > On Sat, Nov 13, 2021 at 09:32:21PM -0800, Yang Shi wrote: > > @@ -2466,7 +2467,18 @@ shmem_write_begin(struct file *file, struct address_space *mapping, > > return -EPERM; > > } > > > > - return shmem_getpage(inode, index, pagep, SGP_WRITE); > > + ret = shmem_getpage(inode, index, pagep, SGP_WRITE); > > + > > + if (ret) > > + return ret; > > + > > + if (*pagep && PageHWPoison(*pagep)) { > > + unlock_page(*pagep); > > + put_page(*pagep); > > + ret = -EIO; > > You definitely need to add: > > *pagep = NULL; Thanks, will do it. > > I'm not entirely convinced that you need the conditional on '*pagep'. > If we returned 0, there had better be a page at pagep! For SGP_WRITE, yes, it has a page at pagep if ret is 0, but shmem_getpage() could return 0 with NULL page at pagep for SGP_READ. In the other thread that Linus elaborated why this commit was reverted and needed some rework, the discussion about not relying on implementation detail for error handling taught me it may be not a robust implementation to assume it is never NULL. We might refactor shmem_getpage() code in the future to make sure when it returns success there must be a valid pagep so that we just need to care about the return value for error handling. > > I also think this would be clearer if written as: > > if (PageHWPoison(*pagep)) { > unlock_page(*pagep); > put_page(*pagep); > *pagep = NULL; > return -EIO; > } > > return 0; > > instead of re-using ret. Sometimes that can make the code flow clearer, > but here, I don't think it does. Sure. > > > @@ -4168,9 +4201,12 @@ struct page *shmem_read_mapping_page_gfp(struct address_space *mapping, > > error = shmem_getpage_gfp(inode, index, &page, SGP_CACHE, > > gfp, NULL, NULL, NULL); > > if (error) > > - page = ERR_PTR(error); > > - else > > - unlock_page(page); > > + return ERR_PTR(error); > > + > > + unlock_page(page); > > + if (PageHWPoison(page)) > > + return ERR_PTR(-EIO); > > Do we need to put_page() the page in this error case? Aha, yes. Sorry for missing this. I was fooled by shmem_pin_map() in i915 driver which does put page, but I realized it just puts the valid pages pinned *before* meeting error page by second look . >