ext4: fix fast commit inode enqueueing during a full journal commit
Subsystems (2)
PATCH v4 1/1 ext4: fix fast commit inode enqueueing during a full journal commit
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 re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.
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.
---
Hi!
v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.
fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
fs/ext4/fast_commit.c +10 -0
@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the +* inode into STAGING, which will then be splice back +* into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + } +/* Make sure EXT4_STATE_FC_COMMITTING bit is clear */ smp_mb ();#if (BITS_PER_LONG <64 )
Quoted 38 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (
i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.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]>---
Hi!v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.
fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/ext4/fast_commit.c +0
-0
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the
one quick question about tid, if one disk is using long time and its tid
get wrapped to 0, is it a valid seq? I don't find code handling this
situation.
Quoted 11 lines
fs/ext4/fast_commit.c +7
-0+* inode into STAGING, which will then be splice back +* into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + } +/* Make sure EXT4_STATE_FC_COMMITTING bit is clear */ smp_mb ();#if (BITS_PER_LONG <64 )
Signature · 1 line
Regards
Quoted 32 lines · wangjianjian (C)
Quoted 31 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().
This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.
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]>---
Hi!
v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)
Quoted 8 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the one quick question about tid, if one disk is using long time and its tid get
wrapped to 0, is it a valid seq? I don't find code handling this situation.
Hmm... OK. So, to answer to your question, the 'tid' is expected to wrap. That's why we use:
if (tid_geq(tid, iter->i_sync_tid))
instead of:
if (tid >= iter->i_sync_tid)
(The second patch in v3 actually fixed a few places where the tid_*() helpers weren't being used.)
But your question shows me that my patch is wrong as '0' may actually be a valid 'tid' value.
Cheers,
Signature · 1 line
Luís
Quoted 14 lines
Quoted 11 lines
fs/ext4/fast_commit.c +7
-0+* inode into STAGING, which will then be splice back +* into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + } +/* Make sure EXT4_STATE_FC_COMMITTING bit is clear */ smp_mb ();#if (BITS_PER_LONG <64 )Signature · 1 line
Regards
Quoted 42 lines · Luis Henriques
Quoted 32 lines · wangjianjian (C)
Quoted 31 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().
This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.
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]>---
Hi!
v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)Quoted 8 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the one quick question about tid, if one disk is using long time and its tid get
wrapped to 0, is it a valid seq? I don't find code handling this situation.
Quoted 4 lines
Hmm... OK. So, to answer to your question, the 'tid' is expected to wrap. That's why we use:
if (tid_geq(tid, iter->i_sync_tid))
Yes, I know this.
Quoted 10 lines
instead of:
if (tid >= iter->i_sync_tid)(The second patch in v3 actually fixed a few places where the tid_*() helpers weren't being used.)
But your question shows me that my patch is wrong as '0' may actually be a valid 'tid' value.
Actually my question is, there are some place use '0' to check if a transaction is valid, e.g.
In ext4_wait_for_tail_page_commit()
5218 while (1) {
5219 struct folio *folio = filemap_lock_folio(inode->i_mapping, 5220 inode->i_size >> PAGE_SHIFT); 5221 if (IS_ERR(folio))
5222 return;
5223 ret = __ext4_journalled_invalidate_folio(folio, offset, 5224 folio_size(folio) - offset);
5225 folio_unlock(folio);
5226 folio_put(folio);
5227 if (ret != -EBUSY)
5228 return;
5229 commit_tid = 0;
5230 read_lock(&journal->j_state_lock);
5231 if (journal->j_committing_transaction)
5232 commit_tid = journal->j_committing_transaction->t_tid; 5233 read_unlock(&journal->j_state_lock);
5234 if (commit_tid)
5235 jbd2_log_wait_commit(journal, commit_tid); 5236 }
5237 We only wait commit if tid is not zero.
And in __jbd2_log_wait_for_space()
79 if (space_left < nblocks) {
80 int chkpt = journal->j_checkpoint_transactions != NULL; 81 tid_t tid = 0;
82
83 if (journal->j_committing_transaction)
84 tid = journal->j_committing_transaction->t_tid; 85 spin_unlock(&journal->j_list_lock);
86 write_unlock(&journal->j_state_lock);
87 if (chkpt) {
88 jbd2_log_do_checkpoint(journal);
89 } else if (jbd2_cleanup_journal_tail(journal) == 0) { 90 /* We were able to recover space; yay! */ 91 ;
92 } else if (tid) {
93 /*
94 * jbd2_journal_commit_transaction() may want 95 * to take the checkpoint_mutex if JBD2_FLUSHED 96 * is set. So we need to temporarily drop it. 97 */
98 mutex_unlock(&journal->j_checkpoint_mutex);
99 jbd2_log_wait_commit(journal, tid); 100 write_lock(&journal->j_state_lock);
101 continue;
We also only wait commit if tid is not zero.
Does it mean all these have bugs if '0' is a valid 'tid' ?
But on the other hand, if we don't consider sync and fsync, and default commit interval is 5s,
time of tid wrap to 0 is nearly 680 years. However, we can run sync/fsync to make tid to increase
more quickly in real world ?
Quoted 1 line
Cheers,
Quoted 111 lines · Wang Jianjian
Quoted 42 lines · Luis Henriques
Quoted 32 lines · wangjianjian (C)
Quoted 31 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().
This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.
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]>---
Hi!
v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)Quoted 8 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the one quick question about tid, if one disk is using long time and its tid get
wrapped to 0, is it a valid seq? I don't find code handling this situation.Quoted 4 lines
Hmm... OK. So, to answer to your question, the 'tid' is expected to wrap. That's why we use:
if (tid_geq(tid, iter->i_sync_tid))Yes, I know this.
Quoted 10 lines
instead of:
if (tid >= iter->i_sync_tid)(The second patch in v3 actually fixed a few places where the tid_*() helpers weren't being used.)
But your question shows me that my patch is wrong as '0' may actually be a valid 'tid' value.
Actually my question is, there are some place use '0' to check if a transaction is valid, e.g.
In
ext4_wait_for_tail_page_commit()5218 while (1) {
5219 struct folio *folio = filemap_lock_folio(inode->i_mapping,
5220inode->i_size>>PAGE_SHIFT);
5221 if (IS_ERR(folio))
5222 return;
5223 ret = __ext4_journalled_invalidate_folio(folio, offset,
5224folio_size(folio)- offset);5225 folio_unlock(folio); 5226 folio_put(folio); 5227 if (ret != -EBUSY) 5228 return; 5229 commit_tid = 0; 5230 read_lock(&journal->j_state_lock); 5231 if (journal->j_committing_transaction)5232
commit_tid=journal->j_committing_transaction->t_tid;
5233read_unlock(&journal->j_state_lock);
5234 if (commit_tid)
5235jbd2_log_wait_commit(journal, commit_tid);
5236 }
5237 We only wait commit if tid is not zero.And in
__jbd2_log_wait_for_space()79 if (
space_left< nblocks) {
80 int chkpt =journal->j_checkpoint_transactions!= NULL;
81tid_ttid = 0;
82
83 if (journal->j_committing_transaction)
84 tid =journal->j_committing_transaction->t_tid;
85spin_unlock(&journal->j_list_lock);
86write_unlock(&journal->j_state_lock);
87 if (chkpt) {
88jbd2_log_do_checkpoint(journal);
89 } else if (jbd2_cleanup_journal_tail(journal)== 0) {
90 /* We were able to recover space; yay! */
91 ;
92 } else if (tid) {
93 /*94 * jbd2_journal_commit_transaction() may want 95 * to take the checkpoint_mutex if JBD2_FLUSHED 96 * is set. So we need to temporarily drop it. 97 */98
mutex_unlock(&journal->j_checkpoint_mutex);
99jbd2_log_wait_commit(journal, tid);
100write_lock(&journal->j_state_lock);
101 continue;
We also only wait commit if tid is not zero.
Quoted 7 lines
Does it mean all these have bugs if '0' is a valid 'tid' ?
But on the other hand, if we don't consider sync and fsync, and default commit interval is 5s,
time of tid wrap to 0 is nearly 680 years. However, we can run sync/fsync to make tid to increase
more quickly in real world ?
The simple solution is that "0" is not a valid sequence. It looks like this is a bug in jbd2_get_transaction() where it is incrementing the TID:
transaction->t_tid = journal->j_transaction_sequence++;
it should add a check to handle the wrap-around:
if (unlikely(transaction->t_tid == 0))
transaction->t_tid = journal->j_transaction_sequence++;Cheers, Andreas
Quoted 123 lines · Andreas Dilger
Quoted 111 lines · Wang Jianjian
Quoted 42 lines · Luis Henriques
Quoted 32 lines · wangjianjian (C)
Quoted 31 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().
This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.
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]>---
Hi!
v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)Quoted 8 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the one quick question about tid, if one disk is using long time and its tid get
wrapped to 0, is it a valid seq? I don't find code handling this situation.Quoted 4 lines
Hmm... OK. So, to answer to your question, the 'tid' is expected to wrap. That's why we use:
if (tid_geq(tid, iter->i_sync_tid))Yes, I know this.
Quoted 10 lines
instead of:
if (tid >= iter->i_sync_tid)(The second patch in v3 actually fixed a few places where the tid_*() helpers weren't being used.)
But your question shows me that my patch is wrong as '0' may actually be a valid 'tid' value.
Actually my question is, there are some place use '0' to check if a transaction is valid, e.g.
In
ext4_wait_for_tail_page_commit()5218 while (1) {
5219 struct folio *folio = filemap_lock_folio(inode->i_mapping,
5220inode->i_size>>PAGE_SHIFT);
5221 if (IS_ERR(folio))
5222 return;
5223 ret = __ext4_journalled_invalidate_folio(folio, offset,
5224folio_size(folio)- offset);5225 folio_unlock(folio); 5226 folio_put(folio); 5227 if (ret != -EBUSY) 5228 return; 5229 commit_tid = 0; 5230 read_lock(&journal->j_state_lock); 5231 if (journal->j_committing_transaction)5232
commit_tid=journal->j_committing_transaction->t_tid;
5233read_unlock(&journal->j_state_lock);
5234 if (commit_tid)
5235jbd2_log_wait_commit(journal, commit_tid);
5236 }
5237 We only wait commit if tid is not zero.And in
__jbd2_log_wait_for_space()79 if (
space_left< nblocks) {
80 int chkpt =journal->j_checkpoint_transactions!= NULL;
81tid_ttid = 0;
82
83 if (journal->j_committing_transaction)
84 tid =journal->j_committing_transaction->t_tid;
85spin_unlock(&journal->j_list_lock);
86write_unlock(&journal->j_state_lock);
87 if (chkpt) {
88jbd2_log_do_checkpoint(journal);
89 } else if (jbd2_cleanup_journal_tail(journal)== 0) {
90 /* We were able to recover space; yay! */
91 ;
92 } else if (tid) {
93 /*94 * jbd2_journal_commit_transaction() may want 95 * to take the checkpoint_mutex if JBD2_FLUSHED 96 * is set. So we need to temporarily drop it. 97 */98
mutex_unlock(&journal->j_checkpoint_mutex);
99jbd2_log_wait_commit(journal, tid);
100write_lock(&journal->j_state_lock);
101 continue;
We also only wait commit if tid is not zero.Quoted 7 lines
Does it mean all these have bugs if '0' is a valid 'tid' ?
But on the other hand, if we don't consider sync and fsync, and default commit interval is 5s,
time of tid wrap to 0 is nearly 680 years. However, we can run sync/fsync to make tid to increase
more quickly in real world ?
The simple solution is that "0" is not a valid sequence. It looks like this is a bug in
jbd2_get_transaction()where it is incrementing the TID:
Quoted 6 lines
transaction->t_tid = journal->j_transaction_sequence++;it should add a check to handle the wrap-around:
if (unlikely(transaction->t_tid == 0)) transaction->t_tid = journal->j_transaction_sequence++;
Yes, I had ever thought about this, just curious why nobody encounter problems here.
Quoted 7 lines
Cheers, Andreas
Signature · 1 line
Regards
Quoted 123 lines · Andreas Dilger
Quoted 111 lines · Wang Jianjian
Quoted 42 lines · Luis Henriques
Quoted 32 lines · wangjianjian (C)
Quoted 31 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().
This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.
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]>---
Hi!
v4 of this patch enqueues the inode into STAGING *only* if the current tid is non-zero. It will be zero when doing an fc commit, and this would mean to always re-enqueue the inode. This fixes the regressions caught by Ted in v3 with fstests generic/472 generic/496 generic/643.
Also, since 2nd patch of v3 has already been merged, I've rebased this patch to be applied on top of it.fs/ext4/fast_commit.c | 10 ++++++++++ 1 file changed, 10 insertions(+)fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)Quoted 8 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the one quick question about tid, if one disk is using long time and its tid get
wrapped to 0, is it a valid seq? I don't find code handling this situation.Quoted 4 lines
Hmm... OK. So, to answer to your question, the 'tid' is expected to wrap. That's why we use:
if (tid_geq(tid, iter->i_sync_tid))Yes, I know this.
Quoted 10 lines
instead of:
if (tid >= iter->i_sync_tid)(The second patch in v3 actually fixed a few places where the tid_*() helpers weren't being used.)
But your question shows me that my patch is wrong as '0' may actually be a valid 'tid' value.
Actually my question is, there are some place use '0' to check if a transaction is valid, e.g.
In
ext4_wait_for_tail_page_commit()5218 while (1) {
5219 struct folio *folio = filemap_lock_folio(inode->i_mapping,
5220inode->i_size>>PAGE_SHIFT);
5221 if (IS_ERR(folio))
5222 return;
5223 ret = __ext4_journalled_invalidate_folio(folio, offset,
5224folio_size(folio)- offset);5225 folio_unlock(folio); 5226 folio_put(folio); 5227 if (ret != -EBUSY) 5228 return; 5229 commit_tid = 0; 5230 read_lock(&journal->j_state_lock); 5231 if (journal->j_committing_transaction)5232
commit_tid=journal->j_committing_transaction->t_tid;
5233read_unlock(&journal->j_state_lock);
5234 if (commit_tid)
5235jbd2_log_wait_commit(journal, commit_tid);
5236 }
5237 We only wait commit if tid is not zero.And in
__jbd2_log_wait_for_space()79 if (
space_left< nblocks) {
80 int chkpt =journal->j_checkpoint_transactions!= NULL;
81tid_ttid = 0;
82
83 if (journal->j_committing_transaction)
84 tid =journal->j_committing_transaction->t_tid;
85spin_unlock(&journal->j_list_lock);
86write_unlock(&journal->j_state_lock);
87 if (chkpt) {
88jbd2_log_do_checkpoint(journal);
89 } else if (jbd2_cleanup_journal_tail(journal)== 0) {
90 /* We were able to recover space; yay! */
91 ;
92 } else if (tid) {
93 /*94 * jbd2_journal_commit_transaction() may want 95 * to take the checkpoint_mutex if JBD2_FLUSHED 96 * is set. So we need to temporarily drop it. 97 */98
mutex_unlock(&journal->j_checkpoint_mutex);
99jbd2_log_wait_commit(journal, tid);
100write_lock(&journal->j_state_lock);
101 continue;
We also only wait commit if tid is not zero.Quoted 7 lines
Does it mean all these have bugs if '0' is a valid 'tid' ?
But on the other hand, if we don't consider sync and fsync, and default commit interval is 5s,
time of tid wrap to 0 is nearly 680 years. However, we can run sync/fsync to make tid to increase
more quickly in real world ?
The simple solution is that "0" is not a valid sequence. It looks like this is a bug in
jbd2_get_transaction()where it is incrementing the TID:
Quoted 6 lines
transaction->t_tid = journal->j_transaction_sequence++;it should add a check to handle the wrap-around:
if (unlikely(transaction->t_tid == 0)) transaction->t_tid = journal->j_transaction_sequence++;
Sound good to me. I can prepare a patch with this change if no one else sees other issues. As far as I can see, this shouldn't be a problem even when replaying journals that still have a '0' tid.
Thanks, Andreas. And thanks Wang, for spotting this.
Cheers,
Signature · 1 line
Luís
RFC PATCH 1/1 jbd2: make '0' an invalid transaction sequence
Since there's code (in fast-commit) that already handles a '0' tid as a special case, it's better to ensure that jbd2 never sets it to that value when journal->j_transaction_sequence increment wraps.
--- fs/jbd2/transaction.c | 2 ++ 1 file changed, 2 insertions(+)
fs/jbd2/transaction.c +2 -0
@@ -84,6 +84,8 @@ static void jbd2_get_transaction(journal_t *journal, transaction->t_state =T_RUNNING ; transaction->t_start_time =ktime_get (); transaction->t_tid = journal->j_transaction_sequence++; +if (unlikely (transaction->t_tid ==0 )) + transaction->t_tid = journal->j_transaction_sequence++; transaction->t_expires = jiffies + journal->j_commit_interval;atomic_set (&transaction->t_updates,0 ); atomic_set(&transaction->t_outstanding_credits,
Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence
Quoted 15 lines · Luis Henriques
Since there's code (in fast-commit) that already handles a '0' tid as a special case, it's better to ensure that jbd2 never sets it to that value when
journal->j_transaction_sequenceincrement wraps.Suggested-by Andreas Dilger <[email protected]>Signed-off-by Luis Henriques (SUSE) <[email protected]>--- fs/jbd2/transaction.c | 2 ++ 1 file changed, 2 insertions(+)fs/jbd2/transaction.c +0
-0
fs/jbd2/transaction.c static void jbd2_get_transaction(journal_t *journal, · line 88
Quoted 5 lines
fs/jbd2/transaction.c +2
-0@@ -84,6 +84,8 @@ static void jbd2_get_transaction(journal_t *journal, transaction->t_state =T_RUNNING ; transaction->t_start_time =ktime_get (); transaction->t_tid = journal->j_transaction_sequence++; +if (unlikely (transaction->t_tid ==0 )) + transaction->t_tid = journal->j_transaction_sequence++;
Do we need add j_transaction_sequence again here? if tansanction->t_tid == 0, then journal->j_trnasaction_sequence must be 1
Quoted 3 lines
fs/jbd2/transaction.c +0
-0transaction->t_expires = jiffies + journal->j_commit_interval;atomic_set (&transaction->t_updates,0 ); atomic_set(&transaction->t_outstanding_credits,
Signature · 1 line
Regards
Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence
Quoted 11 lines · wangjianjian (C)
Quoted 10 lines · Luis Henriques
Since there's code (in fast-commit) that already handles a '0' tid as a special case, it's better to ensure that jbd2 never sets it to that value when
journal->j_transaction_sequenceincrement wraps.Suggested-by Andreas Dilger <[email protected]>Signed-off-by Luis Henriques (SUSE) <[email protected]>---
fs/jbd2/transaction.c| 2 ++1 file changed, 2 insertions(+)
Quoted 11 lines
fs/jbd2/transaction.c static void jbd2_get_transaction(journal_t *journal, · line 88
Quoted 10 lines
fs/jbd2/transaction.c +2
-0@@ -84,6 +84,8 @@ static void jbd2_get_transaction(journal_t *journal, transaction->t_state = T_RUNNING; transaction->t_start_time = ktime_get(); transaction->t_tid = journal->j_transaction_sequence++; + if (unlikely(transaction->t_tid == 0)) + transaction->t_tid = journal->j_transaction_sequence++;Do we need add
j_transaction_sequenceagain here? iftansanction->t_tid== 0, thenjournal->j_trnasaction_sequencemust be 1
still need add 1. Sorry for confuse.
Quoted 4 lines
Quoted 4 lines
fs/jbd2/transaction.c +0
-0transaction->t_expires = jiffies + journal->j_commit_interval; atomic_set(&transaction->t_updates, 0); atomic_set(&transaction->t_outstanding_credits,
Signature · 1 line
Regards
Jan Kara
reviewerRe: [RFC PATCH] jbd2: make '0' an invalid transaction sequence
In reply to Luis HenriquesQuoted 6 lines · Luis Henriques
Since there's code (in fast-commit) that already handles a '0' tid as a special case, it's better to ensure that jbd2 never sets it to that value when
journal->j_transaction_sequenceincrement wraps.Suggested-by Andreas Dilger <[email protected]>Signed-off-by Luis Henriques (SUSE) <[email protected]>
Well, sadly it isn't so simple. If nothing else, journal replay (do_one_pass()) will get broken by the skipped tid as we do check:
if (sequence != next_commit_ID) {
brelse(bh);
break;
}So we'd abort journal replay too early. Secondly, there's also code handling journal replay in libext2fs which would need to be checked and fixed up. Finally, I've found code in mballoc which alternates between two lists based on tid & 1, so this logic would get broken by skipping 0 tid as well.
Overall, I think we might be better off to go and fix places that assume tid 0 is not valid. I can see those assumptions in:
ext4_fc_mark_ineligible()ext4_wait_for_tail_page_commit()__jbd2_log_wait_for_space()jbd2_journal_shrink_checkpoint_list()
Now I don't see it as urgent to fix all these right now. Just for this series let's not add another place making tid 0 special. Later we can fixup the other places...
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [RFC PATCH] jbd2: make '0' an invalid transaction sequence
Quoted 25 lines · Jan Kara
Quoted 6 lines · Luis Henriques
Since there's code (in fast-commit) that already handles a '0' tid as a special case, it's better to ensure that jbd2 never sets it to that value when
journal->j_transaction_sequenceincrement wraps.Suggested-by Andreas Dilger <[email protected]>Signed-off-by Luis Henriques (SUSE) <[email protected]>Well, sadly it isn't so simple. If nothing else, journal replay (
do_one_pass()) will get broken by the skipped tid as we do check:if (sequence != next_commit_ID) { brelse(bh); break; }So we'd abort journal replay too early. Secondly, there's also code handling journal replay in libext2fs which would need to be checked and fixed up. Finally, I've found code in mballoc which alternates between two lists based on tid & 1, so this logic would get broken by skipping 0 tid as well.
Overall, I think we might be better off to go and fix places that assume tid 0 is not valid. I can see those assumptions in:
Quoted 8 lines
ext4_fc_mark_ineligible()ext4_wait_for_tail_page_commit()__jbd2_log_wait_for_space()jbd2_journal_shrink_checkpoint_list()Now I don't see it as urgent to fix all these right now. Just for this series let's not add another place making tid 0 special. Later we can fixup the other places...
Yikes! Looks like I haven't done my homework -- I should have caught at least one of the three breakages you point out. Obviously, because I've seen this assumption in different places, I thought it would be OK.
Anyway, thanks a lot for point this out, Jan. I'll add a new TODO to my list to start looking at other places that need to be fixed.
Cheers,
Signature · 1 line
Luís
Jan Kara
reviewerQuoted 19 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (
i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.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]>
...
Quoted 7 lines
fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1301
Quoted 10 lines
fs/ext4/fast_commit.c +9
-0ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the +* inode into STAGING, which will then be splice back +* into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + }
I don't think this is going to work (even if we fix the tid 0 being special assumption). With this there would be a race like:
Task 1 Task2
modify inode Iext4_fc_commit()
jbd2_fc_begin_commit()
commits changes
jbd2_fc_end_commit()
__jbd2_fc_end_commit(journal, 0, false)
jbd2_journal_unlock_updates(journal)
jbd2_journal_start()
modify inode I
...
ext4_mark_iloc_dirty()
ext4_fc_track_inode()
ext4_fc_track_template()
- doesn't add inode anywhere
because i_fc_list is not empty
ext4_fc_cleanup(journal, 0, 0)
removes inode I from i_fc_list => next fastcommit will not properlyflush it.
To avoid this race I think we could move the journal->j_fc_cleanup_callback() call to happen before we call jbd2_journal_unlock_updates(). Then we are sure that inode cannot be modified (journal is locked) until we are done processing the fastcommit lists when doing fastcommit. Hence your patch could then be changed like:
+ } else if (full) {
+ /*
+ * We are called after a full commit, inode has been
+ * modified while the commit was running. Re-enqueue
+ * the inode into STAGING, which will then be splice
+ * back into MAIN. This cannot happen during
+ * fastcommit because the journal is locked all the
+ * time in that case (and tid doesn't increase so
+ * tid check above isn't reliable).
+ */
+ list_add_tail(&EXT4_I(&iter->vfs_inode)->i_fc_list,
+ &sbi->s_fc_q[FC_Q_STAGING]);
+ }
Later, Harshad's patches change the code to use EXT4_STATE_FC_COMMITTING for protecting inodes during fastcommit and that will also deal with these races without having to keep the whole journal locked.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 84 lines · Jan Kara
Quoted 19 lines · Luis Henriques (SUSE)
When a full journal commit is on-going, any fast commit has to be enqueued into a different queue:
FC_Q_STAGINGinstead ofFC_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 intoFC_Q_MAIN, the next fast commit needs to be done intoFC_Q_STAGING. And this is not being done in functionext4_fc_track_template().This patch fixes the issue by re-enqueuing an inode into the STAGING queue during the fast commit clean-up callback if it has a tid (
i_sync_tid) greater than the one being handled. The STAGING queue will then be spliced back into MAIN.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]>...
Quoted 7 lines
fs/ext4/fast_commit.c +0
-0@@ -1290,6 +1290,16 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) EXT4_STATE_FC_COMMITTING);if (tid_geq (tid, iter->i_sync_tid))fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1301
Quoted 10 lines
fs/ext4/fast_commit.c +9
-0ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* If the tid is valid (i.e. non-zero) re-enqueue the +* inode into STAGING, which will then be splice back +* into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + }I don't think this is going to work (even if we fix the tid 0 being special assumption). With this there would be a race like:
Task 1 Task2
modify inode Iext4_fc_commit()
jbd2_fc_begin_commit()
commits changes
jbd2_fc_end_commit()__jbd2_fc_end_commit(journal, 0, false) jbd2_journal_unlock_updates(journal) jbd2_journal_start() modify inode I ... ext4_mark_iloc_dirty() ext4_fc_track_inode() ext4_fc_track_template() - doesn't add inode anywhere because i_fc_list is not empty ext4_fc_cleanup(journal, 0, 0) removes inode I from i_fc_list => next fastcommit will not properlyflush it.
To avoid this race I think we could move the
journal->j_fc_cleanup_callback()call to happen before we calljbd2_journal_unlock_updates(). Then we are sure that inode cannot be modified (journal is locked) until we are done processing the fastcommit lists when doing fastcommit. Hence your patch could then be changed like:(quoted diff) +13
-0+ } else if (full) { + /* + * We are called after a full commit, inode has been + * modified while the commit was running. Re-enqueue + * the inode into STAGING, which will then be splice + * back into MAIN. This cannot happen during + * fastcommit because the journal is locked all the + * time in that case (and tid doesn't increase so + * tid check above isn't reliable). + */ + list_add_tail(&EXT4_I(&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING]); + }
Quoted 3 lines
Later, Harshad's patches change the code to use
EXT4_STATE_FC_COMMITTINGfor protecting inodes during fastcommit and that will also deal with these races without having to keep the whole journal locked.
OK, this looks like it should fix all the issues I was trying to fix (g/047, g/472, and a few others Ted pointed out). I'll go run a few more tests on this to try to catch any possible regression.
Once again, thanks a lot for your help, Jan.
Cheers,
Signature · 1 line
Luís