Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Mar 2022 09:25:49 +0530
From:   Ritesh Harjani <riteshh@linux.ibm.com>
To:     "Theodore Ts'o" <tytso@mit.edu>
Cc:     linux-ext4@vger.kernel.org, Jan Kara <jack@suse.cz>,
        Harshad Shirwadkar <harshadshirwadkar@gmail.com>
Subject: Re: [PATCHv3 05/10] ext4: Return early for non-eligible fast_commit
 track events
Message-ID: <20220316035549.y2z73orqxio4nor4@riteshh-domain>
References: <cover.1647057583.git.riteshh@linux.ibm.com>
 <3cd025d9c490218a92e6d8fb30b6123e693373e3.1647057583.git.riteshh@linux.ibm.com>
 <YjEIj/elOQ4f9qq2@mit.edu>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YjEIj/elOQ4f9qq2@mit.edu>
MIME-Version: 1.0
Precedence: bulk

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