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@xxxxxxxxx>
Just one style nit below, otherwise looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
