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 67783C021BE for ; Thu, 27 Feb 2025 07:12:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ED06B280003; Thu, 27 Feb 2025 02:12:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E5A666B0088; Thu, 27 Feb 2025 02:12:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CF8F5280003; Thu, 27 Feb 2025 02:12:09 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id ADB746B0085 for ; Thu, 27 Feb 2025 02:12:09 -0500 (EST) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 6AAD81A083B for ; Thu, 27 Feb 2025 07:12:09 +0000 (UTC) X-FDA: 83164855578.02.38FB758 Received: from out-171.mta1.migadu.com (out-171.mta1.migadu.com [95.215.58.171]) by imf29.hostedemail.com (Postfix) with ESMTP id 9992E12000F for ; Thu, 27 Feb 2025 07:12:07 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=oYro76F1; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf29.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.171 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740640327; a=rsa-sha256; cv=none; b=6AAFcSZlDLoIoeXIaeiuEWy1lb+diUvlLygMbSoNYjuWjXbTgHoYc6pHOcQoQn7naQ79D8 C2xfiHcOOQ8kPIfqotNs4knrak6ODngQ4VLq4Hi70Bke35kgQl4psldC1YZxj6N01jmPhb VLxIzz8fBncEYDKGHX4nn+r5App+4FU= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740640327; 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=QSxiF897g4G7kv0+39CQIytewmyhY9s4Wvke+nHP2s0=; b=OO2KoY9PaYPvZf7u5bkzDUEQACq+DJIAVcNbl3gQpIVfXy/AzR63ZouywHseWtyuP8nlgi TmWRf1itkpBsgcKj1mdwfDmyt4PDe6i0QW7QQPpxrxJVlD8+iRvRle6DzkbakpmIFGJ1SB uVObwQG8C4L5hi8L3HpmTnF+6asBQYE= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=oYro76F1; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf29.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.171 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev Date: Thu, 27 Feb 2025 07:11:59 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740640325; 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=QSxiF897g4G7kv0+39CQIytewmyhY9s4Wvke+nHP2s0=; b=oYro76F1d34k+ogEVC27K6e6Utg0y0RwdQKPOwaApd4gh2wGoRpC2b6ditqd/Zo9ZP2Iku OfPRu5PiSX5hntzcTQ56QoH1HWHDIxNcna3rM8k3+5iQB+3zOJBTv96m17rYTfL7DGdDwR GJGOX6S29TxZ6yj7Ih/VxW+CGIwCuEU= 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: <20250227061616.GD110982@cmpxchg.org> X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 9992E12000F X-Rspamd-Server: rspam08 X-Rspam-User: X-Stat-Signature: p5afxr1pfhyriuciqo1oqs8q7ncgpxbj X-HE-Tag: 1740640327-873642 X-HE-Meta: U2FsdGVkX194M0V3pyEeA1UGyfOcQmlb5Yoo/N8+WmvJUHt6fI1Vlu5vHFkBwMhMt55TrIHB92MmIOBFUROH8qL/i09IWudiztjI4N7eXWuoukp+t9W8XyYQI5hzYAf4ax2lnxgDmb2qaT55n/9GYGDeqE0uW+CijM96Qb2ju15jrdGTCXTxq0ZKS3w+qB25diOHoJ15o1QmvpkLjf9jB20p1ZLdkEfhUtb5C2DyhM7B+23p06aW8F0azX49Nlo1YyCItudBWpxtWcX6Xfpze3+U0gHtv64dBqez0+MHPXL+jvESgrGO8KEa8h9weyCZgV337OgXYQGx6o+VVjFbkoVn9Te4UV3pRmKIgeHhsGazr/EdeU0c/QoKTWwV1v44aMZhsiW2JNsZf6ALV2ByK7F8rpkHXB1i0FiECE5bpJGO7AGaUrascmreL1JaJt1qjtzqvpQoSiY1RP59YGwVa61nfivNL+cv+ILIXf1HAS/QzZJaq6OFqLWzviktes8oIGS316VKb5t9DNXyfP7phKivOEyjGQfOGBB/boJ/sCdrCHlqdJH7zmeuqSUK32jhwoUs0CnfQVEI24g9GMk3k6GLEpekIQqynm2eC3hM3wZY8YXOJ+XHke8PBLs2mX+wv9wcw8PR3uiEjcmo9OdINcjQ+ZdQ4l3HtwqOMJpqzMY6GKUvxrFEZn79uBTFas8LQ2svRmM+MaLcpuwPoIJeNUAeMm/iN5w/bQ23n1WveNVD62I3m/2ApTohDa/GV2zzxY4dCSrMynsikcT5opbuP1xTH2+GkrEvsRtXXDsOveIgxDnDqjKImONhSmbXSjpefSpn/oEiF5NKtgHouLcV8rHJAuyC1jQxH8x4TWcpUXaVCHTZXfPp1YB05RtPmallTmPtsyYy9eITrLOigkP/yG/nAs4cwIkMVSn5WqoYi/F1Hq9OowEq0lxPFuc5Lx+u+6+D1MSh2YwJ3USMpqE hNq190+K JBbKgbEj84xFJDFwMaw40d5Kne+mY5bcQplxCuWBvZMTqupUmwD+9JKfhkOeiAuGoWVaLsQSnus/ZAebu387MWFZa04prSqojrJidqziiDbYTM4MBSR0powVpTfJLgiRAVLGQ/Cu5+1iNcrlxUTPsB5SHa0OIW1K5nSWctjnu/edclUtB/Yjrc0v526cJXhFGiG/Nj1itpNCM/LcYvWGrJAntVw== 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 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.