Re:Re: [PATCH v1 4/5] f2fs: add 'folio_in_bio' to handle readahead folios with no BIO submission

["Nanzhe Zhao" <nzzhao@126.com>]

Hi Chao yu:
At 2026-01-06 17:31:20, "Chao Yu" <chao@kernel.org> wrote:
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 66ab7a43a56f..ac569a396914 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2430,6 +2430,7 @@ static int f2fs_read_data_large_folio(struct inode *inode,
>>>   	unsigned nrpages;
>>>   	struct f2fs_folio_state *ffs;
>>>   	int ret = 0;
>>> +	bool folio_in_bio = false;
>>
>>No need to initialize folio_in_bio?

Agreed. It's redundant since we reset it to false for each new folio before processing.

>>> @@ -2539,6 +2542,11 @@ static int f2fs_read_data_large_folio(struct inode *inode,
>>>   	}
>>>   	trace_f2fs_read_folio(folio, DATA);
>>>   	if (rac) {
>>> +		if (!folio_in_bio) {
>>> +			if (!ret)
>>> +				folio_mark_uptodate(folio);
>>> +			folio_unlock(folio);
>>> +	}
>>
>>err_out:
>>	/* Nothing was submitted. */
>>	if (!bio) {
>>		if (!ret)
>>			folio_mark_uptodate(folio);
>>		folio_unlock(folio);
>>
>>                 ^^^^^^^^^^^^
>>
>>If all folios in rac have not been mapped (hole case), will we unlock the folio twice?

Are you worried the folio could be unlocked once in the if (rac) { ... } block and then 
unlocked again at err_out:? If so, I think that won't happen.

In such a case, every non-NULL folio will be unlocked exactly once by:

if (!folio_in_bio) {
       if (!ret)
               folio_mark_uptodate(folio);
       folio_unlock(folio);
}
Specifically, after the last folio runs through the block above, the next call:

folio = readahead_folio(rac);
will return NULL. Then we go to next_folio:, and will directly hit:

if (!folio)
       goto out;
This jumps straight to the out: label, skipping err_out: entirely. 
Therefore, when ret is not an error code, the err_out: label will never be reached.

If ret becomes an error code, then the current folio will immediately goto err_out; 
and be unlocked there once.

If rac is NULL (meaning we only read the single large folio passed in as the function argument), 
we won't enter the if (rac) { ... goto next_folio; } path at all, so we also won't go to next_folio 
and then potentially goto out;. In that case, it will naturally be unlocked once at err_out:.
Or am I missing some edge case here?

Thanks,
Nanzhe