Re: [PATCH v6 02/10] ext4: for committing inode, make ext4_fc_track_inode wait

[Jan Kara <jack@suse.cz>]

On Wed 29-05-24 01:19:55, Harshad Shirwadkar wrote:
> If the inode that's being requested to track using ext4_fc_track_inode
> is being committed, then wait until the inode finishes the
> commit. Also, add calls to ext4_fc_track_inode at the right places.
> 
> With this patch, now calling ext4_reserve_inode_write() results in
> inode being tracked for next fast commit. A subtle lock ordering
> requirement with i_data_sem (which is documented in the code) requires
> that ext4_fc_track_inode() be called before grabbing i_data_sem. So,
> this patch also adds explicit ext4_fc_track_inode() calls in places
> where i_data_sem grabbed.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> ---
>  fs/ext4/fast_commit.c | 34 ++++++++++++++++++++++++++++++++++
>  fs/ext4/inline.c      |  3 +++
>  fs/ext4/inode.c       |  4 ++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index a1aadebfcd66..fa93ce399440 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -581,6 +581,8 @@ static int __track_inode(struct inode *inode, void *arg, bool update)
>  
>  void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>  {
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	wait_queue_head_t *wq;
>  	int ret;
>  
>  	if (S_ISDIR(inode->i_mode))
> @@ -598,6 +600,38 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
>  	if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
>  		return;
>  
> +	if (!test_opt2(inode->i_sb, JOURNAL_FAST_COMMIT) ||
> +	    (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY) ||
> +		ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE) ||
> +		!list_empty(&ei->i_fc_list))

Indentation here is wrong (we never indent conditions by the same amount as
subsequent code block). Also EXT4_MF_FC_INELIGIBLE was just tested above so
why repeat it and ext4_fc_disabled() tested the EXT4_FC_REPLAY and
JOURNAL_FAST_COMMIT flags. So list_empty() should be the only thing needed
here.

BTW a separate helper for ext4_fc_disabled() + EXT4_MF_FC_INELIGIBLE test
would be a nice cleanup as it is a pattern happening in quite a few places.

> +		return;
> +
> +	/*
> +	 * If we come here, we may sleep while waiting for the inode to
> +	 * commit. We shouldn't be holding i_data_sem in write mode when we go
> +	 * to sleep since the commit path needs to grab the lock while
> +	 * committing the inode.
> +	 */
> +	WARN_ON(lockdep_is_held_type(&ei->i_data_sem, 1));

Actually even holding it in read mode is problematic. rwsems can block
other readers from acquiring rwsem if there's some writer waiting to
acquire the lock (and that will be blocked behind us). Shortly, this should
be just lockdep_is_held().

Otherwise the patch looks good.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR