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 876B9C433F5 for ; Mon, 15 Nov 2021 19:08:24 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 37DD563702 for ; Mon, 15 Nov 2021 19:08:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 37DD563702 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 C6CE26B007B; Mon, 15 Nov 2021 14:08:23 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C1CCF6B007D; Mon, 15 Nov 2021 14:08:23 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B0B6E6B007E; Mon, 15 Nov 2021 14:08:23 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id A08026B007B for ; Mon, 15 Nov 2021 14:08:23 -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 5B319844F2 for ; Mon, 15 Nov 2021 19:08:23 +0000 (UTC) X-FDA: 78812100444.27.B4229F2 Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by imf08.hostedemail.com (Postfix) with ESMTP id 43A393000243 for ; Mon, 15 Nov 2021 19:08:06 +0000 (UTC) Received: by mail-lf1-f52.google.com with SMTP id b1so40369612lfs.13 for ; Mon, 15 Nov 2021 11:08:22 -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=iwLQHMmFmuP03jMUUSLt12XkSF61jo0RZ+1g/X4/n9M=; b=NBjTki6BTMMgzxBFuWfZdS2zYmMKk0DpcLFBLGVfks4AiXDnAfMLo23STs70WZRP5Y LuDqerTJfSMiYMX04ySgAZ4BMCttk8Vli/mr9cVJ5e/ZnRz6FiR5QfNBQMZuLqTx5WEA iwkb3jppnLBpR2IPue/rqUcJQGtFpEB+WRy0XNTtTGB3/dGOOUxRKMC6/Ki/TxKxlEgt BFQYEbe+ek87rWalVCm0ayMOvU/XJJsqXMzK4jUVixs+q0F/Qi+KdJt2T0d7nlREoErM wi2RUT3JAFhlvdnYtLsfRU0stkvBmeJ/OP0/68Rvc7QgEEmPFTnAlmyhe6w5HrhIHHVw Obxw== 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=iwLQHMmFmuP03jMUUSLt12XkSF61jo0RZ+1g/X4/n9M=; b=fKVQem9xvCd5ryoRr6KTpgIVF1VGCTWq3V0DEVYINJfOTCwsUSe/gK9o+QA1I/U/eF MOIdGGgg+hWmMcerANDDOwrWm+eNE+njvy7GO0WAAXx7fSBKBZ2n3czSMqHb6ajU5/Zv 3+CFa8wHRknux7UBY5cy3OrfuLZxD5u41v3mAPDdTSoTbxxvP6fz3FxvSXNpSip42XRe yNwJiWNBy/FawKCqRObYHCYm8ZGKWmdJwq8UkanNxp8qO1JRtdjPilWSjO6uAYa/3mhg zaaMUcdTGX9eTkh+WtAyc3bzVH2+DuMroLZ+CDb3ApBJz5vbKuUILmfJh7df6pjHdqHn PWzw== X-Gm-Message-State: AOAM533EbKITrtG3CFsyg96fhM44qYrClTaQEhQlNvUW4SaYrw/y9kJ9 uSDOFUqZ6Pfq4B0/+qhTHfFOJXPUQy9IcPfzKrPO+xakEvnJJQ== X-Google-Smtp-Source: ABdhPJwpVxDVTuDP13LzNiAc04YtdxIig0Ri6JJdLgLNowjyvRFTNYpN1R+lp+3cR29YrKZdh5ZJ826u4VyrXCGvvd0= X-Received: by 2002:a17:907:1dd5:: with SMTP id og21mr1569907ejc.233.1637002834857; Mon, 15 Nov 2021 11:00:34 -0800 (PST) MIME-Version: 1.0 References: <20211114053221.315753-1-shy828301@gmail.com> In-Reply-To: From: Yang Shi Date: Mon, 15 Nov 2021 11:00:22 -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: rspam03 X-Rspamd-Queue-Id: 43A393000243 X-Stat-Signature: nzxb5ao9zs6366zj3p643j8pq3emd7rx Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=NBjTki6B; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf08.hostedemail.com: domain of shy828301@gmail.com designates 209.85.167.52 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-HE-Tag: 1637003286-431330 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 8:54 AM Yang Shi wrote: > > 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; By rethinking the code, I do prefer the above. "if (*pagep && PageHWPoison(*pagep))" does give extra protection from NULL pointer dereference for the future, but on the opposite side it seems confusing to *not* have error handling for NULL page. I bet it may incur more confusion than the protection for future, anyway it can't be NULL if ret == 0 and !SGP_READ. shmem_read_mapping_page_gfp() doesn't check page pointer either. IIUC only SGP_READ case has NULL page pointer checked in shmem code, so I'd like to follow the convention in this patch. If we think it is not good, we could refactor the code (for example, guarantee page have valid page as long as ret value is not error) in a perarate patch. > > > > 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 > . > >