On Jun 27, 2020, at 9:48 PM, Yi Zhuang <zhuangyi1@xxxxxxxxxx> 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.
>
> Signed-off-by: Yi Zhuang <zhuangyi1@xxxxxxxxxx>
Thank you for the patch. It definitely looks like this is fixing the
problem with trace exit, but I would recommend to use a better name
than "out_unlink:" for the label. I was looking at the ext4_unlink()
function, and found it confusing that there is already an "end_unlink:"
label and these two names would be easily confused.
Please change your new label to be "out_trace:", or similar, which
makes it more clear that it is undoing the "trace" part of the code.
It looks like there is another similar problem with the error handling
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;
At this point the journal handle has not been started, and the PTR_ERR()
return is also missing the trace exit, so it would be better to change
these two cases to use the "out_trace:" label as well, like:
bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
if (!bh) {
retval = -ENOENT;
goto out_trace;
}
if (IS_ERR(bh)) {
retval = PTR_ERR(bh);
goto out_trace;
}
Could you please resubmit your patch with these small changes.
There could be a separate small patch to split up the "end_unlink:"
label to be two separate labels, and then remove the "if (handle)"
check, and then use out_bh: before the handle is started:
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);
goto out_bh;
}
:
:
out_handle:
ext4_journal_stop(handle);
out_bh:
brelse(bh);
out_trace:
trace_ext4_unlink_exit(dentry, retval);
return retval;
Cheers, Andreas
> ---
> fs/ext4/namei.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 56738b538ddf..5e41d45915c9 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3193,10 +3193,10 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> * in separate transaction */
> retval = dquot_initialize(dir);
> if (retval)
> - return retval;
> + goto out_unlink;
> retval = dquot_initialize(d_inode(dentry));
> if (retval)
> - return retval;
> + goto out_unlink;
>
> retval = -ENOENT;
> bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
> @@ -3255,6 +3255,7 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> brelse(bh);
> if (handle)
> ext4_journal_stop(handle);
> +out_unlink:
> trace_ext4_unlink_exit(dentry, retval);
> return retval;
> }
> --
> 2.26.0.106.g9fadedd
>
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
