From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA00AC4332F for ; Tue, 15 Nov 2022 21:48:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231168AbiKOVsP (ORCPT ); Tue, 15 Nov 2022 16:48:15 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38782 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231383AbiKOVsK (ORCPT ); Tue, 15 Nov 2022 16:48:10 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B300B2B1B7 for ; Tue, 15 Nov 2022 13:48:04 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 48B3261A32 for ; Tue, 15 Nov 2022 21:48:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 85B05C433B5; Tue, 15 Nov 2022 21:48:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1668548883; bh=CG0dXhmknwCwA3hdVEQE9D8iMO7v+ojqxHnJ/bgzyQI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BJ+DDeaaNph2fnh/K8SKttJN93KgcH5TMqzK8YMGOvh/Bnju6oDFljc1HwjReyBPk Z8rs5+oz8yQ4tyfqL3r3N/rIQoS33RinIXWr34GCHqbYaKHJmBeyeKzwkuvWWZPSbP L4OIzOctSwo+/E+PN7YP7T+BESEz2IyZEJRryiCzwK1vKsVdX8V6lrt5jpb1iXUI2v i9YfvpIiCZoTyHRxA4iuC5fsFDeigVRwhuh0T9P6HKT1DVrHHj40yi7PnPJg9r0pq2 6CG3id/7b48KTq3YUT1OYa36Flaet8mdRCt1BrT/KzmCxL7h/fl9IRQfCzXHQZ8t5v q+jAEsX1UYzpw== Date: Tue, 15 Nov 2022 13:48:01 -0800 From: Jaegeuk Kim To: Sheng Yong Cc: chao@kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] f2fs: fix to enable compress for newly created file if extension matches Message-ID: References: <20221115160155.1037163-1-shengyong@oppo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221115160155.1037163-1-shengyong@oppo.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/16, Sheng Yong wrote: > If compress_extension is set, and a newly created file matches the > extension, the file could be marked as compression file. However, > if inline_data is also enabled, there is no chance to check its > extension since f2fs_should_compress() always returns false. > > This patch moves set_compress_inode(), which do extension check, in > f2fs_should_compress() to check extensions before setting inline > data flag. > > Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion") > Signed-off-by: Sheng Yong > Signed-off-by: Jaegeuk Kim > --- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/namei.c | 336 ++++++++++++++++++++++++------------------------ > 2 files changed, 171 insertions(+), 166 deletions(-) > --- > > Hi, Jaegeuk, Chao, > > How about adding a bool `may_compress' in set_compress_new_inode, set > `my_compress` according to several conditions. If it is false, clear > F2FS_COMPR_FL. > > And set_compress_context is also changed to clear F2FS_NOCOMP_FL, > otherwise, if F2FS_NOCOMP_FL is inherited from parent and hit > compress_extension, both F2FS_NOCOMP_FL and F2FS_COMPR_FL are set. > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 6a8cbf5bb1871..a3420fbb29214 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -4355,6 +4355,7 @@ static inline int set_compress_context(struct inode *inode) > F2FS_I(inode)->i_compress_flag |= > F2FS_OPTION(sbi).compress_level << > COMPRESS_LEVEL_OFFSET; > + F2FS_I(inode)->i_flags &= ~F2FS_NOCOMP_FL; > F2FS_I(inode)->i_flags |= F2FS_COMPR_FL; > set_inode_flag(inode, FI_COMPRESSED_FILE); > stat_inc_compr_inode(inode); > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index e104409c3a0e5..36ec5cf7cf859 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -22,8 +22,170 @@ > #include "acl.h" > #include > > +static inline int is_extension_exist(const unsigned char *s, const char *sub, > + bool tmp_ext) > +{ > + size_t slen = strlen(s); > + size_t sublen = strlen(sub); > + int i; > + > + if (sublen == 1 && *sub == '*') > + return 1; > + > + /* > + * filename format of multimedia file should be defined as: > + * "filename + '.' + extension + (optional: '.' + temp extension)". > + */ > + if (slen < sublen + 2) > + return 0; > + > + if (!tmp_ext) { > + /* file has no temp extension */ > + if (s[slen - sublen - 1] != '.') > + return 0; > + return !strncasecmp(s + slen - sublen, sub, sublen); > + } > + > + for (i = 1; i < slen - sublen; i++) { > + if (s[i] != '.') > + continue; > + if (!strncasecmp(s + i + 1, sub, sublen)) > + return 1; > + } > + > + return 0; > +} > + > +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name, > + bool hot, bool set) > +{ > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > + int cold_count = le32_to_cpu(sbi->raw_super->extension_count); > + int hot_count = sbi->raw_super->hot_ext_count; > + int total_count = cold_count + hot_count; > + int start, count; > + int i; > + > + if (set) { > + if (total_count == F2FS_MAX_EXTENSION) > + return -EINVAL; > + } else { > + if (!hot && !cold_count) > + return -EINVAL; > + if (hot && !hot_count) > + return -EINVAL; > + } > + > + if (hot) { > + start = cold_count; > + count = total_count; > + } else { > + start = 0; > + count = cold_count; > + } > + > + for (i = start; i < count; i++) { > + if (strcmp(name, extlist[i])) > + continue; > + > + if (set) > + return -EINVAL; > + > + memcpy(extlist[i], extlist[i + 1], > + F2FS_EXTENSION_LEN * (total_count - i - 1)); > + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN); > + if (hot) > + sbi->raw_super->hot_ext_count = hot_count - 1; > + else > + sbi->raw_super->extension_count = > + cpu_to_le32(cold_count - 1); > + return 0; > + } > + > + if (!set) > + return -EINVAL; > + > + if (hot) { > + memcpy(extlist[count], name, strlen(name)); > + sbi->raw_super->hot_ext_count = hot_count + 1; > + } else { > + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN]; > + > + memcpy(buf, &extlist[cold_count], > + F2FS_EXTENSION_LEN * hot_count); > + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN); > + memcpy(extlist[cold_count], name, strlen(name)); > + memcpy(&extlist[cold_count + 1], buf, > + F2FS_EXTENSION_LEN * hot_count); > + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1); > + } > + return 0; > +} > + > +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir, > + struct inode *inode, const unsigned char *name) > +{ > + struct f2fs_inode_info *fi = F2FS_I(inode); > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > + unsigned char (*noext)[F2FS_EXTENSION_LEN] = > + F2FS_OPTION(sbi).noextensions; > + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions; > + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt; > + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt; > + bool may_compress = false; > + int i, cold_count, hot_count; > + > + if (!f2fs_sb_has_compression(sbi) || !name) > + return; > + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) > + return; > + > + /* Inherit the compression flag in directory */ > + if (fi->i_flags & FS_COMPR_FL) > + may_compress = true; > + > + /* Start to check extension list for regular file */ > + if ((!ext_cnt && !noext_cnt) || S_ISDIR(inode->i_mode)) This doesn't address the Chao's point. It seems not much motivation to add may_compress. Let me try to combine some with the previous patch. > + goto set_compress; > + > + /* Don't compress hot files. */ > + f2fs_down_read(&sbi->sb_lock); > + cold_count = le32_to_cpu(sbi->raw_super->extension_count); > + hot_count = sbi->raw_super->hot_ext_count; > + for (i = cold_count; i < cold_count + hot_count; i++) > + if (is_extension_exist(name, extlist[i], false)) { > + may_compress = false; > + f2fs_up_read(&sbi->sb_lock); > + goto set_compress; > + } > + f2fs_up_read(&sbi->sb_lock); > + > + /* Don't compress unallowed extension. */ > + for (i = 0; i < noext_cnt; i++) { > + if (is_extension_exist(name, noext[i], false)) { > + may_compress = false; > + goto set_compress; > + } > + } > + > + /* Compress wanting extension. */ > + for (i = 0; i < ext_cnt; i++) { > + if (is_extension_exist(name, ext[i], false)) { > + may_compress = true; > + goto set_compress; > + } > + } > + > +set_compress: > + if (may_compress) > + set_compress_context(inode); > + else if (fi->i_flags & F2FS_COMPR_FL) > + fi->i_flags &= ~F2FS_COMPR_FL; > +} > + > static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns, > - struct inode *dir, umode_t mode) > + struct inode *dir, umode_t mode, > + const char *name) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(dir); > nid_t ino; > @@ -114,12 +276,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns, > if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) > set_inode_flag(inode, FI_PROJ_INHERIT); > > - if (f2fs_sb_has_compression(sbi)) { > - /* Inherit the compression flag in directory */ > - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) && > - f2fs_may_compress(inode)) > - set_compress_context(inode); > - } > + /* Check compression first. */ > + set_compress_new_inode(sbi, dir, inode, name); > > /* Should enable inline_data after compression set */ > if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode)) > @@ -153,40 +311,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns, > return ERR_PTR(err); > } > > -static inline int is_extension_exist(const unsigned char *s, const char *sub, > - bool tmp_ext) > -{ > - size_t slen = strlen(s); > - size_t sublen = strlen(sub); > - int i; > - > - if (sublen == 1 && *sub == '*') > - return 1; > - > - /* > - * filename format of multimedia file should be defined as: > - * "filename + '.' + extension + (optional: '.' + temp extension)". > - */ > - if (slen < sublen + 2) > - return 0; > - > - if (!tmp_ext) { > - /* file has no temp extension */ > - if (s[slen - sublen - 1] != '.') > - return 0; > - return !strncasecmp(s + slen - sublen, sub, sublen); > - } > - > - for (i = 1; i < slen - sublen; i++) { > - if (s[i] != '.') > - continue; > - if (!strncasecmp(s + i + 1, sub, sublen)) > - return 1; > - } > - > - return 0; > -} > - > /* > * Set file's temperature for hot/cold data separation > */ > @@ -217,124 +341,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode * > file_set_hot(inode); > } > > -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name, > - bool hot, bool set) > -{ > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > - int cold_count = le32_to_cpu(sbi->raw_super->extension_count); > - int hot_count = sbi->raw_super->hot_ext_count; > - int total_count = cold_count + hot_count; > - int start, count; > - int i; > - > - if (set) { > - if (total_count == F2FS_MAX_EXTENSION) > - return -EINVAL; > - } else { > - if (!hot && !cold_count) > - return -EINVAL; > - if (hot && !hot_count) > - return -EINVAL; > - } > - > - if (hot) { > - start = cold_count; > - count = total_count; > - } else { > - start = 0; > - count = cold_count; > - } > - > - for (i = start; i < count; i++) { > - if (strcmp(name, extlist[i])) > - continue; > - > - if (set) > - return -EINVAL; > - > - memcpy(extlist[i], extlist[i + 1], > - F2FS_EXTENSION_LEN * (total_count - i - 1)); > - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN); > - if (hot) > - sbi->raw_super->hot_ext_count = hot_count - 1; > - else > - sbi->raw_super->extension_count = > - cpu_to_le32(cold_count - 1); > - return 0; > - } > - > - if (!set) > - return -EINVAL; > - > - if (hot) { > - memcpy(extlist[count], name, strlen(name)); > - sbi->raw_super->hot_ext_count = hot_count + 1; > - } else { > - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN]; > - > - memcpy(buf, &extlist[cold_count], > - F2FS_EXTENSION_LEN * hot_count); > - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN); > - memcpy(extlist[cold_count], name, strlen(name)); > - memcpy(&extlist[cold_count + 1], buf, > - F2FS_EXTENSION_LEN * hot_count); > - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1); > - } > - return 0; > -} > - > -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode, > - const unsigned char *name) > -{ > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions; > - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions; > - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt; > - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt; > - int i, cold_count, hot_count; > - > - if (!f2fs_sb_has_compression(sbi) || > - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL || > - !f2fs_may_compress(inode) || > - (!ext_cnt && !noext_cnt)) > - return; > - > - f2fs_down_read(&sbi->sb_lock); > - > - cold_count = le32_to_cpu(sbi->raw_super->extension_count); > - hot_count = sbi->raw_super->hot_ext_count; > - > - for (i = cold_count; i < cold_count + hot_count; i++) { > - if (is_extension_exist(name, extlist[i], false)) { > - f2fs_up_read(&sbi->sb_lock); > - return; > - } > - } > - > - f2fs_up_read(&sbi->sb_lock); > - > - for (i = 0; i < noext_cnt; i++) { > - if (is_extension_exist(name, noext[i], false)) { > - f2fs_disable_compressed_file(inode); > - return; > - } > - } > - > - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE)) > - return; > - > - for (i = 0; i < ext_cnt; i++) { > - if (!is_extension_exist(name, ext[i], false)) > - continue; > - > - /* Do not use inline_data with compression */ > - stat_dec_inline_inode(inode); > - clear_inode_flag(inode, FI_INLINE_DATA); > - set_compress_context(inode); > - return; > - } > -} > - > static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir, > struct dentry *dentry, umode_t mode, bool excl) > { > @@ -352,15 +358,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, mode); > + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > if (!test_opt(sbi, DISABLE_EXT_IDENTIFY)) > set_file_temperature(sbi, inode, dentry->d_name.name); > > - set_compress_inode(sbi, inode, dentry->d_name.name); > - > inode->i_op = &f2fs_file_inode_operations; > inode->i_fop = &f2fs_file_operations; > inode->i_mapping->a_ops = &f2fs_dblock_aops; > @@ -689,7 +693,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO); > + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > @@ -760,7 +764,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode); > + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, dentry->d_name.name); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > @@ -817,7 +821,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, mode); > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > @@ -856,7 +860,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, mode); > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > -- > 2.25.1 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.sourceforge.net (lists.sourceforge.net [216.105.38.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6AB9EC433FE for ; Tue, 15 Nov 2022 21:48:18 +0000 (UTC) Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1ov3mw-0001tH-Mk; Tue, 15 Nov 2022 21:48:14 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1ov3mv-0001tA-M5 for linux-f2fs-devel@lists.sourceforge.net; Tue, 15 Nov 2022 21:48:13 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=In-Reply-To:Content-Type:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=y/MJxjAhU5GH9fkPJPganNWu5JIHQyySVDQ9WHGC9JA=; b=Qmib3REhD3PIH4raLMrf1UpQOd E8mvxlYGVjMVCPRyKTyd2NOy7RMZseRVmk2n3Nrwa+23pECRQ22Ngq4/cO7XlQ8DZzMMfVC64F+46 MVpRYnVX8EGXz4gSSw5Sz9NvbjrOP/lIYXNQ3rUBW7q1DYt8XSn9CcMOsJRjHWQi4k5E=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To :From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=y/MJxjAhU5GH9fkPJPganNWu5JIHQyySVDQ9WHGC9JA=; b=QbS5eVPBzOco6yaaUKyz4XTKle +GeR+jC5YwiGYUBTcFMZyRSJXh6qJ9lWWW9CkJ8IiskQN3Wl3GX3jesJXPTKcmb/vkuPgFT8+jJI5 iAUhjCNMvudCaxkAeMtYf2Ql3ZmzhaNeSC8XewA0j7itX1QyZ9tUhR7KHY44aPUs+3tw=; Received: from ams.source.kernel.org ([145.40.68.75]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1ov3mu-0000J8-9h for linux-f2fs-devel@lists.sourceforge.net; Tue, 15 Nov 2022 21:48:13 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 037D9B81B73; Tue, 15 Nov 2022 21:48:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 85B05C433B5; Tue, 15 Nov 2022 21:48:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1668548883; bh=CG0dXhmknwCwA3hdVEQE9D8iMO7v+ojqxHnJ/bgzyQI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BJ+DDeaaNph2fnh/K8SKttJN93KgcH5TMqzK8YMGOvh/Bnju6oDFljc1HwjReyBPk Z8rs5+oz8yQ4tyfqL3r3N/rIQoS33RinIXWr34GCHqbYaKHJmBeyeKzwkuvWWZPSbP L4OIzOctSwo+/E+PN7YP7T+BESEz2IyZEJRryiCzwK1vKsVdX8V6lrt5jpb1iXUI2v i9YfvpIiCZoTyHRxA4iuC5fsFDeigVRwhuh0T9P6HKT1DVrHHj40yi7PnPJg9r0pq2 6CG3id/7b48KTq3YUT1OYa36Flaet8mdRCt1BrT/KzmCxL7h/fl9IRQfCzXHQZ8t5v q+jAEsX1UYzpw== Date: Tue, 15 Nov 2022 13:48:01 -0800 From: Jaegeuk Kim To: Sheng Yong Message-ID: References: <20221115160155.1037163-1-shengyong@oppo.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20221115160155.1037163-1-shengyong@oppo.com> X-Headers-End: 1ov3mu-0000J8-9h Subject: Re: [f2fs-dev] [PATCH v3] f2fs: fix to enable compress for newly created file if extension matches X-BeenThere: linux-f2fs-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net On 11/16, Sheng Yong wrote: > If compress_extension is set, and a newly created file matches the > extension, the file could be marked as compression file. However, > if inline_data is also enabled, there is no chance to check its > extension since f2fs_should_compress() always returns false. > > This patch moves set_compress_inode(), which do extension check, in > f2fs_should_compress() to check extensions before setting inline > data flag. > > Fixes: 7165841d578e ("f2fs: fix to check inline_data during compressed inode conversion") > Signed-off-by: Sheng Yong > Signed-off-by: Jaegeuk Kim > --- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/namei.c | 336 ++++++++++++++++++++++++------------------------ > 2 files changed, 171 insertions(+), 166 deletions(-) > --- > > Hi, Jaegeuk, Chao, > > How about adding a bool `may_compress' in set_compress_new_inode, set > `my_compress` according to several conditions. If it is false, clear > F2FS_COMPR_FL. > > And set_compress_context is also changed to clear F2FS_NOCOMP_FL, > otherwise, if F2FS_NOCOMP_FL is inherited from parent and hit > compress_extension, both F2FS_NOCOMP_FL and F2FS_COMPR_FL are set. > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 6a8cbf5bb1871..a3420fbb29214 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -4355,6 +4355,7 @@ static inline int set_compress_context(struct inode *inode) > F2FS_I(inode)->i_compress_flag |= > F2FS_OPTION(sbi).compress_level << > COMPRESS_LEVEL_OFFSET; > + F2FS_I(inode)->i_flags &= ~F2FS_NOCOMP_FL; > F2FS_I(inode)->i_flags |= F2FS_COMPR_FL; > set_inode_flag(inode, FI_COMPRESSED_FILE); > stat_inc_compr_inode(inode); > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index e104409c3a0e5..36ec5cf7cf859 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -22,8 +22,170 @@ > #include "acl.h" > #include > > +static inline int is_extension_exist(const unsigned char *s, const char *sub, > + bool tmp_ext) > +{ > + size_t slen = strlen(s); > + size_t sublen = strlen(sub); > + int i; > + > + if (sublen == 1 && *sub == '*') > + return 1; > + > + /* > + * filename format of multimedia file should be defined as: > + * "filename + '.' + extension + (optional: '.' + temp extension)". > + */ > + if (slen < sublen + 2) > + return 0; > + > + if (!tmp_ext) { > + /* file has no temp extension */ > + if (s[slen - sublen - 1] != '.') > + return 0; > + return !strncasecmp(s + slen - sublen, sub, sublen); > + } > + > + for (i = 1; i < slen - sublen; i++) { > + if (s[i] != '.') > + continue; > + if (!strncasecmp(s + i + 1, sub, sublen)) > + return 1; > + } > + > + return 0; > +} > + > +int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name, > + bool hot, bool set) > +{ > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > + int cold_count = le32_to_cpu(sbi->raw_super->extension_count); > + int hot_count = sbi->raw_super->hot_ext_count; > + int total_count = cold_count + hot_count; > + int start, count; > + int i; > + > + if (set) { > + if (total_count == F2FS_MAX_EXTENSION) > + return -EINVAL; > + } else { > + if (!hot && !cold_count) > + return -EINVAL; > + if (hot && !hot_count) > + return -EINVAL; > + } > + > + if (hot) { > + start = cold_count; > + count = total_count; > + } else { > + start = 0; > + count = cold_count; > + } > + > + for (i = start; i < count; i++) { > + if (strcmp(name, extlist[i])) > + continue; > + > + if (set) > + return -EINVAL; > + > + memcpy(extlist[i], extlist[i + 1], > + F2FS_EXTENSION_LEN * (total_count - i - 1)); > + memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN); > + if (hot) > + sbi->raw_super->hot_ext_count = hot_count - 1; > + else > + sbi->raw_super->extension_count = > + cpu_to_le32(cold_count - 1); > + return 0; > + } > + > + if (!set) > + return -EINVAL; > + > + if (hot) { > + memcpy(extlist[count], name, strlen(name)); > + sbi->raw_super->hot_ext_count = hot_count + 1; > + } else { > + char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN]; > + > + memcpy(buf, &extlist[cold_count], > + F2FS_EXTENSION_LEN * hot_count); > + memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN); > + memcpy(extlist[cold_count], name, strlen(name)); > + memcpy(&extlist[cold_count + 1], buf, > + F2FS_EXTENSION_LEN * hot_count); > + sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1); > + } > + return 0; > +} > + > +static void set_compress_new_inode(struct f2fs_sb_info *sbi, struct inode *dir, > + struct inode *inode, const unsigned char *name) > +{ > + struct f2fs_inode_info *fi = F2FS_I(inode); > + __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > + unsigned char (*noext)[F2FS_EXTENSION_LEN] = > + F2FS_OPTION(sbi).noextensions; > + unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions; > + unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt; > + unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt; > + bool may_compress = false; > + int i, cold_count, hot_count; > + > + if (!f2fs_sb_has_compression(sbi) || !name) > + return; > + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode)) > + return; > + > + /* Inherit the compression flag in directory */ > + if (fi->i_flags & FS_COMPR_FL) > + may_compress = true; > + > + /* Start to check extension list for regular file */ > + if ((!ext_cnt && !noext_cnt) || S_ISDIR(inode->i_mode)) This doesn't address the Chao's point. It seems not much motivation to add may_compress. Let me try to combine some with the previous patch. > + goto set_compress; > + > + /* Don't compress hot files. */ > + f2fs_down_read(&sbi->sb_lock); > + cold_count = le32_to_cpu(sbi->raw_super->extension_count); > + hot_count = sbi->raw_super->hot_ext_count; > + for (i = cold_count; i < cold_count + hot_count; i++) > + if (is_extension_exist(name, extlist[i], false)) { > + may_compress = false; > + f2fs_up_read(&sbi->sb_lock); > + goto set_compress; > + } > + f2fs_up_read(&sbi->sb_lock); > + > + /* Don't compress unallowed extension. */ > + for (i = 0; i < noext_cnt; i++) { > + if (is_extension_exist(name, noext[i], false)) { > + may_compress = false; > + goto set_compress; > + } > + } > + > + /* Compress wanting extension. */ > + for (i = 0; i < ext_cnt; i++) { > + if (is_extension_exist(name, ext[i], false)) { > + may_compress = true; > + goto set_compress; > + } > + } > + > +set_compress: > + if (may_compress) > + set_compress_context(inode); > + else if (fi->i_flags & F2FS_COMPR_FL) > + fi->i_flags &= ~F2FS_COMPR_FL; > +} > + > static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns, > - struct inode *dir, umode_t mode) > + struct inode *dir, umode_t mode, > + const char *name) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(dir); > nid_t ino; > @@ -114,12 +276,8 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns, > if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) > set_inode_flag(inode, FI_PROJ_INHERIT); > > - if (f2fs_sb_has_compression(sbi)) { > - /* Inherit the compression flag in directory */ > - if ((F2FS_I(dir)->i_flags & F2FS_COMPR_FL) && > - f2fs_may_compress(inode)) > - set_compress_context(inode); > - } > + /* Check compression first. */ > + set_compress_new_inode(sbi, dir, inode, name); > > /* Should enable inline_data after compression set */ > if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode)) > @@ -153,40 +311,6 @@ static struct inode *f2fs_new_inode(struct user_namespace *mnt_userns, > return ERR_PTR(err); > } > > -static inline int is_extension_exist(const unsigned char *s, const char *sub, > - bool tmp_ext) > -{ > - size_t slen = strlen(s); > - size_t sublen = strlen(sub); > - int i; > - > - if (sublen == 1 && *sub == '*') > - return 1; > - > - /* > - * filename format of multimedia file should be defined as: > - * "filename + '.' + extension + (optional: '.' + temp extension)". > - */ > - if (slen < sublen + 2) > - return 0; > - > - if (!tmp_ext) { > - /* file has no temp extension */ > - if (s[slen - sublen - 1] != '.') > - return 0; > - return !strncasecmp(s + slen - sublen, sub, sublen); > - } > - > - for (i = 1; i < slen - sublen; i++) { > - if (s[i] != '.') > - continue; > - if (!strncasecmp(s + i + 1, sub, sublen)) > - return 1; > - } > - > - return 0; > -} > - > /* > * Set file's temperature for hot/cold data separation > */ > @@ -217,124 +341,6 @@ static inline void set_file_temperature(struct f2fs_sb_info *sbi, struct inode * > file_set_hot(inode); > } > > -int f2fs_update_extension_list(struct f2fs_sb_info *sbi, const char *name, > - bool hot, bool set) > -{ > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > - int cold_count = le32_to_cpu(sbi->raw_super->extension_count); > - int hot_count = sbi->raw_super->hot_ext_count; > - int total_count = cold_count + hot_count; > - int start, count; > - int i; > - > - if (set) { > - if (total_count == F2FS_MAX_EXTENSION) > - return -EINVAL; > - } else { > - if (!hot && !cold_count) > - return -EINVAL; > - if (hot && !hot_count) > - return -EINVAL; > - } > - > - if (hot) { > - start = cold_count; > - count = total_count; > - } else { > - start = 0; > - count = cold_count; > - } > - > - for (i = start; i < count; i++) { > - if (strcmp(name, extlist[i])) > - continue; > - > - if (set) > - return -EINVAL; > - > - memcpy(extlist[i], extlist[i + 1], > - F2FS_EXTENSION_LEN * (total_count - i - 1)); > - memset(extlist[total_count - 1], 0, F2FS_EXTENSION_LEN); > - if (hot) > - sbi->raw_super->hot_ext_count = hot_count - 1; > - else > - sbi->raw_super->extension_count = > - cpu_to_le32(cold_count - 1); > - return 0; > - } > - > - if (!set) > - return -EINVAL; > - > - if (hot) { > - memcpy(extlist[count], name, strlen(name)); > - sbi->raw_super->hot_ext_count = hot_count + 1; > - } else { > - char buf[F2FS_MAX_EXTENSION][F2FS_EXTENSION_LEN]; > - > - memcpy(buf, &extlist[cold_count], > - F2FS_EXTENSION_LEN * hot_count); > - memset(extlist[cold_count], 0, F2FS_EXTENSION_LEN); > - memcpy(extlist[cold_count], name, strlen(name)); > - memcpy(&extlist[cold_count + 1], buf, > - F2FS_EXTENSION_LEN * hot_count); > - sbi->raw_super->extension_count = cpu_to_le32(cold_count + 1); > - } > - return 0; > -} > - > -static void set_compress_inode(struct f2fs_sb_info *sbi, struct inode *inode, > - const unsigned char *name) > -{ > - __u8 (*extlist)[F2FS_EXTENSION_LEN] = sbi->raw_super->extension_list; > - unsigned char (*noext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).noextensions; > - unsigned char (*ext)[F2FS_EXTENSION_LEN] = F2FS_OPTION(sbi).extensions; > - unsigned char ext_cnt = F2FS_OPTION(sbi).compress_ext_cnt; > - unsigned char noext_cnt = F2FS_OPTION(sbi).nocompress_ext_cnt; > - int i, cold_count, hot_count; > - > - if (!f2fs_sb_has_compression(sbi) || > - F2FS_I(inode)->i_flags & F2FS_NOCOMP_FL || > - !f2fs_may_compress(inode) || > - (!ext_cnt && !noext_cnt)) > - return; > - > - f2fs_down_read(&sbi->sb_lock); > - > - cold_count = le32_to_cpu(sbi->raw_super->extension_count); > - hot_count = sbi->raw_super->hot_ext_count; > - > - for (i = cold_count; i < cold_count + hot_count; i++) { > - if (is_extension_exist(name, extlist[i], false)) { > - f2fs_up_read(&sbi->sb_lock); > - return; > - } > - } > - > - f2fs_up_read(&sbi->sb_lock); > - > - for (i = 0; i < noext_cnt; i++) { > - if (is_extension_exist(name, noext[i], false)) { > - f2fs_disable_compressed_file(inode); > - return; > - } > - } > - > - if (is_inode_flag_set(inode, FI_COMPRESSED_FILE)) > - return; > - > - for (i = 0; i < ext_cnt; i++) { > - if (!is_extension_exist(name, ext[i], false)) > - continue; > - > - /* Do not use inline_data with compression */ > - stat_dec_inline_inode(inode); > - clear_inode_flag(inode, FI_INLINE_DATA); > - set_compress_context(inode); > - return; > - } > -} > - > static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir, > struct dentry *dentry, umode_t mode, bool excl) > { > @@ -352,15 +358,13 @@ static int f2fs_create(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, mode); > + inode = f2fs_new_inode(mnt_userns, dir, mode, dentry->d_name.name); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > if (!test_opt(sbi, DISABLE_EXT_IDENTIFY)) > set_file_temperature(sbi, inode, dentry->d_name.name); > > - set_compress_inode(sbi, inode, dentry->d_name.name); > - > inode->i_op = &f2fs_file_inode_operations; > inode->i_fop = &f2fs_file_operations; > inode->i_mapping->a_ops = &f2fs_dblock_aops; > @@ -689,7 +693,7 @@ static int f2fs_symlink(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO); > + inode = f2fs_new_inode(mnt_userns, dir, S_IFLNK | S_IRWXUGO, NULL); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > @@ -760,7 +764,7 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode); > + inode = f2fs_new_inode(mnt_userns, dir, S_IFDIR | mode, dentry->d_name.name); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > @@ -817,7 +821,7 @@ static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, mode); > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > @@ -856,7 +860,7 @@ static int __f2fs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > if (err) > return err; > > - inode = f2fs_new_inode(mnt_userns, dir, mode); > + inode = f2fs_new_inode(mnt_userns, dir, mode, NULL); > if (IS_ERR(inode)) > return PTR_ERR(inode); > > -- > 2.25.1 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel