[PATCH v2 1/4] jbd2: Make jbd2_journal_get_max_txn_bufs() internal

[PATCH v2 1/4] jbd2: Make jbd2_journal_get_max_txn_bufs() internal

[Jan Kara]

There's no reason to have jbd2_journal_get_max_txn_bufs() public
function. Currently all users are internal and can use
journal->j_max_transaction_buffers instead. This saves some unnecessary
recomputations of the limit as a bonus which becomes important as this
function gets more complex in the following patch.

CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c     | 2 +-
 fs/jbd2/journal.c    | 5 +++++
 include/linux/jbd2.h | 5 -----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 75ea4e9a5cab..e7fc912693bd 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -766,7 +766,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		if (first_block < journal->j_tail)
 			freed += journal->j_last - journal->j_first;
 		/* Update tail only if we free significant amount of space */
-		if (freed < jbd2_journal_get_max_txn_bufs(journal))
+		if (freed < journal->j_max_transaction_buffers)
 			update_tail = 0;
 	}
 	J_ASSERT(commit_transaction->t_state == T_COMMIT);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 03c4b9214f56..1bb73750d307 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1698,6 +1698,11 @@ 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
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index ab04c1c27fae..f91b930abe20 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1660,11 +1660,6 @@ int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode);
 int jbd2_fc_wait_bufs(journal_t *journal, int num_blks);
 int jbd2_fc_release_bufs(journal_t *journal);
 
