Received-SPF: pass (google.com: domain of linux-kernel+bounces-208655-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240610143037.812955-1-usamaarif642@gmail.com>
In-Reply-To: <20240610143037.812955-1-usamaarif642@gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Mon, 10 Jun 2024 10:31:36 -0700
Message-ID: <CAJD7tkYdTvfO8P+aZNmr7bF7vEetxiqQQ4ML8BcLdmKohT-+Cg@mail.gmail.com>
Subject: Re: [PATCH] mm: Do not start/end writeback for pages stored in zswap
To: Usama Arif <usamaarif642@gmail.com>
Cc: akpm@linux-foundation.org, willy@infradead.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

On Mon, Jun 10, 2024 at 7:31=E2=80=AFAM Usama Arif <usamaarif642@gmail.com>=
 wrote:
>
> start/end writeback combination incorrectly increments NR_WRITTEN
> counter, eventhough the pages aren't written to disk. Pages successfully
> stored in zswap should just unlock folio and return from writepage.
>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
>  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 writebac=
k_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 s=
afe.

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.

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?

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 :)


>                 return 0;
>         }
>         if (!mem_cgroup_zswap_writeback_enabled(folio_memcg(folio))) {
> --
> 2.43.0
>