Currently ext4_fc_track_template() checks, whether the trace event
path belongs to replay or does sb has ineligible set, if yes it simply
returns. This patch pulls those checks before calling
ext4_fc_track_template() in the callers of ext4_fc_track_template().
Signed-off-by: Ritesh Harjani <[email protected]>
---
fs/ext4/fast_commit.c | 59 +++++++++++++++++++++++++++++++++++--------
1 file changed, 49 insertions(+), 10 deletions(-)
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 55d33f296cae..6990429daa0e 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -379,13 +379,6 @@ static int ext4_fc_track_template(
tid_t tid = 0;
int ret;
- if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
- (sbi->s_mount_state & EXT4_FC_REPLAY))
- return -EOPNOTSUPP;
-
- if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
- return -EINVAL;
-
tid = handle->h_transaction->t_tid;
mutex_lock(&ei->i_fc_lock);
if (tid == ei->i_sync_tid) {
@@ -499,7 +492,17 @@ void __ext4_fc_track_unlink(handle_t *handle,
void ext4_fc_track_unlink(handle_t *handle, struct dentry *dentry)
{
- __ext4_fc_track_unlink(handle, d_inode(dentry), dentry);
+ struct inode *inode = d_inode(dentry);
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+
+ if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
+ (sbi->s_mount_state & EXT4_FC_REPLAY))
+ return;
+
+ if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ return;
+
+ __ext4_fc_track_unlink(handle, inode, dentry);
}
void __ext4_fc_track_link(handle_t *handle,
@@ -518,7 +521,17 @@ void __ext4_fc_track_link(handle_t *handle,
void ext4_fc_track_link(handle_t *handle, struct dentry *dentry)
{
- __ext4_fc_track_link(handle, d_inode(dentry), dentry);
+ struct inode *inode = d_inode(dentry);
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+
+ if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
+ (sbi->s_mount_state & EXT4_FC_REPLAY))
+ return;
+
+ if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ return;
+
+ __ext4_fc_track_link(handle, inode, dentry);
}
void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
@@ -537,7 +550,17 @@ void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
{
- __ext4_fc_track_create(handle, d_inode(dentry), dentry);
+ struct inode *inode = d_inode(dentry);
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+
+ if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
+ (sbi->s_mount_state & EXT4_FC_REPLAY))
+ return;
+
+ if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ return;
+
+ __ext4_fc_track_create(handle, inode, dentry);
}
/* __track_fn for inode tracking */
@@ -553,6 +576,7 @@ static int __track_inode(struct inode *inode, void *arg, bool update)
void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
{
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
int ret;
if (S_ISDIR(inode->i_mode))
@@ -564,6 +588,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
return;
}
+ if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
+ (sbi->s_mount_state & EXT4_FC_REPLAY))
+ return;
+
+ if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ return;
+
ret = ext4_fc_track_template(handle, inode, __track_inode, NULL, 1);
trace_ext4_fc_track_inode(inode, ret);
}
@@ -603,12 +634,20 @@ static int __track_range(struct inode *inode, void *arg, bool update)
void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t start,
ext4_lblk_t end)
{
+ struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
struct __track_range_args args;
int ret;
if (S_ISDIR(inode->i_mode))
return;
+ if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
+ (sbi->s_mount_state & EXT4_FC_REPLAY))
+ return;
+
+ if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ return;
+
args.start = start;
args.end = end;
--
2.31.1
On 22/03/15 05:43PM, Theodore Ts'o wrote:
> On Sat, Mar 12, 2022 at 11:09:50AM +0530, Ritesh Harjani wrote:
> > Currently ext4_fc_track_template() checks, whether the trace event
> > path belongs to replay or does sb has ineligible set, if yes it simply
> > returns. This patch pulls those checks before calling
> > ext4_fc_track_template() in the callers of ext4_fc_track_template().
> >
> > Signed-off-by: Ritesh Harjani <[email protected]>
>
> I had to add the following patch to this commit in order to prevent a
> BUG when using ext4 to mount a file system without a journal. This is
> because ext4_rename() calls the __ext4_fc_track_* functions directly,
> and moving the checks from __ext4_fc_track_* to ext4_fc_track_* would
> result in a NULL pointer dereference.
Ohk, yes. I had missed to see the callers of __ext4_fc_track_* functions.
Thanks for catching that. I just verified all other call sites too.
It seems only with ext4_fc_track_create/link/unlink we have __ext4_fc_track_*
family of functions and ext4_rename() is the only call site of __ext4_fc_track_*.
>
> - Ted
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 39e223f7bf64..e37da8d5cd0c 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3891,12 +3891,19 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
> ext4_fc_mark_ineligible(old.inode->i_sb,
> EXT4_FC_REASON_RENAME_DIR, handle);
> } else {
> + struct super_block *sb = old.inode->i_sb;
> +
> if (new.inode)
> ext4_fc_track_unlink(handle, new.dentry);
> - __ext4_fc_track_link(handle, old.inode, new.dentry);
> - __ext4_fc_track_unlink(handle, old.inode, old.dentry);
> - if (whiteout)
> - __ext4_fc_track_create(handle, whiteout, old.dentry);
> + if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
> + !(EXT4_SB(sb)->s_mount_state & EXT4_FC_REPLAY) &&
> + !(ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE))) {
> + __ext4_fc_track_link(handle, old.inode, new.dentry);
> + __ext4_fc_track_unlink(handle, old.inode, old.dentry);
> + if (whiteout)
> + __ext4_fc_track_create(handle, whiteout,
> + old.dentry);
> + }
> }
>
> if (new.inode) {
>
Maybe since I pulled these checks out of ext4_fc_track_template(), so the right
call site for these checks are __ext4_fc_track_* family of functions, if they
are present, otherwise ext4_fc_track_* functions.
But that I can consolidate in later change series when I will start working on
improving error handling for fast commit. It seems at some places we don't
properly return the errors in case of fast commit to the callers.
And I guess in past this was discussed too [1]
So in order to fix the current BUG, this change looks good to me.
[1]: https://lore.kernel.org/linux-ext4/[email protected]/
Thanks again for catching and fixing that.
-ritesh
On Sat, Mar 12, 2022 at 11:09:50AM +0530, Ritesh Harjani wrote:
> Currently ext4_fc_track_template() checks, whether the trace event
> path belongs to replay or does sb has ineligible set, if yes it simply
> returns. This patch pulls those checks before calling
> ext4_fc_track_template() in the callers of ext4_fc_track_template().
>
> Signed-off-by: Ritesh Harjani <[email protected]>
I had to add the following patch to this commit in order to prevent a
BUG when using ext4 to mount a file system without a journal. This is
because ext4_rename() calls the __ext4_fc_track_* functions directly,
and moving the checks from __ext4_fc_track_* to ext4_fc_track_* would
result in a NULL pointer dereference.
- Ted
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 39e223f7bf64..e37da8d5cd0c 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3891,12 +3891,19 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
ext4_fc_mark_ineligible(old.inode->i_sb,
EXT4_FC_REASON_RENAME_DIR, handle);
} else {
+ struct super_block *sb = old.inode->i_sb;
+
if (new.inode)
ext4_fc_track_unlink(handle, new.dentry);
- __ext4_fc_track_link(handle, old.inode, new.dentry);
- __ext4_fc_track_unlink(handle, old.inode, old.dentry);
- if (whiteout)
- __ext4_fc_track_create(handle, whiteout, old.dentry);
+ if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
+ !(EXT4_SB(sb)->s_mount_state & EXT4_FC_REPLAY) &&
+ !(ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE))) {
+ __ext4_fc_track_link(handle, old.inode, new.dentry);
+ __ext4_fc_track_unlink(handle, old.inode, old.dentry);
+ if (whiteout)
+ __ext4_fc_track_create(handle, whiteout,
+ old.dentry);
+ }
}
if (new.inode) {