[PATCH v2] ext4: lost matching-pair of trace in ext4_unlink

[PATCH v2] ext4: lost matching-pair of trace in ext4_unlink

[Yi Zhuang]

If dquot_initialize() return non-zero and trace of ext4_unlink_enter/exit
enabled then the matching-pair of trace_exit will lost in log.

v2:
Change the new label to be "out_trace:", which makes it more clear that
it is undoing the "trace" part of the code. At the same time, fix other
similar problems in this function:

	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
	if (IS_ERR(bh))
		return PTR_ERR(bh);
	if (!bh)
		goto end_unlink;

According to Andreas' suggestion, split up the "end_unlink:" label becomes
two separate labels, and then remove the "if (handle)" check, and then
use out_bh: before the handle is started.

Signed-off-by: Yi Zhuang <zhuangyi1@huawei.com>
---
 fs/ext4/namei.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 56738b538ddf..941f66f417f0 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3193,30 +3193,33 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 	 * in separate transaction */
 	retval = dquot_initialize(dir);
 	if (retval)
-		return retval;
+		goto out_trace;
 	retval = dquot_initialize(d_inode(dentry));
 	if (retval)
-		return retval;
+		goto out_trace;
 
-	retval = -ENOENT;
 	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
-	if (IS_ERR(bh))
-		return PTR_ERR(bh);
-	if (!bh)
-		goto end_unlink;
+	if (IS_ERR(bh)) {
+		retval = PTR_ERR(bh);
+		goto out_trace;
+	}
+	if (!bh) {
+		retval = -ENOENT;
+		goto out_trace;
+	}
 
 	inode = d_inode(dentry);
 
-	retval = -EFSCORRUPTED;
-	if (le32_to_cpu(de->inode) != inode->i_ino)
-		goto end_unlink;
+	if (le32_to_cpu(de->inode) != inode->i_ino) {
+		retval = -EFSCORRUPTED;
+		goto out_bh;
+	}
 
 	handle = ext4_journal_start(dir, EXT4_HT_DIR,
 				    EXT4_DATA_TRANS_BLOCKS(dir->i_sb));
 	if (IS_ERR(handle)) {
 		retval = PTR_ERR(handle);
-		handle = NULL;
-		goto end_unlink;
+		goto out_bh;
 	}
 
 	if (IS_DIRSYNC(dir))
@@ -3224,12 +3227,12 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 
 	retval = ext4_delete_entry(handle, dir, de, bh);
 	if (retval)
-		goto end_unlink;
+		goto out_handle;
 	dir->i_ctime = dir->i_mtime = current_time(dir);
 	ext4_update_dx_flag(dir);
 	retval = ext4_mark_inode_dirty(handle, dir);
 	if (retval)
-		goto end_unlink;
+		goto out_handle;
 	if (inode->i_nlink == 0)
 		ext4_warning_inode(inode, "Deleting file '%.*s' with no links",
 				   dentry->d_name.len, dentry->d_name.name);
@@ -3251,10 +3254,11 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 		d_invalidate(dentry);
 #endif
 
-end_unlink:
+out_handle:
+	ext4_journal_stop(handle);
+out_bh:
 	brelse(bh);
-	if (handle)
-		ext4_journal_stop(handle);
+out_trace:
 	trace_ext4_unlink_exit(dentry, retval);
 	return retval;
 }
-- 
2.26.0.106.g9fadedd

Re: [PATCH v2] ext4: lost matching-pair of trace in ext4_unlink

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]


