f2fs: avoid unnecessary function call

PATCH In review Tiezhu Yang 3 participants 4 messages Jun 30, 2017 – Jul 5, 2017 F2FS file system

Share & tools

Tiezhu Yang

author

2017-06-30 23:03 UTC #0 lore raw

PATCH 1/1 f2fs: avoid unnecessary function call

It should call the function fscrypt_fname_free_buffer() in the exception handling only after the function fscrypt_fname_alloc_buffer() returns 0, otherwise it is unnecessary.

Signed-off-by Tiezhu Yang <[email protected]>

---
 fs/f2fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/f2fs/namei.c +4 -3

index c31b40e..3225a82 100644
@@ -1036,12 +1036,12 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
 	if (res)
-		goto errout;
+		goto out;
 
 	/* this is broken symlink case */
 	if (unlikely(pstr.name[0] == 0)) {
 		res = -ENOENT;
-		goto errout;
+		goto out;
 	}
 
 	paddr = pstr.name;

@@ -1052,8 +1052,9 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
 	return paddr;
-errout:
+out:
 	fscrypt_fname_free_buffer(&pstr);
+errout:
 	put_page(cpage);
 	return ERR_PTR(res);
 }

Signature · 4 lines

1.8.3.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Jaegeuk Kim

maintainer

2017-07-01 07:39 UTC #1 lore raw

Quoted 3 lines · Tiezhu Yang

It should call the function fscrypt_fname_free_buffer() in the exception handling only after the function fscrypt_fname_alloc_buffer() returns 0, otherwise it is unnecessary.

Hi, fscrypt_fname_free_buffer returns if crypto_str is null. So, this flow has no problem, which would be a quite conventional flow, no?

Thanks,

Quoted 37 lines

Signed-off-by Tiezhu Yang <[email protected]>

---
 fs/f2fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/f2fs/namei.c +4 -3

index c31b40e..3225a82 100644
@@ -1036,12 +1036,12 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
 	if (res)
-		goto errout;
+		goto out;
 
 	/* this is broken symlink case */
 	if (unlikely(pstr.name[0] == 0)) {
 		res = -ENOENT;
-		goto errout;
+		goto out;
 	}
 
 	paddr = pstr.name;

@@ -1052,8 +1052,9 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
 	return paddr;
-errout:
+out:
 	fscrypt_fname_free_buffer(&pstr);
+errout:
 	put_page(cpage);
 	return ERR_PTR(res);
 }

Signature · 1 line

1.8.3.1

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Tiezhu Yang

author

2017-07-02 16:05 UTC #2 lore raw

Hi, Jaegeuk

Quoted 7 lines · Jaegeuk Kim

Quoted 3 lines · Tiezhu Yang
It should call the function fscrypt_fname_free_buffer() in the exception handling only after the function fscrypt_fname_alloc_buffer() returns 0, otherwise it is unnecessary.
Hi, fscrypt_fname_free_buffer returns if crypto_str is null. So, this flow has no problem, which would be a quite conventional flow, no?

Understood, just two points of view, both are OK.
In my opinion, I would prefer not to call the function fscrypt_fname_free_buffer since I am absolutely sure that its argument is NULL and it will return directly without doing anything, this patch is to avoid unnecessary function call.

Thanks,

Quoted 26 lines

Thanks,

Quoted 23 lines

Signed-off-by Tiezhu Yang <[email protected]>

---
 fs/f2fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

fs/f2fs/namei.c +2 -2

index c31b40e..3225a82 100644
@@ -1036,12 +1036,12 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
 	if (res)
-		goto errout;
+		goto out;
 
 	/* this is broken symlink case */
 	if (unlikely(pstr.name[0] == 0)) {
 		res = -ENOENT;
-		goto errout;
+		goto out;
 	}

Quoted 14 lines

fs/f2fs/namei.c +2 -1

 	paddr = pstr.name;

@@ -1052,8 +1052,9 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
 	return paddr;
-errout:
+out:
 	fscrypt_fname_free_buffer(&pstr);
+errout:
 	put_page(cpage);
 	return ERR_PTR(res);
 }

Signature · 1 line

1.8.3.1

Chao Yu

maintainer

2017-07-05 02:26 UTC #3 lore raw

Hi all,

I'm not against this change, since this follows most error handling rule of f2fs codes that caller will know whether it need to do error handling instead of callee.

Would it be better to change as below:

---
 fs/f2fs/namei.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/f2fs/namei.c +7 -6

index 66d6dd280184..175167a51970 100644
@@ -1086,26 +1086,26 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	/* this is broken symlink case */
 	if (unlikely(cstr.len == 0)) {
 		res = -ENOENT;
-		goto errout;
+		goto out_put;
 	}
 
 	if ((cstr.len + sizeof(struct fscrypt_symlink_data) - 1) > max_size) {
 		/* Symlink data on the disk is corrupted */
 		res = -EIO;
-		goto errout;
+		goto out_put;
 	}
 	res = fscrypt_fname_alloc_buffer(inode, cstr.len, &pstr);
 	if (res)
-		goto errout;
+		goto out_put;
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
 	if (res)
-		goto errout;
+		goto out_free;
 
 	/* this is broken symlink case */
 	if (unlikely(pstr.name[0] == 0)) {
 		res = -ENOENT;
-		goto errout;
+		goto out_free;
 	}
 
 	paddr = pstr.name;

@@ -1116,8 +1116,9 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
 	return paddr;
-errout:
+out_free:
 	fscrypt_fname_free_buffer(&pstr);
+out_put:
 	put_page(cpage);
 	return ERR_PTR(res);
 }

Signature · 1 line

2.13.0.90.g1eb437020

Quoted 57 lines · Tiezhu Yang

Hi, Jaegeuk
Quoted 7 lines · Jaegeuk Kim
Quoted 3 lines · Tiezhu Yang
It should call the function fscrypt_fname_free_buffer() in the exception handling only after the function fscrypt_fname_alloc_buffer() returns 0, otherwise it is unnecessary.
Hi, fscrypt_fname_free_buffer returns if crypto_str is null. So, this flow has no problem, which would be a quite conventional flow, no?
Understood, just two points of view, both are OK.
In my opinion, I would prefer not to call the function fscrypt_fname_free_buffer since I am absolutely sure that its argument is NULL and it will return directly without doing anything, this patch is to avoid unnecessary function call.
Thanks,
Quoted 39 lines
Thanks,
Quoted 36 lines
Signed-off-by Tiezhu Yang <[email protected]>
---
 fs/f2fs/namei.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
fs/f2fs/namei.c +4 -3
index c31b40e..3225a82 100644
@@ -1036,12 +1036,12 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 
 	res = fscrypt_fname_disk_to_usr(inode, 0, 0, &cstr, &pstr);
 	if (res)
-		goto errout;
+		goto out;
 
 	/* this is broken symlink case */
 	if (unlikely(pstr.name[0] == 0)) {
 		res = -ENOENT;
-		goto errout;
+		goto out;
 	}
 
 	paddr = pstr.name;
@@ -1052,8 +1052,9 @@ static const char *f2fs_encrypted_get_link(struct dentry *dentry,
 	put_page(cpage);
 	set_delayed_call(done, kfree_link, paddr);
 	return paddr;
-errout:
+out:
 	fscrypt_fname_free_buffer(&pstr);
+errout:
 	put_page(cpage);
 	return ERR_PTR(res);
 }

Quoted 9 lines

Quoted 1 line
Quoted 1 line
1.8.3.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel