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 B9430C282CD for ; Mon, 3 Mar 2025 22:34:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C4A776B0089; Mon, 3 Mar 2025 17:34:56 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BF8F2280001; Mon, 3 Mar 2025 17:34:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id ACA606B008C; Mon, 3 Mar 2025 17:34:56 -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 8E8A56B0089 for ; Mon, 3 Mar 2025 17:34:56 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 18541121086 for ; Mon, 3 Mar 2025 22:34:56 +0000 (UTC) X-FDA: 83181696192.21.402D44E Received: from out-171.mta1.migadu.com (out-171.mta1.migadu.com [95.215.58.171]) by imf27.hostedemail.com (Postfix) with ESMTP id 2F18840003 for ; Mon, 3 Mar 2025 22:34:53 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=vgvMHXNW; spf=pass (imf27.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.171 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741041294; 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=9L92r6X7Eg96asDUmcRcp3aZhjsIld49Y1+1SOSF1iI=; b=l0gt7wSDWdZAej3QsDP11w46g9FhRtlCLJEVdEAU9DhIycSVEbF75v3Wcjm4Ru5tM7d9EH HCsxOVTMQUFafLiH4w8bt3NPdx9fD3mUeFSBNi1BSAKS7MxHxkNYEiNFTGke1TUZ2Vck43 8xvW6vdhYmqZvun3Vy8a/punEiMVog8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741041294; a=rsa-sha256; cv=none; b=1MRH+sJfoesjWz2hgoW+uJ2SXlkU2UPr2EPfYOKB952ujRW1isivhgPVBtR3sM/Ey1RM9t CA8nsjvXgNrY+1fDQkdoW2nZd7HUhgxvtFvHGGQqOGk9DaKLKB7kSW8375IMSTfmWhnXDi oqIasUo4qehs5i+q1iUQyrzHqX9vs8E= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=vgvMHXNW; spf=pass (imf27.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.171 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev Date: Mon, 3 Mar 2025 22:34:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1741041292; 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=9L92r6X7Eg96asDUmcRcp3aZhjsIld49Y1+1SOSF1iI=; b=vgvMHXNWl6+D+INZwx3XRAEmwDjUiLF96Tki7+GGtO+iZdEVhR3zEiGmnJSNK+l4wkyKoL zTSIOOqFLAVLmfX2x8xaPjJc33u5yHxfZV3sdXMK0eDE8Uy46pqoHYA995Q0DGH6/M3acb MKVG781br9YG2v0vjVEyPDMbq+os+ik= 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 v3] page_io: zswap: do not crash the kernel on decompression failure Message-ID: References: <20250303200627.2102890-1-nphamcs@gmail.com> <20250303215524.GD120597@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250303215524.GD120597@cmpxchg.org> X-Migadu-Flow: FLOW_OUT X-Stat-Signature: oh6u699uzg9ixu4ayydiekykq4bkkf9i X-Rspamd-Queue-Id: 2F18840003 X-Rspam-User: X-Rspamd-Server: rspam01 X-HE-Tag: 1741041293-426213 X-HE-Meta: U2FsdGVkX1/ry3/wQf2Xow+zY4qnKQnnRJMf3JceO6jmyvgyU+IAcmPpN/jNM0X7bO0QMwdFb4Hmxam/4y+ckgYbbHiGpTYejJZ8GOxZ8OsIbRBZBMe8T5sKP4/Av10CCCiQFKqkAfgdl+sMibbNXPQy630lJiVfz1ps8YK4KqxY45pDSbulMlkm3gKJEpdnHejeqEKBisPnN1bSJCtzT6co0iI9fETri1U8PVanvHhOfMm53XkOWN4F6MFyuCB5YVLomwb7EiSjM8Ah2E2tc/y1tm4dILkyWqUzPeP5y1D+U1i8+0ytHSv3lCS9bvoYc5tRgQZL6NWApm4jX8iXd5FO5i0Vj031PLbAAniGlBH+m5pX3EsNmBnMDPVCId8592ghR3BFDgN+9PA2AHZvp6ujdiI+SXv6NHdC5zm/qyNIzWYwVOM+d60D86M3LleMoxQN1FvMFrTwHI3l+h7EB9N41rbHp5JkxNPHOuR6mvY874uIOul1S76nx+t9o84/wguBoyUmoIfv9YeryVQGqkASEBw0xYY7Ghb8AS5PhLXUrrb6+6FPNmH6/p6iLERfkAmWt/NhFrKTBORNm1Jw6Kz6RqD+eUWIKXbLwZ7QMx18p7pUAWPAWVV2YZONqR4GNc7DK5szR2yzbbKMATCwKlcVBvAlMuQD2LBOozXAbMDp6Uv7vxBGSKG37Xbxji6A6a/rVu1vDCW1wSefnRWv5OrPDlX3fRQmqfSjYs3FKwG37i8NWD/Pr6ngD4uzcvhrbQE9e7pEzOLrqWkbnbo4J/QpE6bdUuoTe3Dy1CLZzJGcoyEppnnATAe8fnlJtm/88965E4hWRx8wb+NoomxzDJ6q7dNVeMmVJCEWBI+OPt2XmbWvc7zbZl1Y6g+S6uyJOVJFNWLAomNyP8fQk0B87FUtnu1tKnfxFcc5zKnDj6tI0Bba2Kvyetdt5nfsS4vLHxsiLpDc1rQb4c2dKws Q89MgJcZ GCEgLfzS8kGr/Z6TmLqClqT7M1ypU83Xb5+6j/btOuAc3hYg72S7VlED+cRcOMbAEgju59XA3XJ9k76//YgcmFyPPiV1epQTe197gxq9DOmqbPwUhKk2ZFpAGqmqox3IA54PqOzyFzs3gDfAAHVDUA5meMiMWi7qroxU7cv3eCjxIuyyrYkCRakwY450P4Mj48tbydw/XZpTtd+GHYItKbTnZ6NL1NlB1GTiYE9gbiovahFWj1VxBB7g0FSv3LGsS8KehmFj4ojZU55Q= 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, 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, struct swap_iocb **plug) > > > } > > > delayacct_swapin_start(); > > > > > > - if (swap_read_folio_zeromap(folio)) { > > > - folio_unlock(folio); > > > + if (swap_read_folio_zeromap(folio) != -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_entry *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 != PAGE_SIZE); > > > + decomp_ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); > > > + dlen = acomp_ctx->req->dlen; > > > > > > if (src != acomp_ctx->buffer) > > > zpool_unmap_handle(zpool, entry->handle); > > > acomp_ctx_put_unlock(acomp_ctx); > > > + > > > + if (decomp_ret || dlen != PAGE_SIZE) { > > > + zswap_decompress_fail++; > > > + pr_alert_ratelimited( > > > + "decompression failed with returned value %d on zswap entry with " > > > > nit: Decompression* > > > > I am also wondering how this looks like in dmesg? Is the line too long > > 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 need to read a function called zswap_warn_decompress_failure() in the error path, so it will save people parsing this huge thing. FWIW it would only need to take 3 parameters: decomp_ret, dlen, entry. > > But maybe do if (!decomp_ret && dlen == PAGE_SIZE) return true, and > then save an indentation for the error part? > > > > + "swap entry value %08lx, swap type %d, and swap offset %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: > > 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. > > > > + decomp_ret, > > > + entry->swpentry.val, > > > + swp_type(entry->swpentry), > > > + swp_offset(entry->swpentry), > > > + entry->pool->tfm_name, > > > + entry->length, > > > + acomp_ctx->req->dlen);