-static inline int jbd2_journal_get_max_txn_bufs(journal_t *journal)
-{
-	return (journal->j_total_len - journal->j_fc_wbufsize) / 4;
-}
-
 /*
  * is_journal_abort
  *
-- 
2.35.3

Re: [PATCH v2 1/4] jbd2: Make jbd2_journal_get_max_txn_bufs() internal

[Zhang Yi]

On 2024/6/25 1:01, Jan Kara wrote:
> There's no reason to have jbd2_journal_get_max_txn_bufs() public
> function. Currently all users are internal and can use
> journal->j_max_transaction_buffers instead. This saves some unnecessary
> recomputations of the limit as a bonus which becomes important as this
> function gets more complex in the following patch.
> 
> CC: stable@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

A good cleanup.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/jbd2/commit.c     | 2 +-
>  fs/jbd2/journal.c    | 5 +++++
>  include/linux/jbd2.h | 5 -----
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 75ea4e9a5cab..e7fc912693bd 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -766,7 +766,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		if (first_block < journal->j_tail)
>  			freed += journal->j_last - journal->j_first;
>  		/* Update tail only if we free significant amount of space */
> -		if (freed < jbd2_journal_get_max_txn_bufs(journal))
> +		if (freed < journal->j_max_transaction_buffers)
>  			update_tail = 0;
>  	}
>  	J_ASSERT(commit_transaction->t_state == T_COMMIT);
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 03c4b9214f56..1bb73750d307 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1698,6 +1698,11 @@ 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
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index ab04c1c27fae..f91b930abe20 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1660,11 +1660,6 @@ int jbd2_wait_inode_data(journal_t *journal, struct jbd2_inode *jinode);
>  int jbd2_fc_wait_bufs(journal_t *journal, int num_blks);
>  int jbd2_fc_release_bufs(journal_t *journal);
>  
> -static inline int jbd2_journal_get_max_txn_bufs(journal_t *journal)
> -{
> -	return (journal->j_total_len - journal->j_fc_wbufsize) / 4;
> -}
> -
>  /*
>   * is_journal_abort
>   *
> 

Re: [PATCH v2 1/4] jbd2: Make jbd2_journal_get_max_txn_bufs() internal

[Theodore Ts'o]

On Mon, 24 Jun 2024 19:01:17 +0200, Jan Kara wrote:
> There's no reason to have jbd2_journal_get_max_txn_bufs() public
> function. Currently all users are internal and can use
> journal->j_max_transaction_buffers instead. This saves some unnecessary
> recomputations of the limit as a bonus which becomes important as this
> function gets more complex in the following patch.
> 
> 
> [...]

Applied, thanks!

[1/4] jbd2: Make jbd2_journal_get_max_txn_bufs() internal
      commit: 4aa99c71e42ad60178c1154ec24e3df9c684fb67
[2/4] jbd2: Precompute number of transaction descriptor blocks
      commit: e3a00a23781c1f2fcda98a7aecaac515558e7a35
[3/4] jbd2: Avoid infinite transaction commit loop
      commit: 27ba5b67312a944576addc4df44ac3b709aabede
[4/4] jbd2: Drop pointless shrinker batch initialization
      commit: 1cf5b024a3ffa479ed14e520f81549fce037f322

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

[PATCH v2 2/4] jbd2: Precompute number of transaction descriptor blocks

[Jan Kara]

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@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.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);
+}
+
+/*
+ * 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:
 	 *
-- 
2.35.3

Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor blocks

[Kemeng Shi]

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@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.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:
>  	 *
> 

Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor blocks

[Jan Kara]

On Tue 25-06-24 17:31:15, Kemeng Shi wrote:
> 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@vger.kernel.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/jbd2/journal.c     | 61 ++++++++++++++++++++++++++++++++-----------
> >  fs/jbd2/transaction.c | 24 +----------------
> >  include/linux/jbd2.h  |  7 +++++
> >  3 files changed, 54 insertions(+), 38 deletions(-)
> > 
> > +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...

So you are correct that the expression is overestimating the number of
descriptor blocks required, essentially because we don't need descriptor
blocks for descriptor blocks. But given tags_per_block is at least over 60,
in common configurations over 250, this imprecision is not really
substantial and I prefer a simple to argue about upper estimate...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor blocks

[Kemeng Shi]

on 6/25/2024 7:07 PM, Jan Kara wrote:
> On Tue 25-06-24 17:31:15, Kemeng Shi wrote:
>> 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@vger.kernel.org
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>>  fs/jbd2/journal.c     | 61 ++++++++++++++++++++++++++++++++-----------
>>>  fs/jbd2/transaction.c | 24 +----------------
>>>  include/linux/jbd2.h  |  7 +++++
>>>  3 files changed, 54 insertions(+), 38 deletions(-)
>>>
>>> +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...
> 
> So you are correct that the expression is overestimating the number of
> descriptor blocks required, essentially because we don't need descriptor
> blocks for descriptor blocks. But given tags_per_block is at least over 60,
> in common configurations over 250, this imprecision is not really
> substantial and I prefer a simple to argue about upper estimate...
Right, I agree with this. Thanks for explanation.
> 
> 								Honza
> 

Re: [PATCH v2 2/4] jbd2: Precompute number of transaction descriptor blocks

[Zhang Yi]

On 2024/6/25 1:01, 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@vger.kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  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);
> +}
> +
> +/*
> + * 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:
>  	 *
> 

[PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Jan Kara]

Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
t_outstanding_credits") started to account descriptor blocks into
transactions outstanding credits. However it didn't appropriately
decrease the maximum amount of credits available to userspace. Thus if
the filesystem requests a transaction smaller than
j_max_transaction_buffers but large enough that when descriptor blocks
are added the size exceeds j_max_transaction_buffers, we confuse
add_transaction_credits() into thinking previous handles have grown the
transaction too much and enter infinite journal commit loop in
start_this_handle() -> add_transaction_credits() trying to create
transaction with enough credits available.

Fix the problem by properly accounting for transaction space reserved
for descriptor blocks when verifying requested transaction handle size.

CC: stable@vger.kernel.org
Fixes: 9f356e5a4f12 ("jbd2: Account descriptor blocks into t_outstanding_credits")
Reported-by: Alexander Coffin <alex.coffin@maticrobots.com>
Link: https://lore.kernel.org/all/CA+hUFcuGs04JHZ_WzA1zGN57+ehL2qmHOt5a7RMpo+rv6Vyxtw@mail.gmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/transaction.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index a095f1a3114b..66513c18ca29 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -191,6 +191,13 @@ static void sub_reserved_credits(journal_t *journal, int blocks)
 	wake_up(&journal->j_wait_reserved);
 }
 
+/* Maximum number of blocks for user transaction payload */
+static int jbd2_max_user_trans_buffers(journal_t *journal)
+{
+	return journal->j_max_transaction_buffers -
+				journal->j_transaction_overhead_buffers;
+}
+
 /*
  * Wait until we can add credits for handle to the running transaction.  Called
  * with j_state_lock held for reading. Returns 0 if handle joined the running
@@ -240,12 +247,12 @@ __must_hold(&journal->j_state_lock)
 		 * big to fit this handle? Wait until reserved credits are freed.
 		 */
 		if (atomic_read(&journal->j_reserved_credits) + total >
-		    journal->j_max_transaction_buffers) {
+		    jbd2_max_user_trans_buffers(journal)) {
 			read_unlock(&journal->j_state_lock);
 			jbd2_might_wait_for_commit(journal);
 			wait_event(journal->j_wait_reserved,
 				   atomic_read(&journal->j_reserved_credits) + total <=
-				   journal->j_max_transaction_buffers);
+				   jbd2_max_user_trans_buffers(journal));
 			__acquire(&journal->j_state_lock); /* fake out sparse */
 			return 1;
 		}
