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 24355C282C6 for ; Mon, 3 Mar 2025 23:23:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A702D6B0082; Mon, 3 Mar 2025 18:23:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A1FC66B0083; Mon, 3 Mar 2025 18:23:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 90FBD6B0085; Mon, 3 Mar 2025 18:23:09 -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 757F36B0082 for ; Mon, 3 Mar 2025 18:23:09 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 3617D1A1150 for ; Mon, 3 Mar 2025 23:23:09 +0000 (UTC) X-FDA: 83181817698.01.D309554 Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) by imf20.hostedemail.com (Postfix) with ESMTP id 5E9621C0005 for ; Mon, 3 Mar 2025 23:23:07 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=MH5PA5en; spf=pass (imf20.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.219.54 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741044187; 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=NPygKGNxAmOTggvaqE/qBsWIYIR+kpzzJ1Nu/3DQXzQ=; b=4puWoqcurm5ABZH0UCjKnlKWjVgoZ4ltJnK3ZT6A//pz/rv+XL/7hf3AYdzCl90WpV58Lm ty85PXNAJwT9YYXTz9OEheu713LB4Lt9j4KyQrOb45SYorgs06wiVtH+po7NXA1pZIdAvP on1h3pfARvf2aRBTTOTMXIfQDQqIKY0= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=MH5PA5en; spf=pass (imf20.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.219.54 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741044187; a=rsa-sha256; cv=none; b=CvbnfQsXMVA+FYQcNdqkYZjRybX/ZXneqtfaCs9kuodrzDB20CHVFMbvh/xeAtlUCCSNQy gXmtysq7x7mfY79sIh21v2as5RUJzfp9BVkWBSsUMbkXT07/C31R7ksr0lGvVbO8GKQWMd 9ISA1Ii0zpIWcQSIe8v4De2jZMbgIec= Received: by mail-qv1-f54.google.com with SMTP id 6a1803df08f44-6e894b81678so25518936d6.0 for ; Mon, 03 Mar 2025 15:23:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741044186; x=1741648986; 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=NPygKGNxAmOTggvaqE/qBsWIYIR+kpzzJ1Nu/3DQXzQ=; b=MH5PA5ensYjNCmB9kbmsnrsBHG51IwpagxcUGhejPiH8XSfoF8C9oybmcbg5qI0xEF KTUNIitAo7efOjcyV9V3t0t/NFKkyPJyV7wzYI32Kez7Mlfg+4ANsULbA8rEgdCdVmLG oDkqx0yU574kbSjpAQLHmT2PGTAEMZOrrwiBJPvVzNf/kD2T8PAf6nJYrINgR6s5ujrL cj1cERq+S8hQ3M9FWGNqSbKF8KqBZvMazvsECMaWyjNDO7EIDRVQbSO+9yJKCWl8UKg6 bhbgFbWOVtDCxH/NvhvR15A4g6d6SgT+4J3aq+mE6XmOvL85lgTMi25k96+KNuyLe28o QhBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741044186; x=1741648986; 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=NPygKGNxAmOTggvaqE/qBsWIYIR+kpzzJ1Nu/3DQXzQ=; b=K2j8VsBS+lSH1H2RSxXCOsFYisPVhQJn7p/nUB4v5tR9vNeO/3sGP6HBwFmty1ZoH0 KzERkqVtmPJCE+l/kUUgEphU1iVt8zDSbU5NCO+iIQfs7ldkiwmx3y04J6/RtHjOTdc8 ogT5lXOnp1e5KSGSA70Bm5U0Hm9Wfl4NLhsgTmRIf0JwQxNvN5rM1fDMUjmxkzUukEyK RiZqyano1xiS/JMgbsvRcy7jRb050lL/aVN30rbQFRJF+fx4psnO2ud4Y3Hzw40EjBeZ I+JH35Y7LxEjKxZ6qp37qlJjztMFTdsgfwe7dcZ2kKr8BfHD2Zvoznq0qo1fYIHvfgQD fqaA== X-Forwarded-Encrypted: i=1; AJvYcCV6KPLaX4yj5xQkuzDjbmgmqVGmPvalynL8J+wzdNbUc1bjO6aPLGxOR3g1L/9MLJUfSIimMuWXYA==@kvack.org X-Gm-Message-State: AOJu0YyY5d/RzAXbV1rMzSPxrs6gbHitfM8vxJgOJiT5uSniuDuse+uY F810fHFRryLGAaBVcnybmAM0anben11TW+cjk4iaJsIxgfopyNnv1COV+im13AkgOOJ62kTBc4R 3RBbeFKxZtV4ScyhUNywn3lEZX0g= X-Gm-Gg: ASbGncuv7qwPlynk+2/D2bdonzxBdz9aGwTHo5TlejvXwFWJrhcxQ4h1OWw2nv/FcOI gb5n8EmnE7gpGDxDTD/cfuVBglKu929tqTJ306YVAqlcYr6qm/3Atcfwp2S7pZ8vZPWCBYmu+ri bvDRQ7gDn11mMf4JZxhu3Y7gjFeOXn8zMaNkFWAOKYeA== X-Google-Smtp-Source: AGHT+IFhxCp0pJhWwbDIpecIyweBSam9v6gQpxT/XC5Zu5zudoXJPcbLPFHoUS5TGiABe8BdwNteCYWMXmXWmwcXPUQ= X-Received: by 2002:a05:6214:2f03:b0:6e8:af1d:b12 with SMTP id 6a1803df08f44-6e8af1d0b31mr188946016d6.19.1741044186445; Mon, 03 Mar 2025 15:23:06 -0800 (PST) MIME-Version: 1.0 References: <20250303200627.2102890-1-nphamcs@gmail.com> <20250303215524.GD120597@cmpxchg.org> In-Reply-To: From: Nhat Pham Date: Mon, 3 Mar 2025 15:22:55 -0800 X-Gm-Features: AQ5f1JobsKbYPeqORFAcpnZPJhOZ8B0oIG5208pNtGVbt8tI2jCKDifQVXHhBKY Message-ID: Subject: Re: [PATCH v3] page_io: zswap: do not crash the kernel on decompression failure To: Yosry Ahmed Cc: Johannes Weiner , akpm@linux-foundation.org, chengming.zhou@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 5E9621C0005 X-Rspamd-Server: rspam09 X-Stat-Signature: a7gte11qxrtbrf16pdsts4zhqr44rdon X-HE-Tag: 1741044187-959679 X-HE-Meta: U2FsdGVkX18pv39hkNZbV7MUW3zNVbCA9ySwfVIR6BMtxjeVyPcvEuOJs4InIpQo0Ea1t3zojVPzxSo89wZbFg8Ce+ZhfrAYB4uuXkZ6kuPP8BanYYLnlNrVwCRjqiv+Ca/t6y2LjKXrZaPV8HgW+uUvDNjpvjgUgdhGi3RbBtD++VfGoNj8gWlGqUQHnBXcubsrHm0GszF7ZvoWdRgtQ+8qWUrrRpqhTRNx+1P5vQI5pyTo9nLKVa3MlV69ZvbjSp+Kaafq7AugWitYNfaYBB2RenfOQUpJUko52yu08oaunbEmUNsB+lA36+6cW9ZQnE8qb/4QP6QACzpiqubZbD0UBKKMBRgt+PzUEb8Pr/tw2oK5U9BTGFWW5yCGnFYTITumfpNx6BRZ5xA4k98JXhvnAPhQ5+3PQsDxP4887PEkGNPpzKszbr8UkLzaT5t7Xx0gUKPqOUEdakKXwjbfG+gsbCTxmkSMvRV8dNs6PRLJgVUC3O1VKON75VH/0GN1GuoPTt4supg+xE5TiBfllnsay1Rq9efTVER53X+hvQl/si9A3iPi4v8+j8f3hK28l0BFyz/MSJ5BAW6DauPQwXfeeQgQAtiF0EGnpo8CCGovoMYfNDGvbn+vh3ric0m/fgVWhKANMx2Uh0/57jfyTDBcnl/zbuGqoWCsb7Nnq3YylrzEZRwXgJbSRF8vW3OG5cecUGEQMlx2Zf6k5Od5fi70TOBgghyMqO/HL+WQMYGgk0RXCHqHIG1UgBYdy1bQphGFfknbCMcrxxeIF30m9iJAufoNQX4/l6M+B0l3ZbEIjwBD6tJ/oHdIt84m1uLAMZzYg6wua4k5m0r9oFW0BiKyFRWarDQMy/sR0gTGK/4KCa9SdL+r+2EegpbXJk8b5wZqS/RyX0etkZCZxQ8Qcl687vwLXSXNUWOgRDgyH6JKiPgrIB6JX6Z/2N/zE+csRZlgsxYVSYQMhvabCmp T4I7kJSf fykaeGSy00uy2RGS+kfdeFG2uarIRkqlsMhXt/Kaj4XrG6nkCL5RwErvDzww1lb4bsHluEiJ/DK0S5XwT4YcgXhNaoZuQtsQNWPpUU/syhfkqMlhOnQXTRe6HrlKwU/9CgTcwoX/4KBNeprrU/+bfQiuvd6aKcksnCdihIqs3l7oFFZTjn5sszUT4LRhKTiLxc5WR/UY4zOWWRnCmvzycimqlbaLXSXGVtG+Obtuuc49gaIDD8k2KP0RiS/DH4IliWsNB3+VMwb4/5zhiW7vFsJMIU0ekazPcY+qdvvja5uLrQGoPTKJijneNar4OzrvI9b0oD28Vzx4N5ygvgwEFwtrJQZiyIAmixae1Mqid6ebxmbQ= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000110, 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, Mar 3, 2025 at 2:34=E2=80=AFPM Yosry Ahmed = wrote: > > On Mon, Mar 03, 2025 at 04:55:24PM -0500, Johannes Weiner wrote: > > On Mon, Mar 03, 2025 at 09:21:27PM +0000, Yosry Ahmed wrote: > > > On Mon, Mar 03, 2025 at 12:06:27PM -0800, Nhat Pham wrote: > > > > @@ -635,13 +652,11 @@ void swap_read_folio(struct folio *folio, str= uct swap_iocb **plug) > > > > } > > > > delayacct_swapin_start(); > > > > > > > > - if (swap_read_folio_zeromap(folio)) { > > > > - folio_unlock(folio); > > > > + if (swap_read_folio_zeromap(folio) !=3D -ENOENT) > > > > goto finish; > > > > > > I would split the zeromap change into a separate patch, but it's > > > probably fine either way. > > > > +1 > > > > > > @@ -1025,12 +1028,31 @@ static void zswap_decompress(struct zswap_e= ntry *entry, struct folio *folio) > > > > sg_init_table(&output, 1); > > > > sg_set_folio(&output, folio, PAGE_SIZE, 0); > > > > acomp_request_set_params(acomp_ctx->req, &input, &output, entry->= length, PAGE_SIZE); > > > > - BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &= acomp_ctx->wait)); > > > > - BUG_ON(acomp_ctx->req->dlen !=3D PAGE_SIZE); > > > > + decomp_ret =3D crypto_wait_req(crypto_acomp_decompress(acomp_ctx-= >req), &acomp_ctx->wait); > > > > + dlen =3D acomp_ctx->req->dlen; > > > > > > > > if (src !=3D acomp_ctx->buffer) > > > > zpool_unmap_handle(zpool, entry->handle); > > > > acomp_ctx_put_unlock(acomp_ctx); > > > > + > > > > + if (decomp_ret || dlen !=3D PAGE_SIZE) { > > > > + zswap_decompress_fail++; > > > > + pr_alert_ratelimited( > > > > + "decompression failed with returned value %d on z= swap entry with " > > > > > > nit: Decompression* > > > > > > I am also wondering how this looks like in dmesg? Is the line too lon= g > > > to be read? Should we add some line breaks (e.g. like > > > warn_sysctl_write()), we could probably also put this in a helper to > > > keep this function visually easy to follow. > > > > If it were more interwoven, I would agree. But it's only followed by > > the return true, false. Moving it out of line would need another name > > in the zswap namespace and also take an awkward amount of parameters, > > so IMO more taxing on the reader. > > My rationale was that no one reading zswap_decompress() will feel the nee= d > to read a function called zswap_warn_decompress_failure() in the error > path, so it will save people parsing this huge thing. I think Johannes' suggestion accomplishes a similar effect (see below). > > FWIW it would only need to take 3 parameters: decomp_ret, dlen, entry. > > > > > But maybe do if (!decomp_ret && dlen =3D=3D PAGE_SIZE) return true, and > > then save an indentation for the error part? I like this. It also moves the (much rarer) failure case to its own corner, which we can skip (most of the time). :) > > > > > > + "swap entry value %08lx, swap type %d, and swap o= ffset %lu. " > > > > + "compression algorithm is %s. compressed size is = %u bytes, and " > > > > + "decompressed size is %u bytes.\n", > > > > Any objections to shortening it and avoiding the line length issue? > > Even with \n's, this is still a lot of characters to dump 10x/5s. And > > it's not like the debug info is super useful to anyone but kernel > > developers, who in turn wouldn't have an issue interpreting this: No objection from my end. > > > > pr_alert_ratelimited("Decompression error from zswap (%d:%lu %s %u->%d)= \n", > > swptype, swpoffset, name, clen, dlen); > > Yeah this looks much more concise. It's a bit harder to parser the dmesg > as you have to cross check the code, but hopefully this is something > that people rarely have to do. > > I don't feel strongly about adding a helper in this case, unless we want > to add local variables (like Johannes did above), in which case a helper > would be a good way to hide them. That said, I'm not so sure about adding local variables here. We would be cluttering the code for a bunch of single-use variables, that are not even the "common" case. I mean, this seems fine to me? pr_alert_ratelimited("Decompression error from zswap (%d:%lu %s %u->%d)\n", swp_type(entry->swpentry), swp_offset(entry->swpentry), entry->pool->tfm_name, entry->length, dlen= ); (with proper indentation, but you get the idea).