| Message ID | 20230113004933.2082072-1-daeho43@gmail.com |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp6770wrt;
Thu, 12 Jan 2023 16:51:39 -0800 (PST)
X-Google-Smtp-Source:
AMrXdXtAr4vy5hUHEgfFIGdtCiwngQSqGaAKwuxscMdmr2+uD/6gGD3w8hqXXtqvQqL4y9hWZYId
X-Received: by 2002:a17:907:6d98:b0:7c1:12b0:7d5d with SMTP id
sb24-20020a1709076d9800b007c112b07d5dmr87461739ejc.4.1673571099469;
Thu, 12 Jan 2023 16:51:39 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; t=1673571099; cv=none;
d=google.com; s=arc-20160816;
b=UgZSi1EqYWNzQaSjDvX8An9WcOFO4GsEwXrB5wdWQKnSKHcz5XflAv5YXkQsZcIAJ7
5hQ4e7BaUleyo0AZdcZDoQkDaEgkuqAN3wanPVubtB9EIEuAv5KbzZTvOgXq6XgzMB3X
6IZRt9YV4OLYmPkbTE8q8broXpMJC7K8BsQf3D6TsxFNjZyyGD7P+/p30UUZu7dMneCW
ajlXo3Hrwdsu1p5QRrrlRFHzODxxJh2hd7p3+WGzM8fjmeQ16UZt2zVRX8KmxcH9hEK4
xPJN61MaiAYdjY8tW20UERPQvmo+p8x+K7BOH/6Q4fIX8A7JlcyADDxqmHlLvzoEBt6B
VW0Q==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:content-transfer-encoding:mime-version
:message-id:date:subject:cc:to:from:dkim-signature;
bh=0aOIcA4HY+kMmpBfAWn8xfATZOZpAZzf7hkPss2yBIk=;
b=UQHlnxmUnKqM3J103phGJ49PJJ2zbcRpVar8OX+vXfKJwTyzNlQt1fkhwD6SssuWb1
MQcTmyPWvNa3yRW9+kV7wJphaL0LwharWiJWqqzvGBqIpD1TXE2016/VxfJIFeg/737i
Js9AiDNMAT6dXu8AK/cOjObIXgq7PnzkT5ATlsUhS4D53qF5fnGDdKaxwhY5Frqus/mu
p+NJUdtqI15/kOadt02UcXxYRkk1CrxyYH7iTTU9Buo9pNEhRhXa2b0HfppabtGo9EBY
xU0q8D4JXDsVNbVxLxqj64/q2lUjCd6uXEZxnP+EsTPjGiO6Oe8c5NyKYACxn/ia5Mgt
aphw==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=pass header.i=@gmail.com header.s=20210112 header.b=cQZ3zGvc;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
gs8-20020a1709072d0800b0085a483a6fcdsi9242197ejc.311.2023.01.12.16.51.14;
Thu, 12 Jan 2023 16:51:39 -0800 (PST)
Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
client-ip=2620:137:e000::1:20;
Authentication-Results: mx.google.com;
dkim=pass header.i=@gmail.com header.s=20210112 header.b=cQZ3zGvc;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S240657AbjAMAvE (ORCPT <rfc822;zhuangel570@gmail.com>
+ 99 others); Thu, 12 Jan 2023 19:51:04 -0500
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46260 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S240575AbjAMAuB (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Thu, 12 Jan 2023 19:50:01 -0500
Received: from mail-pg1-x52d.google.com (mail-pg1-x52d.google.com
[IPv6:2607:f8b0:4864:20::52d])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6BB67621AE
for <linux-kernel@vger.kernel.org>;
Thu, 12 Jan 2023 16:49:52 -0800 (PST)
Received: by mail-pg1-x52d.google.com with SMTP id 78so13955869pgb.8
for <linux-kernel@vger.kernel.org>;
Thu, 12 Jan 2023 16:49:52 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=gmail.com; s=20210112;
h=content-transfer-encoding:mime-version:message-id:date:subject:cc
:to:from:from:to:cc:subject:date:message-id:reply-to;
bh=0aOIcA4HY+kMmpBfAWn8xfATZOZpAZzf7hkPss2yBIk=;
b=cQZ3zGvcLBXrKfRU4C1sQ274E6wzURXM9trpqXGCKP9aC/5tAJT3OLT/w+ZMpC7xgK
c6JWemKme9xsDnoavf/P9wmEh9ti9BucyQpYLgGJo1ERA0RLgc1FwjMDl9nOb8cY6pNG
mEUUdqeU9sh95euoy0Nxfwf8Y6TXtdkMNe8SE/Y533fXfEf0sev7dntuHtPkupgsCj5D
gVGoUe19IqU9WTtYqtBkTOgQ52vNOM+xkECEL5FAp5PCcu3Mf/uycnZG0PI6DYFMWYvM
2eEB5sH/Y/juf6ZICJfWmFiqpSomof7VrOpjow4gmWsD6ChqNQCx/D8B8pLiBnSP6wBi
thZA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20210112;
h=content-transfer-encoding:mime-version:message-id:date:subject:cc
:to:from:x-gm-message-state:from:to:cc:subject:date:message-id
:reply-to;
bh=0aOIcA4HY+kMmpBfAWn8xfATZOZpAZzf7hkPss2yBIk=;
b=eC/qOZqPpWkWbIrv9tDSo0KrV02Sc5jxwxA6FTBASumR5Gc+mCHLZYccMX5sINeO+2
sLCXjsdxD9Z9HICT73HSN92yp5C/xG6gb5Xmxp6h8HGZRo4HmATxhHQXx5nxgjxDV67j
s7gHIXR0kBCeuil08xzD19Hlqs5LUCVWx6MTFOev4UC3HZFv2gff0tmz4UH9Tv7o4Jlz
VhJVISMN7qwLK2JOOCNoe+++lIBD3RhhCNS55KD44s6Fz2XJYcRCMHMSqAFAkB4SB1Xo
sw8+4A28PdjbJ4VGX0390wRhTuO9ctC30PBKu102uZR4kM5O02In8YSXpOKK9N7BjMAo
p2Gg==
X-Gm-Message-State: AFqh2koC+oW+E5cd7hH9kpiObSzmhK+8R7x0RjTCufZ2TL+hv4tcED2+
o5NLp1/ZdU3KviNP8UlxZgoIeMW3ixs=
X-Received: by 2002:a05:6a00:130d:b0:580:fb8e:3044 with SMTP id
j13-20020a056a00130d00b00580fb8e3044mr76639150pfu.22.1673570991526;
Thu, 12 Jan 2023 16:49:51 -0800 (PST)
Received: from daehojeong-desktop.mtv.corp.google.com
([2620:15c:211:201:9cd1:e03e:3432:197d])
by smtp.gmail.com with ESMTPSA id
r21-20020aa79635000000b00580c0219f16sm12467614pfg.169.2023.01.12.16.49.50
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Thu, 12 Jan 2023 16:49:50 -0800 (PST)
From: Daeho Jeong <daeho43@gmail.com>
To: linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com
Cc: Daeho Jeong <daehojeong@google.com>,
syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com
Subject: [PATCH] f2fs: synchronize atomic write aborts
Date: Thu, 12 Jan 2023 16:49:33 -0800
Message-Id: <20230113004933.2082072-1-daeho43@gmail.com>
X-Mailer: git-send-email 2.39.0.246.g2a6d74b583-goog
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT,
FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham
autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1754866489336709651?=
X-GMAIL-MSGID: =?utf-8?q?1754866489336709651?=
|
| Series |
f2fs: synchronize atomic write aborts
|
|
Commit Message
Daeho Jeong
Jan. 13, 2023, 12:49 a.m. UTC
From: Daeho Jeong <daehojeong@google.com> To fix a race condition between atomic write aborts, I use the inode lock and make COW inode to be re-usable thoroughout the whole atomic file inode lifetime. Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") Signed-off-by: Daeho Jeong <daehojeong@google.com> --- fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- fs/f2fs/inode.c | 11 +++++++++-- fs/f2fs/segment.c | 3 --- fs/f2fs/super.c | 2 -- 4 files changed, 37 insertions(+), 22 deletions(-)
Comments
Hi Daeho, Jaegeuk, Please take a look at patchset in below link: https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me know your preference. :) One comment as below. On 2023/1/13 8:49, Daeho Jeong wrote: > From: Daeho Jeong <daehojeong@google.com> > > To fix a race condition between atomic write aborts, I use the inode > lock and make COW inode to be re-usable thoroughout the whole > atomic file inode lifetime. > > Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > Signed-off-by: Daeho Jeong <daehojeong@google.com> > --- > fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > fs/f2fs/inode.c | 11 +++++++++-- > fs/f2fs/segment.c | 3 --- > fs/f2fs/super.c | 2 -- > 4 files changed, 37 insertions(+), 22 deletions(-) > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index ecbc8c135b49..ff072a9ed258 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > atomic_read(&inode->i_writecount) != 1) > return 0; > > + inode_lock(inode); > f2fs_abort_atomic_write(inode, true); > + inode_unlock(inode); > + > return 0; > } > > @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > * until all the writers close its file. Since this should be done > * before dropping file lock, it needs to do in ->flush. > */ > - if (F2FS_I(inode)->atomic_write_task == current) > + if (F2FS_I(inode)->atomic_write_task == current) { > + inode_lock(inode); > f2fs_abort_atomic_write(inode, true); > + inode_unlock(inode); > + } > return 0; > } > > @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > goto out; > } > > - /* Create a COW inode for atomic write */ > - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > - if (IS_ERR(pinode)) { > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > - ret = PTR_ERR(pinode); > - goto out; > - } > + /* Check if the inode already has a COW inode */ > + if (fi->cow_inode == NULL) { > + /* Create a COW inode for atomic write */ > + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > + if (IS_ERR(pinode)) { > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > + ret = PTR_ERR(pinode); > + goto out; > + } > > - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > - iput(pinode); > - if (ret) { > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > - goto out; > + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > + iput(pinode); > + if (ret) { > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > + goto out; > + } > + > + set_inode_flag(fi->cow_inode, FI_COW_FILE); > + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > + } else { > + /* Reuse the already created COW inode */ > + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > } > > f2fs_write_inode(inode, NULL); > @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > stat_inc_atomic_inode(inode); > > set_inode_flag(inode, FI_ATOMIC_FILE); > - set_inode_flag(fi->cow_inode, FI_COW_FILE); > - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > isize = i_size_read(inode); > fi->original_i_size = isize; > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > index ff6cf66ed46b..4921f7209e28 100644 > --- a/fs/f2fs/inode.c > +++ b/fs/f2fs/inode.c > @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > void f2fs_evict_inode(struct inode *inode) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > + struct f2fs_inode_info *fi = F2FS_I(inode); > + nid_t xnid = fi->i_xattr_nid; > int err = 0; > > f2fs_abort_atomic_write(inode, true); > > + if (fi->cow_inode) { > + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > + iput(fi->cow_inode); > + fi->cow_inode = NULL; > + } > + > trace_f2fs_evict_inode(inode); > truncate_inode_pages_final(&inode->i_data); > > @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > stat_dec_inline_inode(inode); > stat_dec_compr_inode(inode); > stat_sub_compr_blocks(inode, > - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > + atomic_read(&fi->i_compr_blocks)); > > if (likely(!f2fs_cp_error(sbi) && > !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index ae3c4e5474ef..536d7c674b04 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > if (!f2fs_is_atomic_file(inode)) > return; > > - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > - iput(fi->cow_inode); > - fi->cow_inode = NULL; > release_atomic_write_cnt(inode); > clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > clear_inode_flag(inode, FI_ATOMIC_REPLACE); > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 1f812b9ce985..10463f084d30 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > atomic_inc(&inode->i_count); > spin_unlock(&inode->i_lock); > > - f2fs_abort_atomic_write(inode, true); In order to avoid caching obsolete page of cow_inode, how about truncating them here? if (f2fs_is_atomic_file() && cow_inode) truncate_inode_pages_final(&cow_inode->i_data); Thanks, > - > /* should remain fi->extent_tree for writepage */ > f2fs_destroy_extent_node(inode); >
Hi Chao, I read your patch series now and I like it. However, how about a race condition between start_atomic_write and abort_atomic_write? abort_atomic_write is called without inode_lock in closing filp scenarios. What do you think about this? Thanks, On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: > > Hi Daeho, Jaegeuk, > > Please take a look at patchset in below link: > > https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t > > In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me > know your preference. :) > > One comment as below. > > On 2023/1/13 8:49, Daeho Jeong wrote: > > From: Daeho Jeong <daehojeong@google.com> > > > > To fix a race condition between atomic write aborts, I use the inode > > lock and make COW inode to be re-usable thoroughout the whole > > atomic file inode lifetime. > > > > Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > > Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > > Signed-off-by: Daeho Jeong <daehojeong@google.com> > > --- > > fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > > fs/f2fs/inode.c | 11 +++++++++-- > > fs/f2fs/segment.c | 3 --- > > fs/f2fs/super.c | 2 -- > > 4 files changed, 37 insertions(+), 22 deletions(-) > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > index ecbc8c135b49..ff072a9ed258 100644 > > --- a/fs/f2fs/file.c > > +++ b/fs/f2fs/file.c > > @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > > atomic_read(&inode->i_writecount) != 1) > > return 0; > > > > + inode_lock(inode); > > f2fs_abort_atomic_write(inode, true); > > + inode_unlock(inode); > > + > > return 0; > > } > > > > @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > > * until all the writers close its file. Since this should be done > > * before dropping file lock, it needs to do in ->flush. > > */ > > - if (F2FS_I(inode)->atomic_write_task == current) > > + if (F2FS_I(inode)->atomic_write_task == current) { > > + inode_lock(inode); > > f2fs_abort_atomic_write(inode, true); > > + inode_unlock(inode); > > + } > > return 0; > > } > > > > @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > goto out; > > } > > > > - /* Create a COW inode for atomic write */ > > - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > - if (IS_ERR(pinode)) { > > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > - ret = PTR_ERR(pinode); > > - goto out; > > - } > > + /* Check if the inode already has a COW inode */ > > + if (fi->cow_inode == NULL) { > > + /* Create a COW inode for atomic write */ > > + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > + if (IS_ERR(pinode)) { > > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > + ret = PTR_ERR(pinode); > > + goto out; > > + } > > > > - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > - iput(pinode); > > - if (ret) { > > - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > - goto out; > > + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > + iput(pinode); > > + if (ret) { > > + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > + goto out; > > + } > > + > > + set_inode_flag(fi->cow_inode, FI_COW_FILE); > > + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > + } else { > > + /* Reuse the already created COW inode */ > > + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > > } > > > > f2fs_write_inode(inode, NULL); > > @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > stat_inc_atomic_inode(inode); > > > > set_inode_flag(inode, FI_ATOMIC_FILE); > > - set_inode_flag(fi->cow_inode, FI_COW_FILE); > > - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > > > isize = i_size_read(inode); > > fi->original_i_size = isize; > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > > index ff6cf66ed46b..4921f7209e28 100644 > > --- a/fs/f2fs/inode.c > > +++ b/fs/f2fs/inode.c > > @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > > void f2fs_evict_inode(struct inode *inode) > > { > > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > > + struct f2fs_inode_info *fi = F2FS_I(inode); > > + nid_t xnid = fi->i_xattr_nid; > > int err = 0; > > > > f2fs_abort_atomic_write(inode, true); > > > > + if (fi->cow_inode) { > > + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > + iput(fi->cow_inode); > > + fi->cow_inode = NULL; > > + } > > + > > trace_f2fs_evict_inode(inode); > > truncate_inode_pages_final(&inode->i_data); > > > > @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > > stat_dec_inline_inode(inode); > > stat_dec_compr_inode(inode); > > stat_sub_compr_blocks(inode, > > - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > > + atomic_read(&fi->i_compr_blocks)); > > > > if (likely(!f2fs_cp_error(sbi) && > > !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > index ae3c4e5474ef..536d7c674b04 100644 > > --- a/fs/f2fs/segment.c > > +++ b/fs/f2fs/segment.c > > @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > > if (!f2fs_is_atomic_file(inode)) > > return; > > > > - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > - iput(fi->cow_inode); > > - fi->cow_inode = NULL; > > release_atomic_write_cnt(inode); > > clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > > clear_inode_flag(inode, FI_ATOMIC_REPLACE); > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 1f812b9ce985..10463f084d30 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > > atomic_inc(&inode->i_count); > > spin_unlock(&inode->i_lock); > > > > - f2fs_abort_atomic_write(inode, true); > > In order to avoid caching obsolete page of cow_inode, how about truncating > them here? > > if (f2fs_is_atomic_file() && cow_inode) > truncate_inode_pages_final(&cow_inode->i_data); > > Thanks, > > > - > > /* should remain fi->extent_tree for writepage */ > > f2fs_destroy_extent_node(inode); > >
Hi Daeho, On 2023/1/31 0:34, Daeho Jeong wrote: > Hi Chao, > > I read your patch series now and I like it. Thank you for checking the patches. :) > However, how about a race condition between start_atomic_write and > abort_atomic_write? Yup, I noticed that issue, I guess we can avoid this race condition by covering these two flows w/ i_atomic_sem. > abort_atomic_write is called without inode_lock in closing filp scenarios. > What do you think about this? I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's page cache if atomic_write is committed or aborted to avoid caching obsolete page? Thanks, > > Thanks, > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: >> >> Hi Daeho, Jaegeuk, >> >> Please take a look at patchset in below link: >> >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t >> >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me >> know your preference. :) >> >> One comment as below. >> >> On 2023/1/13 8:49, Daeho Jeong wrote: >>> From: Daeho Jeong <daehojeong@google.com> >>> >>> To fix a race condition between atomic write aborts, I use the inode >>> lock and make COW inode to be re-usable thoroughout the whole >>> atomic file inode lifetime. >>> >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>> --- >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- >>> fs/f2fs/inode.c | 11 +++++++++-- >>> fs/f2fs/segment.c | 3 --- >>> fs/f2fs/super.c | 2 -- >>> 4 files changed, 37 insertions(+), 22 deletions(-) >>> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>> index ecbc8c135b49..ff072a9ed258 100644 >>> --- a/fs/f2fs/file.c >>> +++ b/fs/f2fs/file.c >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) >>> atomic_read(&inode->i_writecount) != 1) >>> return 0; >>> >>> + inode_lock(inode); >>> f2fs_abort_atomic_write(inode, true); >>> + inode_unlock(inode); >>> + >>> return 0; >>> } >>> >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) >>> * until all the writers close its file. Since this should be done >>> * before dropping file lock, it needs to do in ->flush. >>> */ >>> - if (F2FS_I(inode)->atomic_write_task == current) >>> + if (F2FS_I(inode)->atomic_write_task == current) { >>> + inode_lock(inode); >>> f2fs_abort_atomic_write(inode, true); >>> + inode_unlock(inode); >>> + } >>> return 0; >>> } >>> >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>> goto out; >>> } >>> >>> - /* Create a COW inode for atomic write */ >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>> - if (IS_ERR(pinode)) { >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> - ret = PTR_ERR(pinode); >>> - goto out; >>> - } >>> + /* Check if the inode already has a COW inode */ >>> + if (fi->cow_inode == NULL) { >>> + /* Create a COW inode for atomic write */ >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>> + if (IS_ERR(pinode)) { >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> + ret = PTR_ERR(pinode); >>> + goto out; >>> + } >>> >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>> - iput(pinode); >>> - if (ret) { >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> - goto out; >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>> + iput(pinode); >>> + if (ret) { >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>> + goto out; >>> + } >>> + >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>> + } else { >>> + /* Reuse the already created COW inode */ >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); >>> } >>> >>> f2fs_write_inode(inode, NULL); >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>> stat_inc_atomic_inode(inode); >>> >>> set_inode_flag(inode, FI_ATOMIC_FILE); >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>> >>> isize = i_size_read(inode); >>> fi->original_i_size = isize; >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>> index ff6cf66ed46b..4921f7209e28 100644 >>> --- a/fs/f2fs/inode.c >>> +++ b/fs/f2fs/inode.c >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >>> void f2fs_evict_inode(struct inode *inode) >>> { >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; >>> + struct f2fs_inode_info *fi = F2FS_I(inode); >>> + nid_t xnid = fi->i_xattr_nid; >>> int err = 0; >>> >>> f2fs_abort_atomic_write(inode, true); >>> >>> + if (fi->cow_inode) { >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>> + iput(fi->cow_inode); >>> + fi->cow_inode = NULL; >>> + } >>> + >>> trace_f2fs_evict_inode(inode); >>> truncate_inode_pages_final(&inode->i_data); >>> >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) >>> stat_dec_inline_inode(inode); >>> stat_dec_compr_inode(inode); >>> stat_sub_compr_blocks(inode, >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); >>> + atomic_read(&fi->i_compr_blocks)); >>> >>> if (likely(!f2fs_cp_error(sbi) && >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index ae3c4e5474ef..536d7c674b04 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) >>> if (!f2fs_is_atomic_file(inode)) >>> return; >>> >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>> - iput(fi->cow_inode); >>> - fi->cow_inode = NULL; >>> release_atomic_write_cnt(inode); >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 1f812b9ce985..10463f084d30 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) >>> atomic_inc(&inode->i_count); >>> spin_unlock(&inode->i_lock); >>> >>> - f2fs_abort_atomic_write(inode, true); >> >> In order to avoid caching obsolete page of cow_inode, how about truncating >> them here? >> >> if (f2fs_is_atomic_file() && cow_inode) >> truncate_inode_pages_final(&cow_inode->i_data); >> >> Thanks, >> >>> - >>> /* should remain fi->extent_tree for writepage */ >>> f2fs_destroy_extent_node(inode); >>>
Hi Chao, On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote: > > Hi Daeho, > > On 2023/1/31 0:34, Daeho Jeong wrote: > > Hi Chao, > > > > I read your patch series now and I like it. > > Thank you for checking the patches. :) > > > However, how about a race condition between start_atomic_write and > > abort_atomic_write? > > Yup, I noticed that issue, I guess we can avoid this race condition by > covering these two flows w/ i_atomic_sem. > > > abort_atomic_write is called without inode_lock in closing filp scenarios. > > What do you think about this? > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's > page cache if atomic_write is committed or aborted to avoid caching obsolete page? It's better to put that part in f2fs_abort_atomic_write(). On top of that, maybe, we should move f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too. Thanks, > > Thanks, > > > > > Thanks, > > > > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: > >> > >> Hi Daeho, Jaegeuk, > >> > >> Please take a look at patchset in below link: > >> > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t > >> > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me > >> know your preference. :) > >> > >> One comment as below. > >> > >> On 2023/1/13 8:49, Daeho Jeong wrote: > >>> From: Daeho Jeong <daehojeong@google.com> > >>> > >>> To fix a race condition between atomic write aborts, I use the inode > >>> lock and make COW inode to be re-usable thoroughout the whole > >>> atomic file inode lifetime. > >>> > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > >>> --- > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > >>> fs/f2fs/inode.c | 11 +++++++++-- > >>> fs/f2fs/segment.c | 3 --- > >>> fs/f2fs/super.c | 2 -- > >>> 4 files changed, 37 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>> index ecbc8c135b49..ff072a9ed258 100644 > >>> --- a/fs/f2fs/file.c > >>> +++ b/fs/f2fs/file.c > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > >>> atomic_read(&inode->i_writecount) != 1) > >>> return 0; > >>> > >>> + inode_lock(inode); > >>> f2fs_abort_atomic_write(inode, true); > >>> + inode_unlock(inode); > >>> + > >>> return 0; > >>> } > >>> > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > >>> * until all the writers close its file. Since this should be done > >>> * before dropping file lock, it needs to do in ->flush. > >>> */ > >>> - if (F2FS_I(inode)->atomic_write_task == current) > >>> + if (F2FS_I(inode)->atomic_write_task == current) { > >>> + inode_lock(inode); > >>> f2fs_abort_atomic_write(inode, true); > >>> + inode_unlock(inode); > >>> + } > >>> return 0; > >>> } > >>> > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > >>> goto out; > >>> } > >>> > >>> - /* Create a COW inode for atomic write */ > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > >>> - if (IS_ERR(pinode)) { > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>> - ret = PTR_ERR(pinode); > >>> - goto out; > >>> - } > >>> + /* Check if the inode already has a COW inode */ > >>> + if (fi->cow_inode == NULL) { > >>> + /* Create a COW inode for atomic write */ > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > >>> + if (IS_ERR(pinode)) { > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>> + ret = PTR_ERR(pinode); > >>> + goto out; > >>> + } > >>> > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > >>> - iput(pinode); > >>> - if (ret) { > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>> - goto out; > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > >>> + iput(pinode); > >>> + if (ret) { > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>> + goto out; > >>> + } > >>> + > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > >>> + } else { > >>> + /* Reuse the already created COW inode */ > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > >>> } > >>> > >>> f2fs_write_inode(inode, NULL); > >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > >>> stat_inc_atomic_inode(inode); > >>> > >>> set_inode_flag(inode, FI_ATOMIC_FILE); > >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); > >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > >>> > >>> isize = i_size_read(inode); > >>> fi->original_i_size = isize; > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > >>> index ff6cf66ed46b..4921f7209e28 100644 > >>> --- a/fs/f2fs/inode.c > >>> +++ b/fs/f2fs/inode.c > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > >>> void f2fs_evict_inode(struct inode *inode) > >>> { > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > >>> + struct f2fs_inode_info *fi = F2FS_I(inode); > >>> + nid_t xnid = fi->i_xattr_nid; > >>> int err = 0; > >>> > >>> f2fs_abort_atomic_write(inode, true); > >>> > >>> + if (fi->cow_inode) { > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > >>> + iput(fi->cow_inode); > >>> + fi->cow_inode = NULL; > >>> + } > >>> + > >>> trace_f2fs_evict_inode(inode); > >>> truncate_inode_pages_final(&inode->i_data); > >>> > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > >>> stat_dec_inline_inode(inode); > >>> stat_dec_compr_inode(inode); > >>> stat_sub_compr_blocks(inode, > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > >>> + atomic_read(&fi->i_compr_blocks)); > >>> > >>> if (likely(!f2fs_cp_error(sbi) && > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >>> index ae3c4e5474ef..536d7c674b04 100644 > >>> --- a/fs/f2fs/segment.c > >>> +++ b/fs/f2fs/segment.c > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > >>> if (!f2fs_is_atomic_file(inode)) > >>> return; > >>> > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > >>> - iput(fi->cow_inode); > >>> - fi->cow_inode = NULL; > >>> release_atomic_write_cnt(inode); > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>> index 1f812b9ce985..10463f084d30 100644 > >>> --- a/fs/f2fs/super.c > >>> +++ b/fs/f2fs/super.c > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > >>> atomic_inc(&inode->i_count); > >>> spin_unlock(&inode->i_lock); > >>> > >>> - f2fs_abort_atomic_write(inode, true); > >> > >> In order to avoid caching obsolete page of cow_inode, how about truncating > >> them here? > >> > >> if (f2fs_is_atomic_file() && cow_inode) > >> truncate_inode_pages_final(&cow_inode->i_data); > >> > >> Thanks, > >> > >>> - > >>> /* should remain fi->extent_tree for writepage */ > >>> f2fs_destroy_extent_node(inode); > >>>
On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote: > > Hi Chao, > > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote: > > > > Hi Daeho, > > > > On 2023/1/31 0:34, Daeho Jeong wrote: > > > Hi Chao, > > > > > > I read your patch series now and I like it. > > > > Thank you for checking the patches. :) > > > > > However, how about a race condition between start_atomic_write and > > > abort_atomic_write? > > > > Yup, I noticed that issue, I guess we can avoid this race condition by > > covering these two flows w/ i_atomic_sem. > > > > > abort_atomic_write is called without inode_lock in closing filp scenarios. > > > What do you think about this? > > > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's > > page cache if atomic_write is committed or aborted to avoid caching obsolete page? > > It's better to put that part in f2fs_abort_atomic_write(). > On top of that, maybe, we should move > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too. Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there might be some left writeback after aborting atomic write. Plz. review it related to the timing of calling truncate_inode_pages_final(). > > Thanks, > > > > > Thanks, > > > > > > > > Thanks, > > > > > > > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: > > >> > > >> Hi Daeho, Jaegeuk, > > >> > > >> Please take a look at patchset in below link: > > >> > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t > > >> > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me > > >> know your preference. :) > > >> > > >> One comment as below. > > >> > > >> On 2023/1/13 8:49, Daeho Jeong wrote: > > >>> From: Daeho Jeong <daehojeong@google.com> > > >>> > > >>> To fix a race condition between atomic write aborts, I use the inode > > >>> lock and make COW inode to be re-usable thoroughout the whole > > >>> atomic file inode lifetime. > > >>> > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > > >>> --- > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > > >>> fs/f2fs/inode.c | 11 +++++++++-- > > >>> fs/f2fs/segment.c | 3 --- > > >>> fs/f2fs/super.c | 2 -- > > >>> 4 files changed, 37 insertions(+), 22 deletions(-) > > >>> > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > >>> index ecbc8c135b49..ff072a9ed258 100644 > > >>> --- a/fs/f2fs/file.c > > >>> +++ b/fs/f2fs/file.c > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > > >>> atomic_read(&inode->i_writecount) != 1) > > >>> return 0; > > >>> > > >>> + inode_lock(inode); > > >>> f2fs_abort_atomic_write(inode, true); > > >>> + inode_unlock(inode); > > >>> + > > >>> return 0; > > >>> } > > >>> > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > > >>> * until all the writers close its file. Since this should be done > > >>> * before dropping file lock, it needs to do in ->flush. > > >>> */ > > >>> - if (F2FS_I(inode)->atomic_write_task == current) > > >>> + if (F2FS_I(inode)->atomic_write_task == current) { > > >>> + inode_lock(inode); > > >>> f2fs_abort_atomic_write(inode, true); > > >>> + inode_unlock(inode); > > >>> + } > > >>> return 0; > > >>> } > > >>> > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > >>> goto out; > > >>> } > > >>> > > >>> - /* Create a COW inode for atomic write */ > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > >>> - if (IS_ERR(pinode)) { > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > >>> - ret = PTR_ERR(pinode); > > >>> - goto out; > > >>> - } > > >>> + /* Check if the inode already has a COW inode */ > > >>> + if (fi->cow_inode == NULL) { > > >>> + /* Create a COW inode for atomic write */ > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > >>> + if (IS_ERR(pinode)) { > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > >>> + ret = PTR_ERR(pinode); > > >>> + goto out; > > >>> + } > > >>> > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > >>> - iput(pinode); > > >>> - if (ret) { > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > >>> - goto out; > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > >>> + iput(pinode); > > >>> + if (ret) { > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > >>> + goto out; > > >>> + } > > >>> + > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > >>> + } else { > > >>> + /* Reuse the already created COW inode */ > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > > >>> } > > >>> > > >>> f2fs_write_inode(inode, NULL); > > >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > >>> stat_inc_atomic_inode(inode); > > >>> > > >>> set_inode_flag(inode, FI_ATOMIC_FILE); > > >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); > > >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > >>> > > >>> isize = i_size_read(inode); > > >>> fi->original_i_size = isize; > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > > >>> index ff6cf66ed46b..4921f7209e28 100644 > > >>> --- a/fs/f2fs/inode.c > > >>> +++ b/fs/f2fs/inode.c > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > > >>> void f2fs_evict_inode(struct inode *inode) > > >>> { > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode); > > >>> + nid_t xnid = fi->i_xattr_nid; > > >>> int err = 0; > > >>> > > >>> f2fs_abort_atomic_write(inode, true); > > >>> > > >>> + if (fi->cow_inode) { > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > >>> + iput(fi->cow_inode); > > >>> + fi->cow_inode = NULL; > > >>> + } > > >>> + > > >>> trace_f2fs_evict_inode(inode); > > >>> truncate_inode_pages_final(&inode->i_data); > > >>> > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > > >>> stat_dec_inline_inode(inode); > > >>> stat_dec_compr_inode(inode); > > >>> stat_sub_compr_blocks(inode, > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > > >>> + atomic_read(&fi->i_compr_blocks)); > > >>> > > >>> if (likely(!f2fs_cp_error(sbi) && > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > >>> index ae3c4e5474ef..536d7c674b04 100644 > > >>> --- a/fs/f2fs/segment.c > > >>> +++ b/fs/f2fs/segment.c > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > > >>> if (!f2fs_is_atomic_file(inode)) > > >>> return; > > >>> > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > >>> - iput(fi->cow_inode); > > >>> - fi->cow_inode = NULL; > > >>> release_atomic_write_cnt(inode); > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > >>> index 1f812b9ce985..10463f084d30 100644 > > >>> --- a/fs/f2fs/super.c > > >>> +++ b/fs/f2fs/super.c > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > > >>> atomic_inc(&inode->i_count); > > >>> spin_unlock(&inode->i_lock); > > >>> > > >>> - f2fs_abort_atomic_write(inode, true); > > >> > > >> In order to avoid caching obsolete page of cow_inode, how about truncating > > >> them here? > > >> > > >> if (f2fs_is_atomic_file() && cow_inode) > > >> truncate_inode_pages_final(&cow_inode->i_data); > > >> > > >> Thanks, > > >> > > >>> - > > >>> /* should remain fi->extent_tree for writepage */ > > >>> f2fs_destroy_extent_node(inode); > > >>>
On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote: > > On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote: > > > > Hi Chao, > > > > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote: > > > > > > Hi Daeho, > > > > > > On 2023/1/31 0:34, Daeho Jeong wrote: > > > > Hi Chao, > > > > > > > > I read your patch series now and I like it. > > > > > > Thank you for checking the patches. :) > > > > > > > However, how about a race condition between start_atomic_write and > > > > abort_atomic_write? > > > > > > Yup, I noticed that issue, I guess we can avoid this race condition by > > > covering these two flows w/ i_atomic_sem. > > > > > > > abort_atomic_write is called without inode_lock in closing filp scenarios. > > > > What do you think about this? > > > > > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's > > > page cache if atomic_write is committed or aborted to avoid caching obsolete page? > > > > It's better to put that part in f2fs_abort_atomic_write(). > > On top of that, maybe, we should move > > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from > > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too. > > Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there > might be some left writeback after aborting atomic write. > Plz. review it related to the timing of calling truncate_inode_pages_final(). Looks like the scenario becomes too complicated if I think about more than one writer's scenario. How about we check writecount in commit_atomic_write ioctl and return EBUSY when it's not only one writer? In that case, we can make the scenario simple and effective, and we can release all the resources in abort_atomic_write(). > > > > > Thanks, > > > > > > > > Thanks, > > > > > > > > > > > Thanks, > > > > > > > > > > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: > > > >> > > > >> Hi Daeho, Jaegeuk, > > > >> > > > >> Please take a look at patchset in below link: > > > >> > > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t > > > >> > > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me > > > >> know your preference. :) > > > >> > > > >> One comment as below. > > > >> > > > >> On 2023/1/13 8:49, Daeho Jeong wrote: > > > >>> From: Daeho Jeong <daehojeong@google.com> > > > >>> > > > >>> To fix a race condition between atomic write aborts, I use the inode > > > >>> lock and make COW inode to be re-usable thoroughout the whole > > > >>> atomic file inode lifetime. > > > >>> > > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > > > >>> --- > > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > > > >>> fs/f2fs/inode.c | 11 +++++++++-- > > > >>> fs/f2fs/segment.c | 3 --- > > > >>> fs/f2fs/super.c | 2 -- > > > >>> 4 files changed, 37 insertions(+), 22 deletions(-) > > > >>> > > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > >>> index ecbc8c135b49..ff072a9ed258 100644 > > > >>> --- a/fs/f2fs/file.c > > > >>> +++ b/fs/f2fs/file.c > > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > > > >>> atomic_read(&inode->i_writecount) != 1) > > > >>> return 0; > > > >>> > > > >>> + inode_lock(inode); > > > >>> f2fs_abort_atomic_write(inode, true); > > > >>> + inode_unlock(inode); > > > >>> + > > > >>> return 0; > > > >>> } > > > >>> > > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > > > >>> * until all the writers close its file. Since this should be done > > > >>> * before dropping file lock, it needs to do in ->flush. > > > >>> */ > > > >>> - if (F2FS_I(inode)->atomic_write_task == current) > > > >>> + if (F2FS_I(inode)->atomic_write_task == current) { > > > >>> + inode_lock(inode); > > > >>> f2fs_abort_atomic_write(inode, true); > > > >>> + inode_unlock(inode); > > > >>> + } > > > >>> return 0; > > > >>> } > > > >>> > > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > > >>> goto out; > > > >>> } > > > >>> > > > >>> - /* Create a COW inode for atomic write */ > > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > > >>> - if (IS_ERR(pinode)) { > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > >>> - ret = PTR_ERR(pinode); > > > >>> - goto out; > > > >>> - } > > > >>> + /* Check if the inode already has a COW inode */ > > > >>> + if (fi->cow_inode == NULL) { > > > >>> + /* Create a COW inode for atomic write */ > > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > > >>> + if (IS_ERR(pinode)) { > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > >>> + ret = PTR_ERR(pinode); > > > >>> + goto out; > > > >>> + } > > > >>> > > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > > >>> - iput(pinode); > > > >>> - if (ret) { > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > >>> - goto out; > > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > > >>> + iput(pinode); > > > >>> + if (ret) { > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > >>> + goto out; > > > >>> + } > > > >>> + > > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); > > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > > >>> + } else { > > > >>> + /* Reuse the already created COW inode */ > > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > > > >>> } > > > >>> > > > >>> f2fs_write_inode(inode, NULL); > > > >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > > >>> stat_inc_atomic_inode(inode); > > > >>> > > > >>> set_inode_flag(inode, FI_ATOMIC_FILE); > > > >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); > > > >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > > >>> > > > >>> isize = i_size_read(inode); > > > >>> fi->original_i_size = isize; > > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > > > >>> index ff6cf66ed46b..4921f7209e28 100644 > > > >>> --- a/fs/f2fs/inode.c > > > >>> +++ b/fs/f2fs/inode.c > > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > > > >>> void f2fs_evict_inode(struct inode *inode) > > > >>> { > > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode); > > > >>> + nid_t xnid = fi->i_xattr_nid; > > > >>> int err = 0; > > > >>> > > > >>> f2fs_abort_atomic_write(inode, true); > > > >>> > > > >>> + if (fi->cow_inode) { > > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > > >>> + iput(fi->cow_inode); > > > >>> + fi->cow_inode = NULL; > > > >>> + } > > > >>> + > > > >>> trace_f2fs_evict_inode(inode); > > > >>> truncate_inode_pages_final(&inode->i_data); > > > >>> > > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > > > >>> stat_dec_inline_inode(inode); > > > >>> stat_dec_compr_inode(inode); > > > >>> stat_sub_compr_blocks(inode, > > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > > > >>> + atomic_read(&fi->i_compr_blocks)); > > > >>> > > > >>> if (likely(!f2fs_cp_error(sbi) && > > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > > >>> index ae3c4e5474ef..536d7c674b04 100644 > > > >>> --- a/fs/f2fs/segment.c > > > >>> +++ b/fs/f2fs/segment.c > > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > > > >>> if (!f2fs_is_atomic_file(inode)) > > > >>> return; > > > >>> > > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > > >>> - iput(fi->cow_inode); > > > >>> - fi->cow_inode = NULL; > > > >>> release_atomic_write_cnt(inode); > > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); > > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > > >>> index 1f812b9ce985..10463f084d30 100644 > > > >>> --- a/fs/f2fs/super.c > > > >>> +++ b/fs/f2fs/super.c > > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > > > >>> atomic_inc(&inode->i_count); > > > >>> spin_unlock(&inode->i_lock); > > > >>> > > > >>> - f2fs_abort_atomic_write(inode, true); > > > >> > > > >> In order to avoid caching obsolete page of cow_inode, how about truncating > > > >> them here? > > > >> > > > >> if (f2fs_is_atomic_file() && cow_inode) > > > >> truncate_inode_pages_final(&cow_inode->i_data); > > > >> > > > >> Thanks, > > > >> > > > >>> - > > > >>> /* should remain fi->extent_tree for writepage */ > > > >>> f2fs_destroy_extent_node(inode); > > > >>>
On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote: > > On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote: > > > > On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote: > > > > > > Hi Chao, > > > > > > On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote: > > > > > > > > Hi Daeho, > > > > > > > > On 2023/1/31 0:34, Daeho Jeong wrote: > > > > > Hi Chao, > > > > > > > > > > I read your patch series now and I like it. > > > > > > > > Thank you for checking the patches. :) > > > > > > > > > However, how about a race condition between start_atomic_write and > > > > > abort_atomic_write? > > > > > > > > Yup, I noticed that issue, I guess we can avoid this race condition by > > > > covering these two flows w/ i_atomic_sem. > > > > > > > > > abort_atomic_write is called without inode_lock in closing filp scenarios. > > > > > What do you think about this? > > > > > > > > I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's > > > > page cache if atomic_write is committed or aborted to avoid caching obsolete page? > > > > > > It's better to put that part in f2fs_abort_atomic_write(). > > > On top of that, maybe, we should move > > > f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from > > > f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too. > > > > Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there > > might be some left writeback after aborting atomic write. > > Plz. review it related to the timing of calling truncate_inode_pages_final(). > > Looks like the scenario becomes too complicated if I think about more > than one writer's scenario. > How about we check writecount in commit_atomic_write ioctl and return > EBUSY when it's not only one writer? > In that case, we can make the scenario simple and effective, and we > can release all the resources in abort_atomic_write(). Oh, I totally forgot this. We don't use pages of COW inode. So, we don't need to clean them up. > > > > > > > > > Thanks, > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: > > > > >> > > > > >> Hi Daeho, Jaegeuk, > > > > >> > > > > >> Please take a look at patchset in below link: > > > > >> > > > > >> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t > > > > >> > > > > >> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me > > > > >> know your preference. :) > > > > >> > > > > >> One comment as below. > > > > >> > > > > >> On 2023/1/13 8:49, Daeho Jeong wrote: > > > > >>> From: Daeho Jeong <daehojeong@google.com> > > > > >>> > > > > >>> To fix a race condition between atomic write aborts, I use the inode > > > > >>> lock and make COW inode to be re-usable thoroughout the whole > > > > >>> atomic file inode lifetime. > > > > >>> > > > > >>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > > > > >>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > > > > >>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > > > > >>> --- > > > > >>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > > > > >>> fs/f2fs/inode.c | 11 +++++++++-- > > > > >>> fs/f2fs/segment.c | 3 --- > > > > >>> fs/f2fs/super.c | 2 -- > > > > >>> 4 files changed, 37 insertions(+), 22 deletions(-) > > > > >>> > > > > >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > >>> index ecbc8c135b49..ff072a9ed258 100644 > > > > >>> --- a/fs/f2fs/file.c > > > > >>> +++ b/fs/f2fs/file.c > > > > >>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > > > > >>> atomic_read(&inode->i_writecount) != 1) > > > > >>> return 0; > > > > >>> > > > > >>> + inode_lock(inode); > > > > >>> f2fs_abort_atomic_write(inode, true); > > > > >>> + inode_unlock(inode); > > > > >>> + > > > > >>> return 0; > > > > >>> } > > > > >>> > > > > >>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > > > > >>> * until all the writers close its file. Since this should be done > > > > >>> * before dropping file lock, it needs to do in ->flush. > > > > >>> */ > > > > >>> - if (F2FS_I(inode)->atomic_write_task == current) > > > > >>> + if (F2FS_I(inode)->atomic_write_task == current) { > > > > >>> + inode_lock(inode); > > > > >>> f2fs_abort_atomic_write(inode, true); > > > > >>> + inode_unlock(inode); > > > > >>> + } > > > > >>> return 0; > > > > >>> } > > > > >>> > > > > >>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > > > >>> goto out; > > > > >>> } > > > > >>> > > > > >>> - /* Create a COW inode for atomic write */ > > > > >>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > > > >>> - if (IS_ERR(pinode)) { > > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > > >>> - ret = PTR_ERR(pinode); > > > > >>> - goto out; > > > > >>> - } > > > > >>> + /* Check if the inode already has a COW inode */ > > > > >>> + if (fi->cow_inode == NULL) { > > > > >>> + /* Create a COW inode for atomic write */ > > > > >>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > > > > >>> + if (IS_ERR(pinode)) { > > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > > >>> + ret = PTR_ERR(pinode); > > > > >>> + goto out; > > > > >>> + } > > > > >>> > > > > >>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > > > >>> - iput(pinode); > > > > >>> - if (ret) { > > > > >>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > > >>> - goto out; > > > > >>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > > > > >>> + iput(pinode); > > > > >>> + if (ret) { > > > > >>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > > > > >>> + goto out; > > > > >>> + } > > > > >>> + > > > > >>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); > > > > >>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > > > >>> + } else { > > > > >>> + /* Reuse the already created COW inode */ > > > > >>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > > > > >>> } > > > > >>> > > > > >>> f2fs_write_inode(inode, NULL); > > > > >>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > > > > >>> stat_inc_atomic_inode(inode); > > > > >>> > > > > >>> set_inode_flag(inode, FI_ATOMIC_FILE); > > > > >>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); > > > > >>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > > > > >>> > > > > >>> isize = i_size_read(inode); > > > > >>> fi->original_i_size = isize; > > > > >>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > > > > >>> index ff6cf66ed46b..4921f7209e28 100644 > > > > >>> --- a/fs/f2fs/inode.c > > > > >>> +++ b/fs/f2fs/inode.c > > > > >>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > > > > >>> void f2fs_evict_inode(struct inode *inode) > > > > >>> { > > > > >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > > > >>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > > > > >>> + struct f2fs_inode_info *fi = F2FS_I(inode); > > > > >>> + nid_t xnid = fi->i_xattr_nid; > > > > >>> int err = 0; > > > > >>> > > > > >>> f2fs_abort_atomic_write(inode, true); > > > > >>> > > > > >>> + if (fi->cow_inode) { > > > > >>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > > > >>> + iput(fi->cow_inode); > > > > >>> + fi->cow_inode = NULL; > > > > >>> + } > > > > >>> + > > > > >>> trace_f2fs_evict_inode(inode); > > > > >>> truncate_inode_pages_final(&inode->i_data); > > > > >>> > > > > >>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > > > > >>> stat_dec_inline_inode(inode); > > > > >>> stat_dec_compr_inode(inode); > > > > >>> stat_sub_compr_blocks(inode, > > > > >>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > > > > >>> + atomic_read(&fi->i_compr_blocks)); > > > > >>> > > > > >>> if (likely(!f2fs_cp_error(sbi) && > > > > >>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > > > > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > > > >>> index ae3c4e5474ef..536d7c674b04 100644 > > > > >>> --- a/fs/f2fs/segment.c > > > > >>> +++ b/fs/f2fs/segment.c > > > > >>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > > > > >>> if (!f2fs_is_atomic_file(inode)) > > > > >>> return; > > > > >>> > > > > >>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > > > > >>> - iput(fi->cow_inode); > > > > >>> - fi->cow_inode = NULL; > > > > >>> release_atomic_write_cnt(inode); > > > > >>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > > > > >>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); > > > > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > > > >>> index 1f812b9ce985..10463f084d30 100644 > > > > >>> --- a/fs/f2fs/super.c > > > > >>> +++ b/fs/f2fs/super.c > > > > >>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > > > > >>> atomic_inc(&inode->i_count); > > > > >>> spin_unlock(&inode->i_lock); > > > > >>> > > > > >>> - f2fs_abort_atomic_write(inode, true); > > > > >> > > > > >> In order to avoid caching obsolete page of cow_inode, how about truncating > > > > >> them here? > > > > >> > > > > >> if (f2fs_is_atomic_file() && cow_inode) > > > > >> truncate_inode_pages_final(&cow_inode->i_data); > > > > >> > > > > >> Thanks, > > > > >> > > > > >>> - > > > > >>> /* should remain fi->extent_tree for writepage */ > > > > >>> f2fs_destroy_extent_node(inode); > > > > >>>
Hi Daeho, On 2023/2/1 6:34, Daeho Jeong wrote: > On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote: >> >> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote: >>> >>> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote: >>>> >>>> Hi Chao, >>>> >>>> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote: >>>>> >>>>> Hi Daeho, >>>>> >>>>> On 2023/1/31 0:34, Daeho Jeong wrote: >>>>>> Hi Chao, >>>>>> >>>>>> I read your patch series now and I like it. >>>>> >>>>> Thank you for checking the patches. :) >>>>> >>>>>> However, how about a race condition between start_atomic_write and >>>>>> abort_atomic_write? >>>>> >>>>> Yup, I noticed that issue, I guess we can avoid this race condition by >>>>> covering these two flows w/ i_atomic_sem. >>>>> >>>>>> abort_atomic_write is called without inode_lock in closing filp scenarios. >>>>>> What do you think about this? >>>>> >>>>> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's >>>>> page cache if atomic_write is committed or aborted to avoid caching obsolete page? >>>> >>>> It's better to put that part in f2fs_abort_atomic_write(). >>>> On top of that, maybe, we should move >>>> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from >>>> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too. >>> >>> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there >>> might be some left writeback after aborting atomic write. >>> Plz. review it related to the timing of calling truncate_inode_pages_final(). >> >> Looks like the scenario becomes too complicated if I think about more >> than one writer's scenario. >> How about we check writecount in commit_atomic_write ioctl and return >> EBUSY when it's not only one writer? >> In that case, we can make the scenario simple and effective, and we >> can release all the resources in abort_atomic_write(). > > Oh, I totally forgot this. We don't use pages of COW inode. So, we Yes, after atomic_write is committed or aborted, we don't use pages of cow_inode, so they are obsolete, IMO, we'd better to reclaim them immediately to avoid unnecessary memory use. The implementation may be: - f2fs_ioc_abort_atomic_write - f2fs_abort_atomic_write - truncate_inode_pages_final(cow_inode) - f2fs_ioc_commit_atomic_write - f2fs_abort_atomic_write - truncate_inode_pages_final(cow_inode) Thanks, > don't need to clean them up. > >> >>> >>>> >>>> Thanks, >>>> >>>>> >>>>> Thanks, >>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> >>>>>> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: >>>>>>> >>>>>>> Hi Daeho, Jaegeuk, >>>>>>> >>>>>>> Please take a look at patchset in below link: >>>>>>> >>>>>>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t >>>>>>> >>>>>>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me >>>>>>> know your preference. :) >>>>>>> >>>>>>> One comment as below. >>>>>>> >>>>>>> On 2023/1/13 8:49, Daeho Jeong wrote: >>>>>>>> From: Daeho Jeong <daehojeong@google.com> >>>>>>>> >>>>>>>> To fix a race condition between atomic write aborts, I use the inode >>>>>>>> lock and make COW inode to be re-usable thoroughout the whole >>>>>>>> atomic file inode lifetime. >>>>>>>> >>>>>>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com >>>>>>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") >>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> >>>>>>>> --- >>>>>>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- >>>>>>>> fs/f2fs/inode.c | 11 +++++++++-- >>>>>>>> fs/f2fs/segment.c | 3 --- >>>>>>>> fs/f2fs/super.c | 2 -- >>>>>>>> 4 files changed, 37 insertions(+), 22 deletions(-) >>>>>>>> >>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>>>>> index ecbc8c135b49..ff072a9ed258 100644 >>>>>>>> --- a/fs/f2fs/file.c >>>>>>>> +++ b/fs/f2fs/file.c >>>>>>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) >>>>>>>> atomic_read(&inode->i_writecount) != 1) >>>>>>>> return 0; >>>>>>>> >>>>>>>> + inode_lock(inode); >>>>>>>> f2fs_abort_atomic_write(inode, true); >>>>>>>> + inode_unlock(inode); >>>>>>>> + >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) >>>>>>>> * until all the writers close its file. Since this should be done >>>>>>>> * before dropping file lock, it needs to do in ->flush. >>>>>>>> */ >>>>>>>> - if (F2FS_I(inode)->atomic_write_task == current) >>>>>>>> + if (F2FS_I(inode)->atomic_write_task == current) { >>>>>>>> + inode_lock(inode); >>>>>>>> f2fs_abort_atomic_write(inode, true); >>>>>>>> + inode_unlock(inode); >>>>>>>> + } >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>>>>>>> goto out; >>>>>>>> } >>>>>>>> >>>>>>>> - /* Create a COW inode for atomic write */ >>>>>>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>>>>>>> - if (IS_ERR(pinode)) { >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>>>>>>> - ret = PTR_ERR(pinode); >>>>>>>> - goto out; >>>>>>>> - } >>>>>>>> + /* Check if the inode already has a COW inode */ >>>>>>>> + if (fi->cow_inode == NULL) { >>>>>>>> + /* Create a COW inode for atomic write */ >>>>>>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); >>>>>>>> + if (IS_ERR(pinode)) { >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>>>>>>> + ret = PTR_ERR(pinode); >>>>>>>> + goto out; >>>>>>>> + } >>>>>>>> >>>>>>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>>>>>>> - iput(pinode); >>>>>>>> - if (ret) { >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>>>>>>> - goto out; >>>>>>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); >>>>>>>> + iput(pinode); >>>>>>>> + if (ret) { >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); >>>>>>>> + goto out; >>>>>>>> + } >>>>>>>> + >>>>>>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>>>>>>> + } else { >>>>>>>> + /* Reuse the already created COW inode */ >>>>>>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); >>>>>>>> } >>>>>>>> >>>>>>>> f2fs_write_inode(inode, NULL); >>>>>>>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) >>>>>>>> stat_inc_atomic_inode(inode); >>>>>>>> >>>>>>>> set_inode_flag(inode, FI_ATOMIC_FILE); >>>>>>>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); >>>>>>>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); >>>>>>>> >>>>>>>> isize = i_size_read(inode); >>>>>>>> fi->original_i_size = isize; >>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >>>>>>>> index ff6cf66ed46b..4921f7209e28 100644 >>>>>>>> --- a/fs/f2fs/inode.c >>>>>>>> +++ b/fs/f2fs/inode.c >>>>>>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) >>>>>>>> void f2fs_evict_inode(struct inode *inode) >>>>>>>> { >>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>>>>>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; >>>>>>>> + struct f2fs_inode_info *fi = F2FS_I(inode); >>>>>>>> + nid_t xnid = fi->i_xattr_nid; >>>>>>>> int err = 0; >>>>>>>> >>>>>>>> f2fs_abort_atomic_write(inode, true); >>>>>>>> >>>>>>>> + if (fi->cow_inode) { >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>>>>>>> + iput(fi->cow_inode); >>>>>>>> + fi->cow_inode = NULL; >>>>>>>> + } >>>>>>>> + >>>>>>>> trace_f2fs_evict_inode(inode); >>>>>>>> truncate_inode_pages_final(&inode->i_data); >>>>>>>> >>>>>>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) >>>>>>>> stat_dec_inline_inode(inode); >>>>>>>> stat_dec_compr_inode(inode); >>>>>>>> stat_sub_compr_blocks(inode, >>>>>>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); >>>>>>>> + atomic_read(&fi->i_compr_blocks)); >>>>>>>> >>>>>>>> if (likely(!f2fs_cp_error(sbi) && >>>>>>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) >>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>>>>>> index ae3c4e5474ef..536d7c674b04 100644 >>>>>>>> --- a/fs/f2fs/segment.c >>>>>>>> +++ b/fs/f2fs/segment.c >>>>>>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) >>>>>>>> if (!f2fs_is_atomic_file(inode)) >>>>>>>> return; >>>>>>>> >>>>>>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); >>>>>>>> - iput(fi->cow_inode); >>>>>>>> - fi->cow_inode = NULL; >>>>>>>> release_atomic_write_cnt(inode); >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); >>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>>>>>>> index 1f812b9ce985..10463f084d30 100644 >>>>>>>> --- a/fs/f2fs/super.c >>>>>>>> +++ b/fs/f2fs/super.c >>>>>>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) >>>>>>>> atomic_inc(&inode->i_count); >>>>>>>> spin_unlock(&inode->i_lock); >>>>>>>> >>>>>>>> - f2fs_abort_atomic_write(inode, true); >>>>>>> >>>>>>> In order to avoid caching obsolete page of cow_inode, how about truncating >>>>>>> them here? >>>>>>> >>>>>>> if (f2fs_is_atomic_file() && cow_inode) >>>>>>> truncate_inode_pages_final(&cow_inode->i_data); >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>>> - >>>>>>>> /* should remain fi->extent_tree for writepage */ >>>>>>>> f2fs_destroy_extent_node(inode); >>>>>>>>
On Tue, Jan 31, 2023 at 5:40 PM Chao Yu <chao@kernel.org> wrote: > > Hi Daeho, > > On 2023/2/1 6:34, Daeho Jeong wrote: > > On Tue, Jan 31, 2023 at 1:57 PM Daeho Jeong <daeho43@gmail.com> wrote: > >> > >> On Tue, Jan 31, 2023 at 1:38 PM Daeho Jeong <daeho43@gmail.com> wrote: > >>> > >>> On Tue, Jan 31, 2023 at 11:13 AM Daeho Jeong <daeho43@gmail.com> wrote: > >>>> > >>>> Hi Chao, > >>>> > >>>> On Tue, Jan 31, 2023 at 3:37 AM Chao Yu <chao@kernel.org> wrote: > >>>>> > >>>>> Hi Daeho, > >>>>> > >>>>> On 2023/1/31 0:34, Daeho Jeong wrote: > >>>>>> Hi Chao, > >>>>>> > >>>>>> I read your patch series now and I like it. > >>>>> > >>>>> Thank you for checking the patches. :) > >>>>> > >>>>>> However, how about a race condition between start_atomic_write and > >>>>>> abort_atomic_write? > >>>>> > >>>>> Yup, I noticed that issue, I guess we can avoid this race condition by > >>>>> covering these two flows w/ i_atomic_sem. > >>>>> > >>>>>> abort_atomic_write is called without inode_lock in closing filp scenarios. > >>>>>> What do you think about this? > >>>>> > >>>>> I'm fine w/ your change as it's more clean, but it's better to drop cow_inode's > >>>>> page cache if atomic_write is committed or aborted to avoid caching obsolete page? > >>>> > >>>> It's better to put that part in f2fs_abort_atomic_write(). > >>>> On top of that, maybe, we should move > >>>> f2fs_do_truncate_blocks(fi->cow_inode, 0, true) part from > >>>> f2fs_ioc_start_atomic_write() to f2fs_abort_atomic_write(), too. > >>> > >>> Oh, we shouldn't touch the f2fs_do_truncate_blocks() part, since there > >>> might be some left writeback after aborting atomic write. > >>> Plz. review it related to the timing of calling truncate_inode_pages_final(). > >> > >> Looks like the scenario becomes too complicated if I think about more > >> than one writer's scenario. > >> How about we check writecount in commit_atomic_write ioctl and return > >> EBUSY when it's not only one writer? > >> In that case, we can make the scenario simple and effective, and we > >> can release all the resources in abort_atomic_write(). > > > > Oh, I totally forgot this. We don't use pages of COW inode. So, we > > Yes, after atomic_write is committed or aborted, we don't use pages of > cow_inode, so they are obsolete, IMO, we'd better to reclaim them > immediately to avoid unnecessary memory use. I mean we don't use page cache of the COW inode at all during the whole atomic write procedure. We only use the page cache of the original file. > > The implementation may be: > > - f2fs_ioc_abort_atomic_write > - f2fs_abort_atomic_write > - truncate_inode_pages_final(cow_inode) > > - f2fs_ioc_commit_atomic_write > - f2fs_abort_atomic_write > - truncate_inode_pages_final(cow_inode) > > Thanks, > > > don't need to clean them up. > > > >> > >>> > >>>> > >>>> Thanks, > >>>> > >>>>> > >>>>> Thanks, > >>>>> > >>>>>> > >>>>>> Thanks, > >>>>>> > >>>>>> > >>>>>> On Fri, Jan 27, 2023 at 6:07 PM Chao Yu <chao@kernel.org> wrote: > >>>>>>> > >>>>>>> Hi Daeho, Jaegeuk, > >>>>>>> > >>>>>>> Please take a look at patchset in below link: > >>>>>>> > >>>>>>> https://lore.kernel.org/linux-f2fs-devel/20230109034453.490176-1-chao@kernel.org/T/#t > >>>>>>> > >>>>>>> In PATCH 4/5, I'm trying to fix the same issue w/ alternative way, let me > >>>>>>> know your preference. :) > >>>>>>> > >>>>>>> One comment as below. > >>>>>>> > >>>>>>> On 2023/1/13 8:49, Daeho Jeong wrote: > >>>>>>>> From: Daeho Jeong <daehojeong@google.com> > >>>>>>>> > >>>>>>>> To fix a race condition between atomic write aborts, I use the inode > >>>>>>>> lock and make COW inode to be re-usable thoroughout the whole > >>>>>>>> atomic file inode lifetime. > >>>>>>>> > >>>>>>>> Reported-by: syzbot+823000d23b3400619f7c@syzkaller.appspotmail.com > >>>>>>>> Fixes: 3db1de0e582c ("f2fs: change the current atomic write way") > >>>>>>>> Signed-off-by: Daeho Jeong <daehojeong@google.com> > >>>>>>>> --- > >>>>>>>> fs/f2fs/file.c | 43 ++++++++++++++++++++++++++++--------------- > >>>>>>>> fs/f2fs/inode.c | 11 +++++++++-- > >>>>>>>> fs/f2fs/segment.c | 3 --- > >>>>>>>> fs/f2fs/super.c | 2 -- > >>>>>>>> 4 files changed, 37 insertions(+), 22 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > >>>>>>>> index ecbc8c135b49..ff072a9ed258 100644 > >>>>>>>> --- a/fs/f2fs/file.c > >>>>>>>> +++ b/fs/f2fs/file.c > >>>>>>>> @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) > >>>>>>>> atomic_read(&inode->i_writecount) != 1) > >>>>>>>> return 0; > >>>>>>>> > >>>>>>>> + inode_lock(inode); > >>>>>>>> f2fs_abort_atomic_write(inode, true); > >>>>>>>> + inode_unlock(inode); > >>>>>>>> + > >>>>>>>> return 0; > >>>>>>>> } > >>>>>>>> > >>>>>>>> @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) > >>>>>>>> * until all the writers close its file. Since this should be done > >>>>>>>> * before dropping file lock, it needs to do in ->flush. > >>>>>>>> */ > >>>>>>>> - if (F2FS_I(inode)->atomic_write_task == current) > >>>>>>>> + if (F2FS_I(inode)->atomic_write_task == current) { > >>>>>>>> + inode_lock(inode); > >>>>>>>> f2fs_abort_atomic_write(inode, true); > >>>>>>>> + inode_unlock(inode); > >>>>>>>> + } > >>>>>>>> return 0; > >>>>>>>> } > >>>>>>>> > >>>>>>>> @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > >>>>>>>> goto out; > >>>>>>>> } > >>>>>>>> > >>>>>>>> - /* Create a COW inode for atomic write */ > >>>>>>>> - pinode = f2fs_iget(inode->i_sb, fi->i_pino); > >>>>>>>> - if (IS_ERR(pinode)) { > >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>>>>>>> - ret = PTR_ERR(pinode); > >>>>>>>> - goto out; > >>>>>>>> - } > >>>>>>>> + /* Check if the inode already has a COW inode */ > >>>>>>>> + if (fi->cow_inode == NULL) { > >>>>>>>> + /* Create a COW inode for atomic write */ > >>>>>>>> + pinode = f2fs_iget(inode->i_sb, fi->i_pino); > >>>>>>>> + if (IS_ERR(pinode)) { > >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>>>>>>> + ret = PTR_ERR(pinode); > >>>>>>>> + goto out; > >>>>>>>> + } > >>>>>>>> > >>>>>>>> - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > >>>>>>>> - iput(pinode); > >>>>>>>> - if (ret) { > >>>>>>>> - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>>>>>>> - goto out; > >>>>>>>> + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); > >>>>>>>> + iput(pinode); > >>>>>>>> + if (ret) { > >>>>>>>> + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); > >>>>>>>> + goto out; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + set_inode_flag(fi->cow_inode, FI_COW_FILE); > >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > >>>>>>>> + } else { > >>>>>>>> + /* Reuse the already created COW inode */ > >>>>>>>> + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); > >>>>>>>> } > >>>>>>>> > >>>>>>>> f2fs_write_inode(inode, NULL); > >>>>>>>> @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) > >>>>>>>> stat_inc_atomic_inode(inode); > >>>>>>>> > >>>>>>>> set_inode_flag(inode, FI_ATOMIC_FILE); > >>>>>>>> - set_inode_flag(fi->cow_inode, FI_COW_FILE); > >>>>>>>> - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); > >>>>>>>> > >>>>>>>> isize = i_size_read(inode); > >>>>>>>> fi->original_i_size = isize; > >>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c > >>>>>>>> index ff6cf66ed46b..4921f7209e28 100644 > >>>>>>>> --- a/fs/f2fs/inode.c > >>>>>>>> +++ b/fs/f2fs/inode.c > >>>>>>>> @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) > >>>>>>>> void f2fs_evict_inode(struct inode *inode) > >>>>>>>> { > >>>>>>>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > >>>>>>>> - nid_t xnid = F2FS_I(inode)->i_xattr_nid; > >>>>>>>> + struct f2fs_inode_info *fi = F2FS_I(inode); > >>>>>>>> + nid_t xnid = fi->i_xattr_nid; > >>>>>>>> int err = 0; > >>>>>>>> > >>>>>>>> f2fs_abort_atomic_write(inode, true); > >>>>>>>> > >>>>>>>> + if (fi->cow_inode) { > >>>>>>>> + clear_inode_flag(fi->cow_inode, FI_COW_FILE); > >>>>>>>> + iput(fi->cow_inode); > >>>>>>>> + fi->cow_inode = NULL; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> trace_f2fs_evict_inode(inode); > >>>>>>>> truncate_inode_pages_final(&inode->i_data); > >>>>>>>> > >>>>>>>> @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) > >>>>>>>> stat_dec_inline_inode(inode); > >>>>>>>> stat_dec_compr_inode(inode); > >>>>>>>> stat_sub_compr_blocks(inode, > >>>>>>>> - atomic_read(&F2FS_I(inode)->i_compr_blocks)); > >>>>>>>> + atomic_read(&fi->i_compr_blocks)); > >>>>>>>> > >>>>>>>> if (likely(!f2fs_cp_error(sbi) && > >>>>>>>> !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) > >>>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >>>>>>>> index ae3c4e5474ef..536d7c674b04 100644 > >>>>>>>> --- a/fs/f2fs/segment.c > >>>>>>>> +++ b/fs/f2fs/segment.c > >>>>>>>> @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) > >>>>>>>> if (!f2fs_is_atomic_file(inode)) > >>>>>>>> return; > >>>>>>>> > >>>>>>>> - clear_inode_flag(fi->cow_inode, FI_COW_FILE); > >>>>>>>> - iput(fi->cow_inode); > >>>>>>>> - fi->cow_inode = NULL; > >>>>>>>> release_atomic_write_cnt(inode); > >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_COMMITTED); > >>>>>>>> clear_inode_flag(inode, FI_ATOMIC_REPLACE); > >>>>>>>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>>>>>>> index 1f812b9ce985..10463f084d30 100644 > >>>>>>>> --- a/fs/f2fs/super.c > >>>>>>>> +++ b/fs/f2fs/super.c > >>>>>>>> @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) > >>>>>>>> atomic_inc(&inode->i_count); > >>>>>>>> spin_unlock(&inode->i_lock); > >>>>>>>> > >>>>>>>> - f2fs_abort_atomic_write(inode, true); > >>>>>>> > >>>>>>> In order to avoid caching obsolete page of cow_inode, how about truncating > >>>>>>> them here? > >>>>>>> > >>>>>>> if (f2fs_is_atomic_file() && cow_inode) > >>>>>>> truncate_inode_pages_final(&cow_inode->i_data); > >>>>>>> > >>>>>>> Thanks, > >>>>>>> > >>>>>>>> - > >>>>>>>> /* should remain fi->extent_tree for writepage */ > >>>>>>>> f2fs_destroy_extent_node(inode); > >>>>>>>>
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index ecbc8c135b49..ff072a9ed258 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -1866,7 +1866,10 @@ static int f2fs_release_file(struct inode *inode, struct file *filp) atomic_read(&inode->i_writecount) != 1) return 0; + inode_lock(inode); f2fs_abort_atomic_write(inode, true); + inode_unlock(inode); + return 0; } @@ -1880,8 +1883,11 @@ static int f2fs_file_flush(struct file *file, fl_owner_t id) * until all the writers close its file. Since this should be done * before dropping file lock, it needs to do in ->flush. */ - if (F2FS_I(inode)->atomic_write_task == current) + if (F2FS_I(inode)->atomic_write_task == current) { + inode_lock(inode); f2fs_abort_atomic_write(inode, true); + inode_unlock(inode); + } return 0; } @@ -2087,19 +2093,28 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) goto out; } - /* Create a COW inode for atomic write */ - pinode = f2fs_iget(inode->i_sb, fi->i_pino); - if (IS_ERR(pinode)) { - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); - ret = PTR_ERR(pinode); - goto out; - } + /* Check if the inode already has a COW inode */ + if (fi->cow_inode == NULL) { + /* Create a COW inode for atomic write */ + pinode = f2fs_iget(inode->i_sb, fi->i_pino); + if (IS_ERR(pinode)) { + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); + ret = PTR_ERR(pinode); + goto out; + } - ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); - iput(pinode); - if (ret) { - f2fs_up_write(&fi->i_gc_rwsem[WRITE]); - goto out; + ret = f2fs_get_tmpfile(mnt_userns, pinode, &fi->cow_inode); + iput(pinode); + if (ret) { + f2fs_up_write(&fi->i_gc_rwsem[WRITE]); + goto out; + } + + set_inode_flag(fi->cow_inode, FI_COW_FILE); + clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); + } else { + /* Reuse the already created COW inode */ + f2fs_do_truncate_blocks(fi->cow_inode, 0, true); } f2fs_write_inode(inode, NULL); @@ -2107,8 +2122,6 @@ static int f2fs_ioc_start_atomic_write(struct file *filp, bool truncate) stat_inc_atomic_inode(inode); set_inode_flag(inode, FI_ATOMIC_FILE); - set_inode_flag(fi->cow_inode, FI_COW_FILE); - clear_inode_flag(fi->cow_inode, FI_INLINE_DATA); isize = i_size_read(inode); fi->original_i_size = isize; diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c index ff6cf66ed46b..4921f7209e28 100644 --- a/fs/f2fs/inode.c +++ b/fs/f2fs/inode.c @@ -766,11 +766,18 @@ int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc) void f2fs_evict_inode(struct inode *inode) { struct f2fs_sb_info *sbi = F2FS_I_SB(inode); - nid_t xnid = F2FS_I(inode)->i_xattr_nid; + struct f2fs_inode_info *fi = F2FS_I(inode); + nid_t xnid = fi->i_xattr_nid; int err = 0; f2fs_abort_atomic_write(inode, true); + if (fi->cow_inode) { + clear_inode_flag(fi->cow_inode, FI_COW_FILE); + iput(fi->cow_inode); + fi->cow_inode = NULL; + } + trace_f2fs_evict_inode(inode); truncate_inode_pages_final(&inode->i_data); @@ -857,7 +864,7 @@ void f2fs_evict_inode(struct inode *inode) stat_dec_inline_inode(inode); stat_dec_compr_inode(inode); stat_sub_compr_blocks(inode, - atomic_read(&F2FS_I(inode)->i_compr_blocks)); + atomic_read(&fi->i_compr_blocks)); if (likely(!f2fs_cp_error(sbi) && !is_sbi_flag_set(sbi, SBI_CP_DISABLED))) diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index ae3c4e5474ef..536d7c674b04 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -192,9 +192,6 @@ void f2fs_abort_atomic_write(struct inode *inode, bool clean) if (!f2fs_is_atomic_file(inode)) return; - clear_inode_flag(fi->cow_inode, FI_COW_FILE); - iput(fi->cow_inode); - fi->cow_inode = NULL; release_atomic_write_cnt(inode); clear_inode_flag(inode, FI_ATOMIC_COMMITTED); clear_inode_flag(inode, FI_ATOMIC_REPLACE); diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 1f812b9ce985..10463f084d30 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1430,8 +1430,6 @@ static int f2fs_drop_inode(struct inode *inode) atomic_inc(&inode->i_count); spin_unlock(&inode->i_lock); - f2fs_abort_atomic_write(inode, true); - /* should remain fi->extent_tree for writepage */ f2fs_destroy_extent_node(inode);