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 29004C27C55 for ; Mon, 10 Jun 2024 20:12:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AECF66B009A; Mon, 10 Jun 2024 16:12:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A9CBB6B009C; Mon, 10 Jun 2024 16:12:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 93E106B009D; Mon, 10 Jun 2024 16:12:05 -0400 (EDT) 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 727F46B009A for ; Mon, 10 Jun 2024 16:12:05 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id E1A43C02D4 for ; Mon, 10 Jun 2024 20:12:04 +0000 (UTC) X-FDA: 82216075368.04.A1022A4 Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by imf25.hostedemail.com (Postfix) with ESMTP id EBDACA0013 for ; Mon, 10 Jun 2024 20:12:02 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=2wdvcpvO; spf=pass (imf25.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.44 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718050323; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=0+OMv4uqkZA3HiJnLFERokf4naiqqWK7dCPgmQs4qAU=; b=J6a7DU0B0UDGyLImuuhZNcly8uxIVme7YEKKasaZfRMeHJu5wBH9vFe2sT5C04SFwbX/RY 3m8H7oEiFAB1cbNG9RSuwnv4qAjy/13z3gMHiLFsJdmuUA6sgADRPUGaT845T2esUy10Ry iB6lhBy5vZr5pzXTNkveziRWSDzgWcc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718050323; a=rsa-sha256; cv=none; b=CpjLloCrwd47n07WvHLNVO9N3JKpVDzTzrYapsxWKwIJykl5wSxqwaJX0mE/VKaIfNcuKJ VVvKYNpx8d4xEElPOi5jN6IYZGzsOWFlTVOJfin2nELiaMz6OOSIeKDcU4xJJNwix7n0fz f5IGyoJCYwXlrsxGiGfRgc4kfkm7oFo= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=2wdvcpvO; spf=pass (imf25.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.44 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-a6f13dddf7eso222817466b.0 for ; Mon, 10 Jun 2024 13:12:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1718050321; x=1718655121; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=0+OMv4uqkZA3HiJnLFERokf4naiqqWK7dCPgmQs4qAU=; b=2wdvcpvOW1qHn5YXlkz4SwcJf0kFQFFdzZzFYhz7TPmWVnNaCSdA7pmt5HQvmh4koy Le1eaZCgRXDp8li7mnYKIzXj8CsMJl9ELNV86JUYp8i369jsoiDTN0Rfclq0zbTLmS0q jMv8JqPfGVty1GrH+7SgQTsN7sEIKQIk7Tb0RkmK2m3d2vwnP9alMXWzl1/8hz2SzTyH zBH42FqzEnbuaOsu/G32ZweBwx/5MDLzuE1jeOo3yX/5XLzPi9czUwSJDiM6HGEFFMoO GyyfsCJphaWOMSjEU69tvlzQEtfEv6LXQb1K00UtcVVCGtL2VuXki5bFqOPPgZqDreY6 fIoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718050321; x=1718655121; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=0+OMv4uqkZA3HiJnLFERokf4naiqqWK7dCPgmQs4qAU=; b=oNtPj3c06UU9HU0BsNXdl76FUjyU9Aa3GULysQQNCWl3JwBK3n2x1oELDSsmQauuFx prL3xORj2nMjIdeJyB4eZG+QlJXIrScJUPgQCNxfJJY8MpCr5sdcDmqZc00MSyui1sr7 MitOsdPx6IOuUIGWzKDx/1OKJrWvMY+REx5YNpYv/b40cjYZAGMNM+nBGIRqVT6J7Mym F1Wrp59HgGEM8q6LIw1lXFTAW6c+inxpHLxRDV9gvJuJ9TlQiQVEqgXVE/oYWagzz58H 192jq/TVJlOHvoxM4StwIgXl7lX3eAPTsV0epFVrtINRlKj53UlEv7sYMLnPf6ecz2Gc UvAw== X-Forwarded-Encrypted: i=1; AJvYcCURvHpfGBbTyxt8UmTm3Rarcd5ihxJKFBKtMnWMT9S2qyklLOXgCICAhzqrs+vMKgDhZOk2tkWA0ON8iWxjmYQqFvY= X-Gm-Message-State: AOJu0YxWW/ERRhDgHyNh96nMgDTMIOmd/9GmhxMKou8otFf5Q9pR3m02 Rm4ajWHVLorLZccBQ1ECfduBrVdd4SWvyymbJXjpwHA/f1cRsgROomfAyC5tiEC9JF7SPbZFlFa mM9oRb/6G+MChAJV5q7F+/BUXGzVDrgdeRV24 X-Google-Smtp-Source: AGHT+IHWilHeDBacjA8+ws/Bwz7RYmfSL0ig50tdAjhy7t5F1gRRT/tcPiVNwm/5kt6HeAHM0Zcet6kEMywTDGeWjOQ= X-Received: by 2002:a17:906:2994:b0:a6f:1dda:b04f with SMTP id a640c23a62f3a-a6f1ddab1c3mr243791266b.17.1718050321093; Mon, 10 Jun 2024 13:12:01 -0700 (PDT) MIME-Version: 1.0 References: <20240608023654.3513385-1-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Mon, 10 Jun 2024 13:11:24 -0700 Message-ID: Subject: Re: [PATCH v2] mm: zswap: handle incorrect attempts to load of large folios To: Barry Song <21cnbao@gmail.com> Cc: Andrew Morton , Johannes Weiner , Nhat Pham , Chengming Zhou , Baolin Wang , Chris Li , Ryan Roberts , David Hildenbrand , Matthew Wilcox , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Stat-Signature: 1h5dj1kbeaz11yjdtus14aut179coso9 X-Rspamd-Queue-Id: EBDACA0013 X-Rspam-User: X-HE-Tag: 1718050322-125415 X-HE-Meta: U2FsdGVkX1+dp20tzsuSbStjziAjjUazfstlBQHttPTcQ8gYTIM5mnQjcSUrOmevTfpHO1mncRx5m2+LPamdDlm+HDaL9TTI96I3XXWnNqSQZ57zDeyqlwhEQqdQcutZnHo84TZLLoC71BymHPP54u9ESCEfeh8gH7Pj9TOcCk5GjlF40Wp2ZWomIFXg5/HiU5iMYlRHCJyTCdi80vDN5jlO0BpcwD0kdvhDscTG55Ai6GXwgziPms9QJsRHP3w7RvlC8T0IZKKfV5PZwWOGCM3M7sAIx44bliLmCzIjamaBwzW+wI/EF2pTK+N7PSKzIOu1+C1/UcUI/Lsy2jPE6koL56z4y7QWpfRk0Mzfnt8KtYHFh0l9lWkq0uSoTOfu1wneB+XQpZfqYj+RPcLUUFT4Tx/+pbzEWYNNxMjKFeETn75nKKHath4SIN0KO2vxdslZG8QyUSZ6BUE8HTs+VQ89m4RZJ1XudnARYkJe+RJsSIOK8j3heGHTtRdgQ+YSwq/KkFFRtFHrX40AlDBVvqYZIKl8a1a8wk0NS+ub29HeEvMXgBbj9GGkp/Ogpzr7pDYgHe2ofFR9UpuO5mS5QJbxjMhkdVedkQEVAxWaA4PHEEgc77XNu5jYWWz3ytzk4edtrtBMWRzp4h7RdusH8S03isBelwejS9f2kB+ZkyGAt4R/ITJTZxUu1XaSxB+4buza0iAWN+IaI1YF55CAYTmW1Zx/CZzksfspT6vXn6eWSle9N/HOVh9DXngYI2UbR+eQ4TZbrJf8ce++jMW+D4qxfqQF3zDj33TBED3aH/n5YhWgg1Hw/FLlewKV6NPpNrtvgvSYJ4V9m96uCFCtnKFYD2MzxPT/4LcTFX5sPdgjv3LGSxols4nAaNjWpJr5+1TnaBtrYnpa2iOXyS8/kcrYzQoDe0haoarTECVNd57XhMkuC775RrCmsmB9RQe5D/3rkqvLGUagIlhkF7Z LE7DJ6hG HvCX+GTMIziTlJRQYNPXXl5d04uBGbgurGhPqwMgePhAk6biZtae23P1s8Hal0qCQozr6N3bxqKUAR1t4cQAhbecD7y8lGXXjzvXCVLJRenzv//dlB+bdJHJtNhDyqCniOFUzj1bN6iSpCxujDkijrikzsQJUMe5f8ahDK+rSM3bVnSSQaAhosVm6uH01Xe84v/5Mxv5jbdrWJMGoWfoJ14WbSBHSHQh4fxkV1Wj+yASb4/VV6HGvi/7ACw== 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 Mon, Jun 10, 2024 at 1:06=E2=80=AFPM Barry Song <21cnbao@gmail.com> wrot= e: > > On Tue, Jun 11, 2024 at 1:42=E2=80=AFAM Yosry Ahmed wrote: > > > > On Fri, Jun 7, 2024 at 9:13=E2=80=AFPM Barry Song <21cnbao@gmail.com> w= rote: > > > > > > On Sat, Jun 8, 2024 at 10:37=E2=80=AFAM Yosry Ahmed wrote: > > > > > > > > Zswap does not support storing or loading large folios. Until prope= r > > > > support is added, attempts to load large folios from zswap are a bu= g. > > > > > > > > For example, if a swapin fault observes that contiguous PTEs are > > > > pointing to contiguous swap entries and tries to swap them in as a = large > > > > folio, swap_read_folio() will pass in a large folio to zswap_load()= , but > > > > zswap_load() will only effectively load the first page in the folio= . If > > > > the first page is not in zswap, the folio will be read from disk, e= ven > > > > though other pages may be in zswap. > > > > > > > > In both cases, this will lead to silent data corruption. Proper sup= port > > > > needs to be added before large folio swapins and zswap can work > > > > together. > > > > > > > > Looking at callers of swap_read_folio(), it seems like they are eit= her > > > > allocated from __read_swap_cache_async() or do_swap_page() in the > > > > SWP_SYNCHRONOUS_IO path. Both of which allocate order-0 folios, so > > > > everything is fine for now. > > > > > > > > However, there is ongoing work to add to support large folio swapin= s > > > > [1]. To make sure new development does not break zswap (or get brok= en by > > > > zswap), add minimal handling of incorrect loads of large folios to > > > > zswap. > > > > > > > > First, move the call folio_mark_uptodate() inside zswap_load(). > > > > > > > > If a large folio load is attempted, and any page in that folio is i= n > > > > zswap, return 'true' without calling folio_mark_uptodate(). This wi= ll > > > > prevent the folio from being read from disk, and will emit an IO er= ror > > > > because the folio is not uptodate (e.g. do_swap_fault() will return > > > > VM_FAULT_SIGBUS). It may not be reliable recovery in all cases, but= it > > > > is better than nothing. > > > > > > > > This was tested by hacking the allocation in __read_swap_cache_asyn= c() > > > > to use order 2 and __GFP_COMP. > > > > > > > > In the future, to handle this correctly, the swapin code should: > > > > (a) Fallback to order-0 swapins if zswap was ever used on the machi= ne, > > > > because compressed pages remain in zswap after it is disabled. > > > > (b) Add proper support to swapin large folios from zswap (fully or > > > > partially). > > > > > > > > Probably start with (a) then followup with (b). > > > > > > > > [1]https://lore.kernel.org/linux-mm/20240304081348.197341-6-21cnbao= @gmail.com/ > > > > > > > > Signed-off-by: Yosry Ahmed > > > > --- > > > > > > > > v1: https://lore.kernel.org/lkml/20240606184818.1566920-1-yosryahme= d@google.com/ > > > > > > > > v1 -> v2: > > > > - Instead of using VM_BUG_ON() use WARN_ON_ONCE() and add some reco= very > > > > handling (David Hildenbrand). > > > > > > > > --- > > > > mm/page_io.c | 1 - > > > > mm/zswap.c | 22 +++++++++++++++++++++- > > > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/mm/page_io.c b/mm/page_io.c > > > > index f1a9cfab6e748..8f441dd8e109f 100644 > > > > --- a/mm/page_io.c > > > > +++ b/mm/page_io.c > > > > @@ -517,7 +517,6 @@ void swap_read_folio(struct folio *folio, struc= t swap_iocb **plug) > > > > delayacct_swapin_start(); > > > > > > > > if (zswap_load(folio)) { > > > > - folio_mark_uptodate(folio); > > > > folio_unlock(folio); > > > > } else if (data_race(sis->flags & SWP_FS_OPS)) { > > > > swap_read_folio_fs(folio, plug); > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > > index b9b35ef86d9be..ebb878d3e7865 100644 > > > > --- a/mm/zswap.c > > > > +++ b/mm/zswap.c > > > > @@ -1557,6 +1557,26 @@ bool zswap_load(struct folio *folio) > > > > > > > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > > > > > > > > + /* > > > > + * Large folios should not be swapped in while zswap is bei= ng used, as > > > > + * they are not properly handled. Zswap does not properly l= oad large > > > > + * folios, and a large folio may only be partially in zswap= . > > > > + * > > > > + * If any of the subpages are in zswap, reading from disk w= ould result > > > > + * in data corruption, so return true without marking the f= olio uptodate > > > > + * so that an IO error is emitted (e.g. do_swap_page() will= sigfault). > > > > + * > > > > + * Otherwise, return false and read the folio from disk. > > > > + */ > > > > + if (folio_test_large(folio)) { > > > > + if (xa_find(tree, &offset, > > > > + offset + folio_nr_pages(folio) - 1, XA_= PRESENT)) { > > > > + WARN_ON_ONCE(1); > > > > + return true; > > > > + } > > > > + return false; > > > > > > IMHO, this appears to be over-designed. Personally, I would opt to > > > use > > > > > > if (folio_test_large(folio)) > > > return true; > > > > I am sure you mean "return false" here. Always returning true means we > > will never read a large folio from either zswap or disk, whether it's > > in zswap or not. Basically guaranteeing corrupting data for large > > folio swapin, even if zswap is disabled :) > > > > > > > > Before we address large folio support in zswap, it=E2=80=99s essentia= l > > > not to let them coexist. Expecting valid data by lunchtime is > > > not advisable. > > > > The goal here is to enable development for large folio swapin without > > breaking zswap or being blocked on adding support in zswap. If we > > always return false for large folios, as you suggest, then even if the > > folio is in zswap (or parts of it), we will go read it from disk. This > > will result in silent data corruption. > > > > As you mentioned before, you spent a week debugging problems with your > > large folio swapin series because of a zswap problem, and even after > > then, the zswap_is_enabled() check you had is not enough to prevent > > problems as I mentioned before (if zswap was enabled before). So we > > need stronger checks to make sure we don't break things when we > > support large folio swapin. > > > > Since we can't just check if zswap is enabled or not, we need to > > rather check if the folio (or any part of it) is in zswap or not. We > > can only WARN in that case, but delivering the error to userspace is a > > couple of extra lines of code (not set uptodate), and will make the > > problem much easier to notice. > > > > I am not sure I understand what you mean. The alternative is to > > introduce a config option (perhaps internal) for large folio swapin, > > and make this depend on !CONFIG_ZSWAP, or make zswap refuse to get > > enabled if large folio swapin is enabled (through config or boot > > option). This is until proper handling is added, of course. > > Hi Yosry, > My point is that anybody attempts to do large folios swap-in should > either > 1. always use small folios if zswap has been once enabled before or now > or > 2. address the large folios swapin issues in zswap > > there is no 3rd way which you are providing. > > it is over-designed to give users true or false based on if data is zswap > as there is always a chance data could be in zswap. so before approach > 2 is done, we should always WARN_ON large folios and report data > corruption. We can't always WARN_ON for large folios, as this will fire even if zswap was never enabled. The alternative is tracking whether zswap was ever enabled, and checking that instead of checking if any part of the folio is in zswap. Basically replacing xa_find(..) with zswap_was_enabled(..) or something. What I don't like about this is that we will report data corruption even in cases where data is not really corrupted and it exists on disk. For example, if zswap is globally enabled but disabled in a cgroup, there shouldn't be any corruption swapping in large folios. That being said, I don't feel strongly, as long as we either check that part of the folio is in zswap or that zswap was ever enabled (or maybe check if a page was ever stored, just in case zswap was enabled and immediately disabled). Johannes, Nhat, any opinions on which way to handle this?