On Wed 12-11-14 20:18:02, Namjae Jeon wrote:
> Convert frequent small dynamic memory allocation to static allocation
> for filename.
>
> It has been observed that there is huge performance drop in udf lookup
> performance when CONFIG_SLUB_DEBUG is enable. As per analysis with perf
> tool, most of time is spend in kmalloc and kfree in this scenario.
> Performance drop seems very severe as compared to other file system in
> similar scenario.
>
> Consider below lookup test results on large udf mounted directory which has
> 4300 files
>
> [Original Kernel]:
> $ mount -t udf -o ro,relatime,uid=0,gid=1010,mode=775,dmode=775,utf8 /dev/cdrom /mnt/
> $ cd /mnt/dir1
> $ time ls
> ...................
> 00222610.png 00340607.png 00442904.png 00571618.png 01105104.png
> real 4m 31.20s
> user 0m 0.05s
> sys 4m 9.48s
>
> [After Applying Patch]
> $ mount -t udf -o ro,relatime,uid=0,gid=1010,mode=775,dmode=775,utf8 /dev/cdrom /mnt/
> $ cd /mnt/dir1
> $ time ls
> ................
> 00222610.png 00340607.png 00442904.png 00571618.png 01105104.png
> real 0m 21.11s
> user 0m 0.04s
> sys 0m 4.65s
>
> Signed-off-by: Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
> Signed-off-by: Vivek Trivedi <t.vivek@xxxxxxxxxxx>
This is wrong. Your patch will make UDF consume 512-bytes of stack more
just in udf_get_filename(), udf_readdir() which calls this adds another 256
bytes. You cannot just waste kernel stack stack space like this or we soon
overflow it.
Frankly I don't think improving performance with SLUB_DEBUG is a sensible
goal...
Honza
> ---
> fs/udf/dir.c | 9 +--------
> fs/udf/namei.c | 23 +++--------------------
> fs/udf/unicode.c | 37 +++++++++++++------------------------
> 3 files changed, 17 insertions(+), 52 deletions(-)
>
> diff --git a/fs/udf/dir.c b/fs/udf/dir.c
> index a012c51..490b9dd 100644
> --- a/fs/udf/dir.c
> +++ b/fs/udf/dir.c
> @@ -46,7 +46,7 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
> int block, iblock;
> loff_t nf_pos;
> int flen;
> - unsigned char *fname = NULL;
> + unsigned char fname[UDF_NAME_LEN];
> unsigned char *nameptr;
> uint16_t liu;
> uint8_t lfi;
> @@ -67,12 +67,6 @@ static int udf_readdir(struct file *file, struct dir_context *ctx)
> if (nf_pos >= size)
> goto out;
>
> - fname = kmalloc(UDF_NAME_LEN, GFP_NOFS);
> - if (!fname) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> if (nf_pos == 0)
> nf_pos = udf_ext0_offset(dir);
>
> @@ -184,7 +178,6 @@ out:
> brelse(fibh.ebh);
> brelse(fibh.sbh);
> brelse(epos.bh);
> - kfree(fname);
>
> return ret;
> }
> diff --git a/fs/udf/namei.c b/fs/udf/namei.c
> index c12e260..45cbbb9 100644
> --- a/fs/udf/namei.c
> +++ b/fs/udf/namei.c
> @@ -147,7 +147,7 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
> struct fileIdentDesc *fi = NULL;
> loff_t f_pos;
> int block, flen;
> - unsigned char *fname = NULL;
> + unsigned char fname[UDF_NAME_LEN];
> unsigned char *nameptr;
> uint8_t lfi;
> uint16_t liu;
> @@ -183,10 +183,6 @@ static struct fileIdentDesc *udf_find_entry(struct inode *dir,
> goto out_err;
> }
>
> - fname = kmalloc(UDF_NAME_LEN, GFP_NOFS);
> - if (!fname)
> - goto out_err;
> -
> while (f_pos < size) {
> fi = udf_fileident_read(dir, &f_pos, fibh, cfi, &epos, &eloc,
> &elen, &offset);
> @@ -245,7 +241,6 @@ out_err:
> brelse(fibh->sbh);
> out_ok:
> brelse(epos.bh);
> - kfree(fname);
>
> return fi;
> }
> @@ -298,7 +293,7 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
> {
> struct super_block *sb = dir->i_sb;
> struct fileIdentDesc *fi = NULL;
> - unsigned char *name = NULL;
> + unsigned char name[UDF_NAME_LEN];
> int namelen;
> loff_t f_pos;
> loff_t size = udf_ext0_offset(dir) + dir->i_size;
> @@ -313,11 +308,6 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
> struct udf_inode_info *dinfo;
>
> fibh->sbh = fibh->ebh = NULL;
> - name = kmalloc(UDF_NAME_LEN, GFP_NOFS);
> - if (!name) {
> - *err = -ENOMEM;
> - goto out_err;
> - }
>
> if (dentry) {
> if (!dentry->d_name.len) {
> @@ -532,7 +522,6 @@ out_err:
> brelse(fibh->sbh);
> out_ok:
> brelse(epos.bh);
> - kfree(name);
> return fi;
> }
>
> @@ -858,7 +847,7 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
> uint8_t *ea;
> int err;
> int block;
> - unsigned char *name = NULL;
> + unsigned char name[UDF_NAME_LEN];
> int namelen;
> struct udf_inode_info *iinfo;
> struct super_block *sb = dir->i_sb;
> @@ -868,11 +857,6 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
>
> iinfo = UDF_I(inode);
> down_write(&iinfo->i_data_sem);
> - name = kmalloc(UDF_NAME_LEN, GFP_NOFS);
> - if (!name) {
> - err = -ENOMEM;
> - goto out_no_entry;
> - }
>
> inode->i_data.a_ops = &udf_symlink_aops;
> inode->i_op = &udf_symlink_inode_operations;
> @@ -984,7 +968,6 @@ static int udf_symlink(struct inode *dir, struct dentry *dentry,
>
> err = udf_add_nondir(dentry, inode);
> out:
> - kfree(name);
> return err;
>
> out_no_entry:
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index afd470e..3fbd900 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -336,42 +336,31 @@ try_again:
> int udf_get_filename(struct super_block *sb, uint8_t *sname, uint8_t *dname,
> int flen)
> {
> - struct ustr *filename, *unifilename;
> + struct ustr filename, unifilename;
> int len = 0;
>
> - filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> - if (!filename)
> - return 0;
> -
> - unifilename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> - if (!unifilename)
> - goto out1;
> -
> - if (udf_build_ustr_exact(unifilename, sname, flen))
> - goto out2;
> + if (udf_build_ustr_exact(&unifilename, sname, flen))
> + goto out;
>
> if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8)) {
> - if (!udf_CS0toUTF8(filename, unifilename)) {
> + if (!udf_CS0toUTF8(&filename, &unifilename)) {
> udf_debug("Failed in udf_get_filename: sname = %s\n",
> sname);
> - goto out2;
> + goto out;
> }
> } else if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP)) {
> - if (!udf_CS0toNLS(UDF_SB(sb)->s_nls_map, filename,
> - unifilename)) {
> + if (!udf_CS0toNLS(UDF_SB(sb)->s_nls_map, &filename,
> + &unifilename)) {
> udf_debug("Failed in udf_get_filename: sname = %s\n",
> sname);
> - goto out2;
> + goto out;
> }
> } else
> - goto out2;
> -
> - len = udf_translate_to_linux(dname, filename->u_name, filename->u_len,
> - unifilename->u_name, unifilename->u_len);
> -out2:
> - kfree(unifilename);
> -out1:
> - kfree(filename);
> + goto out;
> +
> + len = udf_translate_to_linux(dname, filename.u_name, filename.u_len,
> + unifilename.u_name, unifilename.u_len);
> +out:
> return len;
> }
>
> --
> 1.8.5.5
>
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
