ext4: two small fast commit fixes

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().

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) <[email protected]>

---
 fs/ext4/fast_commit.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

fs/ext4/fast_commit.c +0 -4

index 87c009e0c59a..337b5289cf11 100644
@@ -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) ?

Quoted 7 lines

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.

Quoted 25 lines

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) <[email protected]>

---
 fs/ext4/fast_commit.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

fs/ext4/fast_commit.c +0 -4

index 87c009e0c59a..337b5289cf11 100644
@@ -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) ?

fs/ext4/fast_commit.c static int ext4_fc_track_template( · line 406

Quoted 10 lines

fs/ext4/fast_commit.c +8 -2

-				&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.

fs/ext4/fast_commit.c static int ext4_fc_track_template( · line 414

Quoted 8 lines

fs/ext4/fast_commit.c +5 -0

+	} 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

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

When doing a journal commit, the fast journal offset (journal->j_fc_off) is set to zero too early in the process. Since ext4 filesystem calls function jbd2_fc_release_bufs() in its j_fc_cleanup_callback (ext4_fc_cleanup()), that call will be a no-op exactly because the offset is zero.

Move the fast commit offset further down in the journal commit code, until it's mostly done, immediately before clearing the on-going commit flags.

Signed-off-by Luis Henriques (SUSE) <[email protected]>

---
 fs/jbd2/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/jbd2/commit.c +1 -1

index 75ea4e9a5cab..88b834c7c9c9 100644
@@ -435,7 +435,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 			commit_transaction->t_tid);
 
 	write_lock(&journal->j_state_lock);
-	journal->j_fc_off = 0;
 	J_ASSERT(commit_transaction->t_state == T_RUNNING);
 	commit_transaction->t_state = T_LOCKED;
 

@@ -1133,6 +1132,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		  journal->j_commit_sequence, journal->j_tail_sequence);
 
 	write_lock(&journal->j_state_lock);
+	journal->j_fc_off = 0;
 	journal->j_flags &= ~JBD2_FULL_COMMIT_ONGOING;
 	journal->j_flags &= ~JBD2_FAST_COMMIT_ONGOING;
 	spin_lock(&journal->j_list_lock);

Quoted 8 lines

When doing a journal commit, the fast journal offset (journal->j_fc_off) is set to zero too early in the process. Since ext4 filesystem calls function jbd2_fc_release_bufs() in its j_fc_cleanup_callback (ext4_fc_cleanup()), that call will be a no-op exactly because the offset is zero.

Move the fast commit offset further down in the journal commit code, until it's mostly done, immediately before clearing the on-going commit flags.

Quoted 1 line

Signed-off-by Luis Henriques (SUSE) <[email protected]>

Did you see any particular failure because of this? Because AFAICS the buffers cleaned up by jbd2_fc_release_bufs() are only allocated during fast commit (from ext4_fc_reserve_space()). And the code in jbd2_journal_commit_transaction() is making sure fast commit isn't running before we set journal->j_fc_off to 0.

Honza

Quoted 25 lines

---
 fs/jbd2/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/jbd2/commit.c +1 -1

index 75ea4e9a5cab..88b834c7c9c9 100644
@@ -435,7 +435,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 			commit_transaction->t_tid);
 
 	write_lock(&journal->j_state_lock);
-	journal->j_fc_off = 0;
 	J_ASSERT(commit_transaction->t_state == T_RUNNING);
 	commit_transaction->t_state = T_LOCKED;
 

@@ -1133,6 +1132,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		  journal->j_commit_sequence, journal->j_tail_sequence);
 
 	write_lock(&journal->j_state_lock);
+	journal->j_fc_off = 0;
 	journal->j_flags &= ~JBD2_FULL_COMMIT_ONGOING;
 	journal->j_flags &= ~JBD2_FAST_COMMIT_ONGOING;
 	spin_lock(&journal->j_list_lock);

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

