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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87B7EC021BE for ; Thu, 27 Feb 2025 07:29:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BD141280002; Thu, 27 Feb 2025 02:29:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B89B4280001; Thu, 27 Feb 2025 02:29:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A4764280002; Thu, 27 Feb 2025 02:29:54 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 87D03280001 for ; Thu, 27 Feb 2025 02:29:54 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 271F5C0832 for ; Thu, 27 Feb 2025 07:29:54 +0000 (UTC) X-FDA: 83164900308.14.E971410 Received: from out-174.mta0.migadu.com (out-174.mta0.migadu.com [91.218.175.174]) by imf27.hostedemail.com (Postfix) with ESMTP id 46DC340007 for ; Thu, 27 Feb 2025 07:29:52 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=IDf+qrhO; spf=pass (imf27.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.174 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740641392; a=rsa-sha256; cv=none; b=HNI6UaHbGLgif5aPXfCMNyMO2JCgcvzUqX8xIn/z4yjYOvZ01M+KcRWtF73cYGMZJa9neJ CG6gMmg7iG1l5uqhxoDjjGtioa1J4vMA36E8yc3QRHrDz6dd95xRgvDRvmyznmrYLNKtB4 qW1+W3lRMtrCN53Tza7hZjJExRcb3bk= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=IDf+qrhO; spf=pass (imf27.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.174 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740641392; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=SXNwsf4ANZAQ/D0DGM+1iMj7YQ8P/MXBqdHWaKj4AyM=; b=MjhXzzClrdVAyaV4UU4N8CHU70871a3YmvQLaMDv0IJvFz1Ki4FL6NYk7fVcd9wD8uGSDv S6s6Qh0hImIuNfXlYZvRfihxbARSTNLJVGxfvEEA44oN0v9WwgNy6Hx/jxMgKGnPrNOFu7 F5ER5f9Qb3aRGdbiK2QNj63tT/hiILE= Date: Thu, 27 Feb 2025 07:29:45 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740641390; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=SXNwsf4ANZAQ/D0DGM+1iMj7YQ8P/MXBqdHWaKj4AyM=; b=IDf+qrhO8kaYB9sCbvYzwC6wZ4hYRbhuBpC7c2ACdnRcF3gcXlrNBdHslhkeuDG/qNG7Pw sbw8afEhsemg/mCWCimoxTvh448CR3ewB5cA0JEJQXI38VV2On/+LWzpaYCcMvb4m4nnzn bv3IwBK07M0Dir7kHQB/aruwWbk7peo= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: Johannes Weiner Cc: Nhat Pham , akpm@linux-foundation.org, chengming.zhou@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] zswap: do not crash the kernel on decompression failure Message-ID: References: <20250227001445.1099203-1-nphamcs@gmail.com> <20250227043141.GB110982@cmpxchg.org> <20250227061616.GD110982@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Stat-Signature: h4eg6xwf18dnemouchcwnaweone6bn1w X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 46DC340007 X-Rspam-User: X-HE-Tag: 1740641392-575421 X-HE-Meta: U2FsdGVkX1+LKh1LcyPTSHDMxhaW2EWsgjL7CbkvhWo+mxrAmhPkBrvwlqYA3Hsm6w1ejBl5eKsqfwbRpFGIYv/QR3x4VTGAs6hWsAHiev7ggXiju04i3pnh1pTM74r3wh8lzleesdMH9HeJqJrp3y2yGs3sAA97ZOklqk5NY1HBwT2cIrLEeCnNVZR2oDCET+GbLDGea4/SHE8K2bw5DALQ1jrKxPdNsAYKDXmBgbfzbHr+x6IY2VXK2/gVAJ/o0NnUSioJqDjBGARn5aJs6lFDSqoA8/oFKkapoSubXOdVp6yWP3d3IpLoJs9wuVH7a8zahPpd5Swh7kdhIQqt+0LhpBMGWOBBYIVeK7JWepOLlnUjD3K9uSfm2dDbIl/85OK3t82GVfCA7AcxsfplGhs66EC/SeQOoCVwwT62MVmOr809reIP3/WEp6m2Zz7hunXefNFa/9wxXurCGRscbnsLp9o+gWYhNB3pPEumQT08tZA6ECgJfW/dRr1Yxpz0cn/vxmHxDt8oL0b8KJg599n7UG93lFKSCopN0xFBJlz5zgFwLfKbq3vUmzRdNSuXcAGmwXahR8aViv6XVuNxrq4KKlQmzAFwaVgADZanhftxmIGPTtK2oeBKWetmWHFYlNM380ohhKZKM+UiieGxNLda+k0sws95EkzgHZbltPnIOC3tY+PclELPqL8Vs4VDAfAj4iMthbzivyTEb1KFaNiF59+5IJvt0TowBJxkfS5ytbC6uTJ0os1auctR8DT4gwffISebKVct7O9IaKzEa/B7tCQqyVEFb59+M+QUajPc40WhNQcwxBMTBIeqojdgWDZrNkMwqaLQh5RLrkNaK+OSgpVEkmOoNPJ27MNHT56QNpuxG8oOLlOuHSUSKJH3z/2PZLthNpH5PBdIZT3+jtnmjKS0hpXgC1FtrPZZc0XLAo1B6OVsZBh7ZmO4+YgfrPDgLQIHY1riscLf4BB ru8XCfPU P+NIr10PvKK4iIEfdID7Ue+GEUSZc1SFTRZvRs5dQse6UkKgrWui0k3FF9zfvCwLi7NxdSKbYN8f/nT/bo4km+fcaJ0YAazc9sl8Q1WJI88zPe/RQmXlSBNE3nMlxDguQkgrYqftrxRpzn1LcGDCKXuJEtwCN7/pDF3Jn8le0uMsoNxCiBECP0FZzj2AN0kaXChzek+CaJH+cZY73huH8HsjGOw== 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: List-Subscribe: List-Unsubscribe: On Thu, Feb 27, 2025 at 07:11:59AM +0000, Yosry Ahmed wrote: > On Thu, Feb 27, 2025 at 01:16:16AM -0500, Johannes Weiner wrote: > > On Thu, Feb 27, 2025 at 05:44:29AM +0000, Yosry Ahmed wrote: > > > On Wed, Feb 26, 2025 at 11:31:41PM -0500, Johannes Weiner wrote: > > > > On Thu, Feb 27, 2025 at 01:19:31AM +0000, Yosry Ahmed wrote: > > > > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > > > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > > > > > return true; > > > > > > > > > > > > + entry = xa_load(tree, offset); > > > > > > + if (!entry) > > > > > > + return false; > > > > > > + > > > > > > > > > > A small comment here pointing out that we are deliberatly not setting > > > > > uptodate because of the failure may make things more obvious, or do you > > > > > think that's not needed? > > > > > > > > > > > + if (!zswap_decompress(entry, folio)) > > > > > > + return true; > > > > > > > > How about an actual -ev and have this in swap_read_folio(): > > > > > > Good idea, I was going to suggest an enum but this is simpler. > > > > > > > > > > > ret = zswap_load(folio); > > > > if (ret != -ENOENT) { > > > > folio_unlock(folio); > > > > goto finish; > > > > } > > > > > > > > read from swapfile... > > > > > > > > Then in zswap_load(), move uptodate further up like this (I had > > > > previously suggested this): > > > > > > > > if (!zswap_decompress(entry, folio)) > > > > return -EIO; > > > > > > > > folio_mark_uptodate(folio); > > > > > > > > and I think it would be clear, even without or just minimal comments. > > > > > > Another possibility is moving folio_mark_uptodate() back to > > > swap_read_folio(), which should make things even clearer imo as the > > > success/failure logic is all in one place: > > > > That works. bdev, swapfile and zeromap set the flag in that file. > > > > > ret = zswap_load(folio); > > > if (ret != -ENOENT) { > > > folio_unlock(folio); > > > /* Comment about not marking uptodate */ > > > if (!ret) > > > folio_mark_uptodate(); > > > goto finish; > > > } > > > > Personally, I like this one ^. The comment isn't needed IMO, as now > > zswap really isn't doing anything special compared to the others. > > > > > or we can make it crystal clear we have 3 distinct cases: > > > > > > ret = zswap_load(folio); > > > if (!ret) { > > > folio_unlock(folio); > > > folio_mark_uptodate(); > > > goto finish; > > > } else if (ret != -ENOENT) { > > > /* Comment about not marking uptodate */ > > > folio_unlock(folio); > > > goto finish; > > > } > > > > This seems unnecessarily repetetive. > > I know, but looking at the two, this one makes it clearer to me there > are 3 distinct cases, and the redundancy is not terrible. > > So I personally prefer the latter, but I am fine either way. I just realized that swap_read_folio_zeromap() does the same trick, so we should probably also move the folio_mark_uptodate() in there to swap_read_folio(). Maybe we can do something like this: /* Returns true if the folio was in the zeromap or zswap */ bool swap_read_folio_in_memory(struct folio *folio) { int ret; ret = swap_read_folio_zeromap(folio); if (ret == -ENOENT) ret = zswap_load(folio); if (ret == 0) { folio_mark_uptodate(folio); folio_unlock(folio); return true; } else if (ret != -ENOENT) { folio_unlock(folio); return true; } else { return false; } } void swap_read_folio(struct folio *folio, struct swap_iocb **plug) { ... if (swap_read_folio_in_memory(folio)) goto finish; ... } Admittedly, swap_read_folio_in_memory() is not a good name. Maybe swap_read_folio_zeromap_or_zswap() :)