ext4: fix fast commit inode enqueueing during a full journal commit
PATCH v2 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 flagging an inode that is already enqueued in either queues. Later, during the fast commit clean-up callback, if the inode has a tid that is bigger than the one being handled, that inode is re-enqueued into STAGING and the 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!
(Now Cc'ing Harshad, as I should have done in the initial RFC.)
This v2 is a complete different solution, hinted by Jan Kara. I hope my understanding of his suggestion is correct. Also, I've dropped the second patch as it didn't made sense, as Jan also pointed out.
Finally, I haven't yet done a review of Harshad's patchset [1] (hope to get to it soon), but a quick test shows the issue is still present there. The good news is that patch can be trivially applied on top of it.
[1] https://lore.kernel.org/all/[email protected]
Cheers,
--
Luis
fs/ext4/ext4.h | 11 ++++++++++- fs/ext4/fast_commit.c | 11 +++++++++++ fs/ext4/super.c | 1 + 3 files changed, 22 insertions(+), 1 deletion(-)
fs/ext4/ext4.h +10 -1
@@ -1062,9 +1062,18 @@ struct ext4_inode_info {/* Fast commit wait queue for this inode */ wait_queue_head_t i_fc_wait; -/* Protect concurrent accesses on i_fc_lblk_start, i_fc_lblk_len */ +/* +* Protect concurrent accesses on i_fc_lblk_start, i_fc_lblk_len, +* i_fc_next +*/ struct mutex i_fc_lock; +/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +/* * i_disksize keeps track of what the inode size is ON DISK, not * in memory. During truncate, i_size is set to the new size by
fs/ext4/fast_commit.c +11 -0
@@ -402,6 +402,8 @@ static int ext4_fc_track_template( sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ? &sbi->s_fc_q[FC_Q_STAGING ] : &sbi->s_fc_q[FC_Q_MAIN]); + else + ei->i_fc_next = tid;spin_unlock (&sbi->s_fc_lock);return ret;
@@ -1280,6 +1282,15 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_for_each_entry_safe (iter, iter_n, &sbi->s_fc_q[FC_Q_MAIN ], i_fc_list) {list_del_init (&iter->i_fc_list); +if (iter->i_fc_next == tid) + iter->i_fc_next =0 ; +else if (iter->i_fc_next > tid) +/* +* re-enqueue inode into STAGING, which will later be +* splice back into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING );if (iter->i_sync_tid <= tid)
fs/ext4/super.c +1 -0
@@ -1437,6 +1437,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)INIT_WORK (&ei->i_rsv_conversion_work, ext4_end_io_rsv_work);ext4_fc_init_inode (&ei->vfs_inode);mutex_init (&ei->i_fc_lock); + ei->i_fc_next =0 ;return &ei->vfs_inode; }
Jan Kara
reviewerQuoted 13 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 flagging an inode that is already enqueued in either queues. Later, during the fast commit clean-up callback, if the inode has a tid that is bigger than the one being handled, that inode is re-enqueued into STAGING and the spliced back into MAIN.
Quoted 6 lines
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]>
Thanks for the fix. Some comments below:
Quoted 11 lines
fs/ext4/ext4.h +2
-1@@ -1062,9 +1062,18 @@ struct ext4_inode_info {/* Fast commit wait queue for this inode */ wait_queue_head_t i_fc_wait; -/* Protect concurrent accesses on i_fc_lblk_start, i_fc_lblk_len */ +/* +* Protect concurrent accesses on i_fc_lblk_start, i_fc_lblk_len,
fs/ext4/ext4.h struct ext4_inode_info { · line 1076
Quoted 10 lines
fs/ext4/ext4.h +8
-0+* i_fc_next +*/ struct mutex i_fc_lock; +/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +
Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in ext4_fc_track_template() and used for similar comparisons in fast commit code.
Quoted 14 lines
fs/ext4/fast_commit.c +2
-0@@ -402,6 +402,8 @@ static int ext4_fc_track_template( sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ? &sbi->s_fc_q[FC_Q_STAGING ] : &sbi->s_fc_q[FC_Q_MAIN]); + else + ei->i_fc_next = tid;spin_unlock (&sbi->s_fc_lock);return ret;
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1287
Quoted 6 lines
fs/ext4/fast_commit.c +3
-0@@ -1280,6 +1282,15 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_for_each_entry_safe (iter, iter_n, &sbi->s_fc_q[FC_Q_MAIN ], i_fc_list) {list_del_init (&iter->i_fc_list); +if (iter->i_fc_next == tid) + iter->i_fc_next =0 ; +else if (iter->i_fc_next > tid)
^^^ careful here, TIDs do wrap so you need to usetid_geq() for comparison.
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1296
Quoted 9 lines
fs/ext4/fast_commit.c +6
-0+/* +* re-enqueue inode into STAGING, which will later be +* splice back into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING );if (iter->i_sync_tid <= tid)
^^^ and I can see this is buggy as
well and needs tid_geq() (not your fault obviously).
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
On Fri 24 May 2024 06:22:31 PM +02, Jan Kara wrote;
Quoted 38 lines · Luis Henriques (SUSE)
Quoted 13 lines
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 flagging an inode that is already enqueued in either queues. Later, during the fast commit clean-up callback, if the inode has a tid that is bigger than the one being handled, that inode is re-enqueued into STAGING and the spliced back into MAIN.
Quoted 6 lines
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]>Thanks for the fix. Some comments below:
Quoted 15 lines
fs/ext4/ext4.h +4
-1@@ -1062,9 +1062,18 @@ struct ext4_inode_info {/* Fast commit wait queue for this inode */ wait_queue_head_t i_fc_wait; -/* Protect concurrent accesses on i_fc_lblk_start, i_fc_lblk_len */ +/* +* Protect concurrent accesses on i_fc_lblk_start, i_fc_lblk_len, +* i_fc_next +*/ struct mutex i_fc_lock;
Quoted 11 lines
fs/ext4/ext4.h struct ext4_inode_info { · line 1076
Quoted 6 lines
fs/ext4/ext4.h +6
-0+/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in
ext4_fc_track_template()and used for similar comparisons in fast commit code.
Ah, true. It looks like it could be used indeed. We'll still need a flag here, but a simple bool should be enough for that.
Quoted 13 lines
Quoted 13 lines
fs/ext4/fast_commit.c +2
-0@@ -402,6 +402,8 @@ static int ext4_fc_track_template( sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ? &sbi->s_fc_q[FC_Q_STAGING ] : &sbi->s_fc_q[FC_Q_MAIN]); + else + ei->i_fc_next = tid;spin_unlock (&sbi->s_fc_lock);
Quoted 11 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1287
Quoted 8 lines
fs/ext4/fast_commit.c +3
-0return ret;@@ -1280,6 +1282,15 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_for_each_entry_safe (iter, iter_n, &sbi->s_fc_q[FC_Q_MAIN ], i_fc_list) {list_del_init (&iter->i_fc_list); +if (iter->i_fc_next == tid) + iter->i_fc_next =0 ; +else if (iter->i_fc_next > tid)^^^ careful here, TIDs do wrap so you need to use
tid_geq()for comparison.
Yikes! Thanks, I'll update the code to do that.
Quoted 11 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1296
Quoted 9 lines
fs/ext4/fast_commit.c +6
-0+/* +* re-enqueue inode into STAGING, which will later be +* splice back into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING );if (iter->i_sync_tid <= tid)^^^ and I can see this is buggy as
well and needstid_geq()(not your fault obviously).
Yeah, good point. I can that too in v3.
Again, thanks a lot for your review!
Cheers,
Signature · 1 line
Luís
On Mon 27 May 2024 09:29:40 AM +01, Luis Henriques wrote;
<snip>
Quoted 12 lines
Quoted 11 lines
fs/ext4/ext4.h struct ext4_inode_info { · line 1076
Quoted 6 lines
fs/ext4/ext4.h +6
-0+/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in
ext4_fc_track_template()and used for similar comparisons in fast commit code.
Quoted 2 lines
Ah, true. It looks like it could be used indeed. We'll still need a flag here, but a simple bool should be enough for that.
After looking again at the code, I'm not 100% sure that this is actually doable. For example, if I replace the above by
bool i_fc_next;
and set to to 'true' below:
Quoted 10 lines
Quoted 10 lines
fs/ext4/fast_commit.c static int ext4_fc_track_template( · line 406
Quoted 10 lines
fs/ext4/fast_commit.c +2
-0@@ -402,6 +402,8 @@ static int ext4_fc_track_template( sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ? &sbi->s_fc_q[FC_Q_STAGING ] : &sbi->s_fc_q[FC_Q_MAIN]); + else + ei->i_fc_next = tid;
ei->i_fc_next = true;
Then, when we get to the ext4_fc_cleanup(), the value of iter->i_sync_tid may have changed in the meantime from, e.g., ext4_do_update_inode() or __ext4_iget(). This would cause the clean-up code to be bogus if it still implements a the logic below, by comparing the tid with i_sync_tid. (Although, to be honest, I couldn't see any visible effect in the quick testing I've done.) Or am I missing something, and this is *exactly* the behaviour you'd expect?
Cheers,
Signature · 1 line
Luis
Quoted 35 lines
Quoted 13 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1287
Quoted 10 lines
fs/ext4/fast_commit.c +3
-0spin_unlock (&sbi->s_fc_lock);return ret;@@ -1280,6 +1282,15 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_for_each_entry_safe (iter, iter_n, &sbi->s_fc_q[FC_Q_MAIN ], i_fc_list) {list_del_init (&iter->i_fc_list); +if (iter->i_fc_next == tid) + iter->i_fc_next =0 ; +else if (iter->i_fc_next > tid)^^^ careful here, TIDs do wrap so you need to use
tid_geq()for comparison.Yikes! Thanks, I'll update the code to do that.
Quoted 11 lines
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1296
Quoted 9 lines
fs/ext4/fast_commit.c +6
-0+/* +* re-enqueue inode into STAGING, which will later be +* splice back into MAIN +*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING );if (iter->i_sync_tid <= tid)^^^ and I can see this is buggy as
well and needstid_geq()(not your fault obviously).Yeah, good point. I can that too in v3.
Again, thanks a lot for your review!
Cheers,
Signature · 1 line
Luís
Jan Kara
reviewerQuoted 36 lines · Luis Henriques
On Mon 27 May 2024 09:29:40 AM +01, Luis Henriques wrote;
Quoted 12 lines
Quoted 11 lines
fs/ext4/ext4.h struct ext4_inode_info { · line 1076
Quoted 6 lines
fs/ext4/ext4.h +6
-0+/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in
ext4_fc_track_template()and used for similar comparisons in fast commit code.Quoted 2 lines
Ah, true. It looks like it could be used indeed. We'll still need a flag here, but a simple bool should be enough for that.
After looking again at the code, I'm not 100% sure that this is actually doable. For example, if I replace the above by
bool i_fc_next;and set to to 'true' below:
Quoted 10 lines
Quoted 10 lines
fs/ext4/fast_commit.c static int ext4_fc_track_template( · line 406
Quoted 10 lines
fs/ext4/fast_commit.c +2
-0@@ -402,6 +402,8 @@ static int ext4_fc_track_template( sbi->s_journal->j_flags & JBD2_FAST_COMMIT_ONGOING) ? &sbi->s_fc_q[FC_Q_STAGING ] : &sbi->s_fc_q[FC_Q_MAIN]); + else + ei->i_fc_next = tid;ei->i_fc_next = true;
Quoted 7 lines
Then, when we get to the
ext4_fc_cleanup(), the value ofiter->i_sync_tidmay have changed in the meantime from, e.g.,ext4_do_update_inode()or__ext4_iget(). This would cause the clean-up code to be bogus if it still implements a the logic below, by comparing the tid withi_sync_tid. (Although, to be honest, I couldn't see any visible effect in the quick testing I've done.) Or am I missing something, and this is *exactly* the behaviour you'd expect?
Yes, this is the behavior I'd expect. The rationale is that if i_sync_tid points to the running transaction, it means the inode was modified in it, which means fastcommit needs to write it out. In fact the ext4_update_inode_fsync_trans() calls usually happen together with ext4_fc_track_...() calls. This could use some cleanup so that we don't set i_sync_tid in two places unnecessarily but that's for some other time...
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jan Kara
reviewerQuoted 17 lines · Jan Kara
Quoted 16 lines · Luis Henriques
On Mon 27 May 2024 09:29:40 AM +01, Luis Henriques wrote;
Quoted 14 lines
Quoted 11 lines
fs/ext4/ext4.h struct ext4_inode_info { · line 1076
Quoted 6 lines
fs/ext4/ext4.h +6
-0+/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in
ext4_fc_track_template()and used for similar comparisons in fast commit code.Ah, true. It looks like it could be used indeed. We'll still need a flag here, but a simple bool should be enough for that.
Quoted 6 lines
Quoted 6 lines
After looking again at the code, I'm not 100% sure that this is actually doable. For example, if I replace the above by
bool i_fc_next;and set to to 'true' below:
Forgot to comment on this one: I don't think you even need 'bool i_fc_next'
- simply whenever i_sync_tid is greater than committing transaction's tid, you move the inode to FC_Q_STAGING list in ext4_fc_cleanup().
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
On Tue 28 May 2024 12:52:03 PM +02, Jan Kara wrote;
Quoted 21 lines · Jan Kara
Quoted 20 lines · Luis Henriques
Quoted 19 lines
On Mon 27 May 2024 09:29:40 AM +01, Luis Henriques wrote;
Quoted 12 lines
Quoted 11 lines
fs/ext4/ext4.h struct ext4_inode_info { · line 1076
Quoted 6 lines
fs/ext4/ext4.h +6
-0+/* +* Used to flag an inode as part of the next fast commit; will be +* reset during fast commit clean-up +*/ +tid_t i_fc_next; +Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in
ext4_fc_track_template()and used for similar comparisons in fast commit code.Quoted 2 lines
Ah, true. It looks like it could be used indeed. We'll still need a flag here, but a simple bool should be enough for that.
After looking again at the code, I'm not 100% sure that this is actually doable. For example, if I replace the above by
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
bool i_fc_next;and set to to 'true' below:
Forgot to comment on this one: I don't think you even need 'bool
i_fc_next'
- simply wheneveri_sync_tidis greater than committing transaction's tid, you move the inode toFC_Q_STAGINGlist inext4_fc_cleanup().
Yeah, I got that from your other comment in the previous email. And that means the actual fix will be a pretty small patch (almost a one-liner).
I'm running some more tests on v3, I'll probably send it later today or tomorrow. Thanks a lot for your review (and patience), Jan.
Cheers,
Signature · 1 line
Luís
Sorry for getting back late on your patchset - I was on vacation and checked your patch just now. This is a good catch! My patchset does not fix this issue. Looking forward to your V3 fix.
Also, using i_sync_tid as Jan suggested sounds like a good way to handle this.
- Harshad
Quoted 41 lines · Luis Henriques
On Tue 28 May 2024 12:52:03 PM +02, Jan Kara wrote;
Quoted 21 lines · Jan Kara
Quoted 20 lines · Luis Henriques
Quoted 19 lines
On Mon 27 May 2024 09:29:40 AM +01, Luis Henriques wrote;
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
(quoted diff) +6
-0+ /* + * Used to flag an inode as part of the next fast commit; will be + * reset during fast commit clean-up + */ + tid_t i_fc_next; +Quoted 7 lines
Quoted 4 lines
Do we really need new tid in the inode? I'd be kind of hoping we could use EXT4_I(inode)->i_sync_tid for this - I can see we even already set it in
ext4_fc_track_template()and used for similar comparisons in fast commit code.Ah, true. It looks like it could be used indeed. We'll still need a flag here, but a simple bool should be enough for that.
After looking again at the code, I'm not 100% sure that this is actually doable. For example, if I replace the above by
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
bool i_fc_next;and set to to 'true' below:
Forgot to comment on this one: I don't think you even need 'bool
i_fc_next'
- simply wheneveri_sync_tidis greater than committing transaction's tid, you move the inode toFC_Q_STAGINGlist inext4_fc_cleanup().Yeah, I got that from your other comment in the previous email. And that means the actual fix will be a pretty small patch (almost a one-liner).
I'm running some more tests on v3, I'll probably send it later today or tomorrow. Thanks a lot for your review (and patience), Jan.
Cheers,
--
Luís