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 5B729C27C55 for ; Mon, 10 Jun 2024 18:30:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C4FF96B0095; Mon, 10 Jun 2024 14:30:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C00E96B009B; Mon, 10 Jun 2024 14:30:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AC7536B009C; Mon, 10 Jun 2024 14:30:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 8E3E56B0095 for ; Mon, 10 Jun 2024 14:30:06 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 951331408A3 for ; Mon, 10 Jun 2024 18:30:05 +0000 (UTC) X-FDA: 82215818370.12.7407D4E Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com [209.85.208.180]) by imf26.hostedemail.com (Postfix) with ESMTP id 9F58814000E for ; Mon, 10 Jun 2024 18:30:03 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=DHyeKGt8; spf=pass (imf26.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.180 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=1718044203; 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=jzeYDX53YhzVDP5G9Sqj56BiX8iqkARqJWojqclpCyw=; b=V45RaOpKlVwfolA4NY480vcmbrr2ERoG8dsi5tljhSAOqpslDoZBtznJwpR7U4POFGNVkM 27N6Yb84k2bgii87t2UpoDmo65pVg4FGSp7fm3ob8rjvK7+tBeKjp3J4vBrTu/WF9rv+a7 ay+AmMsYqOojAWiWCw+vVDt3/omW/6k= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718044203; a=rsa-sha256; cv=none; b=DJvFSgs0Mc/t9P1kzsw5rsWncOpOSeK5RbFy7cM3NNi4wdjqrLed88wmEyHsfLoBvUVJKh IyZwjJm8IbxdNEH4U0+hFzeS99jKq+vwIMBaRI9VLESjOXZ/OL+zvWMFPCYx6m5jwZ9CWx VlmcuS9HTsxN6qXAMwo5LjxXVvQsOu0= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=DHyeKGt8; spf=pass (imf26.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.180 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-lj1-f180.google.com with SMTP id 38308e7fff4ca-2eaea28868dso40861151fa.3 for ; Mon, 10 Jun 2024 11:30:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1718044202; x=1718649002; 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=jzeYDX53YhzVDP5G9Sqj56BiX8iqkARqJWojqclpCyw=; b=DHyeKGt8BDell1d/jCdQBY4o3j9YAIhFn4hviBsVLNq/d4BjJJQVTKGUKqfRAu94VF NLVxsQ9ZeQ4Vx0Xx8ozKIObp17Nxe/SKEicKUnDdf7Y4LhxSQc/6JjQbxi2XEyNykABZ oRlZfJFrR+axUgMta4P1WJfXBVFN6gRSrpgInNy1wWsHamnXn8jBO9sqfmzuxZp2CnOU EFMYMVDlZ2T8je/6I22iKhxxozAMZ6SUvcOXclimpjz5vNxWzhSbRFBDjFkbHXkpBbgS Fc/6DAMAikm/vWce1WR/mBMQaoIrO0IBCFOeuw/ybnT+k6/Df5pbNSl0uweShY1YyvJS 4aEQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718044202; x=1718649002; 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=jzeYDX53YhzVDP5G9Sqj56BiX8iqkARqJWojqclpCyw=; b=THlGcYBUTW21uUDFqGhHXK9hKRYDHDD2vySp1mdIi9YF0de3n63c8l4kSoDwVprdkq Z1TTdO4XOAO1yVKJz73iQZ+CM+1f1HZ9lViEv5+4S2E9bHoWHG2+Gpn+OU6yWZSbSqh8 R98LGg+B2A+lh7y9K4gYOHZQ9H8R9kn6q5v3yBMSEC+2n6C0mrccdzpbPa/g8KgIomUP d9AWkZVyoTrN82jTZakPK+YXcaqoUNi9QWgooh4yKmVpBkbqYQlbrmFacBrKJjpni+m/ 3GBLTMvAL0eVPgH7Qe+dpbrJ4VhgkP+ViWpN96fdIfzXbXJO7tXeIaBXz1AEXpagAQfU xZQA== X-Forwarded-Encrypted: i=1; AJvYcCVNRd1W3XIw7QtFOdMW1YwD/PiivGQJdQuGb9bliM5cY96ia2zSwTz6u1VlGGXEJ2jw3+GQcMGcP7ZE0G3uXttOCr8= X-Gm-Message-State: AOJu0YzAfi1vR1nd9d2laXnnfE0qLmWCdwn2CDsQ6xxzDfWZzuocjuPL wFVVd5oAXd3slmEFz7O01rh4vBAGxCISRu1criALtV/Dk/Tvc1MHKh+DJnINnlxH3U5STFaJlRw aCl4aQZ80gYpTgRvL8x6YVj3S7VZ/vS6T+Hj+ X-Google-Smtp-Source: AGHT+IGmh8GUrcfNd9P4PJNsnOpWrRuzEdCySgfMDkRVKMBnetTsvdx4DVoBEuWzusjxpM/QJjEPz0RZgPZRkv9MyDM= X-Received: by 2002:a2e:a7c8:0:b0:2eb:e505:ebd6 with SMTP id 38308e7fff4ca-2ebe505ee6bmr41847781fa.0.1718044201601; Mon, 10 Jun 2024 11:30:01 -0700 (PDT) MIME-Version: 1.0 References: <20240610143037.812955-1-usamaarif642@gmail.com> In-Reply-To: From: Yosry Ahmed Date: Mon, 10 Jun 2024 11:29:25 -0700 Message-ID: Subject: Re: [PATCH] mm: Do not start/end writeback for pages stored in zswap To: Usama Arif Cc: willy@infradead.org, akpm@linux-foundation.org, hannes@cmpxchg.org, nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 9F58814000E X-Rspam-User: X-Stat-Signature: b7asetnsmpims8wg3c58qetq1x78snzm X-HE-Tag: 1718044203-533444 X-HE-Meta: U2FsdGVkX1/v+0+uSObpZ4L3RuaOmuUnia0epwU8ZTYFzc56Jg3fTchFbVbOE9d0dNTs+pAL2ZK0trG8gTCNb39in6c75J7U++MixPg3mM358GFw7shWa3v7qdS5Frr2/gmPx4XiGwaeHwQcrPs07Eh4Eo1TWnKn7UNbPF3HBRYV1evdDCY8kSymDSJjO/V6cm96V2NkB5mFC6bmNlz0Oq79CBXn8dERhXDBJAYCQBaUTzJKvsRVwAw/jCEvIKSykIjOg4IacMe1rOdGh6J6aIgRN1zzKbS4rtnyYO4wNFRNFCq41oBbOoeTVEKBPhQtsx+ClzNarV84+fYTpOeByhDB7t/bdpinU05WF0XSz59xVMnfLE3+zrlmk3E0K8+UEbpWoSnd+BZRJaUeFv2yMm5JuHAY7WOxHcf0r0tceCc7t0A/TGmFce+J4JJPZii+fchN5ThzEjCgEZzr1XUgpIBfa1atYxtOxwbZY1AFIW5lcJH+EC8g9xDzDbvLqpyT1dqACEPRYTj6HXvqoAgRzRcVIMyu1ca+l0JWIJC3N4v2f2aVQr76IRT7NqboyzdHcVNleFpLXLfkAZyK1X9oYjKwfizuCys45BUdAD1nALS8OxH9OQ7Y9njvVPdTB+wzymAnCa+qBS94N7GO3YztzVKnGV0gcKVd0JL029agbSm0+MLxU1DQt0e7mSszrWg0ajjpRXr3HFCnK26/HZOnY08Emj6/ive6UncZHBHBU8IrNxaqKUfAZzqN6AKZywq2tMSLIhrWU+uLiIVnG/MKHavg0Qlwfyq0XAgejFaMImdhQ6dYfeWeJp4bQZrkHuw/cJ0GIdAj8Y9Hm3N4pNGfnrzCc5dht2k0yX8bEFySNO3+m8w5oqrOHi00IDGgYso668Wda+VgK8wDidhMd7Bx4+oPn1btqVlXJfujRnPVtSHAbJMC1q4lcRF+y8Krx6TC6lzK+sObgaahxJSlzFc ndOymeHj fljAKOEGwVuJYghiSvolhfr6oOnriD4IapWG1HycqWY1k/ITzoTw21xxtCTzxIfwu3nvi71USj/I2n5fuj1Eexh5EiC6NrdabLA09cgg59uRyPTNjPA2ZjG5Dn4t7fSTjo6v8RV81gU4oOZzEyY1dchisc7XJ5jf8NmeRaFtCu9/ipjX2g0flTudVyv7ro2JsxIFZ6Dm0busC9VCvxYG2N3AbZA7uByNG0nz3mWgYHUPHAD3X8Y1JQUhzHvPrOCJhO04wXAqE8gU0YQAF8hnhctJRZn5nvJroyS/LZX14avNe9nf1xN2dI3oGupARzpkVW9nSHRkU7hK0bTD9spFcabEcPj30RyETjuuc3WcEvfZo5lE8lzEn2wZaviU2UyPzQP+bxBravpDiIMQuBdpBf2g+U85fyuEiDX/2orH0iAA44yU= 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 11:11=E2=80=AFAM Usama Arif wrote: > > > On 10/06/2024 18:31, Yosry Ahmed wrote: > > On Mon, Jun 10, 2024 at 7:31=E2=80=AFAM Usama Arif wrote: > >> start/end writeback combination incorrectly increments NR_WRITTEN > >> counter, eventhough the pages aren't written to disk. Pages successful= ly > >> stored in zswap should just unlock folio and return from writepage. > >> > >> Signed-off-by: Usama Arif > >> --- > >> mm/page_io.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/mm/page_io.c b/mm/page_io.c > >> index a360857cf75d..501784d79977 100644 > >> --- a/mm/page_io.c > >> +++ b/mm/page_io.c > >> @@ -196,9 +196,7 @@ int swap_writepage(struct page *page, struct write= back_control *wbc) > >> return ret; > >> } > >> if (zswap_store(folio)) { > >> - folio_start_writeback(folio); > >> folio_unlock(folio); > >> - folio_end_writeback(folio); > > Removing these calls will have several effects, I am not really sure it= 's safe. > > > > 1. As you note in the commit log, NR_WRITTEN stats (and apparently > > others) will no longer be updated. While this may make sense, it's a > > user-visible change. I am not sure if anyone relies on this. > > Thanks for the review. > > This patch would correct NR_WRITTEN stat, so I think its a good thing? > But yeah as you said for people relying on that stat, suddenly this > number would be lowered if they pick up a kernel with this patch, not > sure how such changes would be dealt with in the kernel. > > > 2. folio_end_writeback() calls folio_rotate_reclaimable() after > > writeback completes to put a folio that has been marked with > > PG_reclaim at the tail of the LRU, to be reclaimed first next time. Do > > we get this call through other paths now? > > We could add > > if (folio_test_reclaim(folio)) { > folio_clear_reclaim(folio); > folio_rotate_reclaimable(folio); > } > > to if zswap_store is successful to fix this? > > > 3. If I remember correctly, there was some sort of state machine where > > folios go from dirty to writeback to clean. I am not sure what happens > > if we take the writeback phase out of the equation. > > > > Overall, the change seems like it will special case the folios written > > to zswap vs. to disk further, and we may end up missing important > > things (like folio_rotate_reclaimable()). I would like to see a much > > stronger argument for why it is safe and justified tbh :) > > > The patch came about from zero page swap optimization series > (https://lore.kernel.org/all/ZmcITDhdBzUGEHuY@casper.infradead.org/), > where Matthew pointed out that NR_WRITTEN would be wrong with the way I > was doing it. > > Overall, I thought it would be good to have consistency with how > zeropages and zswap pages would be dealt with, and have a more correct > NR_WRITTEN stat. > > In the next revision of the zero page patch, I will just add > folio_rotate_reclaimable after folio_unlock if folio is zero filled. I would wait until others weigh in here. I am not sure we can just change the stat handling from under the users, even if it is the right thing to do :/ I also think we need more analysis before we decide it's safe to remove the writeback calls otherwise. I noticed folio_rotate_reclaimable() on a quick look, but there may be other problems. I am not very familiar with the dirty -> writeback -> clean state machine. What's the benefit of this patch beyond making the code (and stats) make more sense semantically? It feels like a significant risk with little reward to me.