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@xxxxxxxxxxxxx>
>
> 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+@xxxxxxx/
Thanks again for catching and fixing that.
-ritesh
