Received-SPF: pass (google.com: domain of linux-ext4+bounces-2624-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 22 May 2024 12:35:45 +0200
From: Jan Kara <jack@suse.cz>
To: "Luis Henriques (SUSE)" <luis.henriques@linux.dev>
Cc: Theodore Ts'o <tytso@mit.edu>, Andreas Dilger <adilger@dilger.ca>,
	Jan Kara <jack@suse.com>, linux-ext4@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] ext4: fix fast commit inode enqueueing during a
 full journal commit
Message-ID: <20240522103545.ypmmoyxvls52i6yl@quack3>
References: <20240521154535.12911-1-luis.henriques@linux.dev>
 <20240521154535.12911-2-luis.henriques@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240521154535.12911-2-luis.henriques@linux.dev>

On Tue 21-05-24 16:45:34, Luis Henriques (SUSE) wrote:
> When a full journal commit is on-going, any fast commit has to be enqueued
> into a different queue: FC_Q_STAGING instead of FC_Q_MAIN.  This enqueueing
> is done only once, i.e. if an inode is already queued in a previous fast
> commit entry it won't be enqueued again.  However, if a full commit starts
> _after_ the inode is enqueued into FC_Q_MAIN, the next fast commit needs to
> be done into FC_Q_STAGING.  And this is not being done in function
> ext4_fc_track_template().

Ah, good catch.

> This patch fixes the issue by simply re-enqueuing the inode from the MAIN
> into the STAGING queue.
> 
> This bug was found using fstest generic/047.  This test creates several 32k
> bytes files, sync'ing each of them after it's creation, and then shutting
> down the filesystem.  Some data may be loss in this operation; for example a
> file may have it's size truncated to zero.
> 
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
> ---
>  fs/ext4/fast_commit.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 87c009e0c59a..337b5289cf11 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -396,12 +396,19 @@ static int ext4_fc_track_template(
>  		return ret;
>  
>  	spin_lock(&sbi->s_fc_lock);
> -	if (list_empty(&EXT4_I(inode)->i_fc_list))
> -		list_add_tail(&EXT4_I(inode)->i_fc_list,
> -				(sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
> -				 sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ?
> -				&sbi->s_fc_q[FC_Q_STAGING] :
> -				&sbi->s_fc_q[FC_Q_MAIN]);
> +	if (sbi->s_journal->j_flags & JBD2_FULL_COMMIT_ONGOING ||
> +	    sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) {
> +		if (list_empty(&EXT4_I(inode)->i_fc_list))
> +			list_add_tail(&EXT4_I(inode)->i_fc_list,
> +				      &sbi->s_fc_q[FC_Q_STAGING]);
> +		else
> +			list_move_tail(&EXT4_I(inode)->i_fc_list,
> +				       &sbi->s_fc_q[FC_Q_STAGING]);

So I'm not sure this is actually safe. I'm concerned about the following
race:

Task1					Task2

					handle = ext4_journal_start(..)
modify inode_X
  ext4_fc_track_inode(inode_X)
ext4_fsync(inode_X)
  ext4_fc_commit()
    jbd2_fc_begin_commit()
      journal->j_flags |= JBD2_FAST_COMMIT_ONGOING;
      ...
      jbd2_journal_lock_updates()
        blocks waiting for handle of Task2
					modify inode_X
					  ext4_fc_track_inode(inode_X)
					    - moves inode out of FC_Q_MAIN
					ext4_journal_stop()
    fast commit proceeds but skips inode_X...

How we deal with a similar issue in jbd2 for ordinary buffers is that we
just mark the buffer as *also* belonging to the next transaction (by
setting jh->b_next_transaction) and during commit cleanup we move the bh to
the appropriate list of the next transaction. Here, we could mark the inode
as also being part of the next fast commit and during fastcommit cleanup we
could move it to FC_Q_STAGING which is then spliced back to FC_Q_MAIN.

Also Harshad has recently posted changes to fast commit code that modify
how fast commits are serialized (in particular jbd2_journal_lock_updates()
is gone). I didn't read them yet but your change definitely needs a careful
verification against those changes to make sure we don't introduce new data
integrity issues.

> +	} else {
> +		if (list_empty(&EXT4_I(inode)->i_fc_list))
> +			list_add_tail(&EXT4_I(inode)->i_fc_list,
> +				      &sbi->s_fc_q[FC_Q_MAIN]);
> +	}
>  	spin_unlock(&sbi->s_fc_lock);
>  
>  	return ret;

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