@@ -285,14 +292,14 @@ __must_hold(&journal->j_state_lock)
 
 	needed = atomic_add_return(rsv_blocks, &journal->j_reserved_credits);
 	/* We allow at most half of a transaction to be reserved */
-	if (needed > journal->j_max_transaction_buffers / 2) {
+	if (needed > jbd2_max_user_trans_buffers(journal) / 2) {
 		sub_reserved_credits(journal, rsv_blocks);
 		atomic_sub(total, &t->t_outstanding_credits);
 		read_unlock(&journal->j_state_lock);
 		jbd2_might_wait_for_commit(journal);
 		wait_event(journal->j_wait_reserved,
 			 atomic_read(&journal->j_reserved_credits) + rsv_blocks
-			 <= journal->j_max_transaction_buffers / 2);
+			 <= jbd2_max_user_trans_buffers(journal) / 2);
 		__acquire(&journal->j_state_lock); /* fake out sparse */
 		return 1;
 	}
@@ -322,12 +329,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle,
 	 * size and limit the number of total credits to not exceed maximum
 	 * transaction size per operation.
 	 */
-	if ((rsv_blocks > journal->j_max_transaction_buffers / 2) ||
-	    (rsv_blocks + blocks > journal->j_max_transaction_buffers)) {
+	if (rsv_blocks > jbd2_max_user_trans_buffers(journal) / 2 ||
+	    rsv_blocks + blocks > jbd2_max_user_trans_buffers(journal)) {
 		printk(KERN_ERR "JBD2: %s wants too many credits "
 		       "credits:%d rsv_credits:%d max:%d\n",
 		       current->comm, blocks, rsv_blocks,
-		       journal->j_max_transaction_buffers);
+		       jbd2_max_user_trans_buffers(journal));
 		WARN_ON(1);
 		return -ENOSPC;
 	}
-- 
2.35.3

Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Zhang Yi]

On 2024/6/25 1:01, Jan Kara wrote:
> Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
> t_outstanding_credits") started to account descriptor blocks into
> transactions outstanding credits. However it didn't appropriately
> decrease the maximum amount of credits available to userspace. Thus if
> the filesystem requests a transaction smaller than
> j_max_transaction_buffers but large enough that when descriptor blocks
> are added the size exceeds j_max_transaction_buffers, we confuse
> add_transaction_credits() into thinking previous handles have grown the
> transaction too much and enter infinite journal commit loop in
> start_this_handle() -> add_transaction_credits() trying to create
> transaction with enough credits available.
> 
Hello Jan!

