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'.
Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
---
fs/ext4/fast_commit.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 3926a05eceee..3e0793cfea38 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -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]++;
On Tue 23-07-24 16:44:02, Luis Henriques (SUSE) wrote: > 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'. > > Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev> Just one style nit below, otherwise looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> 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 :) > --- > fs/ext4/fast_commit.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c > index 3926a05eceee..3e0793cfea38 100644 > --- a/fs/ext4/fast_commit.c > +++ b/fs/ext4/fast_commit.c > @@ -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) || ^^ these extra braces look strange > + (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 -- Jan Kara <jack@suse.com> SUSE Labs, CR
On Wed, Jul 24 2024, Jan Kara wrote: > On Tue 23-07-24 16:44:02, Luis Henriques (SUSE) wrote: >> 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'. >> >> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev> > > Just one style nit below, otherwise looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > 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. > >> --- >> fs/ext4/fast_commit.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c >> index 3926a05eceee..3e0793cfea38 100644 >> --- a/fs/ext4/fast_commit.c >> +++ b/fs/ext4/fast_commit.c >> @@ -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) || > ^^ 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, -- Luís >> + (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 > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Wed 24-07-24 15:02:49, Luis Henriques wrote: > On Wed, Jul 24 2024, Jan Kara wrote: > > > On Tue 23-07-24 16:44:02, Luis Henriques (SUSE) wrote: > >> 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'. > >> > >> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev> > > > > Just one style nit below, otherwise looks good. Feel free to add: > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > 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. 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 -- Jan Kara <jack@suse.com> SUSE Labs, CR
© 2016 - 2024 Red Hat, Inc.