From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 3D7A4219A7A for ; Fri, 9 Jan 2026 01:37:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767922665; cv=none; b=djDokodVL01A9yUYkwD3o2FxK8y8V+f9fPO+ktTKXkWzD7bpp7xSMYHPE/op+qYJmU+FgiKxZa0+0BzEMRHssOfu3uAMGIS2mcelqTomwXpM7Y+ShWlmhx+khexEwqDkLDpK7Feif7q937WpnYJQXkC0o5A+8ZXGs3jre+z4iOM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767922665; c=relaxed/simple; bh=E2uqEyReXn4MFKwBBuEVOyfnljmoHCoIVlZsKSbf5C8=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=frVLLJEPLJFHuTeslFbOtwhNF05GcZD/axNAaOZDvSY8T1n9AFU+4ffZmaPX0VJK9stZHQg044InRGxZFnhCrLOPrkCRIxAR7lR9xN6LSt7K28n1CnI22lcjG86SdffG3lqPyq7kkXeqEhkpVGJ5F0rLQE+WN2xLJ3E7Ls82Htw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=c2M8sMpC; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="c2M8sMpC" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A20BEC116C6; Fri, 9 Jan 2026 01:37:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1767922664; bh=E2uqEyReXn4MFKwBBuEVOyfnljmoHCoIVlZsKSbf5C8=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=c2M8sMpC4EdQmE3WHa53+HDOlFTAtxwfX8mAJR5LFNYJX/Ag0w06eSG8auUsw7DTe OezVxeiVGoIl7BCIPgWe4/lxpb4IUInx3cy08rEZTz2ea9hb2qDooAFmh+LAV2tNTN KMW3q+vA32siCLodQSkIrNPOoFJjKTPnWlGm8UbxCvnCW74dWU8PnvUtYeqLNR3j8A R3Gc5Pxpn5gxQFweYj7OCHkepIxxB3wBl5ydLX+PKOus+JV0G/lTldtFOj5QEf8qIt TJ/8c1W2HQigW4+6sH+OExuH8LDo/QAKBxt1M9f5wKO+efAqyIzcNbduWoUURPd1zt TLVZU9j3hwbpQ== Message-ID: <2f63622e-9663-491b-b390-71e6e81474ce@kernel.org> Date: Fri, 9 Jan 2026 09:37:54 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, jaegeuk@kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH] f2fs: fix error path handling in f2fs_read_data_large_folio() To: David Laight References: <20260107214231.24163-1-chao@kernel.org> <20260108224545.3019a411@pumpkin> Content-Language: en-US From: Chao Yu In-Reply-To: <20260108224545.3019a411@pumpkin> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 1/9/2026 6:45 AM, David Laight wrote: > On Thu, 8 Jan 2026 05:42:31 +0800 > Chao Yu 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 >> --- >> 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; >> } >> >