I understand that the incorrect max transaction limit in
start_this_handle() could lead to infinite loop in
start_this_handle()-> add_transaction_credits() with large enough
userspace credits (from j_max_transaction_buffers - overheads to
j_max_transaction_buffers), but I don't get how could it lead to ran
out of space in the journal commit traction? IIUC, below codes in
add_transaction_credits() could make sure that we have enough space
when committing traction:

static int add_transaction_credits()
{
...
	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
		...
		return 1;
		...
	}
...
}

I can't open and download the image Alexander gave, so I can't get to
the bottom of this issue, please let me know what happened with
jbd2_journal_next_log_block().

Thanks,
Yi.

> Fix the problem by properly accounting for transaction space reserved
> for descriptor blocks when verifying requested transaction handle size.
> 
> CC: stable@vger.kernel.org
> Fixes: 9f356e5a4f12 ("jbd2: Account descriptor blocks into t_outstanding_credits")
> Reported-by: Alexander Coffin <alex.coffin@maticrobots.com>
> Link: https://lore.kernel.org/all/CA+hUFcuGs04JHZ_WzA1zGN57+ehL2qmHOt5a7RMpo+rv6Vyxtw@mail.gmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/jbd2/transaction.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index a095f1a3114b..66513c18ca29 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -191,6 +191,13 @@ static void sub_reserved_credits(journal_t *journal, int blocks)
>  	wake_up(&journal->j_wait_reserved);
>  }
>  
> +/* Maximum number of blocks for user transaction payload */
> +static int jbd2_max_user_trans_buffers(journal_t *journal)
> +{
> +	return journal->j_max_transaction_buffers -
> +				journal->j_transaction_overhead_buffers;
> +}
> +
>  /*
>   * Wait until we can add credits for handle to the running transaction.  Called
>   * with j_state_lock held for reading. Returns 0 if handle joined the running
> @@ -240,12 +247,12 @@ __must_hold(&journal->j_state_lock)
>  		 * big to fit this handle? Wait until reserved credits are freed.
>  		 */
>  		if (atomic_read(&journal->j_reserved_credits) + total >
> -		    journal->j_max_transaction_buffers) {
> +		    jbd2_max_user_trans_buffers(journal)) {
>  			read_unlock(&journal->j_state_lock);
>  			jbd2_might_wait_for_commit(journal);
>  			wait_event(journal->j_wait_reserved,
>  				   atomic_read(&journal->j_reserved_credits) + total <=
> -				   journal->j_max_transaction_buffers);
> +				   jbd2_max_user_trans_buffers(journal));
>  			__acquire(&journal->j_state_lock); /* fake out sparse */
>  			return 1;
>  		}
> @@ -285,14 +292,14 @@ __must_hold(&journal->j_state_lock)
>  
>  	needed = atomic_add_return(rsv_blocks, &journal->j_reserved_credits);
>  	/* We allow at most half of a transaction to be reserved */
> -	if (needed > journal->j_max_transaction_buffers / 2) {
> +	if (needed > jbd2_max_user_trans_buffers(journal) / 2) {
>  		sub_reserved_credits(journal, rsv_blocks);
>  		atomic_sub(total, &t->t_outstanding_credits);
>  		read_unlock(&journal->j_state_lock);
>  		jbd2_might_wait_for_commit(journal);
>  		wait_event(journal->j_wait_reserved,
>  			 atomic_read(&journal->j_reserved_credits) + rsv_blocks
> -			 <= journal->j_max_transaction_buffers / 2);
> +			 <= jbd2_max_user_trans_buffers(journal) / 2);
>  		__acquire(&journal->j_state_lock); /* fake out sparse */
>  		return 1;
>  	}
> @@ -322,12 +329,12 @@ static int start_this_handle(journal_t *journal, handle_t *handle,
>  	 * size and limit the number of total credits to not exceed maximum
>  	 * transaction size per operation.
>  	 */
> -	if ((rsv_blocks > journal->j_max_transaction_buffers / 2) ||
> -	    (rsv_blocks + blocks > journal->j_max_transaction_buffers)) {
> +	if (rsv_blocks > jbd2_max_user_trans_buffers(journal) / 2 ||
> +	    rsv_blocks + blocks > jbd2_max_user_trans_buffers(journal)) {
>  		printk(KERN_ERR "JBD2: %s wants too many credits "
>  		       "credits:%d rsv_credits:%d max:%d\n",
>  		       current->comm, blocks, rsv_blocks,
> -		       journal->j_max_transaction_buffers);
> +		       jbd2_max_user_trans_buffers(journal));
>  		WARN_ON(1);
>  		return -ENOSPC;
>  	}
> 

Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Jan Kara]

