ext4: fix fast commit inode enqueueing during a full journal commit
v3 cover ext4: fix fast commit inode enqueueing during a full journal commit
Hi!
Here's v3 of this fix to the fast commit enqueuing bug triggered by fstest generic/047. This version simplifies the previous patch version by re-using the i_sync_tid field in struct ext4_inode_info instead of adding a new one.
The extra patch includes a few extra fixes to the tid_t type handling. Jan brought to my attention the fact that this sequence number may wrap, and I quickly found a few places in the code where the tid_geq() and tid_gt() helpers had to be used.
Again, please note that this fix requires [1] to be applied too.
[1] https://lore.kernel.org/all/[email protected]
Luis Henriques (SUSE) (2):
ext4: fix fast commit inode enqueueing during a full journal commit
ext4: fix possible tid_t sequence overflows
fs/ext4/fast_commit.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
PATCH v3 1/2 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.
--- fs/ext4/fast_commit.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
fs/ext4/fast_commit.c +10 -1
@@ -1282,8 +1282,17 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) +if (iter->i_sync_tid <= tid) {ext4_fc_reset_inode (&iter->vfs_inode); + }else { +/* +* re-enqueue inode into STAGING, which later will 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 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 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.
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]>
Looks good to me! Feel free to add:
Just a typo correction below.
Quoted 5 lines
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 1290
Quoted 10 lines
fs/ext4/fast_commit.c +5
-1@@ -1282,8 +1282,17 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) +if (iter->i_sync_tid <= tid) {ext4_fc_reset_inode (&iter->vfs_inode); + }else { +/* +* re-enqueue inode into STAGING, which later will be +* splice back into MAIN
^^^ spliced
fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 5 lines
fs/ext4/fast_commit.c +5
-0+*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + } +
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
harshad shirwadkar
Reviewed-byLooks good!
Quoted 55 lines · Jan Kara
Quoted 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 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.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]>Looks good to me! Feel free to add:
Reviewed-by Jan Kara <[email protected]>Just a typo correction below.
Quoted 5 lines
fs/ext4/fast_commit.c +0
-0fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1290
Quoted 10 lines
fs/ext4/fast_commit.c +5
-1@@ -1282,8 +1282,17 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) +if (iter->i_sync_tid <= tid) {ext4_fc_reset_inode (&iter->vfs_inode); + }else { +/* +* re-enqueue inode into STAGING, which later will be +* splice back into MAIN ^^^ splicedfs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1295
Quoted 5 lines
fs/ext4/fast_commit.c +5
-0+*/ +list_add_tail (&EXT4_I (&iter->vfs_inode)->i_fc_list, + &sbi->s_fc_q[FC_Q_STAGING ]); + } +Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
Theodore Ts'o
maintainerQuoted 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 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.
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]>
This patch is causing a regression for the test generic/472 generic/496 generic/643 if fast_commit is enabled. So using the ext4/adv or ext4/fast_commit configuration, e.g:
% kvm-xfstests -c ext4/fast_commit generic/472 generic/496 generic/643
For all of these test, the failures seem to involve the swapon command erroring out:
--- tests/generic/496.out 2024-06-13 18:57:39.000000000 -0400
+++ /results/ext4/results-fast_commit/generic/496.out.bad 2024-07-08 23:46:39.720
@@ -1,3 +1,4 @@
QA output created by 496
fallocate swap
mixed swap
+swapon: Invalid argument
...but it's unclear why this patch would affect swapon.
I've never been able to see generic/047 failure in any of my ext4/dev testing, nor in any of my daily fs-next CI testing. So for that reason, I'm going to drop this patch from my tree.
The second patch in this series appears to be independent at least from a logical perspective --- although a minor change is needed to resolve a merge conflict after dropping this change.
Luis, Harshad, could you look in this failure and then resubmit once it's been fixed? Thanks!! Also, Luis, can you give more details about the generic/047 failure that you had seen? Is it one of those flaky tests where you need to run the test dozens or hundreds of time to see the failure?
Many thanks!!
- Ted
Quoted 39 lines · Theodore Ts'o
Quoted 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 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.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]>This patch is causing a regression for the test generic/472 generic/496 generic/643 if
fast_commitis enabled. So using the ext4/adv or ext4/fast_commit configuration, e.g:% kvm-xfstests -c ext4/fast_commit generic/472 generic/496 generic/643
For all of these test, the failures seem to involve the swapon command erroring out:
--- tests/generic/496.out 2024-06-13 18:57:39.000000000 -0400 +++ /results/ext4/results-fast_commit/generic/496.out.bad 2024-07-08 23:46:39.720 @@ -1,3 +1,4 @@ QA output created by 496 fallocate swap mixed swap +swapon: Invalid argument ...
Quoted 1 line
but it's unclear why this patch would affect swapon.
OK, that's... embarrassing. I should have caught these failures :-(
Quoted 3 lines
I've never been able to see generic/047 failure in any of my ext4/dev testing, nor in any of my daily fs-next CI testing. So for that reason, I'm going to drop this patch from my tree.
There's nothing special about my test environment. I can reproduce the generic/047 failure (although not 100% of the times) by running it manually in a virtme-ng test environment, using MKFS_OPTIONS="-O fast_commit". Here's what I see when running it:
FSTYP -- ext4 PLATFORM -- Linux/x86_64 virtme-ng 6.10.0-rc7+ #269 SMP PREEMPT_DYNAMIC Tue Jul 9 14:24:22 WEST 2024 MKFS_OPTIONS -- -F -O fast_commit /dev/vdb1
MOUNT_OPTIONS -- -o acl,user_xattr /dev/vdb1 /tmp/mnt/scratch
generic/047 162s ... - output mismatch (see [...]/testing/xfstests-dev/results//generic/047.out.bad)
--- tests/generic/047.out 2021-01-11 12:08:14.972458324 +0000
+++ [...]/testing/xfstests-dev/results//generic/047.out.bad 2024-07-09 14:28:36.626435948 +0100
@@ -1 +1,2 @@
QA output created by 047
+file /tmp/mnt/scratch/944 has incorrect size - fsync failed
...
(Run 'diff -u [...]/testing/xfstests-dev/tests/generic/047.out [...]/testing/xfstests-dev/results//generic/047.out.bad' to see the entire diff)Ran: generic/047
Failures: generic/047
Failed 1 of 1 tests
Quoted 9 lines
The second patch in this series appears to be independent at least from a logical perspective --- although a minor change is needed to resolve a merge conflict after dropping this change.
Luis, Harshad, could you look in this failure and then resubmit once it's been fixed? Thanks!! Also, Luis, can you give more details about the generic/047 failure that you had seen? Is it one of those flaky tests where you need to run the test dozens or hundreds of time to see the failure?
So, I've done some quick tests, but I'll need some more time to dig into it. And this is what I _think_ it's happening:
When activating a swap file, the kernel forces an fsync, calling ext4_sync_file() which will then call ext4_fc_commit() and, eventually, the ext4_fc_cleanup().
With this patch an inode may be re-enqueued into the STAGING queue and then spliced back into MAIN; and that's exactly what I see happening.
Later, still on the swap activation path, ext4_set_iomap() will be called and will do this:
if (ext4_inode_datasync_dirty(inode) || offset + length > i_size_read(inode)) iomap->flags |= IOMAP_F_DIRTY;
'ext4_inode_datasync_dirty()' will be true because '->i_fc_list' is not empty. And that's why the swapoff will fail.
Anyway, I'll try to figure out what's missing here (or what's wrong with my patch).
Cheers,
Signature · 1 line
Luís
Quoted 99 lines · Luis Henriques
Quoted 39 lines · Theodore Ts'o
Quoted 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 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.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]>This patch is causing a regression for the test generic/472 generic/496 generic/643 if
fast_commitis enabled. So using the ext4/adv or ext4/fast_commit configuration, e.g:% kvm-xfstests -c ext4/fast_commit generic/472 generic/496 generic/643
For all of these test, the failures seem to involve the swapon command erroring out:
--- tests/generic/496.out 2024-06-13 18:57:39.000000000 -0400 +++ /results/ext4/results-fast_commit/generic/496.out.bad 2024-07-08 23:46:39.720 @@ -1,3 +1,4 @@ QA output created by 496 fallocate swap mixed swap +swapon: Invalid argument ...Quoted 1 line
but it's unclear why this patch would affect swapon.
OK, that's... embarrassing. I should have caught these failures :-(
Quoted 3 lines
I've never been able to see generic/047 failure in any of my ext4/dev testing, nor in any of my daily fs-next CI testing. So for that reason, I'm going to drop this patch from my tree.
There's nothing special about my test environment. I can reproduce the generic/047 failure (although not 100% of the times) by running it manually in a virtme-ng test environment, using MKFS_OPTIONS="-O
fast_commit". Here's what I see when running it:FSTYP -- ext4 PLATFORM -- Linux/x86_64 virtme-ng 6.10.0-rc7+ #269 SMP PREEMPT_DYNAMIC Tue Jul 9 14:24:22 WEST 2024 MKFS_OPTIONS -- -F -O fast_commit /dev/vdb1
MOUNT_OPTIONS-- -o acl,user_xattr/dev/vdb1/tmp/mnt/scratchgeneric/047 162s ... - output mismatch (see [...]/testing/xfstests-dev/results//generic/047.out.bad)
--- tests/generic/047.out 2021-01-11 12:08:14.972458324 +0000 +++ [...]/testing/xfstests-dev/results//generic/047.out.bad 2024-07-09 14:28:36.626435948 +0100 @@ -1 +1,2 @@ QA output created by 047 +file /tmp/mnt/scratch/944 has incorrect size - fsync failed ... (Run 'diff -u [...]/testing/xfstests-dev/tests/generic/047.out [...]/testing/xfstests-dev/results//generic/047.out.bad' to see the entire diff)Ran: generic/047
Failures: generic/047
Failed 1 of 1 testsQuoted 9 lines
The second patch in this series appears to be independent at least from a logical perspective --- although a minor change is needed to resolve a merge conflict after dropping this change.
Luis, Harshad, could you look in this failure and then resubmit once it's been fixed? Thanks!! Also, Luis, can you give more details about the generic/047 failure that you had seen? Is it one of those flaky tests where you need to run the test dozens or hundreds of time to see the failure?
So, I've done some quick tests, but I'll need some more time to dig into it. And this is what I _think_ it's happening:
When activating a swap file, the kernel forces an fsync, calling
ext4_sync_file()which will then callext4_fc_commit()and, eventually, theext4_fc_cleanup().With this patch an inode may be re-enqueued into the STAGING queue and then spliced back into MAIN; and that's exactly what I see happening.
Later, still on the swap activation path,
ext4_set_iomap()will be called and will do this:if (ext4_inode_datasync_dirty(inode) || offset + length > i_size_read(inode)) iomap->flags |= IOMAP_F_DIRTY;
Quoted 5 lines
'
ext4_inode_datasync_dirty()' will be true because '->i_fc_list' is not empty. And that's why the swapoff will fail.Anyway, I'll try to figure out what's missing here (or what's wrong with my patch).
I believe I found the issue with the patch. The ext4_fc_cleanup() callback can be invoked in three different situations:
1) when there's a full commit
2) when there's a fc commit but with fallback to full commit
3) when there's a fc commit
For both 1) and 2) the cleanup callback will get a real 'tid' value; however, for the regular fc commit 3) the 'tid' will be 0. And for those cases the inode should not be enqueued back in enqueued into STAGING. See below an updated diff with this fix.
Does this make sense? I'll run a few more tests before sending a v4.
Cheers
Signature · 1 line
Luís
fs/ext4/fast_commit.c +10 -1
@@ -1282,8 +1282,17 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) +if (iter->i_sync_tid <= tid) {ext4_fc_reset_inode (&iter->vfs_inode); + }else if (tid) { +/* +* re-enqueue inode into STAGING, which later will 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 )
PATCH v3 2/2 ext4: fix possible tid_t sequence overflows
In reply to Luis Henriques (SUSE)In the fast commit code there are a few places where tid_t variables are being compared without taking into account the fact that these sequence numbers may wrap. Fix this issue by using the helper functions tid_gt() and tid_geq().
--- fs/ext4/fast_commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
fs/ext4/fast_commit.c +4 -4
@@ -353,7 +353,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handlread_unlock (&sbi->s_journal->j_state_lock); }spin_lock (&sbi->s_fc_lock); -if (sbi->s_fc_ineligible_tid < tid) +if (tid_gt (tid, sbi->s_fc_ineligible_tid)) sbi->s_fc_ineligible_tid = tid;spin_unlock (&sbi->s_fc_lock);WARN_ON (reason >=EXT4_FC_REASON_MAX );
@@ -1207,7 +1207,7 @@ int ext4_fc_commit(journal_t *journal, tid_t commit_tid)if (ret == -EALREADY ) {/* There was an ongoing commit, check if we need to restart */ if (atomic_read (&sbi->s_fc_subtid) <= subtid && - commit_tid > journal->j_commit_sequence) +tid_gt (commit_tid, journal->j_commit_sequence)) goto restart_fc;ext4_fc_update_stats (sb,EXT4_FC_STATUS_SKIPPED ,0 ,0 , commit_tid);
@@ -1282,7 +1282,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) { +if (tid_geq (tid, iter->i_sync_tid)) {ext4_fc_reset_inode (&iter->vfs_inode); }else {/*
@@ -1322,7 +1322,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_splice_init (&sbi->s_fc_q[FC_Q_STAGING ], &sbi->s_fc_q[FC_Q_MAIN ]); -if (tid >= sbi->s_fc_ineligible_tid) { +if (tid_geq (tid, sbi->s_fc_ineligible_tid)) { sbi->s_fc_ineligible_tid =0 ;ext4_clear_mount_flag (sb,EXT4_MF_FC_INELIGIBLE ); }
Re: [PATCH v3 2/2] ext4: fix possible tid_t sequence overflows
Quoted 6 lines · Luis Henriques (SUSE)
In the fast commit code there are a few places where
tid_tvariables are being compared without taking into account the fact that these sequence numbers may wrap. Fix this issue by using the helper functionstid_gt()andtid_geq().Signed-off-by Luis Henriques (SUSE) <[email protected]>
Thanks! Feel free to add:
Honza
Quoted 45 lines
--- fs/ext4/fast_commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/ext4/fast_commit.c +4
-4@@ -353,7 +353,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handlread_unlock (&sbi->s_journal->j_state_lock); }spin_lock (&sbi->s_fc_lock); -if (sbi->s_fc_ineligible_tid < tid) +if (tid_gt (tid, sbi->s_fc_ineligible_tid)) sbi->s_fc_ineligible_tid = tid;spin_unlock (&sbi->s_fc_lock);WARN_ON (reason >=EXT4_FC_REASON_MAX );@@ -1207,7 +1207,7 @@ int ext4_fc_commit(journal_t *journal, tid_t commit_tid)if (ret == -EALREADY ) {/* There was an ongoing commit, check if we need to restart */ if (atomic_read (&sbi->s_fc_subtid) <= subtid && - commit_tid > journal->j_commit_sequence) +tid_gt (commit_tid, journal->j_commit_sequence)) goto restart_fc;ext4_fc_update_stats (sb,EXT4_FC_STATUS_SKIPPED ,0 ,0 , commit_tid);@@ -1282,7 +1282,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) { +if (tid_geq (tid, iter->i_sync_tid)) {ext4_fc_reset_inode (&iter->vfs_inode); }else {/* @@ -1322,7 +1322,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_splice_init (&sbi->s_fc_q[FC_Q_STAGING ], &sbi->s_fc_q[FC_Q_MAIN ]); -if (tid >= sbi->s_fc_ineligible_tid) { +if (tid_geq (tid, sbi->s_fc_ineligible_tid)) { sbi->s_fc_ineligible_tid =0 ;ext4_clear_mount_flag (sb,EXT4_MF_FC_INELIGIBLE ); }
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
harshad shirwadkar
Reviewed-byRe: [PATCH v3 2/2] ext4: fix possible tid_t sequence overflows
Looks good, thanks for the patch!
Quoted 63 lines · Jan Kara
Quoted 6 lines · Luis Henriques (SUSE)
In the fast commit code there are a few places where
tid_tvariables are being compared without taking into account the fact that these sequence numbers may wrap. Fix this issue by using the helper functionstid_gt()andtid_geq().Signed-off-by Luis Henriques (SUSE) <[email protected]>Thanks! Feel free to add:
Reviewed-by Jan Kara <[email protected]>Honza
Quoted 37 lines
--- fs/ext4/fast_commit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/ext4/fast_commit.c +3
-3@@ -353,7 +353,7 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handlread_unlock (&sbi->s_journal->j_state_lock); }spin_lock (&sbi->s_fc_lock); -if (sbi->s_fc_ineligible_tid < tid) +if (tid_gt (tid, sbi->s_fc_ineligible_tid)) sbi->s_fc_ineligible_tid = tid;spin_unlock (&sbi->s_fc_lock);WARN_ON (reason >=EXT4_FC_REASON_MAX );@@ -1207,7 +1207,7 @@ int ext4_fc_commit(journal_t *journal, tid_t commit_tid)if (ret == -EALREADY ) {/* There was an ongoing commit, check if we need to restart */ if (atomic_read (&sbi->s_fc_subtid) <= subtid && - commit_tid > journal->j_commit_sequence) +tid_gt (commit_tid, journal->j_commit_sequence)) goto restart_fc;ext4_fc_update_stats (sb,EXT4_FC_STATUS_SKIPPED ,0 ,0 , commit_tid);@@ -1282,7 +1282,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_del_init (&iter->i_fc_list);ext4_clear_inode_state (&iter->vfs_inode,EXT4_STATE_FC_COMMITTING ); -if (iter->i_sync_tid <= tid) { +if (tid_geq (tid, iter->i_sync_tid)) {ext4_fc_reset_inode (&iter->vfs_inode); }else {/* fs/ext4/fast_commit.c static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid) · line 1328
Quoted 8 lines
fs/ext4/fast_commit.c +1
-1@@ -1322,7 +1322,7 @@ static void ext4_fc_cleanup(journal_t *journal, int full, tid_t tid)list_splice_init (&sbi->s_fc_q[FC_Q_STAGING ], &sbi->s_fc_q[FC_Q_MAIN ]); -if (tid >= sbi->s_fc_ineligible_tid) { +if (tid_geq (tid, sbi->s_fc_ineligible_tid)) { sbi->s_fc_ineligible_tid =0 ;ext4_clear_mount_flag (sb,EXT4_MF_FC_INELIGIBLE ); }--
Jan Kara <[email protected]>
SUSE Labs, CR
Quoted 11 lines · Luis Henriques (SUSE)
Hi!
Here's v3 of this fix to the fast commit enqueuing bug triggered by fstest generic/047. This version simplifies the previous patch version by re-using the
i_sync_tidfield in structext4_inode_infoinstead of adding a new one.The extra patch includes a few extra fixes to the
tid_ttype handling. Jan brought to my attention the fact that this sequence number may wrap, and I quickly found a few places in the code where thetid_geq()andtid_gt()helpers had to be used.
Quoted 7 lines
Again, please note that this fix requires [1] to be applied too.
[1] https://lore.kernel.org/all/[email protected]
Luis Henriques (SUSE) (2):
ext4: fix fast commit inode enqueueing during a full journal commit
ext4: fix possibletid_tsequence overflows
Gentle ping... Has this fell through the cracks?
Cheers,
Signature · 1 line
Luis
Quoted 4 lines
fs/ext4/fast_commit.c| 17 +++++++++++++----1 file changed, 13 insertions(+), 4 deletions(-)
Theodore Ts'o
maintainerQuoted 15 lines · Luis Henriques
Quoted 13 lines · Luis Henriques (SUSE)
Hi!
Here's v3 of this fix to the fast commit enqueuing bug triggered by fstest generic/047. This version simplifies the previous patch version by re-using the
i_sync_tidfield in structext4_inode_infoinstead of adding a new one.The extra patch includes a few extra fixes to the
tid_ttype handling. Jan brought to my attention the fact that this sequence number may wrap, and I quickly found a few places in the code where thetid_geq()andtid_gt()helpers had to be used.Again, please note that this fix requires [1] to be applied too.
Quoted 7 lines
Quoted 5 lines
[1] https://lore.kernel.org/all/[email protected]
Luis Henriques (SUSE) (2):
ext4: fix fast commit inode enqueueing during a full journal commit
ext4: fix possibletid_tsequence overflowsGentle ping... Has this fell through the cracks?
Sorry, I'm still catching up after being on vacation. There is a batch of commits which I've reviewed (up to May 17th) which is currently undergoing testing. So that doesn't include this patch yet, but it's on the list of patches to be reviewed at patchworks.ozlabs.org/project/linux-ext4 so it won't fall through the cracks.
- Ted
Quoted 22 lines · Theodore Ts'o
Quoted 21 lines · Luis Henriques
Quoted 18 lines · Luis Henriques (SUSE)
Hi!
Here's v3 of this fix to the fast commit enqueuing bug triggered by fstest generic/047. This version simplifies the previous patch version by re-using the
i_sync_tidfield in structext4_inode_infoinstead of adding a new one.The extra patch includes a few extra fixes to the
tid_ttype handling. Jan brought to my attention the fact that this sequence number may wrap, and I quickly found a few places in the code where thetid_geq()andtid_gt()helpers had to be used.Again, please note that this fix requires [1] to be applied too.
[1] https://lore.kernel.org/all/[email protected]
Luis Henriques (SUSE) (2):
ext4: fix fast commit inode enqueueing during a full journal commit
ext4: fix possibletid_tsequence overflows
Quoted 8 lines
Quoted 1 line
Gentle ping... Has this fell through the cracks?
Sorry, I'm still catching up after being on vacation. There is a batch of commits which I've reviewed (up to May 17th) which is currently undergoing testing. So that doesn't include this patch yet, but it's on the list of patches to be reviewed at patchworks.ozlabs.org/project/linux-ext4 so it won't fall through the cracks.
Awesome, thanks for the update. And sorry for being impatient. /me goes back under his rock.
Cheers,
Signature · 1 line
Luís
Theodore Ts'o
maintainerQuoted 10 lines · Luis Henriques (SUSE)
Here's v3 of this fix to the fast commit enqueuing bug triggered by fstest generic/047. This version simplifies the previous patch version by re-using the
i_sync_tidfield in structext4_inode_infoinstead of adding a new one.The extra patch includes a few extra fixes to the
tid_ttype handling. Jan brought to my attention the fact that this sequence number may wrap, and I quickly found a few places in the code where thetid_geq()andtid_gt()helpers had to be used.[...]
The second patch in this series was applied:
[2/2] ext4: fix possible tid_t sequence overflows
commit: 63469662cc45d41705f14b4648481d5d29cf5999
Best regards,
Signature · 1 line
Theodore Ts'o <[email protected]>