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 D4A2A3211 for ; Wed, 4 Sep 2024 03:35:20 +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=1725420920; cv=none; b=MjLyScS87ufd0ysHAqXZ8QpVgdKQsKsWZy11xpwWyhmwMBVOVDVBJU/QYM+j3zICTd9zRRrcHgQqsBvpnFY1fGwMjnywbIHeXQB/+tDMS33iVUp9SHMltvi7ufYfzkR8iFYdRgsrc2it2UEOLvCGgrLd+EcxfOj2WBHFkWCQT40= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725420920; c=relaxed/simple; bh=LsZ0BFKb8xBu32Daz1pIfErk3ubckOAkNIFMU1lcMtE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=CO7QJWta1slKh4wtuSjrbAHx2C6ciNv0pFyMFQhO9kMGZTTu6whh3PlSAxNWMF00J4hBrA1LDFmXhMMsoXBszEpuU51zr/Ia6dAMQy+wgP1Pos1qDUCUpaovlSqiayGv2/u9uXMopynOqwYgnqfeErbp8AeTiLfDlajxlvMy5kU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Ln2L/LFB; 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="Ln2L/LFB" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3BA32C4CEC2; Wed, 4 Sep 2024 03:35:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1725420920; bh=LsZ0BFKb8xBu32Daz1pIfErk3ubckOAkNIFMU1lcMtE=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Ln2L/LFB2oUIM9ZaroxKZsJ1iTE+6gkw4Q2PddjwffGh8gHBjIvmILuLXezWwPxq7 6/wL+Vq4QA8JeB52rntfIy85Pu+P/8o1OzrN17rGpx5Hmbi0e+5chF1z8djPoIV8so vEtg20OQB83Mek59WwqkSCdH1ewYKY2hF+MEDZNxcVpRnt18lf4/ru+zVhupdWNxqk E7E5bFqqlGwBNdAYPQuRfhkPw7w9d9d15DK+oMprmbSu+HNu4C5/1s6HYJicwyHYb7 QY4feqY7090G/UTc0SLE2rqP36KaUaD/2/7r7+jO1ZvvEiEcq7cr/nbXjygZD89mjI n2r2y3crRgywg== Message-ID: <5c7b34d8-6efa-4716-ab89-a0b7b7583cb2@kernel.org> Date: Wed, 4 Sep 2024 11:35:16 +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 Subject: Re: [f2fs-dev] [PATCH] f2fs: prevent atomic file from being dirtied before commit To: Daeho Jeong Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com, Daeho Jeong References: <20240826202352.2150294-1-daeho43@gmail.com> <45a8a9f3-27b8-433e-a0ac-e457f4cdf1eb@kernel.org> Content-Language: en-US From: Chao Yu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 2024/9/4 10:52, Daeho Jeong wrote: > On Tue, Sep 3, 2024 at 7:26 PM Chao Yu wrote: >> >> On 2024/9/4 1:07, Daeho Jeong wrote: >>> On Mon, Sep 2, 2024 at 3:08 AM Chao Yu wrote: >>>> >>>> On 2024/8/27 4:23, Daeho Jeong wrote: >>>>> From: Daeho Jeong >>>>> >>>>> Keep atomic file clean while updating and make it dirtied during commit >>>>> in order to avoid unnecessary and excessive inode updates in the previous >>>>> fix. >>>>> >>>>> Fixes: 4bf78322346f ("f2fs: mark inode dirty for FI_ATOMIC_COMMITTED flag") >>>>> Signed-off-by: Daeho Jeong >>>>> --- >>>>> fs/f2fs/f2fs.h | 3 +-- >>>>> fs/f2fs/inode.c | 10 ++++++---- >>>>> fs/f2fs/segment.c | 10 ++++++++-- >>>>> 3 files changed, 15 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>>> index 465b2fd50c70..5a7f6fa8b585 100644 >>>>> --- a/fs/f2fs/f2fs.h >>>>> +++ b/fs/f2fs/f2fs.h >>>>> @@ -801,7 +801,7 @@ enum { >>>>> FI_COMPRESS_RELEASED, /* compressed blocks were released */ >>>>> FI_ALIGNED_WRITE, /* enable aligned write */ >>>>> FI_COW_FILE, /* indicate COW file */ >>>>> - FI_ATOMIC_COMMITTED, /* indicate atomic commit completed except disk sync */ >>>>> + FI_ATOMIC_DIRTIED, /* indicate atomic file is dirtied */ >>>>> FI_ATOMIC_REPLACE, /* indicate atomic replace */ >>>>> FI_OPENED_FILE, /* indicate file has been opened */ >>>>> FI_MAX, /* max flag, never be used */ >>>>> @@ -3042,7 +3042,6 @@ static inline void __mark_inode_dirty_flag(struct inode *inode, >>>>> case FI_INLINE_DOTS: >>>>> case FI_PIN_FILE: >>>>> case FI_COMPRESS_RELEASED: >>>>> - case FI_ATOMIC_COMMITTED: >>>>> f2fs_mark_inode_dirty_sync(inode, true); >>>>> } >>>>> } >>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>>>> index 1eb250c6b392..5dd3e55d2be2 100644 >>>>> --- a/fs/f2fs/inode.c >>>>> +++ b/fs/f2fs/inode.c >>>>> @@ -35,6 +35,11 @@ void f2fs_mark_inode_dirty_sync(struct inode *inode, bool sync) >>>>> if (f2fs_inode_dirtied(inode, sync)) >>>> >>>> It will return directly here if inode was dirtied, so it may missed to set >>>> FI_ATOMIC_DIRTIED flag? >>> >>> Is it possible for it to be already dirty, since we already made it >>> clean with f2fs_write_inode() when we started the atomic write? >> >> Some ioctl interfaces may race w/ atomic write? e.g. set_pin_file won't >> check atomic_file status, and may dirty inode after we started atomic >> write, so we'd better detect such race condition and break ioctl to >> avoid ruin atomic write? and maybe we can add f2fs_bug_on() in >> f2fs_mark_inode_dirty_sync() to detect any other missing cases? >> > > How about exchanging the positions of f2fs_write_inode() and > set_inode_flag() in f2fs_ioc_start_atomic_write()? > > ... > f2fs_write_inode(inode, NULL); > > stat_inc_atomic_inode(inode); > > set_inode_flag(inode, FI_ATOMIC_FILE); > ... Oh, I'm not sure I've got your point, after exchanging we still may suffer below race condition, right? - f2fs_ioc_start_atomic_write - set_inode_flag(inode, FI_ATOMIC_FILE) - f2fs_write_inode(inode, NULL) - f2fs_ioc_set_pin_file - set_inode_flag(inode, FI_PIN_FILE) - __mark_inode_dirty_flag() - f2fs_ioc_commit_atomic_write So that I proposed a fix for this: https://lore.kernel.org/linux-f2fs-devel/20240904032047.1264706-1-chao@kernel.org Thanks, > >> Thanks, >> >>> >>>> >>>> Thanks, >>>> >>>>> return; >>>>> >>>>> + if (f2fs_is_atomic_file(inode)) { >>>>> + set_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>>> + return; >>>>> + } >>>>> + >>>>> mark_inode_dirty_sync(inode); >>>>> } >>>>> >>>>> @@ -653,10 +658,7 @@ void f2fs_update_inode(struct inode *inode, struct page *node_page) >>>>> ri->i_gid = cpu_to_le32(i_gid_read(inode)); >>>>> ri->i_links = cpu_to_le32(inode->i_nlink); >>>>> ri->i_blocks = cpu_to_le64(SECTOR_TO_BLOCK(inode->i_blocks) + 1); >>>>> - >>>>> - if (!f2fs_is_atomic_file(inode) || >>>>> - is_inode_flag_set(inode, FI_ATOMIC_COMMITTED)) >>>>> - ri->i_size = cpu_to_le64(i_size_read(inode)); >>>>> + ri->i_size = cpu_to_le64(i_size_read(inode)); >>>>> >>>>> if (et) { >>>>> read_lock(&et->lock); >>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>>> index 78c3198a6308..2b5391b229a8 100644 >>>>> --- a/fs/f2fs/segment.c >>>>> +++ b/fs/f2fs/segment.c >>>>> @@ -196,9 +196,12 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) >>>>> truncate_inode_pages_final(inode->i_mapping); >>>>> >>>>> release_atomic_write_cnt(inode); >>>>> - clear_inode_flag(inode, FI_ATOMIC_COMMITTED); >>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); >>>>> clear_inode_flag(inode, FI_ATOMIC_FILE); >>>>> + if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>>>> + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>>> + f2fs_mark_inode_dirty_sync(inode, true); >>>>> + } >>>>> stat_dec_atomic_inode(inode); >>>>> >>>>> F2FS_I(inode)->atomic_write_task = NULL; >>>>> @@ -365,7 +368,10 @@ static int __f2fs_commit_atomic_write(struct inode *inode) >>>>> sbi->revoked_atomic_block += fi->atomic_write_cnt; >>>>> } else { >>>>> sbi->committed_atomic_block += fi->atomic_write_cnt; >>>>> - set_inode_flag(inode, FI_ATOMIC_COMMITTED); >>>>> + if (is_inode_flag_set(inode, FI_ATOMIC_DIRTIED)) { >>>>> + clear_inode_flag(inode, FI_ATOMIC_DIRTIED); >>>>> + f2fs_mark_inode_dirty_sync(inode, true); >>>>> + } >>>>> } >>>>> >>>>> __complete_revoke_list(inode, &revoke_list, ret ? true : false); >>>> >>