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 ED203C021BE for ; Thu, 27 Feb 2025 05:44:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 802446B008A; Thu, 27 Feb 2025 00:44:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7B1A56B008C; Thu, 27 Feb 2025 00:44:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 67927280001; Thu, 27 Feb 2025 00:44:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 49AE16B008A for ; Thu, 27 Feb 2025 00:44:40 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id F3A07C04D9 for ; Thu, 27 Feb 2025 05:44:39 +0000 (UTC) X-FDA: 83164635078.12.43590B5 Received: from out-189.mta1.migadu.com (out-189.mta1.migadu.com [95.215.58.189]) by imf30.hostedemail.com (Postfix) with ESMTP id 246858000A for ; Thu, 27 Feb 2025 05:44:37 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=bfhCq9Jj; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf30.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.189 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740635078; a=rsa-sha256; cv=none; b=j4J23h590ZdAq1Le/jJuJF6rH7K/KDMMfttrkK5lizSzgFpBMLvugziMmFK/AemerSfPg8 A2TEP/Cd1TjqQNrAfXht8Zek8fvfjh1uSf6uvI9QEk/9dRvw4O5LyWG0J+WWQsgS77KO+K DOFruIQCSikYLvVg2T/JJg9knnZtu/Q= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=bfhCq9Jj; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf30.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.189 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740635078; 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=ERFgDI+aEGThEtR4yr1u5l1He+Oh0pNjxy51oH28XYU=; b=wz5gkX5anoq/lfOmOvH0xGUrtswKbSBDHr9/JHwNX0A7y5C2JCs0z3+qaFGoZ9mLoG7C5c KNSj5mjYPlLmDC9iYuQ0oNoXex5Co6/bp8Iu3hhR6GALt0XVY1yOQflW0viBITLDg7Hug/ 2kYKYI39FgEaPqdB88fiZtKCMdOve44= Date: Thu, 27 Feb 2025 05:44:29 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740635076; 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=ERFgDI+aEGThEtR4yr1u5l1He+Oh0pNjxy51oH28XYU=; b=bfhCq9JjZq9L5aFtZPxk1fa1qWyZuVgIo/bgY4wCBec0FvHVZD4+7CQOHLiM/8JOjP5XE4 ZqSU7hSJSFYj+4175CeWuu6k7Pwm0mL95991iBSSqqXcTYh+bt42yB13oPbZvGBK6zqCn0 z01q9dyKNo1dP+EinZcYjqK9dtX6VwQ= 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250227043141.GB110982@cmpxchg.org> X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 246858000A X-Stat-Signature: tp85sktkjhqxtp3kgkiqns57x3r4jzhz X-Rspam-User: X-HE-Tag: 1740635077-219190 X-HE-Meta: U2FsdGVkX19LAhKu8GbZjttkp4vXYR087j2+qkq7ieJdaqMz5O0bIKzyxhr2fhLocnXtxCg9HIuCTnulTDoXj4OiSZLgRk06ntEJ0HrY+lbSOreo5v7s9p16iKjg7FUNMrgVHehog1u1BXBPDWO0oSS4XeXKY/iKHqjOXpzcX2GQyU5DEvcdBtjgTiJRNw0pbr5GGnrw6IVtA8Juasm+/T+4Nhx5LH8La2TzNA79HFVENDOTDJ7kLgO53MFvWgzZ6rqEEPmm1YJh4WJv0cZq149y088bV0p/XYnaZq9va5BLInCovMWN5tu0sOo24y3nUogppJHoE1p2UlT+KSXYWzO3mYD78mMdhRD7icSSqWGsSzIapn/z6XDkPMQWCM3eFSDrudNhLX6oRO1xCs+aMgjB9+MYLVfWFoJwvZSVaqxbhteiW6k8m2hWnTR+H3EkWpASWPFD0DPKnWVi5fnzJCdlP3zrwK0A1aHIvMTBw/fd/Ttaiyq5wytQBiNXv5uNAIfr0ApibgO/6vR6uOvbr+eajGUv5fDWeVEsbOXvwC32jaASqUQ9teVUXaH81NFRR4uXQ3t+9aL3lGPVz828xXhxKykpVe1fdKQogTbrGsb+fEms9C87GETC5zOZ1QNLvca/faj5mYJMC3cAwlykVGj93MNGZ7d02aUF8Jw3TuPNtVyXfvKQmHGopt0emIzXIw2gj7S1c/UyBj1eNHsSph5vZcCl89tL7fjEFb1AYuqS1Qiva8wTlnGMoCZzJJJDFnFT+txMz0+FxL8VM1jfxLY/3dS//KDsOPVU5pkulr7DwFRpbpvLsNU2xTXVCDjEr8YB525MLxwV+BJytPC7nm02obiefNUGjXDYRIa2bcO28eqmVex5KmuKJprfX6XcGUSBWtmXiM7dL1BgNbvO8ijiG3/2eFnYo2lYoHEvrf7we2OHqKJMfBHkXPHVAVFmLJaxPDosekEyPu3p5xI KCUUdIK/ +iyklAGWVhOI1IIayULk7eP+grIZYBItyhxmBlXIIuP9eKmqC5Uq2xdNAJQsCapWmntkoUzbubaECHz51Bkgthm/7KTpAZlCr08NINkOenPV14RBjEboELNtZOo2yZb7gmL9wpvil3NZwXdicN8CICGy7ibV4O6JktULbG3SL5DqzcN31gx/tUlHo0AcSxMzXxrML6UBXu3KUwM8qUCPsujriYA== 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 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: ret = zswap_load(folio); if (ret != -ENOENT) { folio_unlock(folio); /* Comment about not marking uptodate */ if (!ret) folio_mark_uptodate(); goto finish; } 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; } WDYT?