| Message ID | 20240521154535.12911-3-luis.henriques@linux.dev |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: two small fast commit fixes | expand |
On Tue 21-05-24 16:45:35, Luis Henriques (SUSE) wrote: > 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) <luis.henriques@linux.dev> 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 > --- > fs/jbd2/commit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c > index 75ea4e9a5cab..88b834c7c9c9 100644 > --- a/fs/jbd2/commit.c > +++ b/fs/jbd2/commit.c > @@ -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); >
On Wed 22 May 2024 12:45:00 PM +02, Jan Kara wrote; > On Tue 21-05-24 16:45:35, Luis Henriques (SUSE) wrote: >> 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) <luis.henriques@linux.dev> > > 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.) Cheers,
On Wed 22-05-24 14:36:20, Luis Henriques wrote: > On Wed 22 May 2024 12:45:00 PM +02, Jan Kara wrote; > > > On Tue 21-05-24 16:45:35, Luis Henriques (SUSE) wrote: > >> 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) <luis.henriques@linux.dev> > > > > 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.) 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... Honza
On Thu 23 May 2024 09:44:34 AM +02, Jan Kara wrote; > On Wed 22-05-24 14:36:20, Luis Henriques wrote: >> On Wed 22 May 2024 12:45:00 PM +02, Jan Kara wrote; >> >> > On Tue 21-05-24 16:45:35, Luis Henriques (SUSE) wrote: >> >> 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) <luis.henriques@linux.dev> >> > >> > 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.) > > 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... Oops! I totally missed that second callback execution. Yeah, it does make sense now, of course. Sorry for the noise and thank you for looking into it. I'll go back and focus on reworking on the other patch (and also look into Harshad's patchset). Cheers,
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c index 75ea4e9a5cab..88b834c7c9c9 100644 --- a/fs/jbd2/commit.c +++ b/fs/jbd2/commit.c @@ -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);
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) <luis.henriques@linux.dev> --- fs/jbd2/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)