lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <483983e0-8827-9801-5268-abfd97865d94@huaweicloud.com>
Date: Tue, 25 Jun 2024 17:31:15 +0800
From: Kemeng Shi <shikemeng@...weicloud.com>
To: Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>
Cc: linux-ext4@...r.kernel.org, Alexander Coffin
 <alex.coffin@...icrobots.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor
 blocks


Hello Jan,
on 6/25/2024 1:01 AM, Jan Kara wrote:
> Instead of computing the number of descriptor blocks a transaction can
> have each time we need it (which is currently when starting each
> transaction but will become more frequent later) precompute the number
> once during journal initialization together with maximum transaction
> size. We perform the precomputation whenever journal feature set is
> updated similarly as for computation of
> journal->j_revoke_records_per_block.
> 
> CC: stable@...r.kernel.org
> Signed-off-by: Jan Kara <jack@...e.cz>
> ---
>  fs/jbd2/journal.c     | 61 ++++++++++++++++++++++++++++++++-----------
>  fs/jbd2/transaction.c | 24 +----------------
>  include/linux/jbd2.h  |  7 +++++
>  3 files changed, 54 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 1bb73750d307..ae5b544ed0cc 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1451,6 +1451,48 @@ static int journal_revoke_records_per_block(journal_t *journal)
>  	return space / record_size;
>  }
>  
> +static int jbd2_journal_get_max_txn_bufs(journal_t *journal)
> +{
> +	return (journal->j_total_len - journal->j_fc_wbufsize) / 4;
> +}
> +
> +/*
> + * Base amount of descriptor blocks we reserve for each transaction.
> + */
> +static int jbd2_descriptor_blocks_per_trans(journal_t *journal)
> +{
> +	int tag_space = journal->j_blocksize - sizeof(journal_header_t);
> +	int tags_per_block;
> +
> +	/* Subtract UUID */
> +	tag_space -= 16;
> +	if (jbd2_journal_has_csum_v2or3(journal))
> +		tag_space -= sizeof(struct jbd2_journal_block_tail);
> +	/* Commit code leaves a slack space of 16 bytes at the end of block */
> +	tags_per_block = (tag_space - 16) / journal_tag_bytes(journal);
> +	/*
> +	 * Revoke descriptors are accounted separately so we need to reserve
> +	 * space for commit block and normal transaction descriptor blocks.
> +	 */
> +	return 1 + DIV_ROUND_UP(jbd2_journal_get_max_txn_bufs(journal),
> +				tags_per_block);
> +}
The change looks good to me. I wonder if the original calculation of
number of JBD2_DESCRIPTOR_BLOCK blocks is correct.
In my opinion, it should be:
DIV_ROUND_UP(jbd2_journal_get_max_txn_bufs(journal), tags_per_block *+ 1*)
Assume max_txn_bufs is 6, tags_per_block is 1, then we have one tag block
after each JBD2_DESCRIPTOR_BLOCK block. Then we could get 3
JBD2_DESCRIPTOR_BLOCK block at most rather than 6.
Please let me konw if I miss something, this confused me for sometime...
Thanks.
> +
> +/*
> + * Initialize number of blocks each transaction reserves for its bookkeeping
> + * and maximum number of blocks a transaction can use. This needs to be called
> + * after the journal size and the fastcommit area size are initialized.
> + */
> +static void jbd2_journal_init_transaction_limits(journal_t *journal)
> +{
> +	journal->j_revoke_records_per_block =
> +				journal_revoke_records_per_block(journal);
> +	journal->j_transaction_overhead_buffers =
> +				jbd2_descriptor_blocks_per_trans(journal);
> +	journal->j_max_transaction_buffers =
> +				jbd2_journal_get_max_txn_bufs(journal);
> +}
> +
>  /*
>   * Load the on-disk journal superblock and read the key fields into the
>   * journal_t.
> @@ -1492,8 +1534,8 @@ static int journal_load_superblock(journal_t *journal)
>  	if (jbd2_journal_has_csum_v2or3(journal))
>  		journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid,
>  						   sizeof(sb->s_uuid));
> -	journal->j_revoke_records_per_block =
> -				journal_revoke_records_per_block(journal);
> +	/* After journal features are set, we can compute transaction limits */
> +	jbd2_journal_init_transaction_limits(journal);
>  
>  	if (jbd2_has_feature_fast_commit(journal)) {
>  		journal->j_fc_last = be32_to_cpu(sb->s_maxlen);
> @@ -1698,11 +1740,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
>  	return journal;
>  }
>  
> -static int jbd2_journal_get_max_txn_bufs(journal_t *journal)
> -{
> -	return (journal->j_total_len - journal->j_fc_wbufsize) / 4;
> -}
> -
>  /*
>   * Given a journal_t structure, initialise the various fields for
>   * startup of a new journaling session.  We use this both when creating
> @@ -1748,8 +1785,6 @@ static int journal_reset(journal_t *journal)
>  	journal->j_commit_sequence = journal->j_transaction_sequence - 1;
>  	journal->j_commit_request = journal->j_commit_sequence;
>  
> -	journal->j_max_transaction_buffers = jbd2_journal_get_max_txn_bufs(journal);
> -
>  	/*
>  	 * Now that journal recovery is done, turn fast commits off here. This
>  	 * way, if fast commit was enabled before the crash but if now FS has
> @@ -2290,8 +2325,6 @@ jbd2_journal_initialize_fast_commit(journal_t *journal)
>  	journal->j_fc_first = journal->j_last + 1;
>  	journal->j_fc_off = 0;
>  	journal->j_free = journal->j_last - journal->j_first;
> -	journal->j_max_transaction_buffers =
> -		jbd2_journal_get_max_txn_bufs(journal);
>  
>  	return 0;
>  }
> @@ -2379,8 +2412,7 @@ int jbd2_journal_set_features(journal_t *journal, unsigned long compat,
>  	sb->s_feature_ro_compat |= cpu_to_be32(ro);
>  	sb->s_feature_incompat  |= cpu_to_be32(incompat);
>  	unlock_buffer(journal->j_sb_buffer);
> -	journal->j_revoke_records_per_block =
> -				journal_revoke_records_per_block(journal);
> +	jbd2_journal_init_transaction_limits(journal);
>  
>  	return 1;
>  #undef COMPAT_FEATURE_ON
> @@ -2411,8 +2443,7 @@ void jbd2_journal_clear_features(journal_t *journal, unsigned long compat,
>  	sb->s_feature_compat    &= ~cpu_to_be32(compat);
>  	sb->s_feature_ro_compat &= ~cpu_to_be32(ro);
>  	sb->s_feature_incompat  &= ~cpu_to_be32(incompat);
> -	journal->j_revoke_records_per_block =
> -				journal_revoke_records_per_block(journal);
> +	jbd2_journal_init_transaction_limits(journal);
>  }
>  EXPORT_SYMBOL(jbd2_journal_clear_features);
>  
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index cb0b8d6fc0c6..a095f1a3114b 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -62,28 +62,6 @@ void jbd2_journal_free_transaction(transaction_t *transaction)
>  	kmem_cache_free(transaction_cache, transaction);
>  }
>  
> -/*
> - * Base amount of descriptor blocks we reserve for each transaction.
> - */
> -static int jbd2_descriptor_blocks_per_trans(journal_t *journal)
> -{
> -	int tag_space = journal->j_blocksize - sizeof(journal_header_t);
> -	int tags_per_block;
> -
> -	/* Subtract UUID */
> -	tag_space -= 16;
> -	if (jbd2_journal_has_csum_v2or3(journal))
> -		tag_space -= sizeof(struct jbd2_journal_block_tail);
> -	/* Commit code leaves a slack space of 16 bytes at the end of block */
> -	tags_per_block = (tag_space - 16) / journal_tag_bytes(journal);
> -	/*
> -	 * Revoke descriptors are accounted separately so we need to reserve
> -	 * space for commit block and normal transaction descriptor blocks.
> -	 */
> -	return 1 + DIV_ROUND_UP(journal->j_max_transaction_buffers,
> -				tags_per_block);
> -}
> -
>  /*
>   * jbd2_get_transaction: obtain a new transaction_t object.
>   *
> @@ -109,7 +87,7 @@ static void jbd2_get_transaction(journal_t *journal,
>  	transaction->t_expires = jiffies + journal->j_commit_interval;
>  	atomic_set(&transaction->t_updates, 0);
>  	atomic_set(&transaction->t_outstanding_credits,
> -		   jbd2_descriptor_blocks_per_trans(journal) +
> +		   journal->j_transaction_overhead_buffers +
>  		   atomic_read(&journal->j_reserved_credits));
>  	atomic_set(&transaction->t_outstanding_revokes, 0);
>  	atomic_set(&transaction->t_handle_count, 0);
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index f91b930abe20..b900c642210c 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1085,6 +1085,13 @@ struct journal_s
>  	 */
>  	int			j_revoke_records_per_block;
>  
> +	/**
> +	 * @j_transaction_overhead:
> +	 *
> +	 * Number of blocks each transaction needs for its own bookkeeping
> +	 */
> +	int			j_transaction_overhead_buffers;
> +
>  	/**
>  	 * @j_commit_interval:
>  	 *
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