Message ID | 20231210113547.3412782-5-chao@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp6473219vqy; Sun, 10 Dec 2023 03:36:44 -0800 (PST) X-Google-Smtp-Source: AGHT+IGTG5tt5mxVps9V6/y+DX1dxlPROlb1oWz2zWoCQuTslU6omAmSrnOQmg2QGMQLIlyXqdAL X-Received: by 2002:a05:6a00:10cc:b0:6ce:71af:842b with SMTP id d12-20020a056a0010cc00b006ce71af842bmr3739496pfu.8.1702208204652; Sun, 10 Dec 2023 03:36:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702208204; cv=none; d=google.com; s=arc-20160816; b=jnBgODJ+F2j/r5F65oYc4lSw30ND5kA0SxWCsWS/1sW/j+JxcQE9W8jmbh4u4URyxl s48nW6viUUKz+xxrhw0RlPeuX62n2CFc77tB56JE6piIFTmocP9jwS4NhDtU/IMAc7vH KRuoAgP+yICi4j5mw9QPoT5B6Heq1+pxS58xh49VZcK7uhkzHPhf3pISbOQURayHskA5 Xth6WfCf2VZ6B8aCjQ0LS1X5vJc8tab9F2B9y1ycGVsiQZRwRvLPbMYMdqAkEWsl1f+T G0/3bzLUETpBdjRrgLgcHbO884QERizbo8DN459TqNG8u8QwN7Xgm9Tu4VmTgA7YUncp +PYg== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=AHfFwKYujQQVoHs5zJPh6KRKf6rlctCcGvr++i9cGOI=; fh=X1/oBJvTGJUQ0YtRp03u+3tPfRw/dXXALWynfOSlfQQ=; b=py2CKtwNimtP0pKzoHmv/N0VIF2VRTSIhXIZe77iMF4uaNMj/LoUt7Fp6sWpBkGNmv EG70Sz40QItEC0NASr+7HpvN7hk3l8M95k7VXZtCMijRS18TEF+YsXRRXUGmndIdcbbs O2hHJxcTOVlo3zDJBjTC4ILHbY606rKguVE13yZqXvNyzqvrlF+Zusf9o7QMFSVRPbk1 w8BbSV9bWbL1ejyL5RSC5wOHQx5/aN2M+4txx7AzsdkR5WItUIbW30I0KsKhG/uuT1Xd lHFiySfcQyn+Hc7uhZf/J+saG8D+4bFBZV2lcE/nE6aKYOGtYrHWlo7b38UArwl0dLFE rdkQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=VhKpZ6bM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id k21-20020aa788d5000000b006cc096fa2d6si4432659pff.70.2023.12.10.03.36.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 10 Dec 2023 03:36:44 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=VhKpZ6bM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 536B28092DA0; Sun, 10 Dec 2023 03:36:22 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232204AbjLJLgK (ORCPT <rfc822;rua109.linux@gmail.com> + 99 others); Sun, 10 Dec 2023 06:36:10 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232064AbjLJLgE (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 10 Dec 2023 06:36:04 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B156F11F for <linux-kernel@vger.kernel.org>; Sun, 10 Dec 2023 03:36:09 -0800 (PST) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C726C433CB; Sun, 10 Dec 2023 11:36:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1702208168; bh=17M2pG5LF+yTUT42vRyb2J9RQolF4UPApTsylmOgOqo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=VhKpZ6bMkrzIwnh96I+1vV0Y2OCIZV/ymzd7baK1mrWUah3lbAoHJjqI7MB/s3SRm Vl4hGgaRM7HzUXf8iw7IiZQTR7CQzPHnmb0NF2RrsVZY68DRUwdGsaX3fwKzgXd2zJ JKdR5UcjWAcNU3PFutjamQmTrdJ6ubvaKMJdHjtaw927MIKe9aFFER8GfR5+fqAqdJ s7YwuahFs/xFV/RUX/bvsLCDeKpZ+TG9cuTsIiDfGsdYKC8ihmdV605qwAcFRtmUXk FyPeza8fPKgHSCoeyAaBiwEjhicRI1nEZKg88Q61Hs5j/YhVbRf2K4ZkP0h16h6wGZ 5KsDwZJHvABfQ== From: Chao Yu <chao@kernel.org> To: jaegeuk@kernel.org Cc: linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, Chao Yu <chao@kernel.org> Subject: [PATCH 5/6] f2fs: fix to restrict condition of compress inode conversion Date: Sun, 10 Dec 2023 19:35:46 +0800 Message-Id: <20231210113547.3412782-5-chao@kernel.org> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20231210113547.3412782-1-chao@kernel.org> References: <20231210113547.3412782-1-chao@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE 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-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Sun, 10 Dec 2023 03:36:22 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784894670321207408 X-GMAIL-MSGID: 1784894670321207408 |
Series |
[1/6] f2fs: fix to tag gcing flag on page during block migration
|
|
Commit Message
Chao Yu
Dec. 10, 2023, 11:35 a.m. UTC
This patch adds i_size check during compress inode conversion in order
to avoid .page_mkwrite races w/ conversion.
Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/f2fs/f2fs.h | 8 +++++++-
fs/f2fs/file.c | 5 ++---
2 files changed, 9 insertions(+), 4 deletions(-)
Comments
On 12/10, Chao Yu wrote: > This patch adds i_size check during compress inode conversion in order > to avoid .page_mkwrite races w/ conversion. Which race condition do you see? > > Fixes: 4c8ff7095bef ("f2fs: support data compression") > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/f2fs.h | 8 +++++++- > fs/f2fs/file.c | 5 ++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 65294e3b0bef..c9b8a1953913 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) > #endif > } > > +static inline bool inode_has_data(struct inode *inode) > +{ > + return (S_ISREG(inode->i_mode) && > + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); > +} > + > static inline bool f2fs_disable_compressed_file(struct inode *inode) > { > struct f2fs_inode_info *fi = F2FS_I(inode); > > if (!f2fs_compressed_file(inode)) > return true; > - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) > + if (inode_has_data(inode)) > return false; > > fi->i_flags &= ~F2FS_COMPR_FL; > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index 1a3c29a9a6a0..8af4b29c3e1a 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) > > f2fs_down_write(&F2FS_I(inode)->i_sem); > if (!f2fs_may_compress(inode) || > - (S_ISREG(inode->i_mode) && > - F2FS_HAS_BLOCKS(inode))) { > + inode_has_data(inode)) { > f2fs_up_write(&F2FS_I(inode)->i_sem); > return -EINVAL; > } > @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) > goto out; > } > > - if (F2FS_HAS_BLOCKS(inode)) { > + if (inode_has_data(inode)) { > ret = -EFBIG; > goto out; > } > -- > 2.40.1
On 2023/12/12 6:11, Jaegeuk Kim wrote: > On 12/10, Chao Yu wrote: >> This patch adds i_size check during compress inode conversion in order >> to avoid .page_mkwrite races w/ conversion. > > Which race condition do you see? Something like: - f2fs_setflags_common - check S_ISREG && F2FS_HAS_BLOCKS - mkwrite - f2fs_get_block_locked : update metadata in old inode's disk layout - set_compress_context Thanks, > >> >> Fixes: 4c8ff7095bef ("f2fs: support data compression") >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/f2fs.h | 8 +++++++- >> fs/f2fs/file.c | 5 ++--- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 65294e3b0bef..c9b8a1953913 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) >> #endif >> } >> >> +static inline bool inode_has_data(struct inode *inode) >> +{ >> + return (S_ISREG(inode->i_mode) && >> + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); >> +} >> + >> static inline bool f2fs_disable_compressed_file(struct inode *inode) >> { >> struct f2fs_inode_info *fi = F2FS_I(inode); >> >> if (!f2fs_compressed_file(inode)) >> return true; >> - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) >> + if (inode_has_data(inode)) >> return false; >> >> fi->i_flags &= ~F2FS_COMPR_FL; >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >> index 1a3c29a9a6a0..8af4b29c3e1a 100644 >> --- a/fs/f2fs/file.c >> +++ b/fs/f2fs/file.c >> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) >> >> f2fs_down_write(&F2FS_I(inode)->i_sem); >> if (!f2fs_may_compress(inode) || >> - (S_ISREG(inode->i_mode) && >> - F2FS_HAS_BLOCKS(inode))) { >> + inode_has_data(inode)) { >> f2fs_up_write(&F2FS_I(inode)->i_sem); >> return -EINVAL; >> } >> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) >> goto out; >> } >> >> - if (F2FS_HAS_BLOCKS(inode)) { >> + if (inode_has_data(inode)) { >> ret = -EFBIG; >> goto out; >> } >> -- >> 2.40.1
On 12/12, Chao Yu wrote: > On 2023/12/12 6:11, Jaegeuk Kim wrote: > > On 12/10, Chao Yu wrote: > > > This patch adds i_size check during compress inode conversion in order > > > to avoid .page_mkwrite races w/ conversion. > > > > Which race condition do you see? > > Something like: > > - f2fs_setflags_common > - check S_ISREG && F2FS_HAS_BLOCKS > - mkwrite > - f2fs_get_block_locked > : update metadata in old inode's disk layout Don't we need to prevent setting this for mmapped file? > - set_compress_context > > Thanks, > > > > > > > > > Fixes: 4c8ff7095bef ("f2fs: support data compression") > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > --- > > > fs/f2fs/f2fs.h | 8 +++++++- > > > fs/f2fs/file.c | 5 ++--- > > > 2 files changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > index 65294e3b0bef..c9b8a1953913 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) > > > #endif > > > } > > > +static inline bool inode_has_data(struct inode *inode) > > > +{ > > > + return (S_ISREG(inode->i_mode) && > > > + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); > > > +} > > > + > > > static inline bool f2fs_disable_compressed_file(struct inode *inode) > > > { > > > struct f2fs_inode_info *fi = F2FS_I(inode); > > > if (!f2fs_compressed_file(inode)) > > > return true; > > > - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) > > > + if (inode_has_data(inode)) > > > return false; > > > fi->i_flags &= ~F2FS_COMPR_FL; > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > index 1a3c29a9a6a0..8af4b29c3e1a 100644 > > > --- a/fs/f2fs/file.c > > > +++ b/fs/f2fs/file.c > > > @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) > > > f2fs_down_write(&F2FS_I(inode)->i_sem); > > > if (!f2fs_may_compress(inode) || > > > - (S_ISREG(inode->i_mode) && > > > - F2FS_HAS_BLOCKS(inode))) { > > > + inode_has_data(inode)) { > > > f2fs_up_write(&F2FS_I(inode)->i_sem); > > > return -EINVAL; > > > } > > > @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) > > > goto out; > > > } > > > - if (F2FS_HAS_BLOCKS(inode)) { > > > + if (inode_has_data(inode)) { > > > ret = -EFBIG; > > > goto out; > > > } > > > -- > > > 2.40.1
On 2023/12/13 6:21, Jaegeuk Kim wrote: > On 12/12, Chao Yu wrote: >> On 2023/12/12 6:11, Jaegeuk Kim wrote: >>> On 12/10, Chao Yu wrote: >>>> This patch adds i_size check during compress inode conversion in order >>>> to avoid .page_mkwrite races w/ conversion. >>> >>> Which race condition do you see? >> >> Something like: >> >> - f2fs_setflags_common >> - check S_ISREG && F2FS_HAS_BLOCKS >> - mkwrite >> - f2fs_get_block_locked >> : update metadata in old inode's disk layout > > Don't we need to prevent setting this for mmapped file? Oh, we have used i_sem lock to prevent such race case, however we missed f2fs_disable_compressed_file(): - f2fs_disable_compressed_file - check inode_has_data - f2fs_file_mmap - mkwrite - f2fs_get_block_locked : update metadata in compressed inode's disk layout - fi->i_flags &= ~F2FS_COMPR_FL - clear_inode_flag(inode, FI_COMPRESSED_FILE); Thanks, > >> - set_compress_context >> >> Thanks, >> >>> >>>> >>>> Fixes: 4c8ff7095bef ("f2fs: support data compression") >>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>> --- >>>> fs/f2fs/f2fs.h | 8 +++++++- >>>> fs/f2fs/file.c | 5 ++--- >>>> 2 files changed, 9 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>> index 65294e3b0bef..c9b8a1953913 100644 >>>> --- a/fs/f2fs/f2fs.h >>>> +++ b/fs/f2fs/f2fs.h >>>> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) >>>> #endif >>>> } >>>> +static inline bool inode_has_data(struct inode *inode) >>>> +{ >>>> + return (S_ISREG(inode->i_mode) && >>>> + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); >>>> +} >>>> + >>>> static inline bool f2fs_disable_compressed_file(struct inode *inode) >>>> { >>>> struct f2fs_inode_info *fi = F2FS_I(inode); >>>> if (!f2fs_compressed_file(inode)) >>>> return true; >>>> - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) >>>> + if (inode_has_data(inode)) >>>> return false; >>>> fi->i_flags &= ~F2FS_COMPR_FL; >>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>> index 1a3c29a9a6a0..8af4b29c3e1a 100644 >>>> --- a/fs/f2fs/file.c >>>> +++ b/fs/f2fs/file.c >>>> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) >>>> f2fs_down_write(&F2FS_I(inode)->i_sem); >>>> if (!f2fs_may_compress(inode) || >>>> - (S_ISREG(inode->i_mode) && >>>> - F2FS_HAS_BLOCKS(inode))) { >>>> + inode_has_data(inode)) { >>>> f2fs_up_write(&F2FS_I(inode)->i_sem); >>>> return -EINVAL; >>>> } >>>> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) >>>> goto out; >>>> } >>>> - if (F2FS_HAS_BLOCKS(inode)) { >>>> + if (inode_has_data(inode)) { >>>> ret = -EFBIG; >>>> goto out; >>>> } >>>> -- >>>> 2.40.1
On 12/28, Chao Yu wrote: > On 2023/12/13 6:21, Jaegeuk Kim wrote: > > On 12/12, Chao Yu wrote: > > > On 2023/12/12 6:11, Jaegeuk Kim wrote: > > > > On 12/10, Chao Yu wrote: > > > > > This patch adds i_size check during compress inode conversion in order > > > > > to avoid .page_mkwrite races w/ conversion. > > > > > > > > Which race condition do you see? > > > > > > Something like: > > > > > > - f2fs_setflags_common > > > - check S_ISREG && F2FS_HAS_BLOCKS > > > - mkwrite > > > - f2fs_get_block_locked > > > : update metadata in old inode's disk layout > > > > Don't we need to prevent setting this for mmapped file? > > Oh, we have used i_sem lock to prevent such race case, however > we missed f2fs_disable_compressed_file(): > > - f2fs_disable_compressed_file > - check inode_has_data > - f2fs_file_mmap > - mkwrite > - f2fs_get_block_locked > : update metadata in compressed > inode's disk layout > - fi->i_flags &= ~F2FS_COMPR_FL > - clear_inode_flag(inode, FI_COMPRESSED_FILE); So, needing i_sem for disabling it on mmapped file? It seems i_size would not be enough? > > Thanks, > > > > > > - set_compress_context > > > > > > Thanks, > > > > > > > > > > > > > > > > > Fixes: 4c8ff7095bef ("f2fs: support data compression") > > > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > > > --- > > > > > fs/f2fs/f2fs.h | 8 +++++++- > > > > > fs/f2fs/file.c | 5 ++--- > > > > > 2 files changed, 9 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > > > index 65294e3b0bef..c9b8a1953913 100644 > > > > > --- a/fs/f2fs/f2fs.h > > > > > +++ b/fs/f2fs/f2fs.h > > > > > @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) > > > > > #endif > > > > > } > > > > > +static inline bool inode_has_data(struct inode *inode) > > > > > +{ > > > > > + return (S_ISREG(inode->i_mode) && > > > > > + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); > > > > > +} > > > > > + > > > > > static inline bool f2fs_disable_compressed_file(struct inode *inode) > > > > > { > > > > > struct f2fs_inode_info *fi = F2FS_I(inode); > > > > > if (!f2fs_compressed_file(inode)) > > > > > return true; > > > > > - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) > > > > > + if (inode_has_data(inode)) > > > > > return false; > > > > > fi->i_flags &= ~F2FS_COMPR_FL; > > > > > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > > > > > index 1a3c29a9a6a0..8af4b29c3e1a 100644 > > > > > --- a/fs/f2fs/file.c > > > > > +++ b/fs/f2fs/file.c > > > > > @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) > > > > > f2fs_down_write(&F2FS_I(inode)->i_sem); > > > > > if (!f2fs_may_compress(inode) || > > > > > - (S_ISREG(inode->i_mode) && > > > > > - F2FS_HAS_BLOCKS(inode))) { > > > > > + inode_has_data(inode)) { > > > > > f2fs_up_write(&F2FS_I(inode)->i_sem); > > > > > return -EINVAL; > > > > > } > > > > > @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) > > > > > goto out; > > > > > } > > > > > - if (F2FS_HAS_BLOCKS(inode)) { > > > > > + if (inode_has_data(inode)) { > > > > > ret = -EFBIG; > > > > > goto out; > > > > > } > > > > > -- > > > > > 2.40.1
On 2024/1/3 6:54, Jaegeuk Kim wrote: > On 12/28, Chao Yu wrote: >> On 2023/12/13 6:21, Jaegeuk Kim wrote: >>> On 12/12, Chao Yu wrote: >>>> On 2023/12/12 6:11, Jaegeuk Kim wrote: >>>>> On 12/10, Chao Yu wrote: >>>>>> This patch adds i_size check during compress inode conversion in order >>>>>> to avoid .page_mkwrite races w/ conversion. >>>>> >>>>> Which race condition do you see? >>>> >>>> Something like: >>>> >>>> - f2fs_setflags_common >>>> - check S_ISREG && F2FS_HAS_BLOCKS >>>> - mkwrite >>>> - f2fs_get_block_locked >>>> : update metadata in old inode's disk layout >>> >>> Don't we need to prevent setting this for mmapped file? >> >> Oh, we have used i_sem lock to prevent such race case, however >> we missed f2fs_disable_compressed_file(): >> >> - f2fs_disable_compressed_file >> - check inode_has_data >> - f2fs_file_mmap >> - mkwrite >> - f2fs_get_block_locked >> : update metadata in compressed >> inode's disk layout >> - fi->i_flags &= ~F2FS_COMPR_FL >> - clear_inode_flag(inode, FI_COMPRESSED_FILE); > > So, needing i_sem for disabling it on mmapped file? It seems i_size would not > be enough? Agreed, let me update the patch. Thanks, > >> >> Thanks, >> >>> >>>> - set_compress_context >>>> >>>> Thanks, >>>> >>>>> >>>>>> >>>>>> Fixes: 4c8ff7095bef ("f2fs: support data compression") >>>>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>>>> --- >>>>>> fs/f2fs/f2fs.h | 8 +++++++- >>>>>> fs/f2fs/file.c | 5 ++--- >>>>>> 2 files changed, 9 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>>>>> index 65294e3b0bef..c9b8a1953913 100644 >>>>>> --- a/fs/f2fs/f2fs.h >>>>>> +++ b/fs/f2fs/f2fs.h >>>>>> @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) >>>>>> #endif >>>>>> } >>>>>> +static inline bool inode_has_data(struct inode *inode) >>>>>> +{ >>>>>> + return (S_ISREG(inode->i_mode) && >>>>>> + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); >>>>>> +} >>>>>> + >>>>>> static inline bool f2fs_disable_compressed_file(struct inode *inode) >>>>>> { >>>>>> struct f2fs_inode_info *fi = F2FS_I(inode); >>>>>> if (!f2fs_compressed_file(inode)) >>>>>> return true; >>>>>> - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) >>>>>> + if (inode_has_data(inode)) >>>>>> return false; >>>>>> fi->i_flags &= ~F2FS_COMPR_FL; >>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c >>>>>> index 1a3c29a9a6a0..8af4b29c3e1a 100644 >>>>>> --- a/fs/f2fs/file.c >>>>>> +++ b/fs/f2fs/file.c >>>>>> @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) >>>>>> f2fs_down_write(&F2FS_I(inode)->i_sem); >>>>>> if (!f2fs_may_compress(inode) || >>>>>> - (S_ISREG(inode->i_mode) && >>>>>> - F2FS_HAS_BLOCKS(inode))) { >>>>>> + inode_has_data(inode)) { >>>>>> f2fs_up_write(&F2FS_I(inode)->i_sem); >>>>>> return -EINVAL; >>>>>> } >>>>>> @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) >>>>>> goto out; >>>>>> } >>>>>> - if (F2FS_HAS_BLOCKS(inode)) { >>>>>> + if (inode_has_data(inode)) { >>>>>> ret = -EFBIG; >>>>>> goto out; >>>>>> } >>>>>> -- >>>>>> 2.40.1
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 65294e3b0bef..c9b8a1953913 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -4397,13 +4397,19 @@ static inline int set_compress_context(struct inode *inode) #endif } +static inline bool inode_has_data(struct inode *inode) +{ + return (S_ISREG(inode->i_mode) && + (F2FS_HAS_BLOCKS(inode) || i_size_read(inode))); +} + static inline bool f2fs_disable_compressed_file(struct inode *inode) { struct f2fs_inode_info *fi = F2FS_I(inode); if (!f2fs_compressed_file(inode)) return true; - if (S_ISREG(inode->i_mode) && F2FS_HAS_BLOCKS(inode)) + if (inode_has_data(inode)) return false; fi->i_flags &= ~F2FS_COMPR_FL; diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 1a3c29a9a6a0..8af4b29c3e1a 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -1922,8 +1922,7 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask) f2fs_down_write(&F2FS_I(inode)->i_sem); if (!f2fs_may_compress(inode) || - (S_ISREG(inode->i_mode) && - F2FS_HAS_BLOCKS(inode))) { + inode_has_data(inode)) { f2fs_up_write(&F2FS_I(inode)->i_sem); return -EINVAL; } @@ -3996,7 +3995,7 @@ static int f2fs_ioc_set_compress_option(struct file *filp, unsigned long arg) goto out; } - if (F2FS_HAS_BLOCKS(inode)) { + if (inode_has_data(inode)) { ret = -EFBIG; goto out; }