Re: [PATCH v6 08/10] ext4: introduce selective flushing in fast commit

[Jan Kara <jack@suse.cz>]

On Wed 29-05-24 01:20:01, Harshad Shirwadkar wrote:
> With fast commits, if the entire commit is contained within a single
> block and there isn't any data that needs a flush, we can avoid sending
> expensive cache flush to disk. Single block metadata only fast commits
> can be written using FUA to guarantee consistency.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> ---
>  fs/ext4/ext4.h        | 12 ++++++++++++
>  fs/ext4/ext4_jbd2.h   | 20 ++++++++++++--------
>  fs/ext4/fast_commit.c | 23 ++++++++++++++++++-----
>  3 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 196c513f82dd..3721daea2890 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1744,6 +1744,13 @@ struct ext4_sb_info {
>  					 */
>  	struct list_head s_fc_dentry_q[2];	/* directory entry updates */
>  	unsigned int s_fc_bytes;
> +
> +	/*
> +	 * This flag indicates whether a full flush is needed on
> +	 * next fast commit.
> +	 */
> +	int fc_flush_required;

I think this storing of fastcommit specific info in the superblock is a bad
practice and actually leads to subtle bugs (see below). I believe you
should have a dedicated structure tracking the fast commit info (and you
would actually have two of them - for the running and the committing fast
transaction).

> @@ -2905,6 +2912,11 @@ void ext4_fc_del(struct inode *inode);
>  bool ext4_fc_replay_check_excluded(struct super_block *sb, ext4_fsblk_t block);
>  void ext4_fc_replay_cleanup(struct super_block *sb);
>  int ext4_fc_commit(journal_t *journal, tid_t commit_tid);
> +static inline void ext4_fc_mark_needs_flush(struct super_block *sb)
> +{
> +	EXT4_SB(sb)->fc_flush_required = 1;
> +}
> +
>  int __init ext4_fc_init_dentry_cache(void);
>  void ext4_fc_destroy_dentry_cache(void);
>  int ext4_fc_record_regions(struct super_block *sb, int ino,
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 0c77697d5e90..e3a4f5c49b6e 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -420,19 +420,23 @@ static inline int ext4_journal_force_commit(journal_t *journal)
>  static inline int ext4_jbd2_inode_add_write(handle_t *handle,
>  		struct inode *inode, loff_t start_byte, loff_t length)
>  {
> -	if (ext4_handle_valid(handle))
> -		return jbd2_journal_inode_ranged_write(handle,
> -				EXT4_I(inode)->jinode, start_byte, length);
> -	return 0;
> +	if (!ext4_handle_valid(handle))
> +		return 0;
> +
> +	ext4_fc_mark_needs_flush(inode->i_sb);
> +	return jbd2_journal_inode_ranged_write(handle,
> +			EXT4_I(inode)->jinode, start_byte, length);
>  }

I think this handling of fc_flush_required introduces a subtle bug. While
fast commit is running, next transaction can be already running in parallel
and thus set fc_flush_required = 1. When fast commit completes, it does
cache flush and sets fc_flush_required = 0. But the data added here in
ext4_jbd2_inode_add_write() is not written out yet so the cache flush
didn't include them and the next fast commit need not flush caches causing
subtle data integrity issues after power failure.

I actually think it will be much less error prone if you track whether we
need to flush or not while writing out the fast commit to the journal. No
need to track it early when things are just being added to the transaction.

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