On Wed 26-06-24 15:38:42, Zhang Yi wrote:
> On 2024/6/25 1:01, Jan Kara wrote:
> > Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
> > t_outstanding_credits") started to account descriptor blocks into
> > transactions outstanding credits. However it didn't appropriately
> > decrease the maximum amount of credits available to userspace. Thus if
> > the filesystem requests a transaction smaller than
> > j_max_transaction_buffers but large enough that when descriptor blocks
> > are added the size exceeds j_max_transaction_buffers, we confuse
> > add_transaction_credits() into thinking previous handles have grown the
> > transaction too much and enter infinite journal commit loop in
> > start_this_handle() -> add_transaction_credits() trying to create
> > transaction with enough credits available.
> 
> I understand that the incorrect max transaction limit in
> start_this_handle() could lead to infinite loop in
> start_this_handle()-> add_transaction_credits() with large enough
> userspace credits (from j_max_transaction_buffers - overheads to
> j_max_transaction_buffers), but I don't get how could it lead to ran
> out of space in the journal commit traction? IIUC, below codes in
> add_transaction_credits() could make sure that we have enough space
> when committing traction:
> 
> static int add_transaction_credits()
> {
> ...
> 	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
> 		...
> 		return 1;
> 		...
> 	}
> ...
> }
> 
> I can't open and download the image Alexander gave, so I can't get to
> the bottom of this issue, please let me know what happened with
> jbd2_journal_next_log_block().

Sure. So what was exactly happening is a loop like this:

start_this_handle()
  blocks = 252 (handle->h_total_credits)
  - starts a new transaction
    - t_outstanding_credits set to 6 to account for the commit block and
      descriptor blocks
  add_transaction_credits(journal, 252, 0)
     needed = atomic_add_return(252, &t->t_outstanding_credits);
     if (needed > journal->j_max_transaction_buffers) {
	/* Yes, this is exceeded due to descriptor blocks being in
	 * t_outstanding_credits */
        ...
        wait_transaction_locked(journal);
	  - this commits an empty transaction - contains only the commit
	    block
        return 1
  goto repeat

So we commit single block transactions in a loop until we exhaust all the
journal space. The condition in add_transaction_credits() whether there's
enough space in the journal is never reached so we don't ever push the
journal tail to make space in the journal.
    
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Zhang Yi]

