From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9EA611DED49 for ; Fri, 13 Mar 2026 06:49:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773384544; cv=none; b=DfCty6saRuvUwgBCh8TP99e+9yXRm2YvnZiV9ydByh1xbdmOgs2aicXi6NhBi2+5ufSMcPfomyFUxaFGfkp1VpK+LTxL46dYinPI2oEQkx15LQ8+VKNSXDkiWGQuOh4AY2J1grdVh7NOjGfAsiI0SZHxGw8gGkmi+aFERWliv0o= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773384544; c=relaxed/simple; bh=ern4CUk8uyBHb+LhhpcdpQtOYG1GRZivwnevMo0iwtU=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=ckgyLKHje21LGMvq9nLTgjVSt5uqsSCb5iuIsXKcO2g4W8rjFIj610ZbHesE0CwFiFrMmy0SvQjhCqsaDtAggecdkZOKDblU99B7Bk9fXjtKi7rruGYqll4mO1R1DEKWLW6IniqWbub1FWuXb6Yjl8nj68U1KmnfvL6TzbudVBM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=tmduMPgq; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="tmduMPgq" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 01614C19421; Fri, 13 Mar 2026 06:49:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773384544; bh=ern4CUk8uyBHb+LhhpcdpQtOYG1GRZivwnevMo0iwtU=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=tmduMPgq5jCRIzwLUv1A8IAg7q6GBQcPWq57Gsoz2AYFSWxak9QmncOnmLWLfcgI5 wwggMaYoRVUFkR4wB8bNdYyDZammGRINzPhFl2jjV0ovZ6ekniBluW33v0BYEsPi1A Nyfc6Hi2/m2rsibV8P/vlTNBPZegxjdsdS+HIr9ebETMgg1KnX7p03o48jnFa8P4G7 dI0wM0C9ud5WejZvgT8z6odqJiM9bWpKnkTQxH3MUtt6EO8Woks0dGlW6XpNM4kSiS Fd1BSKAoYQc2ITq9sDt14+yNzgZAP4ciIy5JOqP7GDaMgAuVcQfPDmEPndVNeoHt7q 0BFif87y75j0w== Message-ID: <2625a3fc-7dc6-4764-877c-ffeeeb42276c@kernel.org> Date: Fri, 13 Mar 2026 14:48:59 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com, Daeho Jeong Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim To: Daeho Jeong References: <20260310175428.1156719-1-daeho43@gmail.com> <615947f2-fe08-4875-87d9-baef36897e81@kernel.org> Content-Language: en-US From: Chao Yu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 3/12/2026 11:28 PM, Daeho Jeong wrote: > On Thu, Mar 12, 2026 at 2:07 AM Chao Yu wrote: >> >> On 2026/3/12 00:05, Daeho Jeong wrote: >>> On Wed, Mar 11, 2026 at 6:44 AM Chao Yu wrote: >>>> >>>> On 2026/3/11 01:54, Daeho Jeong wrote: >>>>> From: Daeho Jeong >>>>> >>>>> In age-based victim selection (ATGC, AT_SSR, or GC_CB), f2fs_get_victim >>>>> can encounter sections with zero valid blocks. This situation often >>>>> arises when checkpoint is disabled or due to race conditions between >>>>> SIT updates and dirty list management. >>>>> >>>>> In such cases, f2fs_get_section_mtime() returns INVALID_MTIME, which >>>>> subsequently triggers a fatal f2fs_bug_on(sbi, mtime == INVALID_MTIME) >>>>> in add_victim_entry() or get_cb_cost(). >>>>> >>>>> This patch adds a check in f2fs_get_victim's selection loop to skip >>>>> sections with no valid blocks. This prevents unnecessary age >>>>> calculations for empty sections and avoids the associated kernel panic. >>>>> This change also allows removing redundant checks in add_victim_entry(). >>>>> >>>>> Signed-off-by: Daeho Jeong >>>>> --- >>>>> fs/f2fs/gc.c | 9 +++------ >>>>> 1 file changed, 3 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c >>>>> index 2e0f67946914..981eac629fe9 100644 >>>>> --- a/fs/f2fs/gc.c >>>>> +++ b/fs/f2fs/gc.c >>>>> @@ -521,12 +521,6 @@ static void add_victim_entry(struct f2fs_sb_info *sbi, >>>>> struct sit_info *sit_i = SIT_I(sbi); >>>>> unsigned long long mtime = 0; >>>>> >>>>> - if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) { >>>>> - if (p->gc_mode == GC_AT && >>>>> - get_valid_blocks(sbi, segno, true) == 0) >>>>> - return; >>>>> - } >>>>> - >>>>> mtime = f2fs_get_section_mtime(sbi, segno); >>>>> f2fs_bug_on(sbi, mtime == INVALID_MTIME); >>>>> >>>>> @@ -889,6 +883,9 @@ int f2fs_get_victim(struct f2fs_sb_info *sbi, unsigned int *result, >>>>> if (sec_usage_check(sbi, secno)) >>>>> goto next; >>>>> >>>>> + if (!get_valid_blocks(sbi, segno, true)) >>>>> + goto next; >>>> >>>> Well, for f2fs_get_victim(, AT_SSR), once there are no dirty segment, if we >>>> don't count free segment as candidates, then, we can not find any valid victim? >>> >>> Oh, AT_SSR needs to select the free section in this case? >> >> I think so, for extreme case. Oh, check the code again, it seems we select victim from dirty bitmap, the victim should not be a free one... But there is some exceptions: locate_dirty_segment() if (valid_blocks == 0 && (!is_sbi_flag_set(sbi, SBI_CP_DISABLED) || ckpt_valid_blocks == usable_blocks)) { __locate_dirty_segment(sbi, segno, PRE); __remove_dirty_segment(sbi, segno, DIRTY); If valid_blocks equals to zero, but if the checkpoint is disabled and also ckpt_valid_blocks doesn't equals to usable_blocks. The segment (or section) will still be dirty state in dirty bitmap. We need to handle this correctly in f2fs_get_victim() correctly before calling into add_victim_entry() or get_gc_cost()? /* Don't touch checkpointed data */ if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) { if (p.alloc_mode == LFS) { /* * LFS is set to find source section during GC. * The victim should have no checkpointed data. */ if (get_ckpt_valid_blocks(sbi, segno, true)) goto next; } else { /* * SSR | AT_SSR are set to find target segment * for writes which can be full by checkpointed * and newly written blocks. */ if (!f2fs_segment_has_free_slot(sbi, segno)) goto next; } if (!get_valid_blocks(sbi, segno, true)) goto next; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Can this be the fix? >> >>> I am confused. Why do we need the below logic? >>> Looks like WA for the AT_SSR case? >>> >>> In f2fs_get_section_mtime() >>> out: >>> if (unlikely(mtime == INVALID_MTIME)) >>> mtime -= 1; >>> return mtime; >> >> There are two conditions, in a section: >> >> a) if there are no valid blocks, it will return INVALID_MTIME. >> b) if there are vaild blocks, it tries to return mtime which is calculated, but >> if unlucky the calculated mtime is equal to INVALID_MTIME, in order to distinguish >> from case a), we will return INVALID_MTIME - 1 instead. > > If we find a free segment and pass it to f2fs_get_section_mtime() for > (!__is_large_section(sbi)) case. > What is the expected output of it? (INVALID_MTIME - 1)? It depends on the status of section that free segment belong to: If there is no valid block in the section, it will return INVALID_MTIME, otherwise it will return calcuated mtime, or INVALID_MTIME - 1 for extreme case that mtime is just unluckily equals to INVALID_MTIME. Thanks, > I don't think this is just an unlucky case. Is this expected result? > >> >> Thanks, >> >>> >>> >>>> >>>> Thanks, >>>> >>>>> + >>>>> /* Don't touch checkpointed data */ >>>>> if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) { >>>>> if (p.alloc_mode == LFS) { >>>> >>