| Message ID | 20220511193146.27526-4-krisman@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean up the case-insensitive lookup path | expand |
On Wed, May 11, 2022 at 03:31:39PM -0400, Gabriel Krisman Bertazi wrote: > Keeping it as qstr avoids the unnecessary conversion in f2fs_match > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/f2fs/dir.c | 52 ++++++++++++++++++++++++++++------------------ > fs/f2fs/f2fs.h | 3 ++- > fs/f2fs/recovery.c | 5 +---- > 3 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index 166f08623362..c2a02003c5b9 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -81,28 +81,47 @@ int f2fs_init_casefolded_name(const struct inode *dir, > { > #if IS_ENABLED(CONFIG_UNICODE) > struct super_block *sb = dir->i_sb; > + unsigned char *buf; > + int len; > > if (IS_CASEFOLDED(dir)) { > - fname->cf_name.name = f2fs_kmem_cache_alloc(f2fs_cf_name_slab, > + buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab, > GFP_NOFS, false, F2FS_SB(sb)); > - if (!fname->cf_name.name) > - return -ENOMEM; > - fname->cf_name.len = utf8_casefold(sb->s_encoding, > - fname->usr_fname, > - fname->cf_name.name, > - F2FS_NAME_LEN); > - if ((int)fname->cf_name.len <= 0) { > - kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name); > + if (!buf) { > fname->cf_name.name = NULL; > + return -ENOMEM; > + } > + > + len = utf8_casefold(sb->s_encoding, fname->usr_fname, > + buf, F2FS_NAME_LEN); > + > + if (len <= 0) { > + kmem_cache_free(f2fs_cf_name_slab, buf); > + buf = NULL; > if (sb_has_strict_encoding(sb)) > return -EINVAL; > /* fall back to treating name as opaque byte sequence */ > } > + fname->cf_name.name = buf; > + fname->cf_name.len = (unsigned int) len; > } > #endif There's some inconsistent behavior above; now sometimes fname->cf_name.name is set to NULL on failure and sometime it's not. Also now fname->cf_name.len can be set to a negative value. Since struct f2fs_filename is always zero-initialized, how about only setting the fname->cf_name fields if we actually have a valid value to assign? I.e. buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab, GFP_NOFS, false, F2FS_SB(sb)); if (!buf) return -ENOMEM; len = utf8_casefold(sb->s_encoding, fname->usr_fname, buf, F2FS_NAME_LEN); if (len <= 0) { kmem_cache_free(f2fs_cf_name_slab, buf); if (sb_has_strict_encoding(sb)) return -EINVAL; /* fall back to treating name as opaque byte sequence */ return 0; } fname->cf_name.name = buf; fname->cf_name.len = len; > +void f2fs_free_casefolded_name(struct f2fs_filename *fname) > +{ > +#if IS_ENABLED(CONFIG_UNICODE) > + unsigned char *buf = (unsigned char *)fname->cf_name.name; > + > + if (buf) { > + kmem_cache_free(f2fs_cf_name_slab, buf); > + fname->cf_name.name = NULL; > + } > + > +#endif > +} Kernel code usually uses static inline stubs for the !CONFIG_$FOO case in cases like this, as that causes the function calls to be compiled away to nothing when they're unneeded. - Eric
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index 166f08623362..c2a02003c5b9 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -81,28 +81,47 @@ int f2fs_init_casefolded_name(const struct inode *dir, { #if IS_ENABLED(CONFIG_UNICODE) struct super_block *sb = dir->i_sb; + unsigned char *buf; + int len; if (IS_CASEFOLDED(dir)) { - fname->cf_name.name = f2fs_kmem_cache_alloc(f2fs_cf_name_slab, + buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab, GFP_NOFS, false, F2FS_SB(sb)); - if (!fname->cf_name.name) - return -ENOMEM; - fname->cf_name.len = utf8_casefold(sb->s_encoding, - fname->usr_fname, - fname->cf_name.name, - F2FS_NAME_LEN); - if ((int)fname->cf_name.len <= 0) { - kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name); + if (!buf) { fname->cf_name.name = NULL; + return -ENOMEM; + } + + len = utf8_casefold(sb->s_encoding, fname->usr_fname, + buf, F2FS_NAME_LEN); + + if (len <= 0) { + kmem_cache_free(f2fs_cf_name_slab, buf); + buf = NULL; if (sb_has_strict_encoding(sb)) return -EINVAL; /* fall back to treating name as opaque byte sequence */ } + fname->cf_name.name = buf; + fname->cf_name.len = (unsigned int) len; } #endif return 0; } +void f2fs_free_casefolded_name(struct f2fs_filename *fname) +{ +#if IS_ENABLED(CONFIG_UNICODE) + unsigned char *buf = (unsigned char *)fname->cf_name.name; + + if (buf) { + kmem_cache_free(f2fs_cf_name_slab, buf); + fname->cf_name.name = NULL; + } + +#endif +} + static int __f2fs_setup_filename(const struct inode *dir, const struct fscrypt_name *crypt_name, struct f2fs_filename *fname) @@ -174,12 +193,7 @@ void f2fs_free_filename(struct f2fs_filename *fname) kfree(fname->crypto_buf.name); fname->crypto_buf.name = NULL; #endif -#if IS_ENABLED(CONFIG_UNICODE) - if (fname->cf_name.name) { - kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name); - fname->cf_name.name = NULL; - } -#endif + f2fs_free_casefolded_name(fname); } static unsigned long dir_block_index(unsigned int level, @@ -267,11 +281,9 @@ static inline int f2fs_match_name(const struct inode *dir, struct fscrypt_name f; #if IS_ENABLED(CONFIG_UNICODE) - if (fname->cf_name.name) { - struct qstr cf = FSTR_TO_QSTR(&fname->cf_name); - - return f2fs_match_ci_name(dir, &cf, de_name, de_name_len); - } + if (fname->cf_name.name) + return f2fs_match_ci_name(dir, &fname->cf_name, + de_name, de_name_len); #endif f.usr_fname = fname->usr_fname; f.disk_name = fname->disk_name; diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 68b44015514f..4b6df34fc8b0 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -497,7 +497,7 @@ struct f2fs_filename { * NULL. In all these cases we fall back to treating the name as an * opaque byte sequence. */ - struct fscrypt_str cf_name; + struct qstr cf_name; #endif }; @@ -3345,6 +3345,7 @@ struct dentry *f2fs_get_parent(struct dentry *child); unsigned char f2fs_get_de_type(struct f2fs_dir_entry *de); int f2fs_init_casefolded_name(const struct inode *dir, struct f2fs_filename *fname); +void f2fs_free_casefolded_name(struct f2fs_filename *fname); int f2fs_setup_filename(struct inode *dir, const struct qstr *iname, int lookup, struct f2fs_filename *fname); int f2fs_prepare_lookup(struct inode *dir, struct dentry *dentry, diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index 79773d322c47..3c3a8abf6953 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -149,11 +149,8 @@ static int init_recovered_filename(const struct inode *dir, if (err) return err; f2fs_hash_filename(dir, fname); -#if IS_ENABLED(CONFIG_UNICODE) /* Case-sensitive match is fine for recovery */ - kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name); - fname->cf_name.name = NULL; -#endif + f2fs_free_casefolded_name(fname); } else { f2fs_hash_filename(dir, fname); }
Keeping it as qstr avoids the unnecessary conversion in f2fs_match Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/f2fs/dir.c | 52 ++++++++++++++++++++++++++++------------------ fs/f2fs/f2fs.h | 3 ++- fs/f2fs/recovery.c | 5 +---- 3 files changed, 35 insertions(+), 25 deletions(-)