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

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

[Chao Yu]

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

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);
 		folio_clear_uptodate(folio);
 		folio_unlock(folio);
 	}
-out:
-	f2fs_submit_read_bio(F2FS_I_SB(inode), bio, DATA);
 	return ret;
 }
 
-- 
2.40.1

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

[David Laight]

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;
>  }
>  

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

[Chao Yu]

On 1/9/2026 6:45 AM, David Laight wrote:
> 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.

It could be, maybe you can clean up w/ a separated patch?

> 
> 
>>
>> 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.

The bio may contain last folio which has not been committed to device, as you
said the folio has been locked, and after IO completion, we will unlock the
folio in f2fs_finish_read_bio(), so no deadlock?

Thanks,

> (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;
>>   }
>>   
>