Re: (2) [PATCH] f2fs: issue multi-device flushes in parallel

Hi Chao,

Thanks for the review.

On 8/3/26 14:59, Chao Yu wrote:
> On 7/13/26 13:57, Yonggil Song wrote:
>> On a multi-device setup, submit_flush_wait() walked the dirty devices
>> in order and aborted the whole loop on the first device whose flush
>> failed, leaving the remaining dirty devices un-flushed. Each device
>> still needs its own data made durable, so a failure on one device must
>> not skip the others. It also waited for one device's flush to complete
>> before issuing the next, even though the devices have independent
>> flush queues and could be flushed concurrently.
>> 
>> Flush every dirty device best-effort and in parallel instead: build
>> one PREFLUSH bio per dirty device, submit them all, then wait for
>> every completion, returning the first error seen (0 if all succeed).
>> This bounds the flush window by the slowest device rather than the sum
>> of all of them. No caller depends on the previous early-abort
>> behaviour -- fsync only checks whether the return value is zero
>> (fs/f2fs/file.c). The checkpoint path (f2fs_flush_device_cache) is
>> unaffected; this only touches the fsync flush path.
>> 
>> Signed-off-by: Yonggil Song <[email protected]>
>> ---
>>   fs/f2fs/segment.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 78 insertions(+), 6 deletions(-)
>> 
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index d71ddb3ee918..51d7f76e3d1d 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -566,24 +566,96 @@ static int __submit_flush_wait(struct f2fs_sb_info *sbi,
>>   	return ret;
>>   }
>>   
>> -static int submit_flush_wait(struct f2fs_sb_info *sbi, nid_t ino)
>> +static void f2fs_flush_end_io(struct bio *bio)
>> +{
>> +	complete(bio->bi_private);
>> +}
>> +
>> +struct f2fs_flush_bio {
>> +	struct bio bio;
>> +	struct completion wait;
>> +};
>> +
>> +/*
>> + * Flush every dirty device best-effort: a failure on one device must not
>> + * skip the flush on the remaining dirty devices, since each device still
>> + * needs its own data made durable. Report the first error.
>> + */
>> +static int submit_flush_wait_serial(struct f2fs_sb_info *sbi, nid_t ino)
>>   {
>>   	int ret = 0;
>>   	int i;
>>   
>> -	if (!f2fs_is_multi_device(sbi))
>> -		return __submit_flush_wait(sbi, sbi->sb->s_bdev);
>> +	for (i = 0; i < sbi->s_ndevs; i++) {
>> +		int err;
>> +
>> +		if (!f2fs_is_dirty_device(sbi, ino, i, FLUSH_INO))
>> +			continue;
>> +		err = __submit_flush_wait(sbi, FDEV(i).bdev);
>> +		if (err && !ret)
>> +			ret = err;
>> +	}
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Same best-effort/first-error contract as submit_flush_wait_serial(), but
>> + * issue every dirty device's flush before waiting for any of them, so the
>> + * per-device flush latencies overlap instead of adding up in series. Fall
>> + * back to the serial path if the bio array cannot be allocated.
>> + */
>> +static int submit_flush_wait_parallel(struct f2fs_sb_info *sbi, nid_t ino)
>> +{
>> +	struct f2fs_flush_bio *flush_bio;
>> +	unsigned long devices = 0;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	flush_bio = kcalloc(sbi->s_ndevs, sizeof(*flush_bio), GFP_NOFS);
> 
> Can we allocate flush_bio in local stack? so that we don't need to fallback
> to submit_flush_wait_serial() for low memory case?

I tried the stack first, but the array is sized by MAX_DEVICES (8) and
each entry is a struct bio (136 bytes) plus a struct completion (88
bytes), so the frame grows to 1832 bytes and trips
-Wframe-larger-than=1024:

  fs/f2fs/segment.c: In function 'submit_flush_wait':
  fs/f2fs/segment.c:661:1: warning: the frame size of 1832 bytes is
  larger than 1024 bytes [-Wframe-larger-than=]

Since the allocation is small and bounded, how about keeping it on the
heap but allocating it with GFP_NOFS | __GFP_NOFAIL, which cannot fail?
That still lets us drop the serial fallback path entirely:

	flush_bio = kmalloc(array_size(sbi->s_ndevs, sizeof(*flush_bio)),
				GFP_NOFS | __GFP_NOFAIL);

v2 would also add REQ_SYNC to the flush bios to match what
submit_bio_wait() sets in the single-device blkdev_issue_flush() path.
> 
>> +	if (!flush_bio)
>> +		return submit_flush_wait_serial(sbi, ino);
>>   
>>   	for (i = 0; i < sbi->s_ndevs; i++) {
>>   		if (!f2fs_is_dirty_device(sbi, ino, i, FLUSH_INO))
>>   			continue;
>> -		ret = __submit_flush_wait(sbi, FDEV(i).bdev);
>> -		if (ret)
>> -			break;
>> +
>> +		bio_init(&flush_bio[i].bio, FDEV(i).bdev, NULL, 0,
>> +			 REQ_OP_WRITE | REQ_PREFLUSH);
>> +		init_completion(&flush_bio[i].wait);
>> +		flush_bio[i].bio.bi_private = &flush_bio[i].wait;
>> +		flush_bio[i].bio.bi_end_io = f2fs_flush_end_io;
>> +		submit_bio(&flush_bio[i].bio);
>> +		devices |= BIT(i);
>> +	}
>> +
>> +	for (i = 0; i < sbi->s_ndevs; i++) {
>> +		int err;
>> +
>> +		if (!(devices & BIT(i)))
>> +			continue;
>> +
>> +		wait_for_completion(&flush_bio[i].wait);
>> +		err = blk_status_to_errno(flush_bio[i].bio.bi_status);
>> +		trace_f2fs_issue_flush(FDEV(i).bdev, test_opt(sbi, NOBARRIER),
>> +				       test_opt(sbi, FLUSH_MERGE), err);
>> +		if (!err)
>> +			f2fs_update_iostat(sbi, NULL, FS_FLUSH_IO, 0);
>> +		else if (!ret)
>> +			ret = err;
>> +		bio_uninit(&flush_bio[i].bio);
> 
> It may leak bio reference previously? For local bio variable, there is no such issue.

I don't think this leaks: the bios are initialized with bio_init() inside
the kcalloc'ed array (not taken from a bioset), every submitted bio is
waited for right here, each one gets bio_uninit() after its completion
is reaped, and the array is kfree'd — so there is no reference left to
put. The point would be moot in v2 anyway since the fallback path is
gone.

If the __GFP_NOFAIL alternative looks good to you, I'll send v2 that
way.

Thanks,
Yonggil
> 
> Thanks,
> 
>>   	}
>> +	kfree(flush_bio);
>>   	return ret;
>>   }
>>   
>> +static int submit_flush_wait(struct f2fs_sb_info *sbi, nid_t ino)
>> +{
>> +	if (!f2fs_is_multi_device(sbi))
>> +		return __submit_flush_wait(sbi, sbi->sb->s_bdev);
>> +
>> +	return submit_flush_wait_parallel(sbi, ino);
>> +}
>> +
>>   static int issue_flush_thread(void *data)
>>   {
>>   	struct f2fs_sb_info *sbi = data;
> 
> 



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