| Message ID | 3cd025d9c490218a92e6d8fb30b6123e693373e3.1647057583.git.riteshh@linux.ibm.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | ext4: Improve FC trace events | expand |
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 <riteshh@linux.ibm.com> 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) {
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 <riteshh@linux.ibm.com> > > 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/YdYotAyQqQgI+Oo+@mit.edu/ Thanks again for catching and fixing that. -ritesh
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;
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 <riteshh@linux.ibm.com> --- fs/ext4/fast_commit.c | 59 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 10 deletions(-)