Received-SPF: pass (google.com: domain of linux-kernel+bounces-13308-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20231024142706.195517-1-hezhongkun.hzk@bytedance.com>
In-Reply-To: <20231024142706.195517-1-hezhongkun.hzk@bytedance.com>
From: Nhat Pham <nphamcs@gmail.com>
Date: Fri, 29 Dec 2023 18:09:07 -0800
Message-ID: <CAKEwX=OiNB+pPhb-3Tf7O=F7psKE3EOpwmbPSeLSOyuHpj3i+Q@mail.gmail.com>
Subject: Re: [PATCH] mm: zswap: fix the lack of page lru flag in zswap_writeback_entry
To: Zhongkun He <hezhongkun.hzk@bytedance.com>
Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, yosryahmed@google.com, 
	sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, 
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Oct 24, 2023 at 7:27=E2=80=AFAM Zhongkun He
<hezhongkun.hzk@bytedance.com> wrote:
>

My apologies for the delayed response. I have a couple of questions.

> The zswap_writeback_entry() will add a page to the swap cache, decompress
> the entry data into the page, and issue a bio write to write the page bac=
k
> to the swap device. Move the page to the tail of lru list  through
> SetPageReclaim(page) and folio_rotate_reclaimable().
>
> Currently, about half of the pages will fail to move to the tail of lru

May I ask what's the downstream effect of this? i.e so what if it
fails? And yes, as Andrew pointed out, it'd be nice if the patch
changelog spells out any observable or measurable change from the
user's POV.

> list because there is no LRU flag in page which is not in the LRU list bu=
t
> the cpu_fbatches. So fix it.

This sentence is a bit confusing to me. Does this mean the page
currently being processed for writeback is not in the LRU list
(!PageLRU(page)), but IN one of the cpu folio batches? Which makes
folio_rotate_reclaimable() fails on this page later on in the
_swap_writepage() path? (hence the necessity of lru_add_drain()?)

Let me know if I'm misunderstanding the intention of this patch. I
know it's a bit pedantic, but spelling things out (ideally in the
changelog itself) will help the reviewers, as well as future
contributors who want to study the codebase and make changes to it.

>
> Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>

Thanks and look forward to your response,
Nhat

P/S: Have a nice holiday season and happy new year!