On 2024/6/26 19:22, Jan Kara wrote:
> On Wed 26-06-24 15:38:42, Zhang Yi wrote:
>> On 2024/6/25 1:01, Jan Kara wrote:
>>> Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
>>> t_outstanding_credits") started to account descriptor blocks into
>>> transactions outstanding credits. However it didn't appropriately
>>> decrease the maximum amount of credits available to userspace. Thus if
>>> the filesystem requests a transaction smaller than
>>> j_max_transaction_buffers but large enough that when descriptor blocks
>>> are added the size exceeds j_max_transaction_buffers, we confuse
>>> add_transaction_credits() into thinking previous handles have grown the
>>> transaction too much and enter infinite journal commit loop in
>>> start_this_handle() -> add_transaction_credits() trying to create
>>> transaction with enough credits available.
>>
>> I understand that the incorrect max transaction limit in
>> start_this_handle() could lead to infinite loop in
>> start_this_handle()-> add_transaction_credits() with large enough
>> userspace credits (from j_max_transaction_buffers - overheads to
>> j_max_transaction_buffers), but I don't get how could it lead to ran
>> out of space in the journal commit traction? IIUC, below codes in
>> add_transaction_credits() could make sure that we have enough space
>> when committing traction:
>>
>> static int add_transaction_credits()
>> {
>> ...
>> 	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
>> 		...
>> 		return 1;
>> 		...
>> 	}
>> ...
>> }
>>
>> I can't open and download the image Alexander gave, so I can't get to
>> the bottom of this issue, please let me know what happened with
>> jbd2_journal_next_log_block().
> 
> Sure. So what was exactly happening is a loop like this:
> 
> start_this_handle()
>   blocks = 252 (handle->h_total_credits)
>   - starts a new transaction
>     - t_outstanding_credits set to 6 to account for the commit block and
>       descriptor blocks
>   add_transaction_credits(journal, 252, 0)
>      needed = atomic_add_return(252, &t->t_outstanding_credits);
>      if (needed > journal->j_max_transaction_buffers) {
> 	/* Yes, this is exceeded due to descriptor blocks being in
> 	 * t_outstanding_credits */
>         ...
>         wait_transaction_locked(journal);
> 	  - this commits an empty transaction - contains only the commit
> 	    block
>         return 1
>   goto repeat
> 
> So we commit single block transactions in a loop until we exhaust all the
> journal space. The condition in add_transaction_credits() whether there's
> enough space in the journal is never reached so we don't ever push the
> journal tail to make space in the journal.
>     

mm-hm, ha, yeah, this will lead to submit an empty transaction in each loop,
but I still have one question, although the journal tail can't be updated
in add_transaction_credits(), I think the journal tail should be updated in
jbd2_journal_commit_transaction()->jbd2_update_log_tail() since we don't
add empty transactions to journal->j_checkpoint_transactions list, the loop
in jbd2_journal_commit_transaction() should like this:

...
jbd2_journal_commit_transaction()
  update_tail = jbd2_journal_get_log_tail()
    //journal->j_checkpoint_transactions should be NULL, tid is the
    //committing transaction's tid, which should be large than the
    //tail tid since the second loop, so this should be true after
    //the second loop
  if (freed < journal->j_max_transaction_buffers)
    update_tail = 0;
    //update_tail should be true after j_max_transaction_buffers loop

jbd2_update_log_tail()  //j_free should be increased in each
                        //j_max_transaction_buffers loop
  if (tid_gt(tid, journal->j_tail_sequence)) //it's true
  __jbd2_update_log_tail()
    journal->j_free += freed; //update log tail and increase j_free
                              //j_max_transaction_buffers blocks
...

As I understand it, the journal space can't be exhausted, I don't know how
it happened, am I missing something?

Thanks,
Yi.

Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Jan Kara]

