linux-ext4 - Re: [PATCH v6 02/10] ext4: for committing inode, make ext4_fc_track

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240621163313.equbxelyeetrxb6w@quack3>
Date: Fri, 21 Jun 2024 18:33:13 +0200
From: Jan Kara <jack@...e.cz>
To: Harshad Shirwadkar <harshadshirwadkar@...il.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, saukad@...gle.com,
	harshads@...gle.com
Subject: Re: [PATCH v6 02/10] ext4: for committing inode, make
 ext4_fc_track_inode wait

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@...il.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@...e.com>
SUSE Labs, CR