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 85798C19F2E for ; Thu, 27 Feb 2025 16:05:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EDDDB6B007B; Thu, 27 Feb 2025 11:05:37 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E8CFC6B0083; Thu, 27 Feb 2025 11:05:37 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D2D596B0085; Thu, 27 Feb 2025 11:05:37 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id B99FD6B007B for ; Thu, 27 Feb 2025 11:05:37 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 3E47CA411F for ; Thu, 27 Feb 2025 16:05:37 +0000 (UTC) X-FDA: 83166199914.17.2555382 Received: from mail-qt1-f170.google.com (mail-qt1-f170.google.com [209.85.160.170]) by imf11.hostedemail.com (Postfix) with ESMTP id 5CF8F40026 for ; Thu, 27 Feb 2025 16:05:34 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=xgl94nff; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf11.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.170 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740672334; a=rsa-sha256; cv=none; b=hXchTjxjgu1IpbTe5zl2ABMWchfOYbL3uybphc6bTN6Ml23EB6lIG5qMkgXnmM61DEpW9T 1YhPg37K4MwlR2shv0Xn1sSR0LB4PpeqwbN4jIivJWMlf9YyKLkbrDRYXWwm3MPq7nLBxN lnjDRTc7rVdTGqnpI8XQodzwNUR13r0= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=xgl94nff; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf11.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.170 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740672334; 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=A91udeNQ6gL/fByIkH0tlUyWahWZ0cLam9LmFQEXaw4=; b=qQCIF5hu5X4ffmsdhewZ0unCYJ+n+1dWiuRmzYQuPM/5VZTv5acguz0PCZc8YBrjvTqCvo L9GB9UeQIoJj82sdFBK+ByiZlDRDsxxc569I/2HzBdTDyaKL/S28XmuEQBAxM/4k5cVjyS GlDBLP90dFP50pq9wUoypM7/OOBwg2Y= Received: by mail-qt1-f170.google.com with SMTP id d75a77b69052e-47210ab1283so15283971cf.1 for ; Thu, 27 Feb 2025 08:05:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1740672333; x=1741277133; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=A91udeNQ6gL/fByIkH0tlUyWahWZ0cLam9LmFQEXaw4=; b=xgl94nff2az6WfZRal6OhFq+edWJCMv+DKwjjTG0mdxA/S/U+nP2qMj3TaXOMknssP JD8GEPC6qwaWDpk2xOIsP+FwG/KpknEwR3s0b/RJc4SODZqXam/FddUeOzYAm6a7pXuO 8uReeoUEDGHg7JPFcgST8lx8vvRVrUzwPa8euzBz5oqtpxyLzAzqAj7caSAHfv76fMow ohg2kkQiBtS1VL0i+sMGJIP3MBliUlFFOw+r4LPzdXIVUbTGZpdC36PbVVEOvX7/X1YI /HQ6HrqNfioPZj2NkqpX8aGUj7OCwujmOJVKf9ELrTu49MSUk9mkdXmk4V0OmgZgH4+o DoXg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740672333; x=1741277133; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=A91udeNQ6gL/fByIkH0tlUyWahWZ0cLam9LmFQEXaw4=; b=cZHEWhZxKzzCzTPUbkZHKFTcQroB1vczr3AnsyLG7Jj0Cp4HhiK4toQP34dlOy0Cj+ Ygsxw+oF2ZzmUSG2F43u8T6qDmO/9iJXxbDXomVPczdLSNCVgTvGHc4YYRLihr1MB+IM 3setjfY5qABBrY3VPv6CS+Wxtj+7UILxcF9saJd6WuidY6ujX0v1BljtXcCDha1x0PHw C0kiazvuJS1FKx5loZK2UZ3mRwz814b/Ycb41lsMRacN3qReO/LCSL+Jbft41uhjODwb OnWOohvTuZMpa4OziphV2npkyAYXmV0S2T9E9QEItWGsz/AWID5fPKjAoaUQ+ePzlRMO HKDQ== X-Forwarded-Encrypted: i=1; AJvYcCWe4BACrrdG/rHtf+jy79djPmitg5op7YTv44GuigeG16igi7Jxx/wfYQYEfoDDS/+8/8aYfc3uXw==@kvack.org X-Gm-Message-State: AOJu0YzbfZUuREVS0m3NdrW+wQG458aQvttw3UNWBmTpqKNeqHjEap2r tlEvmS2QFu7xVn5rRS3HRVmoxBeXkQgenKTLkIP+WmmCVdtdUzulIt2vzt/R3oI= X-Gm-Gg: ASbGncuwqytnPBMKgjnq8JqCmtACinYZWekF+LUJ1hPewfbs5Q3UJHeIgk8C/PZZuaD vdFHriVD4tkjhASMhzLkGr9gFvP0rBdVpLy3w2VXfhFnHelgE5DiElijG+60wIffaMZVWOxIM7C dahPbnBH+CMdTZNy2l+KjuoTMSqSa6nVoPmItunkpuaO5xsNXkYioEm92rrnhA6tLoyQxKfWj5f zVrl+PJohQEL7fMtFLVJuKKE+SVCYBS2Pc5heV9qa/LVn5PIshTD3dBqQdqBTcD+HB/hvcAH4SW y170Oo8oIM7+zYHMuQend98L X-Google-Smtp-Source: AGHT+IFbywb/b1rLYc2GJKkLhDLtOWo7qWH1v8OYahONUwAh6TaO7qzdAkb93JW/B9QcW7JSKSDbAA== X-Received: by 2002:ac8:5741:0:b0:472:5f9:1f71 with SMTP id d75a77b69052e-47377116e6dmr167915131cf.2.1740672332971; Thu, 27 Feb 2025 08:05:32 -0800 (PST) Received: from localhost ([2603:7000:c01:2716:da5e:d3ff:fee7:26e7]) by smtp.gmail.com with UTF8SMTPSA id d75a77b69052e-4746b5eed44sm12539111cf.24.2025.02.27.08.05.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Feb 2025 08:05:32 -0800 (PST) Date: Thu, 27 Feb 2025 11:05:28 -0500 From: Johannes Weiner To: Yosry Ahmed 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: <20250227160528.GF110982@cmpxchg.org> 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-Rspamd-Queue-Id: 5CF8F40026 X-Stat-Signature: zpoc1grweh4cxti7enmeackfg3wdafrf X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1740672334-64930 X-HE-Meta: U2FsdGVkX1/jA9oOVfS6ygHD8GiKkn3v7DDimJUpAlcwyhGFK2rjuxpv3GTX4ec0IvhZGKiGKKfOozaMdugJfHFNQN10koBzgkKBaGAwyTOioCqDuT3aaSqCpTjBAcTpgy4HVm4N1FZV2nF1A905jHUnDhQZpllcb4PT/eHAorRM9c+1fI87WylNYyo+fAr3wMa+iq3/NnENIOFDmugAJ46DFdNgb0UIDplz3BAI5I81gi7pDCnwlANmQkGD6IbyVVnkbnTVu9ps/MldcUj4i5Ks+3t89QNVAspA2kHlDo8JCB2WpdEikIKSeAgiTjQm8N9HWUBCgFTGyUd+P5EgIssGIK2Uq5QFvOjJ48JvdensgW3NNrp3D0aUokLwTtvAKs2ce1XIfeLIaE9Hzams7ymSFEkqfoeqw0DRXtN2F6VYuKjuRTC4xVMO8G89WMKkRbzFtUjZYHWc+b8gHJ4sbVSCSPeIoLUM2pC+ENYasGNHuEP1zmaDvUa1GGr+OZbpnt8vpTYm7+SBg1cZTOkbTes2rdvg6ZgNt9Tb0IODtpc+Vi7r3W5MtsRC2YjEFa4XMOpfWMsg7LcoVExRyxT7EjnEy8oeh6/aTIVr2l7snedYBWN4gduwOh5Rlo118MuaMjtIg92pUkRo1YJs7oYOmkeuk0JYrJs6UnvHpW4pOl/JG4X55yYz67fB1J5WpQVxGBU+A4lbdw/FYPUWDgLXSiW+cgwB2a5TknzpphKU4WwnijbmgXWVb+siYbCfT2Fm/8lxHtrnIIIJTV08HWUWY3DXnhlKyVHOaGg/smWpLk/9xjTVEQsUTjgBS4FFOfBKWVSNozooQPTaH4zLUCUJKZWR2zXgK3yrTHETlYJRJhVRD420I28hYlidSVx0trV3RddP8EBGGjdrUYQ6ZTtAQ5pgTEGvNyQ2DMgjImF1lAmSEoOsahv0mrvW4JOB/JZJHSD6CdNmb3lhuG/mcUZ LfIK2Otj XlEVEMq/AVgE+EX26flQu0zQBPEb6xlkR7hw0Y5pt3z4hhMHMhfih1BGqZfxExh14tnE8EE34fbnEkKjQLdyXAuju9zVmCA0D0XlCNPt+cWILRKIDZ/POMMbFK5afxxwb/Oq9IcbOayTKeIUNF+eOT3D9TCcZysrfL0BActXzZCg6zdbV0wfRcwQP5MNqvqqepgkpIa24L5PQe6VkkifrFG7kuPUtQVf1aKTL97G+bSdvRXzxkf0hKnU0HuNIIeFEh0lQ0rviN/j+GrWUmwT8gwWHr4x/IBGu9TbOJSIwQzMRd9OR6dxyh2ESaKJkE6oDRn0T81gYmsz1gLFpe8OLUuZ4JQuH34EYFSGndmMsDFyPSHP9bdxQzcUeLRAqzo9PQ8CBanOSm/lo3gQ+3XFz0BQd3S/KO9Bi5NMWvAgM8oK0MBOHX8bWyyxUJcky/07JlvFcBE4FpL5M3f9O+6pFX9ez/w== 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:29:45AM +0000, Yosry Ahmed wrote: > 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; > } > } Eh, I think we're getting colder again. This looks repetitive, zswap_load() is kind of awkward in that error leg, and combining the two into one function is a bit of a stretch. There is also something to be said about folio_mark_uptodate() and folio_unlock() ususally being done by the backend implementation - in what the page cache would call the "filler" method - to signal when it's done reading, and what the outcome was. E.g. for fs it's always in the specific ->read implementation: static int simple_read_folio(struct file *file, struct folio *folio) { folio_zero_range(folio, 0, folio_size(folio)); flush_dcache_folio(folio); folio_mark_uptodate(folio); folio_unlock(folio); return 0; } and not in the generic manifold: $ grep -c folio_mark_uptodate mm/filemap.c 0 I'd actually rather push those down into zeromap and zswap as well to follow that pattern more closely: diff --git a/mm/page_io.c b/mm/page_io.c index 9b983de351f9..1fb5ce1884bd 100644 --- a/mm/page_io.c +++ b/mm/page_io.c @@ -538,6 +538,7 @@ static bool swap_read_folio_zeromap(struct folio *folio) folio_zero_range(folio, 0, folio_size(folio)); folio_mark_uptodate(folio); + folio_unlock(folio); return true; } @@ -635,13 +636,11 @@ void swap_read_folio(struct folio *folio, struct swap_iocb **plug) } delayacct_swapin_start(); - if (swap_read_folio_zeromap(folio)) { - folio_unlock(folio); + if (swap_read_folio_zeromap(folio)) goto finish; - } else if (zswap_load(folio)) { - folio_unlock(folio); + + if (zswap_load(folio) != -ENOENT) goto finish; - } /* We have to read from slower devices. Increase zswap protection. */ zswap_folio_swapin(folio); diff --git a/mm/zswap.c b/mm/zswap.c index 6dbf31bd2218..76b2a964b0cd 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1620,7 +1620,7 @@ bool zswap_store(struct folio *folio) return ret; } -bool zswap_load(struct folio *folio) +int zswap_load(struct folio *folio) { swp_entry_t swp = folio->swap; pgoff_t offset = swp_offset(swp); @@ -1631,7 +1631,7 @@ bool zswap_load(struct folio *folio) VM_WARN_ON_ONCE(!folio_test_locked(folio)); if (zswap_never_enabled()) - return false; + return -ENOENT; /* * Large folios should not be swapped in while zswap is being used, as @@ -1641,8 +1641,25 @@ bool zswap_load(struct folio *folio) * Return true without marking the folio uptodate so that an IO error is * emitted (e.g. do_swap_page() will sigbus). */ - if (WARN_ON_ONCE(folio_test_large(folio))) - return true; + if (WARN_ON_ONCE(folio_test_large(folio))) { + folio_unlock(folio); + return -EINVAL; + } + + entry = xa_load(tree, offset); + if (!entry) + return -ENOENT; + + if (!zswap_decompress(entry, folio)) { + folio_unlock(folio); + return -EIO; + } + + folio_mark_uptodate(folio); + + count_vm_event(ZSWPIN); + if (entry->objcg) + count_objcg_events(entry->objcg, ZSWPIN, 1); /* * When reading into the swapcache, invalidate our entry. The @@ -1656,27 +1673,14 @@ bool zswap_load(struct folio *folio) * files, which reads into a private page and may free it if * the fault fails. We remain the primary owner of the entry.) */ - if (swapcache) - entry = xa_erase(tree, offset); - else - entry = xa_load(tree, offset); - - if (!entry) - return false; - - zswap_decompress(entry, folio); - - count_vm_event(ZSWPIN); - if (entry->objcg) - count_objcg_events(entry->objcg, ZSWPIN, 1); - if (swapcache) { - zswap_entry_free(entry); folio_mark_dirty(folio); + xa_erase(tree, offset); + zswap_entry_free(entry); } - folio_mark_uptodate(folio); - return true; + folio_unlock(folio); + return 0; } void zswap_invalidate(swp_entry_t swp)