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 9515FC021B8 for ; Thu, 27 Feb 2025 00:03:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 132D7280005; Wed, 26 Feb 2025 19:03:56 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0E3A0280003; Wed, 26 Feb 2025 19:03:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EEE04280005; Wed, 26 Feb 2025 19:03:55 -0500 (EST) 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 D1EA7280003 for ; Wed, 26 Feb 2025 19:03:55 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 35770C0FA7 for ; Thu, 27 Feb 2025 00:03:55 +0000 (UTC) X-FDA: 83163776430.26.FE4CA9C Received: from out-180.mta1.migadu.com (out-180.mta1.migadu.com [95.215.58.180]) by imf05.hostedemail.com (Postfix) with ESMTP id 76672100004 for ; Thu, 27 Feb 2025 00:03:53 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="rZ/gTDFQ"; spf=pass (imf05.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.180 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740614633; a=rsa-sha256; cv=none; b=VvXLV3VSOipq+GdqwIbNcRb3aIBt8Ax7Ezqty5pT1xvnCmHrB1xB2SbxOCj2FPuhPvBeEK zTkxfMD1BbpmlAzwBxFFMH5FHPg4WUlWP8BDgg5sGfxhmd3BealgN1WC1TFCFw+tBPoSBf qjo4qbMFDHzOFhvBLXIvjA5/jpHKMLM= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b="rZ/gTDFQ"; spf=pass (imf05.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.180 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=1740614633; 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=le6kNlXVRfm086n/45QPAP2GootGePeFsgHvWj72AMU=; b=Z8q4BqCrvNpehttkBwW5xNtoX4uKj0MEPwq8WLF/AHrKB0Rpq/e6KcRaPhZLUNweApvwgc Tgyl92Ic5NfKteedV/O/dTsK1byOD5dlBoIXs9482UANpIZlhxtTx90uVnneiLNWsm/zHz DKAnQuoWUVXaNDvUBR6hZv9VXc7i3zY= Date: Thu, 27 Feb 2025 00:03:47 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740614631; 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=le6kNlXVRfm086n/45QPAP2GootGePeFsgHvWj72AMU=; b=rZ/gTDFQctKHR82tLw2MKwcl/WmrHvA79Q09RSXyT7u/W9CZoc2TZOgwm4cuno4xUxu8oi D6iZdOE3szQ0CbS8JSTXtusVY9zJrsGFnHiOW9SRtWozIhjM5SMRYhD4Ovbc0LstWkDyaP RvSORTY+BJBnZm01EicEVUtj/ZAn4WU= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: Nhat Pham Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, chengming.zhou@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] zswap: do not crash the kernel on decompression failure Message-ID: References: <20250225213200.729056-1-nphamcs@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 76672100004 X-Stat-Signature: o67y7okzzpjsuu46afmxwig8i3meyzir X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1740614633-637586 X-HE-Meta: U2FsdGVkX18mRPGL6qyd9523pYZtW/FHXcLlej0+k/bJJvEA5Jh7QAXJSENiho+iw0X/8wncsHcTYkexHUyv2NFAFd9gEK4CRsiwP2sGEkKymm7Xp6q0mCsFVuCQcOkPyAJgryyrQgV6PmgoQXpvGqOyqEXt7CNJhb6/VqfZ+6gVddaHdgD7omjYmB4/hnqX3FPMntMChR8meFKZk5nw5D8G+EwyVYqrx0d1eE2qdh+trRxyel+2/dwIEN9rlC1wSTQlzZ1W8n8MsD/ImME6gYbCPbfeE7jfaRjqhaSr0Q8s4F+A1K6/kIU2//wiXl6A/SoHod6wsUSGcyxNjhjPQokW/Jn1bQFx1XS8F1ZmiGzMrqzJEFrkGTGrG84otQdCT+24Bxj0ztCnswmLnjrwuwRjmm6K3AfVJ1ZCKaddPdMWTo3yUwBdakqekuNq6jl8a/uxjvKmhSR42T5XbjwIFkGNFwhReptiw5pVEuJgBrom+xHJCFEIzMfY+t+NQdiGALvIEbP8x6YQ7FJziaVaktzLJlIMAI9Rey896OtI2vrx3qfA7b+PFgz4WxxEvwCvwqSWgf9rGoSjGbA9jQrO8KgvKBAeL5fRUQ0DbCuNCBl31XcPClVd93MCeaU9uorBClTjvt8f284QeMmYVes5aN6/s4iJOFzL6e1tuJFxUDf4zkI5MBE1IBYJpOnj0hiGvoTVdh5vX4NTP1K69+/KQajxzpaaTRvzwkGOWPtvG+KpFkY4e+j3gHQzF6bqrxE5yWy6SpqXbRxVbWxApmPR5mJl4vz4Y+YUSdAlkc83FoJu/tz4+TVEt8GlA0owJE27yaCfmBkSkTh+0mVCV1hLyhTyIHQpTdJ0/NMPXQ556XqV2Cv735Xh40fXt9PzlIMrjyXf0O7/05mulAONh747DKoMBXUI1gkSJxhtQLn4Xt1Je6lAHdmHLaU1veRE0pTucO460pNeyavbQhHe0N7 yvu3m1U5 eTRwqUyCLtuxu5LeBUDupTWW6lg/jW2W0wiykmtTp+vn/J/yHjFjxU9P8m6fg/QKj7zArFJR/gXhDkmLABa/5XkF59zhtxPHZkcs/XiJR9i28pwYWaxHPy7xZFHZb9qXpaajIczSXJBaOiFutXBeegTKHKfHcD4vGhi8NcZ3ySpwNmLzVDmFyKgFdg323gM+A4vQwG+97bPE35LSaRgYalph6bh0dmEgveW2L0JTBEVwNr7vu8fgRZntHbKGOhJK8b6Dq 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: [..] > > zswap_decompress() > > { > > mutex_lock(); > > ... > > ret = crypto_wait_req(..); > > ... > > mutex_unlock(); > > ... > > if (ret || acomp_ctx->req->dlen != PAGE_SIZE) { > > Hmmmm, do we need the mutex to protect acomp_ctx->req->dlen? No strong > opinions here from my end TBH. We can copy the length to a local variable if that's the case. > > > /* SHIT */ > > return false; > > } > > return true; > > } > [...] > > > > We are doing load + erase here and in the writeback path now, so two > > xarray walks instead of one. How does this affect performance? We had a > > similar about the possiblity of doing a lockless xas_load() followed by > > a conditional xas_erase() for zswap_invalidate(): > > > > https://lore.kernel.org/lkml/20241018192525.95862-1-ryncsn@gmail.com/ > > > > Unfortunately it seems like we can't trivially do that unless we keep > > the tree locked, which we probably don't want to do throughout > > decompression. > > > > How crazy would it be to remove the entry from the tree and re-add it if > > compression fails? Does swapcache_prepare() provide sufficient > > protection for us to do this without anyone else looking at this entry > > (seems like it)? > > My concern is, what happens if xa_store() in the re-add path fails > because we do not have enough memory? Hmm good point. xa_erase() is essentially xas_store(NULL), but I think at some point if a node is made of entirely NULLs we'll free it, and it would be theoritically possible that we hit this case. Let's start by checking if the added xarray walk adds any noticable overhead and go from there. Ideally we want to test with a large (and sparse?) xarray to try and hit the worst case. > > > > > Anyway, this is all moot if the second walk is not noticeable from a > > perf perspective.