On 2025/1/17 2:02, Jan Kara wrote: > Some users are reporting journal replay takes a long time when there is > excessive number of revoke blocks in the journal. Reported times are > like: > > 1048576 records - 95 seconds > 2097152 records - 580 seconds > > The problem is that hash chains in the revoke table gets excessively > long in these cases. Fix the problem by sizing the revoke table > appropriately before the revoke pass. > > Thanks to Alexey Zhuravlev <azhuravlev@xxxxxxx> for benchmarking the patch with > large numbers of revoke blocks [1]. > > [1] https://lore.kernel.org/all/20250113183107.7bfef7b6@xxxxxxxxxxxxxx > > Signed-off-by: Jan Kara <jack@xxxxxxx> Hi, Jan, This overall patch looks good to me; however, it appears to be not based on the latested version of the upstream kernel, and I have one minor suggestion below. > --- > fs/jbd2/recovery.c | 54 +++++++++++++++++++++++++++++++++++++------- > fs/jbd2/revoke.c | 8 +++---- > include/linux/jbd2.h | 2 ++ > 3 files changed, 52 insertions(+), 12 deletions(-) > > diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c > index 667f67342c52..9845f72e456a 100644 > --- a/fs/jbd2/recovery.c > +++ b/fs/jbd2/recovery.c > @@ -39,7 +39,7 @@ struct recovery_info > > static int do_one_pass(journal_t *journal, > struct recovery_info *info, enum passtype pass); > -static int scan_revoke_records(journal_t *, struct buffer_head *, > +static int scan_revoke_records(journal_t *, enum passtype, struct buffer_head *, > tid_t, struct recovery_info *); > > #ifdef __KERNEL__ > @@ -327,6 +327,12 @@ int jbd2_journal_recover(journal_t *journal) > journal->j_transaction_sequence, journal->j_head); > > jbd2_journal_clear_revoke(journal); > + /* Free revoke table allocated for replay */ > + if (journal->j_revoke != journal->j_revoke_table[0] && > + journal->j_revoke != journal->j_revoke_table[1]) { > + jbd2_journal_destroy_revoke_table(journal->j_revoke); > + journal->j_revoke = journal->j_revoke_table[1]; > + } > err2 = sync_blockdev(journal->j_fs_dev); > if (!err) > err = err2; > @@ -517,6 +523,31 @@ static int do_one_pass(journal_t *journal, > first_commit_ID = next_commit_ID; > if (pass == PASS_SCAN) > info->start_transaction = first_commit_ID; > + else if (pass == PASS_REVOKE) { > + /* > + * Would the default revoke table have too long hash chains > + * during replay? > + */ > + if (info->nr_revokes > JOURNAL_REVOKE_DEFAULT_HASH * 16) { > + unsigned int hash_size; > + > + /* > + * Aim for average chain length of 8, limit at 1M > + * entries to avoid problems with malicious > + * filesystems. > + */ > + hash_size = min(roundup_pow_of_two(info->nr_revokes / 8), > + 1U << 20); > + journal->j_revoke = > + jbd2_journal_init_revoke_table(hash_size); > + if (!journal->j_revoke) { > + printk(KERN_ERR > + "JBD2: failed to allocate revoke table for replay with %u entries. " > + "Journal replay may be slow.\n", hash_size); > + journal->j_revoke = journal->j_revoke_table[1]; > + } > + } > + } > > jbd2_debug(1, "Starting recovery pass %d\n", pass); > > @@ -874,14 +905,16 @@ static int do_one_pass(journal_t *journal, > need_check_commit_time = true; > } > > - /* If we aren't in the REVOKE pass, then we can > - * just skip over this block. */ > - if (pass != PASS_REVOKE) { > + /* > + * If we aren't in the SCAN or REVOKE pass, then we can > + * just skip over this block. > + */ > + if (pass != PASS_REVOKE && pass != PASS_SCAN) { > brelse(bh); > continue; > } How about move this code snippets to the beginning of the JBD2_REVOKE_BLOCK branch case? Thanks, Yi. > > extern void jbd2_journal_destroy_revoke(journal_t *); > extern int jbd2_journal_revoke (handle_t *, unsigned long long, struct buffer_head *);