On Wed 26-06-24 21:24:13, Zhang Yi wrote:
> On 2024/6/26 19:22, Jan Kara wrote:
> > On Wed 26-06-24 15:38:42, Zhang Yi wrote:
> >> On 2024/6/25 1:01, Jan Kara wrote:
> >>> Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
> >>> t_outstanding_credits") started to account descriptor blocks into
> >>> transactions outstanding credits. However it didn't appropriately
> >>> decrease the maximum amount of credits available to userspace. Thus if
> >>> the filesystem requests a transaction smaller than
> >>> j_max_transaction_buffers but large enough that when descriptor blocks
> >>> are added the size exceeds j_max_transaction_buffers, we confuse
> >>> add_transaction_credits() into thinking previous handles have grown the
> >>> transaction too much and enter infinite journal commit loop in
> >>> start_this_handle() -> add_transaction_credits() trying to create
> >>> transaction with enough credits available.
> >>
> >> I understand that the incorrect max transaction limit in
> >> start_this_handle() could lead to infinite loop in
> >> start_this_handle()-> add_transaction_credits() with large enough
> >> userspace credits (from j_max_transaction_buffers - overheads to
> >> j_max_transaction_buffers), but I don't get how could it lead to ran
> >> out of space in the journal commit traction? IIUC, below codes in
> >> add_transaction_credits() could make sure that we have enough space
> >> when committing traction:
> >>
> >> static int add_transaction_credits()
> >> {
> >> ...
> >> 	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
> >> 		...
> >> 		return 1;
> >> 		...
> >> 	}
> >> ...
> >> }
> >>
> >> I can't open and download the image Alexander gave, so I can't get to
> >> the bottom of this issue, please let me know what happened with
> >> jbd2_journal_next_log_block().
> > 
> > Sure. So what was exactly happening is a loop like this:
> > 
> > start_this_handle()
> >   blocks = 252 (handle->h_total_credits)
> >   - starts a new transaction
> >     - t_outstanding_credits set to 6 to account for the commit block and
> >       descriptor blocks
> >   add_transaction_credits(journal, 252, 0)
> >      needed = atomic_add_return(252, &t->t_outstanding_credits);
> >      if (needed > journal->j_max_transaction_buffers) {
> > 	/* Yes, this is exceeded due to descriptor blocks being in
> > 	 * t_outstanding_credits */
> >         ...
> >         wait_transaction_locked(journal);
> > 	  - this commits an empty transaction - contains only the commit
> > 	    block
> >         return 1
> >   goto repeat
> > 
> > So we commit single block transactions in a loop until we exhaust all the
> > journal space. The condition in add_transaction_credits() whether there's
> > enough space in the journal is never reached so we don't ever push the
> > journal tail to make space in the journal.
> >     
> 
> mm-hm, ha, yeah, this will lead to submit an empty transaction in each loop,
> but I still have one question, although the journal tail can't be updated
> in add_transaction_credits(), I think the journal tail should be updated in
> jbd2_journal_commit_transaction()->jbd2_update_log_tail() since we don't
> add empty transactions to journal->j_checkpoint_transactions list, the loop
> in jbd2_journal_commit_transaction() should like this:
> 
> ...
> jbd2_journal_commit_transaction()
>   update_tail = jbd2_journal_get_log_tail()
>     //journal->j_checkpoint_transactions should be NULL, tid is the
>     //committing transaction's tid, which should be large than the
>     //tail tid since the second loop, so this should be true after
>     //the second loop
>   if (freed < journal->j_max_transaction_buffers)
>     update_tail = 0;
>     //update_tail should be true after j_max_transaction_buffers loop
> 
> jbd2_update_log_tail()  //j_free should be increased in each
>                         //j_max_transaction_buffers loop
>   if (tid_gt(tid, journal->j_tail_sequence)) //it's true
>   __jbd2_update_log_tail()
>     journal->j_free += freed; //update log tail and increase j_free
>                               //j_max_transaction_buffers blocks
> ...
> 
> As I understand it, the journal space can't be exhausted, I don't know how
> it happened, am I missing something?

Well, at the beginning of the journal there are a few non-empty
transactions whose buffers didn't get checkpointed before we entered the
commit loop. So we cannot just push the journal tail without doing a
checkpoint and we never call into the checkpointing code in the commit
loop...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 3/4] jbd2: Avoid infinite transaction commit loop

[Zhang Yi]

