Re: [f2fs-dev] (2) [PATCH v2 2/2] f2fs: introduce gcless mount option to avoid foreground GC

To: Chao Yu <[email protected]>
Cc: [email protected],
    [email protected],
    [email protected],
    [email protected],
    [email protected],
    Dongjin Kim <[email protected]>,
    Daejun Park <[email protected]>
Subject: Re: [PATCH v2 2/2] f2fs: introduce gcless mount option to avoid foreground GC
In-Reply-To: <[email protected]>
References: <20260707225453epcms2p4f8323f4f8b88f3ee892cddc126da51a6@epcms2p4>
 <[email protected]>

Hi Chao,

No problem at all, and thanks a lot for the careful review.

Let me answer the four points in order.  Two of them (the update timing
and the node/data typing) are real defects in v2; for the other two I
think the current code is already correct, and I explain why below so you
can tell me if I am missing something.

On 8/10/26 02:10, Chao Yu wrote:
>> 8 GiB UFS, 2 GiB random overwrite at 99% utilization:
>>    baseline: WAF 76.7, foreground GC calls 578k
>>    gcless:   WAF  1.2, foreground GC calls 504
>
> Can we mitigate this by increasing min_ssr_sections?

I tried it, and it has no effect.  Near-full random overwrite at 99%
utilization, everything fixed except min_ssr_sections: raising it from
the default 66 to 2000 left every counter identical (94 foreground GC
calls, 602823 SSR blocks, FS-WAF 1.53), while lowering it to 0 did
change things (476 GC calls, WAF 1.67) -- so the knob is live, it is
just already saturated at the default.

>> +	if (test_bit(seg, free_i->free_segmap))
>> +		return 0;
>> +	if (test_bit(seg, dirty_i->dirty_segmap[PRE]))
>> +		return 0;
>> +	if (is_curseg(sbi, seg))
>> +		return 0;
>> +	if (ckpt_v >= usable)
>> +		return 0;
>> +
>> +	return usable - ckpt_v;
>
> If SSR is enabled, there may be valid but not checkpointed blocks in section?
> such space can not be treated as free?

Such blocks exist, but they are already excluded, because
ckpt_valid_blocks is not a pure "as of the last checkpoint" counter --
allocation bumps it immediately.  In update_sit_entry_for_alloc():

	/*
	 * SSR should never reuse block which is checkpointed
	 * or newly invalidated.
	 */
	if (!is_sbi_flag_set(sbi, SBI_CP_DISABLED)) {
		if (!f2fs_test_and_set_bit(offset, se->ckpt_valid_map)) {
			se->ckpt_valid_blocks++;
			...
	}

	if (!f2fs_test_bit(offset, se->ckpt_valid_map)) {
		se->ckpt_valid_blocks += del;
		...

So every block written after the last checkpoint (LFS or SSR, and with
checkpointing disabled too, via the second hunk) is counted in
ckpt_valid_blocks right away, and update_sit_entry_for_del() only
decrements it for blocks that are *not* set in ckpt_valid_map.  As a
result ckpt_valid_blocks is exactly the population count of
ckpt_valid_map | cur_valid_map, which is precisely the target_map that
__next_free_blkoff() refuses to allocate from:

	for (i = 0; i < entries; i++)
		target_map[i] = ckpt_map[i] | cur_map[i];

	return __find_rev_next_zero_bit(target_map, BLKS_PER_SEG(sbi), start);

So "usable - ckpt_valid_blocks" is the number of blocks SSR can actually
hand out in that segment, not an upper bound that includes freshly
written blocks.  If you still see a case where the two diverge I would
very much like to know -- that would be a bug in the counter itself.

> And, only updating sbi->cib_total_blocks w/ f2fs_update_cib() in checkpoint()
> is not enough? since free space in section may change due to SSR allocation
> and deletion. right?

You are right, and this is the weaker half of the same argument.  The
per-segment counter is accurate at all times as described above, but
sbi->cib_total_blocks is only a snapshot of it taken at checkpoint time,
so between two checkpoints the aggregate goes stale: SSR allocation
consumes slack that the watermark still credits, and truncation creates
slack that it does not yet see.  The stale direction that matters is the
first one -- it can keep foreground GC skipped after the budget is
already spent.  My reasoning in v2 was that gcless checkpoints often
enough to bound the drift, but "often enough" is not a correctness
argument, and I should not have relied on it.

>> +	if (test_opt(sbi, GCLESS)) {
>> +		unsigned int sec_blks = CAP_BLKS_PER_SEC(sbi);
>> +		unsigned int add_secs = READ_ONCE(sbi->cib_total_blocks) / sec_blks;
>> +		unsigned int room = free_secs < MAIN_SECS(sbi) ?
>> +					MAIN_SECS(sbi) - free_secs : 0;
>> +
>> +		free_secs += min(add_secs, room);
>
> A free section can be reused by any DATA or NODE type write, but above GCLESS
> "free_secs" may not, e.g. all free_secs are from DATA type, then latter checkpoint
> won't write any node into DATA type section w/ SSR.

Agreed, this is a real defect.  SSR never crosses the node/data boundary:
select_policy() restricts the victim search to dirty_segmap[type], and
get_ssr_segment() only falls back among the three node types or among the
three data types, never between them.  So crediting one pooled block count
to free_secs is wrong exactly as you describe -- a budget made entirely of
data-type slack would still let has_not_enough_free_secs() claim there is
room for node writes, and the node allocation would then have to fall back
to a real free section that the watermark just pretended existed.

I will take both points into account and validate the result carefully
before sending the next version.

Thanks a lot for taking the time to review this.

Thanks,
Yonggil


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel