Re: [PATCHv3 05/10] ext4: Return early for non-eligible fast_commit track events

[Ritesh Harjani <riteshh@linux.ibm.com>]

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