Skip to contentSkip to conversation

udf: fix lookup performance issue when SLUB_DEBUG is enable

PATCH In review Namjae Jeon 2 participants 2 messages Nov 12, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 udf: fix lookup performance issue when SLUB_DEBUG is enable

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
---
 fs/udf/dir.c     |  9 +--------
 fs/udf/namei.c   | 23 +++--------------------
 fs/udf/unicode.c | 37 +++++++++++++------------------------
 3 files changed, 17 insertions(+), 52 deletions(-)
fs/udf/dir.c +1 -8
index a012c51..490b9dd 100644
@@ -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;
 }
fs/udf/namei.c +3 -20
index c12e260..45cbbb9 100644
@@ -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:
fs/udf/unicode.c +13 -24
index afd470e..3fbd900 100644
@@ -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;
 }
 
Signature · 1 line
1.8.5.5
reviewer
#1 lore raw
Quoted 32 lines · Namjae Jeon

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
Quoted 2 lines

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

Quoted 193 lines
---
 fs/udf/dir.c     |  9 +--------
 fs/udf/namei.c   | 23 +++--------------------
 fs/udf/unicode.c | 37 +++++++++++++------------------------
 3 files changed, 17 insertions(+), 52 deletions(-)
fs/udf/dir.c +1 -8
index a012c51..490b9dd 100644
@@ -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;
 }
fs/udf/namei.c +3 -20
index c12e260..45cbbb9 100644
@@ -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:
fs/udf/unicode.c +13 -24
index afd470e..3fbd900 100644
@@ -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;
 }
 
Signature · 1 line
1.8.5.5
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR