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 3CCB2C021B8 for ; Wed, 26 Feb 2025 15:42:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D2623280011; Wed, 26 Feb 2025 10:42:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CD68428000F; Wed, 26 Feb 2025 10:42:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B9DEC280011; Wed, 26 Feb 2025 10:42:11 -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 9BF5F28000F for ; Wed, 26 Feb 2025 10:42:11 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 37386161756 for ; Wed, 26 Feb 2025 15:42:11 +0000 (UTC) X-FDA: 83162512062.08.74AFC29 Received: from out-189.mta0.migadu.com (out-189.mta0.migadu.com [91.218.175.189]) by imf25.hostedemail.com (Postfix) with ESMTP id 6C14BA001E for ; Wed, 26 Feb 2025 15:42:08 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Y92Tuumh; spf=pass (imf25.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.189 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=1740584529; 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=G0ka5+57U6hHrywsA4GiXl4SPgDlHG+Ia84OVSI5zrg=; b=LSduCy9HOgJ77W00wYm2F3J1CUy9IyX7cmssy8bUINlo4BvfOnMw270UH+2jbWpCcA+RG7 fvOxEMjamh/z03BPmZEvILJewAVFljau/PTjWWxzaNZ0ffW8moi68lmMSzKNfrVaNMN21U a66Wt5ilszQsMGO+GK8vRAAyC/KOXjw= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Y92Tuumh; spf=pass (imf25.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.189 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=1740584529; a=rsa-sha256; cv=none; b=daqTOP4PdV5bwBWzPfYXhsLk5KDeCOLDXg0YI58bZgSeZkXsVN7+Lk3lppJ+k4KaNHpTz2 XPhURKNQ3DAMNkyrSOUL5cW/u08hC+A4+oPz7cldAbQkjeLgSsm7cYS4xtqP/VFnmLY5wB //taEaJkVBvw+yBIs/x1Q4mutuAnboU= Date: Wed, 26 Feb 2025 15:42:00 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740584525; 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=G0ka5+57U6hHrywsA4GiXl4SPgDlHG+Ia84OVSI5zrg=; b=Y92TuumhhX48sxCcHAtjxEUE7+wn9akk6NdxpMBZJJQsRa2Iy/RzGC+H5P165D76CAHDX5 DnFw39WGfHZ9cBqyX3lD2rUlsB4zi9iWbAP89TxnMojgduqiPwtqjvR5Fc3jZhS6wd5wYt fGHMPDavMUJaD9WK+vXJ/G//pOYAJOk= 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] zswap: do not crash the kernel on decompression failure Message-ID: References: <20250225213200.729056-1-nphamcs@gmail.com> <20250226005149.GA1500140@cmpxchg.org> <20250226035730.GA1775487@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250226035730.GA1775487@cmpxchg.org> X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 6C14BA001E X-Stat-Signature: 3xrjc4r7cs11pz4irpibdyk1o1brrpnr X-Rspam-User: X-HE-Tag: 1740584528-893486 X-HE-Meta: U2FsdGVkX18fEy05Srj4oY7W0wGS9BVdyZ+uVGqAJuwzGpJHGdMblqhRyFIByvB/UxCQ4kb8C/tFRgkzaYit8/wCF67tzZbLMK1RrkLmwKFudptHBl2MTt0mhLEd/vkY5HbsX7rFrlv3xSV/itvh6LXbUTGfPSxeG6CbbXtQ0VQDlhMnM2yN/IQzn3HyjtoRweMjAWtBBDmeWI/qgGLpu1wcgn9JhNk8sB7PvQ/ByLlnJN/JAv+GeaYHBKFJWm8AJdXAKdsiH4XjS01+2syupOqwEUvCiEImHNZzU8K+8ElBJhcihMleFZG68H4uN9md9X0drT68Kr1LAfT9xw+Vnbxbc0xZ7dsDctBz1d8bVaEXAUxmR2wQvk/YJI85EHOaOdDYPVVrJeFFL4ldLuUlGwxxT5R9vNW/Rj5ML3QGKjaACkaytzQKgI1THcwb6uq+EiVFQFmEflTRF7tfUlGN77ScNFdephl6pbyzSMugvujXo0DQ9j9jFZl3vwlUtqnsoBOLohHF9Mk3yEzrAIQVLz/fUTGHWOlTKqSttqxA15I7SbwX4QdOy1IADEOiMXqeBG4/EQu7bPVkKssowhKVblISwLto6SEMSMC9VA5ztKuhYVkiiHBx136rvhVAwRP+/C04IprVUfubTaAvXePSBSwZ5zfP+AeF75BS0jg8OOk6Bd7pPxBe23OJGhOdWJJzAOTDUQfXZPzX9ZWBH73JyOig6Pdh2Ajn0FhJP1+pynlB2x0G6FiQY1gqN8h56A0gMfntxEA43q54eWv2NB6oq2K5IN5easpcmkpWGvnGh5QtBhpchAIVghCfjqNdtR4kTMZ/ss7JvU8LsCFkGkyYJlh8LfpBEB7ZdP1s1Me8gRPEmxGJwQy3fdgdF1QxMf8YLUKf3Exc6810LAqPQSvleG0xtCr+PUhRJN8cbq1+r4i2BMUowmvcnCDxQcHxMaECnN8d9egbalCo4t/9gxd SFHeAm75 Lo7+F7qOfLLY/RjI2kRi7dCVUko7EBHRdJ6UVIgo80amDief8/oJFxzgwMwGQ/u6ox/nyyzfT5w52UovvnKMJjwUCH638pMpVNXhurK5GvUTFZqdcJ+M0Ar6ugQVpc/NhDIOK6/UgtCT2Etv0nIVaDg+mgN98J3pBbHge9Qc0bJE8t3F2IaLSoh2Qa5yoIqTJ0Q7F3+yOY5hqSvb5NYILxMfWBw== 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 Tue, Feb 25, 2025 at 10:57:30PM -0500, Johannes Weiner wrote: > On Wed, Feb 26, 2025 at 02:45:41AM +0000, Yosry Ahmed wrote: > > On Tue, Feb 25, 2025 at 07:51:49PM -0500, Johannes Weiner wrote: > > > On Tue, Feb 25, 2025 at 01:32:00PM -0800, Nhat Pham wrote: > > > > + } > > > > mutex_unlock(&acomp_ctx->mutex); > > > > > > > > if (src != acomp_ctx->buffer) > > > > zpool_unmap_handle(zpool, entry->handle); > > > > + return ret; > > > > } > > > > > > > > /********************************* > > > > @@ -1018,6 +1028,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > > struct writeback_control wbc = { > > > > .sync_mode = WB_SYNC_NONE, > > > > }; > > > > + int ret = 0; > > > > > > > > /* try to allocate swap cache folio */ > > > > mpol = get_task_policy(current); > > > > @@ -1034,8 +1045,8 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > > * and freed when invalidated by the concurrent shrinker anyway. > > > > */ > > > > if (!folio_was_allocated) { > > > > - folio_put(folio); > > > > - return -EEXIST; > > > > + ret = -EEXIST; > > > > + goto put_folio; > > > > } > > > > > > > > /* > > > > @@ -1048,14 +1059,17 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > > * be dereferenced. > > > > */ > > > > tree = swap_zswap_tree(swpentry); > > > > - if (entry != xa_cmpxchg(tree, offset, entry, NULL, GFP_KERNEL)) { > > > > - delete_from_swap_cache(folio); > > > > - folio_unlock(folio); > > > > - folio_put(folio); > > > > - return -ENOMEM; > > > > + if (entry != xa_load(tree, offset)) { > > > > + ret = -ENOMEM; > > > > + goto fail; > > > > } > > > > > > > > - zswap_decompress(entry, folio); > > > > + if (!zswap_decompress(entry, folio)) { > > > > + ret = -EIO; > > > > + goto fail; > > > > + } > > > > + > > > > + xa_erase(tree, offset); > > > > > > > > count_vm_event(ZSWPWB); > > > > if (entry->objcg) > > > > @@ -1071,9 +1085,14 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > > > > > > /* start writeback */ > > > > __swap_writepage(folio, &wbc); > > > > - folio_put(folio); > > > > + goto put_folio; > > > > > > > > - return 0; > > > > +fail: > > > > + delete_from_swap_cache(folio); > > > > + folio_unlock(folio); > > > > +put_folio: > > > > + folio_put(folio); > > > > + return ret; > > > > } > > > > > > Nice, yeah it's time for factoring out the error unwinding. If you > > > write it like this, you can save a jump in the main sequence: > > > > > > __swap_writepage(folio, &wbc); > > > ret = 0; > > > put: > > > folio_put(folio); > > > return ret; > > > delete_unlock: > > > > (I like how you sneaked the label rename in here, I didn't like 'fail' > > either :P) > > > > > delete_from_swap_cache(folio); > > > folio_unlock(folio); > > > goto put; > > > > I would go even further and avoid gotos completely (and make it super > > clear what gets executed in the normal path vs the failure path): > > > > __swap_writepage(folio, &wbc); > > folio_put(folio); > > if (ret) { > > delete_from_swap_cache(folio); > > folio_unlock(folio); > > } > > return ret; > > The !folio_was_allocated case only needs the put. I guess that could > stay open-coded. We also do: if (ret && ret != -EEXIST) { ... } or if (ret && !folio_was_allocated) { ... } Probably the latter is clearer. If the folio was already there, we didn't add it to the swapcache or lock it ourselves so we don't unwind that. > > And I think you still need one goto for the other two error legs to > jump past the __swap_writepage. Oh yeah I meant eliminate the jumps within the cleanup/return code, not entirely. Sorry for the confusion. We still need an 'out' label or similar after __swap_writepage(). > > > > Something like this? > > > > > > if (!zswap_decompress(entry, folio)) { > > > /* > > > * The zswap_load() return value doesn't indicate success or > > > * failure, but whether zswap owns the swapped out contents. > > > * This MUST return true here, otherwise swap_readpage() will > > > * read garbage from the backend. > > > * > > > * Success is signaled by marking the folio uptodate. > > > */ > > > > We use the same trick in the folio_test_large() branch, so maybe this > > should be moved to above the function definition. Then we can perhaps > > refer to it in places where we return true wihout setting uptodate for > > added clarity if needed. > > That makes sense to me. Nhat, what do you think?