From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailout1.samsung.com (mailout1.samsung.com [203.254.224.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 37ECB233942 for ; Wed, 5 Aug 2026 22:46:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=203.254.224.24 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785970022; cv=none; b=gdhp1696efz9QonOoR9KFiTkJa5d4dlUSxeIpOQY5dO2blHs5JJ7L1c7OrT3PEZALU3/btydiDzzpbdM4gN/1X8DVNMCbWoNNPsbEgoQS8bmWJM+bfXKx9TxsGkmBPzfcP70nO1lXZcbjvnwtAkq8Ljn7mojzYG1sXvkPYdMin8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785970022; c=relaxed/simple; bh=Zk5tUrWdsVyefAyNGsT+KfjLqF20SCBrNKDdLdUKfaE=; h=Mime-Version:Subject:From:To:CC:In-Reply-To:Message-ID:Date: Content-Type:References; b=YQurfRnxKsHEQDmvLPMQy90zn36w8fmD+sntRmU3zgpwdsRyVZbvo55F2uk92jBdsuPYh1dJOZ7jsqQRIWhmVEr2qIBS5kww6Pj169oMJZ22qLXQPKiZ935EeocoXKb0bNYf7Wju8hGNvKHOhrmqU+NtUvTn7vxL0MLeawJR9Z4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com; spf=pass smtp.mailfrom=samsung.com; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b=Hzdu5FBS; arc=none smtp.client-ip=203.254.224.24 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=samsung.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b="Hzdu5FBS" Received: from epcas2p4.samsung.com (unknown [182.195.41.56]) by mailout1.samsung.com (KnoxPortal) with ESMTP id 20260805224650epoutp01e2383f6740fe3a517dc38f8b0c0de29a~JCkMyYcNW2543625436epoutp01v for ; Wed, 5 Aug 2026 22:46:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.samsung.com 20260805224650epoutp01e2383f6740fe3a517dc38f8b0c0de29a~JCkMyYcNW2543625436epoutp01v DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1785970010; bh=HHiEcNZ2+OviHp+ZSLq4d9BWf4cARvVK6er7zebufzw=; h=Subject:Reply-To:From:To:CC:In-Reply-To:Date:References:From; b=Hzdu5FBSj3oMzv6ggcDIv3HyhNlhzN6VDbyPvbkV3QY6eZJqp57ddt964pT6JoOlJ vA1vsCkbj1ya06lfCtTvXciXrhaglBtjIpbFTJMSpe8+s8LQZCoxEuaVidV0aVN1Ci xTS8L7kVhcTg+QnL5y6sE8z/btxXUcCYb0E3ZxVM= Received: from epsnrtp03.localdomain (unknown [182.195.42.155]) by epcas2p2.samsung.com (KnoxPortal) with ESMTPS id 20260805224650epcas2p2e1c87191529c840bf89d16f95dd0f301~JCkMBtWbg2417524175epcas2p2A; Wed, 5 Aug 2026 22:46:50 +0000 (GMT) Received: from epcas2p1.samsung.com (unknown [182.195.38.207]) by epsnrtp03.localdomain (Postfix) with ESMTP id 4hFls55bVGz3hhTC; Wed, 5 Aug 2026 22:46:49 +0000 (GMT) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Subject: RE:(2) [PATCH] f2fs: issue multi-device flushes in parallel Reply-To: yonggil.song@samsung.com Sender: Yonggil Song From: Yonggil Song To: Chao Yu , "jaegeuk@kernel.org" CC: "linux-f2fs-devel@lists.sourceforge.net" , "linux-kernel@vger.kernel.org" , Dongjin Kim , Daejun Park , Yonggil Song X-Priority: 3 X-Content-Kind-Code: NORMAL In-Reply-To: <9fc81e1e-ec13-4cc0-9aed-8270bf1a4157@kernel.org> X-CPGS-Detection: blocking_info_exchange X-Drm-Type: N,general X-Msg-Generator: Mail X-Msg-Type: PERSONAL X-Reply-Demand: N Message-ID: <20260805224648epcms2p8359e0c2a6b0728dcc3ee3d540c195189@epcms2p8> Date: Thu, 06 Aug 2026 07:46:48 +0900 X-CMS-MailID: 20260805224648epcms2p8359e0c2a6b0728dcc3ee3d540c195189 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" X-Sendblock-Type: AUTO_CONFIDENTIAL CMS-TYPE: 102P cpgsPolicy: CPGSC10-223,Y X-CFilter-Loop: Reflected X-CMS-RootMailID: 20260713055711epcms2p712e7add62211e42e995c54a92fd4ff0c References: <9fc81e1e-ec13-4cc0-9aed-8270bf1a4157@kernel.org> <20260713055711epcms2p712e7add62211e42e995c54a92fd4ff0c@epcms2p7> 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. >>=20 >> 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. >>=20 >> Signed-off-by: Yonggil Song >> --- >> fs/f2fs/segment.c =7C 84 +++++++++++++++++++++++++++++++++++++++++++++= ++++++---- >> 1 file changed, 78 insertions(+), 6 deletions(-) >>=20 >> 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 >> =40=40 -566,24 +566,96 =40=40 static int __submit_flush_wait(struct f2fs= _sb_info *sbi, >> return ret; >> =7D >> =20 >> -static int submit_flush_wait(struct f2fs_sb_info *sbi, nid_t ino) >> +static void f2fs_flush_end_io(struct bio *bio) >> +=7B >> + complete(bio->bi_private); >> +=7D >> + >> +struct f2fs_flush_bio =7B >> + struct bio bio; >> + struct completion wait; >> +=7D; >> + >> +/* >> + * Flush every dirty device best-effort: a failure on one device must n= ot >> + * skip the flush on the remaining dirty devices, since each device sti= ll >> + * needs its own data made durable. Report the first error. >> + */ >> +static int submit_flush_wait_serial(struct f2fs_sb_info *sbi, nid_t ino= ) >> =7B >> int ret =3D 0; >> int i; >> =20 >> - if (=21f2fs_is_multi_device(sbi)) >> - return __submit_flush_wait(sbi, sbi->sb->s_bdev); >> + for (i =3D 0; i < sbi->s_ndevs; i++) =7B >> + int err; >> + >> + if (=21f2fs_is_dirty_device(sbi, ino, i, FLUSH_INO)) >> + continue; >> + err =3D __submit_flush_wait(sbi, FDEV(i).bdev); >> + if (err && =21ret) >> + ret =3D err; >> + =7D >> + return ret; >> +=7D >> + >> +/* >> + * 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. F= all >> + * 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 i= no) >> +=7B >> + struct f2fs_flush_bio *flush_bio; >> + unsigned long devices =3D 0; >> + int ret =3D 0; >> + int i; >> + >> + flush_bio =3D kcalloc(sbi->s_ndevs, sizeof(*flush_bio), GFP_NOFS); >=20 > Can we allocate flush_bio in local stack? so that we don't need to fallba= ck > 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=3D1024: 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 =5B-Wframe-larger-than=3D=5D Since the allocation is small and bounded, how about keeping it on the heap but allocating it with GFP_NOFS =7C __GFP_NOFAIL, which cannot fail? That still lets us drop the serial fallback path entirely: flush_bio =3D kmalloc(array_size(sbi->s_ndevs, sizeof(*flush_bio)), GFP_NOFS =7C __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. >=20 >> + if (=21flush_bio) >> + return submit_flush_wait_serial(sbi, ino); >> =20 >> for (i =3D 0; i < sbi->s_ndevs; i++) =7B >> if (=21f2fs_is_dirty_device(sbi, ino, i, FLUSH_INO)) >> continue; >> - ret =3D __submit_flush_wait(sbi, FDEV(i).bdev); >> - if (ret) >> - break; >> + >> + bio_init(&flush_bio=5Bi=5D.bio, FDEV(i).bdev, NULL, 0, >> + REQ_OP_WRITE =7C REQ_PREFLUSH); >> + init_completion(&flush_bio=5Bi=5D.wait); >> + flush_bio=5Bi=5D.bio.bi_private =3D &flush_bio=5Bi=5D.wait; >> + flush_bio=5Bi=5D.bio.bi_end_io =3D f2fs_flush_end_io; >> + submit_bio(&flush_bio=5Bi=5D.bio); >> + devices =7C=3D BIT(i); >> + =7D >> + >> + for (i =3D 0; i < sbi->s_ndevs; i++) =7B >> + int err; >> + >> + if (=21(devices & BIT(i))) >> + continue; >> + >> + wait_for_completion(&flush_bio=5Bi=5D.wait); >> + err =3D blk_status_to_errno(flush_bio=5Bi=5D.bio.bi_status); >> + trace_f2fs_issue_flush(FDEV(i).bdev, test_opt(sbi, NOBARRIER), >> + test_opt(sbi, FLUSH_MERGE), err); >> + if (=21err) >> + f2fs_update_iostat(sbi, NULL, FS_FLUSH_IO, 0); >> + else if (=21ret) >> + ret =3D err; >> + bio_uninit(&flush_bio=5Bi=5D.bio); >=20 > 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 =E2=80=94=20so=20there=20is=20no=20refe= rence=20left=20to=0D=0Aput.=20The=20point=20would=20be=20moot=20in=20v2=20a= nyway=20since=20the=20fallback=20path=20is=0D=0Agone.=0D=0A=0D=0AIf=20the= =20__GFP_NOFAIL=20alternative=20looks=20good=20to=20you,=20I'll=20send=20v2= =20that=0D=0Away.=0D=0A=0D=0AThanks,=0D=0AYonggil=0D=0A>=20=0D=0A>=20Thanks= ,=0D=0A>=20=0D=0A>>=20=20=20=09=7D=0D=0A>>=20+=09kfree(flush_bio);=0D=0A>>= =20=20=20=09return=20ret;=0D=0A>>=20=20=20=7D=0D=0A>>=20=20=20=0D=0A>>=20+s= tatic=20int=20submit_flush_wait(struct=20f2fs_sb_info=20*sbi,=20nid_t=20ino= )=0D=0A>>=20+=7B=0D=0A>>=20+=09if=20(=21f2fs_is_multi_device(sbi))=0D=0A>>= =20+=09=09return=20__submit_flush_wait(sbi,=20sbi->sb->s_bdev);=0D=0A>>=20+= =0D=0A>>=20+=09return=20submit_flush_wait_parallel(sbi,=20ino);=0D=0A>>=20+= =7D=0D=0A>>=20+=0D=0A>>=20=20=20static=20int=20issue_flush_thread(void=20*d= ata)=0D=0A>>=20=20=20=7B=0D=0A>>=20=20=20=09struct=20f2fs_sb_info=20*sbi=20= =3D=20data;=0D=0A>=20=0D=0A>=20=0D=0A