| Message ID | 20240902225511.757831-7-andrealmeid@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tmpfs: Add case-insesitive support for tmpfs | expand |
Hi André, kernel test robot noticed the following build errors: [auto build test ERROR on akpm-mm/mm-everything] [also build test ERROR on tytso-ext4/dev brauner-vfs/vfs.all linus/master v6.11-rc6 next-20240903] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/unicode-Fix-utf8_load-error-path/20240903-070149 base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20240902225511.757831-7-andrealmeid%40igalia.com patch subject: [PATCH v2 6/8] tmpfs: Add flag FS_CASEFOLD_FL support for tmpfs dirs config: i386-buildonly-randconfig-005-20240903 (https://download.01.org/0day-ci/archive/20240903/202409031620.BOshMDgn-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240903/202409031620.BOshMDgn-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409031620.BOshMDgn-lkp@intel.com/ All errors (new ones prefixed by >>): >> mm/shmem.c:2771:12: error: no member named 's_encoding' in 'struct super_block' 2771 | if (!sb->s_encoding) | ~~ ^ 1 error generated. vim +2771 mm/shmem.c 2760 2761 /* 2762 * chattr's fsflags are unrelated to extended attributes, 2763 * but tmpfs has chosen to enable them under the same config option. 2764 */ 2765 static int shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) 2766 { 2767 unsigned int i_flags = 0, old = inode->i_flags; 2768 struct super_block *sb = inode->i_sb; 2769 2770 if (fsflags & FS_CASEFOLD_FL) { > 2771 if (!sb->s_encoding) 2772 return -EOPNOTSUPP; 2773 2774 if (!S_ISDIR(inode->i_mode)) 2775 return -ENOTDIR; 2776 2777 if (dentry && !simple_empty(dentry)) 2778 return -ENOTEMPTY; 2779 2780 i_flags |= S_CASEFOLD; 2781 } else if (old & S_CASEFOLD) { 2782 if (dentry && !simple_empty(dentry)) 2783 return -ENOTEMPTY; 2784 } 2785 2786 if (fsflags & FS_NOATIME_FL) 2787 i_flags |= S_NOATIME; 2788 if (fsflags & FS_APPEND_FL) 2789 i_flags |= S_APPEND; 2790 if (fsflags & FS_IMMUTABLE_FL) 2791 i_flags |= S_IMMUTABLE; 2792 /* 2793 * But FS_NODUMP_FL does not require any action in i_flags. 2794 */ 2795 inode_set_flags(inode, i_flags, S_NOATIME | S_APPEND | S_IMMUTABLE | S_CASEFOLD); 2796 2797 return 0; 2798 } 2799 #else 2800 static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) 2801 { 2802 } 2803 #define shmem_initxattrs NULL 2804 #endif 2805
Hi André, kernel test robot noticed the following build errors: [auto build test ERROR on akpm-mm/mm-everything] [also build test ERROR on tytso-ext4/dev brauner-vfs/vfs.all linus/master v6.11-rc6 next-20240903] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/unicode-Fix-utf8_load-error-path/20240903-070149 base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20240902225511.757831-7-andrealmeid%40igalia.com patch subject: [PATCH v2 6/8] tmpfs: Add flag FS_CASEFOLD_FL support for tmpfs dirs config: i386-buildonly-randconfig-002-20240903 (https://download.01.org/0day-ci/archive/20240903/202409031642.6kP6Ra8c-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240903/202409031642.6kP6Ra8c-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409031642.6kP6Ra8c-lkp@intel.com/ All errors (new ones prefixed by >>): mm/shmem.c: In function 'shmem_set_inode_flags': >> mm/shmem.c:2771:24: error: 'struct super_block' has no member named 's_encoding' 2771 | if (!sb->s_encoding) | ^~ vim +2771 mm/shmem.c 2760 2761 /* 2762 * chattr's fsflags are unrelated to extended attributes, 2763 * but tmpfs has chosen to enable them under the same config option. 2764 */ 2765 static int shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) 2766 { 2767 unsigned int i_flags = 0, old = inode->i_flags; 2768 struct super_block *sb = inode->i_sb; 2769 2770 if (fsflags & FS_CASEFOLD_FL) { > 2771 if (!sb->s_encoding) 2772 return -EOPNOTSUPP; 2773 2774 if (!S_ISDIR(inode->i_mode)) 2775 return -ENOTDIR; 2776 2777 if (dentry && !simple_empty(dentry)) 2778 return -ENOTEMPTY; 2779 2780 i_flags |= S_CASEFOLD; 2781 } else if (old & S_CASEFOLD) { 2782 if (dentry && !simple_empty(dentry)) 2783 return -ENOTEMPTY; 2784 } 2785 2786 if (fsflags & FS_NOATIME_FL) 2787 i_flags |= S_NOATIME; 2788 if (fsflags & FS_APPEND_FL) 2789 i_flags |= S_APPEND; 2790 if (fsflags & FS_IMMUTABLE_FL) 2791 i_flags |= S_IMMUTABLE; 2792 /* 2793 * But FS_NODUMP_FL does not require any action in i_flags. 2794 */ 2795 inode_set_flags(inode, i_flags, S_NOATIME | S_APPEND | S_IMMUTABLE | S_CASEFOLD); 2796 2797 return 0; 2798 } 2799 #else 2800 static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) 2801 { 2802 } 2803 #define shmem_initxattrs NULL 2804 #endif 2805
André Almeida <andrealmeid@igalia.com> writes: > Enable setting flag FS_CASEFOLD_FL for tmpfs directories, when tmpfs is > mounted with casefold support. A special check is need for this flag, > since it can't be set for non-empty directories. > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > --- > include/linux/shmem_fs.h | 6 +++--- > mm/shmem.c | 40 +++++++++++++++++++++++++++++++++------- > 2 files changed, 36 insertions(+), 10 deletions(-) > > diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h > index 1d06b1e5408a..8367ca2b99d9 100644 > --- a/include/linux/shmem_fs.h > +++ b/include/linux/shmem_fs.h > @@ -42,10 +42,10 @@ struct shmem_inode_info { > struct inode vfs_inode; > }; > > -#define SHMEM_FL_USER_VISIBLE FS_FL_USER_VISIBLE > +#define SHMEM_FL_USER_VISIBLE (FS_FL_USER_VISIBLE | FS_CASEFOLD_FL) > #define SHMEM_FL_USER_MODIFIABLE \ > - (FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL | FS_NOATIME_FL) > -#define SHMEM_FL_INHERITED (FS_NODUMP_FL | FS_NOATIME_FL) > + (FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL | FS_NOATIME_FL | FS_CASEFOLD_FL) > +#define SHMEM_FL_INHERITED (FS_NODUMP_FL | FS_NOATIME_FL | FS_CASEFOLD_FL) > > struct shmem_quota_limits { > qsize_t usrquota_bhardlimit; /* Default user quota block hard limit */ > diff --git a/mm/shmem.c b/mm/shmem.c > index 0f918010bc54..9a0fc7636629 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2617,9 +2617,26 @@ static int shmem_initxattrs(struct inode *, const struct xattr *, void *); > * chattr's fsflags are unrelated to extended attributes, > * but tmpfs has chosen to enable them under the same config option. > */ > -static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) > +static int shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) > { > - unsigned int i_flags = 0; > + unsigned int i_flags = 0, old = inode->i_flags; > + struct super_block *sb = inode->i_sb; > + > + if (fsflags & FS_CASEFOLD_FL) { > + if (!sb->s_encoding) > + return -EOPNOTSUPP; > + > + if (!S_ISDIR(inode->i_mode)) > + return -ENOTDIR; > + > + if (dentry && !simple_empty(dentry)) > + return -ENOTEMPTY; > + > + i_flags |= S_CASEFOLD; > + } else if (old & S_CASEFOLD) { > + if (dentry && !simple_empty(dentry)) > + return -ENOTEMPTY; We don't want to fail if a directory already has the S_CASEFOLD flag and we are not flipping it in the current operation. Something like: if ((fsflags ^ old) & S_CASEFOLD) { if (!sb->s_encoding) return -EOPNOTSUPP; if (!S_ISDIR(inode->i_mode)) return -ENOTDIR; if (dentry && !simple_empty(dentry)) return -ENOTEMPTY; i_flags |= fsflags & S_CASEFOLD; } > > if (fsflags & FS_NOATIME_FL) > i_flags |= S_NOATIME; > @@ -2630,10 +2647,12 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) > /* > * But FS_NODUMP_FL does not require any action in i_flags. > */ > - inode_set_flags(inode, i_flags, S_NOATIME | S_APPEND | S_IMMUTABLE); > + inode_set_flags(inode, i_flags, S_NOATIME | S_APPEND | S_IMMUTABLE | S_CASEFOLD); > + > + return 0; > } > #else > -static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) > +static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) > { > } > #define shmem_initxattrs NULL > @@ -2680,7 +2699,7 @@ static struct inode *__shmem_get_inode(struct mnt_idmap *idmap, > info->fsflags = (dir == NULL) ? 0 : > SHMEM_I(dir)->fsflags & SHMEM_FL_INHERITED; > if (info->fsflags) > - shmem_set_inode_flags(inode, info->fsflags); > + shmem_set_inode_flags(inode, info->fsflags, NULL); > INIT_LIST_HEAD(&info->shrinklist); > INIT_LIST_HEAD(&info->swaplist); > simple_xattrs_init(&info->xattrs); > @@ -3790,16 +3809,23 @@ static int shmem_fileattr_set(struct mnt_idmap *idmap, > { > struct inode *inode = d_inode(dentry); > struct shmem_inode_info *info = SHMEM_I(inode); > + int ret, flags; > > if (fileattr_has_fsx(fa)) > return -EOPNOTSUPP; > if (fa->flags & ~SHMEM_FL_USER_MODIFIABLE) > return -EOPNOTSUPP; > > - info->fsflags = (info->fsflags & ~SHMEM_FL_USER_MODIFIABLE) | > + flags = (info->fsflags & ~SHMEM_FL_USER_MODIFIABLE) | > (fa->flags & SHMEM_FL_USER_MODIFIABLE); > > - shmem_set_inode_flags(inode, info->fsflags); > + ret = shmem_set_inode_flags(inode, flags, dentry); > + > + if (ret) > + return ret; > + > + info->fsflags = flags; > + > inode_set_ctime_current(inode); > inode_inc_iversion(inode); > return 0;
Hi Krisman, Thanks for the feedback! Em 03/09/2024 13:15, Gabriel Krisman Bertazi escreveu: > André Almeida <andrealmeid@igalia.com> writes: > >> Enable setting flag FS_CASEFOLD_FL for tmpfs directories, when tmpfs is >> mounted with casefold support. A special check is need for this flag, >> since it can't be set for non-empty directories. >> >> Signed-off-by: André Almeida <andrealmeid@igalia.com> [...] >> + >> + if (fsflags & FS_CASEFOLD_FL) { >> + if (!sb->s_encoding) >> + return -EOPNOTSUPP; >> + >> + if (!S_ISDIR(inode->i_mode)) >> + return -ENOTDIR; >> + >> + if (dentry && !simple_empty(dentry)) >> + return -ENOTEMPTY; >> + >> + i_flags |= S_CASEFOLD; >> + } else if (old & S_CASEFOLD) { >> + if (dentry && !simple_empty(dentry)) >> + return -ENOTEMPTY; > > We don't want to fail if a directory already has the S_CASEFOLD > flag and we are not flipping it in the current operation. Something like: > > if ((fsflags ^ old) & S_CASEFOLD) { > if (!sb->s_encoding) > return -EOPNOTSUPP; > > if (!S_ISDIR(inode->i_mode)) > return -ENOTDIR; > > if (dentry && !simple_empty(dentry)) > return -ENOTEMPTY; > i_flags |= fsflags & S_CASEFOLD; > } > You are right, it's broken and failing for directories with S_CASEFOLD. Here's a small test showing that we can't add the +d attribute to a non-empty CI folder (+d doesn't require the directory to be empty): folder ) mkdir A folder ) mkdir A/B folder ) chattr +d A/B folder ) chattr +d A chattr: Directory not empty while setting flags on A However, FS_CASEFOLD_FL != S_CASEFOLD and the set of values for inode->i_flags (var old) and fsflags aren't the same, so your proposed snippet didn't work. I see that ext4 has a very similar code as your proposal, but I think they do something different with the flag values. I rewrote my code separating the three possible paths and it worked: /* inheritance from parent dir/keeping the same flags path */ if ((fsflags & FS_CASEFOLD_FL) && (old & S_CASEFOLD)) i_flags |= S_CASEFOLD; /* removing flag path */ if (!(fsflags & FS_CASEFOLD_FL) && (old & S_CASEFOLD)) if (dentry && !simple_empty(dentry)) return -ENOTEMPTY; /* adding flag path */ if ((fsflags & FS_CASEFOLD_FL) && !(old & S_CASEFOLD)) { if (!sb->s_encoding) return -EOPNOTSUPP; if (!S_ISDIR(inode->i_mode)) return -ENOTDIR; if (dentry && !simple_empty(dentry)) return -ENOTEMPTY; i_flags |= S_CASEFOLD; } In that way, the `chattr +d` call doesn't fall into the simple_empty() check. I simplified the code like this for the v3: if (fsflags & FS_CASEFOLD_FL) { if (!(old & S_CASEFOLD)) { if (!sb->s_encoding) return -EOPNOTSUPP; if (!S_ISDIR(inode->i_mode)) return -ENOTDIR; if (dentry && !simple_empty(dentry)) return -ENOTEMPTY; } i_flags |= S_CASEFOLD; } else if (old & S_CASEFOLD) { if (dentry && !simple_empty(dentry)) return -ENOTEMPTY; }
diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h index 1d06b1e5408a..8367ca2b99d9 100644 --- a/include/linux/shmem_fs.h +++ b/include/linux/shmem_fs.h @@ -42,10 +42,10 @@ struct shmem_inode_info { struct inode vfs_inode; }; -#define SHMEM_FL_USER_VISIBLE FS_FL_USER_VISIBLE +#define SHMEM_FL_USER_VISIBLE (FS_FL_USER_VISIBLE | FS_CASEFOLD_FL) #define SHMEM_FL_USER_MODIFIABLE \ - (FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL | FS_NOATIME_FL) -#define SHMEM_FL_INHERITED (FS_NODUMP_FL | FS_NOATIME_FL) + (FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL | FS_NOATIME_FL | FS_CASEFOLD_FL) +#define SHMEM_FL_INHERITED (FS_NODUMP_FL | FS_NOATIME_FL | FS_CASEFOLD_FL) struct shmem_quota_limits { qsize_t usrquota_bhardlimit; /* Default user quota block hard limit */ diff --git a/mm/shmem.c b/mm/shmem.c index 0f918010bc54..9a0fc7636629 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2617,9 +2617,26 @@ static int shmem_initxattrs(struct inode *, const struct xattr *, void *); * chattr's fsflags are unrelated to extended attributes, * but tmpfs has chosen to enable them under the same config option. */ -static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) +static int shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) { - unsigned int i_flags = 0; + unsigned int i_flags = 0, old = inode->i_flags; + struct super_block *sb = inode->i_sb; + + if (fsflags & FS_CASEFOLD_FL) { + if (!sb->s_encoding) + return -EOPNOTSUPP; + + if (!S_ISDIR(inode->i_mode)) + return -ENOTDIR; + + if (dentry && !simple_empty(dentry)) + return -ENOTEMPTY; + + i_flags |= S_CASEFOLD; + } else if (old & S_CASEFOLD) { + if (dentry && !simple_empty(dentry)) + return -ENOTEMPTY; + } if (fsflags & FS_NOATIME_FL) i_flags |= S_NOATIME; @@ -2630,10 +2647,12 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) /* * But FS_NODUMP_FL does not require any action in i_flags. */ - inode_set_flags(inode, i_flags, S_NOATIME | S_APPEND | S_IMMUTABLE); + inode_set_flags(inode, i_flags, S_NOATIME | S_APPEND | S_IMMUTABLE | S_CASEFOLD); + + return 0; } #else -static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) +static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags, struct dentry *dentry) { } #define shmem_initxattrs NULL @@ -2680,7 +2699,7 @@ static struct inode *__shmem_get_inode(struct mnt_idmap *idmap, info->fsflags = (dir == NULL) ? 0 : SHMEM_I(dir)->fsflags & SHMEM_FL_INHERITED; if (info->fsflags) - shmem_set_inode_flags(inode, info->fsflags); + shmem_set_inode_flags(inode, info->fsflags, NULL); INIT_LIST_HEAD(&info->shrinklist); INIT_LIST_HEAD(&info->swaplist); simple_xattrs_init(&info->xattrs); @@ -3790,16 +3809,23 @@ static int shmem_fileattr_set(struct mnt_idmap *idmap, { struct inode *inode = d_inode(dentry); struct shmem_inode_info *info = SHMEM_I(inode); + int ret, flags; if (fileattr_has_fsx(fa)) return -EOPNOTSUPP; if (fa->flags & ~SHMEM_FL_USER_MODIFIABLE) return -EOPNOTSUPP; - info->fsflags = (info->fsflags & ~SHMEM_FL_USER_MODIFIABLE) | + flags = (info->fsflags & ~SHMEM_FL_USER_MODIFIABLE) | (fa->flags & SHMEM_FL_USER_MODIFIABLE); - shmem_set_inode_flags(inode, info->fsflags); + ret = shmem_set_inode_flags(inode, flags, dentry); + + if (ret) + return ret; + + info->fsflags = flags; + inode_set_ctime_current(inode); inode_inc_iversion(inode); return 0;
Enable setting flag FS_CASEFOLD_FL for tmpfs directories, when tmpfs is mounted with casefold support. A special check is need for this flag, since it can't be set for non-empty directories. Signed-off-by: André Almeida <andrealmeid@igalia.com> --- include/linux/shmem_fs.h | 6 +++--- mm/shmem.c | 40 +++++++++++++++++++++++++++++++++------- 2 files changed, 36 insertions(+), 10 deletions(-)