On Wed 22 May 2024 12:45:00 PM +02, Jan Kara wrote;

Quoted 9 lines · Luis Henriques (SUSE)

Quoted 8 lines

When doing a journal commit, the fast journal offset (journal->j_fc_off) is set to zero too early in the process. Since ext4 filesystem calls function jbd2_fc_release_bufs() in its j_fc_cleanup_callback (ext4_fc_cleanup()), that call will be a no-op exactly because the offset is zero.

Move the fast commit offset further down in the journal commit code, until it's mostly done, immediately before clearing the on-going commit flags.

Quoted 7 lines

Quoted 1 line

Signed-off-by Luis Henriques (SUSE) <[email protected]>

Did you see any particular failure because of this? Because AFAICS the buffers cleaned up by jbd2_fc_release_bufs() are only allocated during fast commit (from ext4_fc_reserve_space()). And the code in jbd2_journal_commit_transaction() is making sure fast commit isn't running before we set journal->j_fc_off to 0.

No, I did not see any failure caused by this, this patch is simply based on my understanding of the code after spending some time reviewing it.

The problem I saw was that jbd2_journal_commit_transaction() will run the clean-up callbacks, which includes ext4_fc_cleanup(). One of the first things that this callback will do is to call jbd2_fc_release_bufs(). Because journal->j_fc_off is zero, this call is useless:

	j_fc_off = journal->j_fc_off;

	for (i = j_fc_off - 1; i >= 0; i--) {
		[...]
	}

(It's even a bit odd to start the loop with 'i = -1'...)

So the question is whether this call is actually useful at all. Maybe the thing to do is to simply remove the call to jbd2_fc_release_bufs()? (And in that case, remove the function too, as this is the only call site.)

Quoted 33 lines

On Wed 22 May 2024 12:45:00 PM +02, Jan Kara wrote;

Quoted 9 lines · Luis Henriques (SUSE)

Quoted 8 lines

When doing a journal commit, the fast journal offset (journal->j_fc_off) is set to zero too early in the process. Since ext4 filesystem calls function jbd2_fc_release_bufs() in its j_fc_cleanup_callback (ext4_fc_cleanup()), that call will be a no-op exactly because the offset is zero.

Move the fast commit offset further down in the journal commit code, until it's mostly done, immediately before clearing the on-going commit flags.

Quoted 7 lines

Quoted 1 line

Signed-off-by Luis Henriques (SUSE) <[email protected]>

Did you see any particular failure because of this? Because AFAICS the buffers cleaned up by jbd2_fc_release_bufs() are only allocated during fast commit (from ext4_fc_reserve_space()). And the code in jbd2_journal_commit_transaction() is making sure fast commit isn't running before we set journal->j_fc_off to 0.

No, I did not see any failure caused by this, this patch is simply based on my understanding of the code after spending some time reviewing it.

The problem I saw was that jbd2_journal_commit_transaction() will run the clean-up callbacks, which includes ext4_fc_cleanup(). One of the first things that this callback will do is to call jbd2_fc_release_bufs(). Because journal->j_fc_off is zero, this call is useless:

	j_fc_off = journal->j_fc_off;

	for (i = j_fc_off - 1; i >= 0; i--) {
		[...]
	}

Quoted 5 lines

(It's even a bit odd to start the loop with 'i = -1'...)

So the question is whether this call is actually useful at all. Maybe the thing to do is to simply remove the call to jbd2_fc_release_bufs()? (And in that case, remove the function too, as this is the only call site.)

What is I guess confusing for you (and somewhat for me as well) is that journal->j_fc_cleanup_callback() gets called from __jbd2_fc_end_commit() *and* from jbd2_journal_commit_transaction(). I agree the jbd2_fc_release_bufs() is useless for the call from jbd2_journal_commit_transaction(), it is however needed for the call from __jbd2_fc_end_commit(). There are however other bits - namely the s_fc_dentry_q and s_fc_q list handling that need to happen both for normal and fast commit...