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 2BE0938BF6E for ; Fri, 13 Mar 2026 23:54:10 +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=1773446050; cv=none; b=F3KVxsyDLlH7pdoW/NqVc3gNrjAz1BOCOZVmHQwU8wYcAH+Tvgk2IrxUTnpEpgXQTE8CysaCEGlUGKeY5XeAAQrd/ih548tqPfwNNGb6SKnY3GAvsLVup5JOZJU9dpJxvW84uuLgVMW2nh40O9f2dU4ySTMdv95USLMZtHk8m4Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773446050; c=relaxed/simple; bh=cTly7AG26jp+SrXfvTHxxDlweOt/bOL9lQQAROTKzzY=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=o9BXyhSw8AFnUHCk67oJ6Gox30hgGW6Zz4rq3UcZidZjNnDBSehqav5RMr786QpM2YOeRCmHeQShmXZ5uixeQheaMs675lz0iXtZ253cK24ZjjJMtmnFEi0JlcC8CqCnrcnYXHGaWa7jRqs+P8wBkcNHILfHpBFXHxyQuvLBrEE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=gMjQNj4A; 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="gMjQNj4A" Received: by smtp.kernel.org (Postfix) with ESMTPSA id EA04AC19421; Fri, 13 Mar 2026 23:54:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1773446050; bh=cTly7AG26jp+SrXfvTHxxDlweOt/bOL9lQQAROTKzzY=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=gMjQNj4AR7uNQ3ir0vdJFk6WFO5bymBVTITRM6/1xWzRkAAsCGCbX+/DlUQ9zOh61 pNKcGKJrz3apPg5wPdqDkDtxVf84pimWfGRacZPdrVEa6PLevyUV+y0psAry4trjyb loc1zjz2iOiYgPIB/mxB5f1hN746DCESjKU0+PT5xGCX74/hNnCiKO5whO9rA4rBOc F6u753gyMzDkr9+T3T6tfofeZ6cl639QrA9feIq5Ay9dW35HKkbg0VkW1Hl0IP4ffJ cozY1xkJAvHZOjAf1qcDzv5kz7sXpQkVAqNf64Wt3jotMRq/HsJVBW4VVQ4bDyH9Ac zTA7A4NzRem3Q== Message-ID: Date: Sat, 14 Mar 2026 07:54:05 +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> <2625a3fc-7dc6-4764-877c-ffeeeb42276c@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 2026/3/14 07:50, Daeho Jeong wrote: > On Fri, Mar 13, 2026 at 4:46 PM Chao Yu wrote: >> >> On 2026/3/14 00:21, Daeho Jeong wrote: >>> On Thu, Mar 12, 2026 at 11:49 PM Chao Yu wrote: >>>> >>>> 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? > > Then, I think we agree on this check is enough, right? I think so, let me know once you have any other founds. ;) Thanks, > >>> >>> Did you say AT_SSR can use a free segment? If we put this condition >>> here, AT_SSR will not use a free segment anymore. >> >> Sorry, I remember the wrong place we fallback to allocate a free segment, see >> get_atssr_segment() below, inside get_ssr_segment() we only search dirty >> segment/section, once it failed, we call new_curseg() to find a free one. >> >> 3083 static int get_atssr_segment(struct f2fs_sb_info *sbi, int type, >> 3084 int target_type, int alloc_mode, >> 3085 unsigned long long age) >> 3086 { >> 3087 struct curseg_info *curseg = CURSEG_I(sbi, type); >> 3088 int ret = 0; >> 3089 >> 3090 curseg->seg_type = target_type; >> 3091 >> 3092 if (get_ssr_segment(sbi, type, alloc_mode, age)) { >> 3093 struct seg_entry *se = get_seg_entry(sbi, curseg->next_segno); >> 3094 >> 3095 curseg->seg_type = se->type; >> 3096 ret = change_curseg(sbi, type); >> 3097 } else { >> 3098 /* allocate cold segment by default */ >> 3099 curseg->seg_type = CURSEG_COLD_DATA; >> 3100 ret = new_curseg(sbi, type, true); >> 3101 } >> 3102 stat_inc_seg_type(sbi, curseg); >> 3103 return ret; >> 3104 } >> >> IIUC, in f2fs_get_victim(), we should never expect to find a free segment from dirty >> bitmap, except for the checkpoint disabled case, that's what we need to fix, right? >> >> Thanks, >> >>> >>>> >>>>>> >>>>>>> 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) { >>>>>>>> >>>>>> >>>> >>