[f2fs-dev] [PATCH] f2fs: issue multi-device flushes in parallel

[f2fs-dev] [PATCH] f2fs: issue multi-device flushes in parallel

[Yonggil Song]

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 <yonggil.song@samsung.com>
---
 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);
+	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);
 	}
+	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;
-- 
2.43.0


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH] f2fs: issue multi-device flushes in parallel

[Yonggil Song]

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 <yonggil.song@samsung.com>
---
 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);
+	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);
 	}
+	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;
-- 
2.43.0

Re: [f2fs-dev] [PATCH] f2fs: issue multi-device flushes in parallel

[Chao Yu via Linux-f2fs-devel]

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 <yonggil.song@samsung.com>
> ---
>   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?

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

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
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Chao Yu]

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 <yonggil.song@samsung.com>
> ---
>   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?

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

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;

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

[Yonggil Song]

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 <yonggil.song@samsung.com>
>> ---
>>   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
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Yonggil Song]

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 <yonggil.song@samsung.com>
>> ---
>>   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;
> 
> 

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

[Chao Yu via Linux-f2fs-devel]

On 8/6/26 06:46, Yonggil Song wrote:
> 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 <yonggil.song@samsung.com>
>>> ---
>>>   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=]

Ah, alright.

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

Looks fine to me.

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

Oh, you're right, we don't need to put bio due to bio is not allocated from
bio_alloc().

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

Yes, please go ahead.

Thanks,

> 
> 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
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Chao Yu]

On 8/6/26 06:46, Yonggil Song wrote:
> 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 <yonggil.song@samsung.com>
>>> ---
>>>   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=]

Ah, alright.

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

Looks fine to me.

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

Oh, you're right, we don't need to put bio due to bio is not allocated from
bio_alloc().

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

Yes, please go ahead.

Thanks,

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