Re: [PATCH] f2fs: fix error path handling in f2fs_read_data_large_folio()

[David Laight <david.laight.linux@gmail.com>]

On Thu,  8 Jan 2026 05:42:31 +0800
Chao Yu <chao@kernel.org> wrote:

> In error path of f2fs_read_data_large_folio(), if bio is valid, it
> may submit bio twice, fix it.

That isn't the only bug at the bottom of that function.
I think I've unravelled the strange loops on the copy in linux-next.
The 'goto got_it' could be a normal conditional.

The top has:
	if (rac)
		folio = readahead_folio(rac);
next_folio:
	if (!folio)
		goto out:
which means you can 'goto out' before setting up a pending 'bio'.

I'm sure that could be made a proper loop - although it would cost an indentation.
Would certainly be better with only one call to readahead_foilio(), perhaps:
next_folio:
	if (rac) {
		folio = readahead_folio(rac);
		if (!folio)
			goto out:
	}
with just:
	if (rac)
		goto next_folio;
at the bottom. 


> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/data.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index cabaeeb436bd..386d9adfd4bd 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2568,17 +2568,14 @@ static int f2fs_read_data_large_folio(struct inode *inode,
>  		folio_unlock(folio);
>  		return ret;
>  	}
> -
> +out:
> +	f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
>  	if (ret) {
> -		f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
> -
>  		/* Wait bios and clear uptodate. */
>  		folio_lock(folio);

If I've read the code correctly the 'bio' can contain transfers for a
previous folio(s), and might have transfers for this folio, but might not.
So relocking the folio may just deadlock.
(I've not found the unlock at the end of transfer...)
Quite which 'bio' need the flag changed is another question.

	David

>  		folio_clear_uptodate(folio);
>  		folio_unlock(folio);
>  	}
> -out:
> -	f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
>  	return ret;
>  }
>