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

[Chao Yu]

On 1/7/2026 8:33 AM, Nanzhe Zhao wrote:
> Hi Chao yu:
> At 2026-01-06 17:31:20, "Chao Yu" <chao@xxxxxxxxxx> 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?

Nanzhe,

Oh, yes, I think so, thanks for the explanation.

Thanks,

> Thanks,
> Nanzhe