On 2024/6/26 22:55, Jan Kara wrote:
> On Wed 26-06-24 21:24:13, Zhang Yi wrote:
>> On 2024/6/26 19:22, Jan Kara wrote:
>>> On Wed 26-06-24 15:38:42, Zhang Yi wrote:
>>>> On 2024/6/25 1:01, Jan Kara wrote:
>>>>> Commit 9f356e5a4f12 ("jbd2: Account descriptor blocks into
>>>>> t_outstanding_credits") started to account descriptor blocks into
>>>>> transactions outstanding credits. However it didn't appropriately
>>>>> decrease the maximum amount of credits available to userspace. Thus if
>>>>> the filesystem requests a transaction smaller than
>>>>> j_max_transaction_buffers but large enough that when descriptor blocks
>>>>> are added the size exceeds j_max_transaction_buffers, we confuse
>>>>> add_transaction_credits() into thinking previous handles have grown the
>>>>> transaction too much and enter infinite journal commit loop in
>>>>> start_this_handle() -> add_transaction_credits() trying to create
>>>>> transaction with enough credits available.
>>>>
>>>> I understand that the incorrect max transaction limit in
>>>> start_this_handle() could lead to infinite loop in
>>>> start_this_handle()-> add_transaction_credits() with large enough
>>>> userspace credits (from j_max_transaction_buffers - overheads to
>>>> j_max_transaction_buffers), but I don't get how could it lead to ran
>>>> out of space in the journal commit traction? IIUC, below codes in
>>>> add_transaction_credits() could make sure that we have enough space
>>>> when committing traction:
>>>>
>>>> static int add_transaction_credits()
>>>> {
>>>> ...
>>>> 	if (jbd2_log_space_left(journal) < journal->j_max_transaction_buffers) {
>>>> 		...
>>>> 		return 1;
>>>> 		...
>>>> 	}
>>>> ...
>>>> }
>>>>
>>>> I can't open and download the image Alexander gave, so I can't get to
>>>> the bottom of this issue, please let me know what happened with
>>>> jbd2_journal_next_log_block().
>>>
>>> Sure. So what was exactly happening is a loop like this:
>>>
>>> start_this_handle()
>>>   blocks = 252 (handle->h_total_credits)
>>>   - starts a new transaction
>>>     - t_outstanding_credits set to 6 to account for the commit block and
>>>       descriptor blocks
>>>   add_transaction_credits(journal, 252, 0)
>>>      needed = atomic_add_return(252, &t->t_outstanding_credits);
>>>      if (needed > journal->j_max_transaction_buffers) {
>>> 	/* Yes, this is exceeded due to descriptor blocks being in
>>> 	 * t_outstanding_credits */
>>>         ...
>>>         wait_transaction_locked(journal);
>>> 	  - this commits an empty transaction - contains only the commit
>>> 	    block
>>>         return 1
>>>   goto repeat
>>>
>>> So we commit single block transactions in a loop until we exhaust all the
>>> journal space. The condition in add_transaction_credits() whether there's
>>> enough space in the journal is never reached so we don't ever push the
>>> journal tail to make space in the journal.
>>>     
>>
>> mm-hm, ha, yeah, this will lead to submit an empty transaction in each loop,
>> but I still have one question, although the journal tail can't be updated
>> in add_transaction_credits(), I think the journal tail should be updated in
>> jbd2_journal_commit_transaction()->jbd2_update_log_tail() since we don't
>> add empty transactions to journal->j_checkpoint_transactions list, the loop
>> in jbd2_journal_commit_transaction() should like this:
>>
>> ...
>> jbd2_journal_commit_transaction()
>>   update_tail = jbd2_journal_get_log_tail()
>>     //journal->j_checkpoint_transactions should be NULL, tid is the
>>     //committing transaction's tid, which should be large than the
>>     //tail tid since the second loop, so this should be true after
>>     //the second loop
>>   if (freed < journal->j_max_transaction_buffers)
>>     update_tail = 0;
>>     //update_tail should be true after j_max_transaction_buffers loop
>>
>> jbd2_update_log_tail()  //j_free should be increased in each
>>                         //j_max_transaction_buffers loop
>>   if (tid_gt(tid, journal->j_tail_sequence)) //it's true
>>   __jbd2_update_log_tail()
>>     journal->j_free += freed; //update log tail and increase j_free
>>                               //j_max_transaction_buffers blocks
>> ...
>>
>> As I understand it, the journal space can't be exhausted, I don't know how
>> it happened, am I missing something?
> 
> Well, at the beginning of the journal there are a few non-empty
> transactions whose buffers didn't get checkpointed before we entered the
> commit loop. So we cannot just push the journal tail without doing a
> checkpoint and we never call into the checkpointing code in the commit
> loop...

Oh, indeed, I see, thanks for explaining and this patch looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

Thanks,
Yi.