2020-06-29 20:05:42

by Yi Zhuang

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

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 <[email protected]>
---
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

2020-07-08 10:23:16

by Ritesh Harjani

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

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 <[email protected]>

Nice cleanup. Feel free to add:
Reviewed-by: Ritesh Harjani <[email protected]>

2020-08-06 05:07:23

by Theodore Ts'o

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

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 <[email protected]>

Thanks, applied.

- Ted