Hi!
I've spent some time investigating an fstest failure when running it using
'-O fast_commit'. As a result, I'm sending two patches that hopefully fix
this failure. The first patch is the actual bug fix for the generic/047
fstest. The second patch was just something I saw through code inspection.
Note that this generic/047 test also requires the fix I sent before[1], for
a different fstest failure.
[1] https://lore.kernel.org/all/[email protected]
Luis Henriques (SUSE) (2):
ext4: fix fast commit inode enqueueing during a full journal commit
jbd2: reset fast commit offset only after fs cleanup is done
fs/ext4/fast_commit.c | 19 +++++++++++++------
fs/jbd2/commit.c | 2 +-
2 files changed, 14 insertions(+), 7 deletions(-)
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(-)
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 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(-)
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]);
+ } 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;
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) <[email protected]>
> ---
> 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 <[email protected]>
SUSE Labs, CR
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) <[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
> ---
> 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);
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed 22 May 2024 12:35:45 PM +02, Jan Kara wrote;
> 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) <[email protected]>
>> ---
>> 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...
Hmm... I see, the problem is deeper that I thought.
> 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.
Yeah, I guess that would work. I'll need to add a new field to flag the
'next commit' in struct ext4_inode_info. I'll need to play a bit with it
and see what I can came up with. Thanks for the suggestion.
> 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.
>
Right, I saw his patchset only after sending my RFC (and I should have
probably included him on the CC as well; probably get_maintainer.pl isn't
picking his email).
I'll need to look at those changes too, which will probably take me some
time as most of that code isn't familiar to me.
Thanks a lot for your review, Jan. Much appreciated.
Cheers,
--
Luis
>> + } 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 <[email protected]>
> SUSE Labs, CR
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) <[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.)
Cheers,
--
Luis
>
> 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);
>>
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
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) <[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.)
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
--
Jan Kara <[email protected]>
SUSE Labs, CR
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) <[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.)
>
> 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,
--
Luis