> On Jun 29, 2020, at 6:26 AM, Yi Zhuang <zhuangyi1@huawei.com> wrote:
> 
> If dquot_initialize() return non-zero and trace of ext4_unlink_enter/exit
> enabled then the matching-pair of trace_exit will lost in log.
> 
> v2:
> Change the new label to be "out_trace:", which makes it more clear that
> it is undoing the "trace" part of the code. At the same time, fix other
> similar problems in this function:
> 
> 	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
> 	if (IS_ERR(bh))
> 		return PTR_ERR(bh);
> 	if (!bh)
> 		goto end_unlink;
> 
> According to Andreas' suggestion, split up the "end_unlink:" label becomes
> two separate labels, and then remove the "if (handle)" check, and then
> use out_bh: before the handle is started.
> 
> Signed-off-by: Yi Zhuang <zhuangyi1@huawei.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/namei.c | 38 +++++++++++++++++++++-----------------
> 1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 56738b538ddf..941f66f417f0 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3193,30 +3193,33 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> 	 * in separate transaction */
> 	retval = dquot_initialize(dir);
> 	if (retval)
> -		return retval;
> +		goto out_trace;
> 	retval = dquot_initialize(d_inode(dentry));
> 	if (retval)
> -		return retval;
> +		goto out_trace;
> 
> -	retval = -ENOENT;
> 	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
> -	if (IS_ERR(bh))
> -		return PTR_ERR(bh);
> -	if (!bh)
> -		goto end_unlink;
> +	if (IS_ERR(bh)) {
> +		retval = PTR_ERR(bh);
> +		goto out_trace;
> +	}
> +	if (!bh) {
> +		retval = -ENOENT;
> +		goto out_trace;
> +	}
> 
> 	inode = d_inode(dentry);
> 
> -	retval = -EFSCORRUPTED;
> -	if (le32_to_cpu(de->inode) != inode->i_ino)
> -		goto end_unlink;
> +	if (le32_to_cpu(de->inode) != inode->i_ino) {
> +		retval = -EFSCORRUPTED;
> +		goto out_bh;
> +	}
> 
> 	handle = ext4_journal_start(dir, EXT4_HT_DIR,
> 				    EXT4_DATA_TRANS_BLOCKS(dir->i_sb));
> 	if (IS_ERR(handle)) {
> 		retval = PTR_ERR(handle);
> -		handle = NULL;
> -		goto end_unlink;
> +		goto out_bh;
> 	}
> 
> 	if (IS_DIRSYNC(dir))
> @@ -3224,12 +3227,12 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> 
> 	retval = ext4_delete_entry(handle, dir, de, bh);
> 	if (retval)
> -		goto end_unlink;
> +		goto out_handle;
> 	dir->i_ctime = dir->i_mtime = current_time(dir);
> 	ext4_update_dx_flag(dir);
> 	retval = ext4_mark_inode_dirty(handle, dir);
> 	if (retval)
> -		goto end_unlink;
> +		goto out_handle;
> 	if (inode->i_nlink == 0)
> 		ext4_warning_inode(inode, "Deleting file '%.*s' with no links",
> 				   dentry->d_name.len, dentry->d_name.name);
> @@ -3251,10 +3254,11 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> 		d_invalidate(dentry);
> #endif
> 
> -end_unlink:
> +out_handle:
> +	ext4_journal_stop(handle);
> +out_bh:
> 	brelse(bh);
> -	if (handle)
> -		ext4_journal_stop(handle);
> +out_trace:
> 	trace_ext4_unlink_exit(dentry, retval);
> 	return retval;
> }
> --
> 2.26.0.106.g9fadedd
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH v2] ext4: lost matching-pair of trace in ext4_unlink

[Ritesh Harjani]

On 6/29/20 5:56 PM, Yi Zhuang wrote:
> If dquot_initialize() return non-zero and trace of ext4_unlink_enter/exit
> enabled then the matching-pair of trace_exit will lost in log.
> 
> v2:
> Change the new label to be "out_trace:", which makes it more clear that
> it is undoing the "trace" part of the code. At the same time, fix other
> similar problems in this function:
> 
> 	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
> 	if (IS_ERR(bh))
> 		return PTR_ERR(bh);
> 	if (!bh)
> 		goto end_unlink;
> 
> According to Andreas' suggestion, split up the "end_unlink:" label becomes
> two separate labels, and then remove the "if (handle)" check, and then
> use out_bh: before the handle is started.
> 
> Signed-off-by: Yi Zhuang <zhuangyi1@huawei.com>

Nice cleanup. Feel free to add:
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

Re: [PATCH v2] ext4: lost matching-pair of trace in ext4_unlink

[tytso]

On Mon, Jun 29, 2020 at 08:26:21PM +0800, Yi Zhuang wrote:
> If dquot_initialize() return non-zero and trace of ext4_unlink_enter/exit
> enabled then the matching-pair of trace_exit will lost in log.
> 
> v2:
> Change the new label to be "out_trace:", which makes it more clear that
> it is undoing the "trace" part of the code. At the same time, fix other
> similar problems in this function:
> 
> 	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
> 	if (IS_ERR(bh))
> 		return PTR_ERR(bh);
> 	if (!bh)
> 		goto end_unlink;
> 
> According to Andreas' suggestion, split up the "end_unlink:" label becomes
> two separate labels, and then remove the "if (handle)" check, and then
> use out_bh: before the handle is started.
> 
> Signed-off-by: Yi Zhuang <zhuangyi1@huawei.com>

Thanks, applied.

					- Ted