ext4: fix incorrect tid assumptions
Subsystems (2)
cover ext4: fix incorrect tid assumptions
As discussed here [1], there are a few places in ext4 and jbd2 code where it is assumed that a tid of '0' is not valid. Which isn't true.
This small patchset tries to fix (hopefully!) all these places. Jan Kara had already identified the functions that needed to be fixed. I believe that the only other issue is the handling of sbi->s_fc_ineligible_tid.
Each patch in this series fixes a single function; the last one also fixes the sbi->s_fc_ineligible_tid handling.
[1] https://lore.kernel.org/all/20240716095201.o7kkrhfdy2bps3rw@quack3/
Luis Henriques (SUSE) (4):
ext4: fix incorrect tid assumption in ext4_wait_for_tail_page_commit()
ext4: fix incorrect tid assumption in __jbd2_log_wait_for_space()
ext4: fix incorrect tid assumption in
jbd2_journal_shrink_checkpoint_list()
ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()
fs/ext4/fast_commit.c | 15 +++++++++++---- fs/ext4/inode.c | 10 ++++++---- fs/jbd2/checkpoint.c | 15 +++++++++++---- 3 files changed, 28 insertions(+), 12 deletions(-)
PATCH 1/4 ext4: fix incorrect tid assumption in ext4_wait_for_tail_page_commit()
Function ext4_wait_for_tail_page_commit() assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and invoke jbd2_log_wait_commit() if the journal had a committing transaction instead.
--- fs/ext4/inode.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
fs/ext4/inode.c +6 -4
@@ -5279,8 +5279,9 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode) {unsigned offset;journal_t *journal =EXT4_SB (inode->i_sb)->s_journal; -tid_t commit_tid =0 ; +tid_t commit_tid;int ret; +bool has_transaction = false; offset = inode->i_size & (PAGE_SIZE -1 );/*
@@ -5305,12 +5306,13 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)folio_put (folio);if (ret != -EBUSY )return ; - commit_tid =0 ;read_lock (&journal->j_state_lock); -if (journal->j_committing_transaction) +if (journal->j_committing_transaction) { commit_tid = journal->j_committing_transaction->t_tid; + has_transaction = true; + }read_unlock (&journal->j_state_lock); -if (commit_tid) +if (has_transaction)jbd2_log_wait_commit (journal, commit_tid); } }
Jan Kara
reviewerRe: [PATCH 1/4] ext4: fix incorrect tid assumption in ext4_wait_for_tail_page_commit()
Quoted 25 lines · Luis Henriques (SUSE)
Function
ext4_wait_for_tail_page_commit()assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and invokejbd2_log_wait_commit()if the journal had a committing transaction instead.Signed-off-by Luis Henriques (SUSE) <[email protected]>--- fs/ext4/inode.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)fs/ext4/inode.c +2
-1@@ -5279,8 +5279,9 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode) {unsigned offset;journal_t *journal =EXT4_SB (inode->i_sb)->s_journal; -tid_t commit_tid =0 ; +tid_t commit_tid;int ret; +bool has_transaction = false; offset = inode->i_size & (PAGE_SIZE -1 );/*
fs/ext4/inode.c static void ext4_wait_for_tail_page_commit(struct inode *inode) · line 5308
Quoted 4 lines
fs/ext4/inode.c +0
-1@@ -5305,12 +5306,13 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)folio_put (folio);if (ret != -EBUSY )return ; - commit_tid =0 ;
We should set "has_transaction = false" here to make things work properly when looping... Otherwise looks good.
Honza
Quoted 13 lines
fs/ext4/inode.c +4
-2read_lock (&journal->j_state_lock); -if (journal->j_committing_transaction) +if (journal->j_committing_transaction) { commit_tid = journal->j_committing_transaction->t_tid; + has_transaction = true; + }read_unlock (&journal->j_state_lock); -if (commit_tid) +if (has_transaction)jbd2_log_wait_commit (journal, commit_tid); } }
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 1/4] ext4: fix incorrect tid assumption in ext4_wait_for_tail_page_commit()
Quoted 23 lines · Jan Kara
Quoted 22 lines · Luis Henriques (SUSE)
Function
ext4_wait_for_tail_page_commit()assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and invokejbd2_log_wait_commit()if the journal had a committing transaction instead.Signed-off-by Luis Henriques (SUSE) <[email protected]>--- fs/ext4/inode.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)fs/ext4/inode.c +2
-1@@ -5279,8 +5279,9 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode) {unsigned offset;journal_t *journal =EXT4_SB (inode->i_sb)->s_journal; -tid_t commit_tid =0 ; +tid_t commit_tid;int ret; +bool has_transaction = false;
Quoted 10 lines
fs/ext4/inode.c static void ext4_wait_for_tail_page_commit(struct inode *inode) · line 5308
Quoted 7 lines
fs/ext4/inode.c +0
-1offset = inode->i_size & (PAGE_SIZE -1 );/* @@ -5305,12 +5306,13 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)folio_put (folio);if (ret != -EBUSY )return ; - commit_tid =0 ;We should set "
has_transaction= false" here to make things work properly when looping... Otherwise looks good.
Ah! Good point. I'll fix that, thanks!
Cheers,
Signature · 1 line
Luís
Quoted 19 lines
Honza
Quoted 13 lines
fs/ext4/inode.c +4
-2read_lock (&journal->j_state_lock); -if (journal->j_committing_transaction) +if (journal->j_committing_transaction) { commit_tid = journal->j_committing_transaction->t_tid; + has_transaction = true; + }read_unlock (&journal->j_state_lock); -if (commit_tid) +if (has_transaction)jbd2_log_wait_commit (journal, commit_tid); } }Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH 2/4 ext4: fix incorrect tid assumption in __jbd2_log_wait_for_space()
In reply to Luis Henriques (SUSE)Function __jbd2_log_wait_for_space() assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and invoke jbd2_log_wait_commit() if the journal had a committing transaction instead.
--- fs/jbd2/checkpoint.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
fs/jbd2/checkpoint.c +5 -2
@@ -79,9 +79,12 @@ __releases(&journal->j_state_lock)if (space_left < nblocks) {int chkpt = journal->j_checkpoint_transactions !=NULL ;tid_t tid =0 ; +bool has_transaction = false; -if (journal->j_committing_transaction) +if (journal->j_committing_transaction) { tid = journal->j_committing_transaction->t_tid; + has_transaction = true; + }spin_unlock (&journal->j_list_lock);write_unlock (&journal->j_state_lock);if (chkpt) {
@@ -89,7 +92,7 @@ __releases(&journal->j_state_lock) }else if (jbd2_cleanup_journal_tail (journal) ==0 ) {/* We were able to recover space; yay! */ ; - }else if (tid) { + }else if (has_transaction) {/* * jbd2_journal_commit_transaction() may want * to take the checkpoint_mutex if JBD2_FLUSHED
Re: [PATCH 2/4] ext4: fix incorrect tid assumption in __jbd2_log_wait_for_space()
Quoted 5 lines · Luis Henriques (SUSE)
Function
__jbd2_log_wait_for_space()assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and invokejbd2_log_wait_commit()if the journal had a committing transaction instead.Signed-off-by Luis Henriques (SUSE) <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 32 lines
--- fs/jbd2/checkpoint.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)fs/jbd2/checkpoint.c +5
-2@@ -79,9 +79,12 @@ __releases(&journal->j_state_lock)if (space_left < nblocks) {int chkpt = journal->j_checkpoint_transactions !=NULL ;tid_t tid =0 ; +bool has_transaction = false; -if (journal->j_committing_transaction) +if (journal->j_committing_transaction) { tid = journal->j_committing_transaction->t_tid; + has_transaction = true; + }spin_unlock (&journal->j_list_lock);write_unlock (&journal->j_state_lock);if (chkpt) {@@ -89,7 +92,7 @@ __releases(&journal->j_state_lock) }else if (jbd2_cleanup_journal_tail (journal) ==0 ) {/* We were able to recover space; yay! */ ; - }else if (tid) { + }else if (has_transaction) {/* * jbd2_journal_commit_transaction() may want * to take the checkpoint_mutex if JBD2_FLUSHED
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH 3/4 ext4: fix incorrect tid assumption in jbd2_journal_shrink_checkpoint_list()
In reply to Luis Henriques (SUSE)Function jbd2_journal_shrink_checkpoint_list() assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and use two extra boolean variables to control the loop iterations and keep track of the first and last tid.
--- fs/jbd2/checkpoint.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
fs/jbd2/checkpoint.c +6 -2
@@ -410,6 +410,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,tid_t tid =0 ;unsigned long nr_freed =0 ;unsigned long freed; +bool is_first = true, is_last = false; again:spin_lock (&journal->j_list_lock);
@@ -429,8 +430,10 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,else transaction = journal->j_checkpoint_transactions; -if (!first_tid) +if (is_first) { first_tid = transaction->t_tid; + is_first = false; + } last_transaction = journal->j_checkpoint_transactions->t_cpprev; next_transaction = transaction; last_tid = last_transaction->t_tid;
@@ -455,12 +458,13 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, }else { journal->j_shrink_transaction =NULL ; next_tid =0 ; + is_last = true; }spin_unlock (&journal->j_list_lock);cond_resched (); -if (*nr_to_scan && next_tid) +if (*nr_to_scan && !is_last) goto again; out: trace_jbd2_shrink_checkpoint_list(journal, first_tid, tid, last_tid,
Jan Kara
reviewerRe: [PATCH 3/4] ext4: fix incorrect tid assumption in jbd2_journal_shrink_checkpoint_list()
Quoted 36 lines · Luis Henriques (SUSE)
Function
jbd2_journal_shrink_checkpoint_list()assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and use two extra boolean variables to control the loop iterations and keep track of the first and last tid.Signed-off-by Luis Henriques (SUSE) <[email protected]>--- fs/jbd2/checkpoint.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)fs/jbd2/checkpoint.c +4
-1@@ -410,6 +410,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,tid_t tid =0 ;unsigned long nr_freed =0 ;unsigned long freed; +bool is_first = true, is_last = false; again:spin_lock (&journal->j_list_lock);@@ -429,8 +430,10 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,else transaction = journal->j_checkpoint_transactions; -if (!first_tid) +if (is_first) { first_tid = transaction->t_tid; + is_first = false; + } last_transaction = journal->j_checkpoint_transactions->t_cpprev; next_transaction = transaction; last_tid = last_transaction->t_tid;@@ -455,12 +458,13 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, }else {
fs/jbd2/checkpoint.c unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, · line 467
Quoted 10 lines
fs/jbd2/checkpoint.c +2
-1journal->j_shrink_transaction =NULL ; next_tid =0 ; + is_last = true; }spin_unlock (&journal->j_list_lock);cond_resched (); -if (*nr_to_scan && next_tid) +if (*nr_to_scan && !is_last)
I'd make this:
if (*nr_to_scan && journal->j_shrink_transaction) goto again;
and just remove is_last. Also we might rename is_first to first_set? At least to me it would be more comprehensible. Thanks!
Honza
Quoted 4 lines
fs/jbd2/checkpoint.c +0
-0goto again; out: trace_jbd2_shrink_checkpoint_list(journal, first_tid, tid, last_tid,
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 3/4] ext4: fix incorrect tid assumption in jbd2_journal_shrink_checkpoint_list()
Quoted 50 lines · Jan Kara
Quoted 36 lines · Luis Henriques (SUSE)
Function
jbd2_journal_shrink_checkpoint_list()assumes that '0' is not a valid value for transaction IDs, which is incorrect. Don't assume that and use two extra boolean variables to control the loop iterations and keep track of the first and last tid.Signed-off-by Luis Henriques (SUSE) <[email protected]>--- fs/jbd2/checkpoint.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)fs/jbd2/checkpoint.c +4
-1@@ -410,6 +410,7 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,tid_t tid =0 ;unsigned long nr_freed =0 ;unsigned long freed; +bool is_first = true, is_last = false; again:spin_lock (&journal->j_list_lock);@@ -429,8 +430,10 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal,else transaction = journal->j_checkpoint_transactions; -if (!first_tid) +if (is_first) { first_tid = transaction->t_tid; + is_first = false; + } last_transaction = journal->j_checkpoint_transactions->t_cpprev; next_transaction = transaction; last_tid = last_transaction->t_tid;@@ -455,12 +458,13 @@ unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, }else {fs/jbd2/checkpoint.c unsigned long jbd2_journal_shrink_checkpoint_list(journal_t *journal, · line 467
Quoted 10 lines
fs/jbd2/checkpoint.c +2
-1journal->j_shrink_transaction =NULL ; next_tid =0 ; + is_last = true; }spin_unlock (&journal->j_list_lock);cond_resched (); -if (*nr_to_scan && next_tid) +if (*nr_to_scan && !is_last)I'd make this:
Quoted 5 lines
if (*nr_to_scan && journal->j_shrink_transaction) goto again;and just remove
is_last. Also we might renameis_firsttofirst_set? At least to me it would be more comprehensible. Thanks!
Sure, both suggestions make sense. I'll update the patches for v2.
Cheers,
Signature · 1 line
Luís
Quoted 10 lines
Honza
Quoted 4 lines
fs/jbd2/checkpoint.c +0
-0goto again; out: trace_jbd2_shrink_checkpoint_list(journal, first_tid, tid, last_tid,Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH 4/4 ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()
In reply to Luis Henriques (SUSE)Function jbd2_journal_shrink_checkpoint_list() assumes that '0' is not a valid value for transaction IDs, which is incorrect.
Furthermore, the sbi->s_fc_ineligible_tid handling also makes the same assumption by being initialised to '0'. Fortunately, the sb flag EXT4_MF_FC_INELIGIBLE can be used to check whether sbi->s_fc_ineligible_tid has been previously set instead of comparing it with '0'.
--- fs/ext4/fast_commit.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
fs/ext4/fast_commit.c +11 -4
@@ -339,22 +339,29 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl {struct ext4_sb_info *sbi =EXT4_SB (sb);tid_t tid; +bool has_transaction = true; +bool is_ineligible;if (ext4_fc_disabled (sb))return ; -ext4_set_mount_flag (sb,EXT4_MF_FC_INELIGIBLE );if (handle && !IS_ERR (handle)) tid = handle->h_transaction->t_tid;else {read_lock (&sbi->s_journal->j_state_lock); - tid = sbi->s_journal->j_running_transaction ? - sbi->s_journal->j_running_transaction->t_tid :0 ; +if (sbi->s_journal->j_running_transaction) + tid = sbi->s_journal->j_running_transaction->t_tid; +else + has_transaction = false;read_unlock (&sbi->s_journal->j_state_lock); }spin_lock (&sbi->s_fc_lock); -if (tid_gt (tid, sbi->s_fc_ineligible_tid)) + is_ineligible =ext4_test_mount_flag (sb,EXT4_MF_FC_INELIGIBLE ); +if (has_transaction && + ((!is_ineligible) || + (is_ineligible &&tid_gt (tid, sbi->s_fc_ineligible_tid)))) sbi->s_fc_ineligible_tid = tid; +ext4_set_mount_flag (sb,EXT4_MF_FC_INELIGIBLE );spin_unlock (&sbi->s_fc_lock);WARN_ON (reason >=EXT4_FC_REASON_MAX ); sbi->s_fc_stats.fc_ineligible_reason_count[reason]++;
Re: [PATCH 4/4] ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()
Quoted 9 lines · Luis Henriques (SUSE)
Function
jbd2_journal_shrink_checkpoint_list()assumes that '0' is not a valid value for transaction IDs, which is incorrect.Furthermore, the
sbi->s_fc_ineligible_tidhandling also makes the same assumption by being initialised to '0'. Fortunately, the sb flagEXT4_MF_FC_INELIGIBLEcan be used to check whethersbi->s_fc_ineligible_tidhas been previously set instead of comparing it with '0'.Signed-off-by Luis Henriques (SUSE) <[email protected]>
Just one style nit below, otherwise looks good. Feel free to add:
BTW, the ineligibility handling looks flaky to me, in particular the cases where we call ext4_fc_mark_ineligible() with NULL handle seem racy to me as fastcommit can happen *before* we mark the filesystem as ineligible. But that's not really related to your changes, they just made me look at that code in detail and I couldn't resist complaining :)
Quoted 26 lines
--- fs/ext4/fast_commit.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)fs/ext4/fast_commit.c +3
-3@@ -339,22 +339,29 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl {struct ext4_sb_info *sbi =EXT4_SB (sb);tid_t tid; +bool has_transaction = true; +bool is_ineligible;if (ext4_fc_disabled (sb))return ; -ext4_set_mount_flag (sb,EXT4_MF_FC_INELIGIBLE );if (handle && !IS_ERR (handle)) tid = handle->h_transaction->t_tid;else {read_lock (&sbi->s_journal->j_state_lock); - tid = sbi->s_journal->j_running_transaction ? - sbi->s_journal->j_running_transaction->t_tid :0 ; +if (sbi->s_journal->j_running_transaction)
fs/ext4/fast_commit.c void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl · line 361
Quoted 10 lines
fs/ext4/fast_commit.c +6
-1+ tid = sbi->s_journal->j_running_transaction->t_tid; + else + has_transaction = false;read_unlock (&sbi->s_journal->j_state_lock); }spin_lock (&sbi->s_fc_lock); -if (tid_gt (tid, sbi->s_fc_ineligible_tid)) + is_ineligible =ext4_test_mount_flag (sb,EXT4_MF_FC_INELIGIBLE ); + if (has_transaction && + ((!is_ineligible) ||
^^ these extra braces look strange
fs/ext4/fast_commit.c void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl · line 364
Quoted 3 lines
fs/ext4/fast_commit.c +2
-0+ (is_ineligible && tid_gt(tid, sbi->s_fc_ineligible_tid)))) sbi->s_fc_ineligible_tid = tid; +ext4_set_mount_flag (sb,EXT4_MF_FC_INELIGIBLE );
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 4/4] ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()
Quoted 13 lines · Jan Kara
Quoted 9 lines · Luis Henriques (SUSE)
Function
jbd2_journal_shrink_checkpoint_list()assumes that '0' is not a valid value for transaction IDs, which is incorrect.Furthermore, the
sbi->s_fc_ineligible_tidhandling also makes the same assumption by being initialised to '0'. Fortunately, the sb flagEXT4_MF_FC_INELIGIBLEcan be used to check whethersbi->s_fc_ineligible_tidhas been previously set instead of comparing it with '0'.Signed-off-by Luis Henriques (SUSE) <[email protected]>Just one style nit below, otherwise looks good. Feel free to add:
Quoted 7 lines
Reviewed-by Jan Kara <[email protected]>BTW, the ineligibility handling looks flaky to me, in particular the cases where we call
ext4_fc_mark_ineligible()with NULL handle seem racy to me as fastcommit can happen *before* we mark the filesystem as ineligible. But that's not really related to your changes, they just made me look at that code in detail and I couldn't resist complaining :)
Heh, fair enough. Regarding this race, I may try to look into it but I'll need to dig a bit more. And yeah it's probably better to separate that from this patch.
Quoted 30 lines
Quoted 30 lines
--- fs/ext4/fast_commit.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)fs/ext4/fast_commit.c +6
-3@@ -339,22 +339,29 @@ void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl {struct ext4_sb_info *sbi =EXT4_SB (sb);tid_t tid; +bool has_transaction = true; +bool is_ineligible;if (ext4_fc_disabled (sb))return ; -ext4_set_mount_flag (sb,EXT4_MF_FC_INELIGIBLE );if (handle && !IS_ERR (handle)) tid = handle->h_transaction->t_tid;else {read_lock (&sbi->s_journal->j_state_lock); - tid = sbi->s_journal->j_running_transaction ? - sbi->s_journal->j_running_transaction->t_tid :0 ; +if (sbi->s_journal->j_running_transaction) + tid = sbi->s_journal->j_running_transaction->t_tid; +else + has_transaction = false;
Quoted 9 lines
fs/ext4/fast_commit.c void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl · line 361
Quoted 7 lines
fs/ext4/fast_commit.c +3
-1read_unlock (&sbi->s_journal->j_state_lock); }spin_lock (&sbi->s_fc_lock); -if (tid_gt (tid, sbi->s_fc_ineligible_tid)) + is_ineligible =ext4_test_mount_flag (sb,EXT4_MF_FC_INELIGIBLE ); + if (has_transaction && + ((!is_ineligible) ||^^ these extra braces look strange
They do, indeed. I think my initial version had an explicit comparison with 'false'. v2 will have those removed. And once again, thanks for your review, Jan!
Cheers,
Signature · 1 line
Luís
Quoted 8 lines
fs/ext4/fast_commit.c void ext4_fc_mark_ineligible(struct super_block *sb, int reason, handle_t *handl · line 364
Quoted 3 lines
fs/ext4/fast_commit.c +2
-0+ (is_ineligible && tid_gt(tid, sbi->s_fc_ineligible_tid)))) sbi->s_fc_ineligible_tid = tid; +ext4_set_mount_flag (sb,EXT4_MF_FC_INELIGIBLE );Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jan Kara
reviewerRe: [PATCH 4/4] ext4: fix incorrect tid assumption in ext4_fc_mark_ineligible()
Quoted 23 lines · Luis Henriques
Quoted 13 lines · Jan Kara
Quoted 9 lines · Luis Henriques (SUSE)
Function
jbd2_journal_shrink_checkpoint_list()assumes that '0' is not a valid value for transaction IDs, which is incorrect.Furthermore, the
sbi->s_fc_ineligible_tidhandling also makes the same assumption by being initialised to '0'. Fortunately, the sb flagEXT4_MF_FC_INELIGIBLEcan be used to check whethersbi->s_fc_ineligible_tidhas been previously set instead of comparing it with '0'.Signed-off-by Luis Henriques (SUSE) <[email protected]>Just one style nit below, otherwise looks good. Feel free to add:
Quoted 7 lines
Reviewed-by Jan Kara <[email protected]>BTW, the ineligibility handling looks flaky to me, in particular the cases where we call
ext4_fc_mark_ineligible()with NULL handle seem racy to me as fastcommit can happen *before* we mark the filesystem as ineligible. But that's not really related to your changes, they just made me look at that code in detail and I couldn't resist complaining :)
Quoted 3 lines
Heh, fair enough. Regarding this race, I may try to look into it but I'll need to dig a bit more. And yeah it's probably better to separate that from this patch.
I suspect all the places that mark the fs as ineligible with NULL handle need to actually mark corresponding transactions as ineligible using handle instead. This is going to require a bit of churn e.g. for stuff like resize or __track_dentry_update() but shouldn't be hard to do.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR