[PATCH v2 0/9] A fix and some cleanups to jbd2

[PATCH v2 0/9] A fix and some cleanups to jbd2

[Kemeng Shi]

v1->v2:
-Collect RVB from Yi and Jan
-remove rename "flags" to "escape" in patch 2/9
-goto new copy_done tag in patch 4/9
-add more comment in patch 5/9

Patch 1 fixes memleak in jbd2_journal_write_metadata_buffer.
Patch 2-6 contain some cleanups to jbd2_journal_write_metadata_buffer().
Patch 7-9 contain some cleanups to kjournald2()
All tests in "kvm-xfstest smoke" survive. Please let me konw if more tests
should be ran. Thanks.

Kemeng Shi (9):
  jbd2: avoid memleak in jbd2_journal_write_metadata_buffer
  jbd2: remove unused return info from
    jbd2_journal_write_metadata_buffer
  jbd2: remove unnedded "need_copy_out" in
    jbd2_journal_write_metadata_buffer
  jbd2: jump to new copy_done tag when b_frozen_data is created
    concurrently
  jbd2: remove unneeded kmap to do escape in
    jbd2_journal_write_metadata_buffer
  jbd2: use bh_in instead of jh2bh(jh_in) to simplify code
  jbd2: remove dead equality check of j_commit_[sequence/request] in
    kjournald2
  jbd2: remove dead check of JBD2_UNMOUNT in kjournald2
  jbd2: remove unnecessary "should_sleep" in kjournald2

 fs/jbd2/commit.c  | 10 ++++-----
 fs/jbd2/journal.c | 54 ++++++++++++++++++-----------------------------
 2 files changed, 25 insertions(+), 39 deletions(-)

-- 
2.30.0

[PATCH v2 1/9] jbd2: avoid memleak in jbd2_journal_write_metadata_buffer

[Kemeng Shi]

The new_bh is from alloc_buffer_head, we should call free_buffer_head to
free it in error case.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b6c114c11b97..207b24e12ce9 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -399,6 +399,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		tmp = jbd2_alloc(bh_in->b_size, GFP_NOFS);
 		if (!tmp) {
 			brelse(new_bh);
+			free_buffer_head(new_bh);
 			return -ENOMEM;
 		}
 		spin_lock(&jh_in->b_state_lock);
-- 
2.30.0

[PATCH v2 2/9] jbd2: remove unused return info from jbd2_journal_write_metadata_buffer

[Kemeng Shi]

The done_copy_out info from jbd2_journal_write_metadata_buffer is not
used. Simply remove it.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/jbd2/commit.c  | 10 +++++-----
 fs/jbd2/journal.c |  9 +++------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 5e122586e06e..67077308b56b 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -353,7 +353,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	struct buffer_head *descriptor;
 	struct buffer_head **wbuf = journal->j_wbuf;
 	int bufs;
-	int flags;
+	int escape;
 	int err;
 	unsigned long long blocknr;
 	ktime_t start_time;
@@ -661,10 +661,10 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		 */
 		set_bit(BH_JWrite, &jh2bh(jh)->b_state);
 		JBUFFER_TRACE(jh, "ph3: write metadata");
-		flags = jbd2_journal_write_metadata_buffer(commit_transaction,
+		escape = jbd2_journal_write_metadata_buffer(commit_transaction,
 						jh, &wbuf[bufs], blocknr);
-		if (flags < 0) {
-			jbd2_journal_abort(journal, flags);
+		if (escape < 0) {
+			jbd2_journal_abort(journal, escape);
 			continue;
 		}
 		jbd2_file_log_bh(&io_bufs, wbuf[bufs]);
@@ -673,7 +673,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
                    buffer */
 
 		tag_flag = 0;
-		if (flags & 1)
+		if (escape)
 			tag_flag |= JBD2_FLAG_ESCAPE;
 		if (!first_tag)
 			tag_flag |= JBD2_FLAG_SAME_UUID;
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 207b24e12ce9..2dca2f613a8e 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -316,11 +316,8 @@ static void journal_kill_thread(journal_t *journal)
  *
  * Return value:
  *  <0: Error
- * >=0: Finished OK
- *
- * On success:
- * Bit 0 set == escape performed on the data
- * Bit 1 set == buffer copy-out performed (kfree the data after IO)
+ *  =0: Finished OK without escape
+ *  =1: Finished OK with escape
  */
 
 int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
@@ -455,7 +452,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	set_buffer_shadow(bh_in);
 	spin_unlock(&jh_in->b_state_lock);
 
-	return do_escape | (done_copy_out << 1);
+	return do_escape;
 }
 
 /*
-- 
2.30.0

[PATCH v2 3/9] jbd2: remove unnedded "need_copy_out" in jbd2_journal_write_metadata_buffer

[Kemeng Shi]

As we only need to copy out when we should do escape, need_copy_out
could be simply replaced by "do_escape".

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 2dca2f613a8e..714e2ef0115a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -325,7 +325,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 				  struct buffer_head **bh_out,
 				  sector_t blocknr)
 {
-	int need_copy_out = 0;
 	int done_copy_out = 0;
 	int do_escape = 0;
 	char *mapped_data;
@@ -380,16 +379,14 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	/*
 	 * Check for escaping
 	 */
-	if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER)) {
-		need_copy_out = 1;
+	if (*((__be32 *)mapped_data) == cpu_to_be32(JBD2_MAGIC_NUMBER))
 		do_escape = 1;
-	}
 	kunmap_local(mapped_data);
 
 	/*
 	 * Do we need to do a data copy?
 	 */
-	if (need_copy_out && !done_copy_out) {
+	if (do_escape && !done_copy_out) {
 		char *tmp;
 
 		spin_unlock(&jh_in->b_state_lock);
-- 
2.30.0

[PATCH v2 4/9] jbd2: jump to new copy_done tag when b_frozen_data is created concurrently

[Kemeng Shi]

If b_frozen_data is created concurrently, we can update new_folio and
new_offset with b_frozen_data and then move forward

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/jbd2/journal.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 714e2ef0115a..5fb5062cf7ae 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -351,7 +351,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	atomic_set(&new_bh->b_count, 1);
 
 	spin_lock(&jh_in->b_state_lock);
-repeat:
 	/*
 	 * If a new transaction has already done a buffer copy-out, then
 	 * we use that version of the data for the commit.
@@ -399,22 +398,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		spin_lock(&jh_in->b_state_lock);
 		if (jh_in->b_frozen_data) {
 			jbd2_free(tmp, bh_in->b_size);
-			goto repeat;
+			goto copy_done;
 		}
 
 		jh_in->b_frozen_data = tmp;
 		memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
-
-		new_folio = virt_to_folio(tmp);
-		new_offset = offset_in_folio(new_folio, tmp);
-		done_copy_out = 1;
-
 		/*
 		 * This isn't strictly necessary, as we're using frozen
 		 * data for the escaping, but it keeps consistency with
 		 * b_frozen_data usage.
 		 */
 		jh_in->b_frozen_triggers = jh_in->b_triggers;
+
+copy_done:
+		new_folio = virt_to_folio(jh_in->b_frozen_data);
+		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
+		done_copy_out = 1;
 	}
 
 	/*
-- 
2.30.0

[PATCH v2 5/9] jbd2: remove unneeded kmap to do escape in jbd2_journal_write_metadata_buffer

[Kemeng Shi]

The data to do escape could be accessed directly from b_frozen_data,
just remove unneeded kmap.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 5fb5062cf7ae..c98c06f3113a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -419,12 +419,11 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	/*
 	 * Did we need to do an escaping?  Now we've done all the
 	 * copying, we can finally do so.
+	 * b_frozen_data is from jbd2_alloc() which always provides an
+	 * address from the direct kernels mapping.
 	 */
-	if (do_escape) {
-		mapped_data = kmap_local_folio(new_folio, new_offset);
-		*((unsigned int *)mapped_data) = 0;
-		kunmap_local(mapped_data);
-	}
+	if (do_escape)
+		*((unsigned int *)jh_in->b_frozen_data) = 0;
 
 	folio_set_bh(new_bh, new_folio, new_offset);
 	new_bh->b_size = bh_in->b_size;
-- 
2.30.0

[PATCH v2 6/9] jbd2: use bh_in instead of jh2bh(jh_in) to simplify code

[Kemeng Shi]

We save jh2bh(jh_in) to bh_in, so use bh_in directly instead of
jh2bh(jh_in) to simplify the code.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index c98c06f3113a..71568cb82db7 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -360,8 +360,8 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		new_folio = virt_to_folio(jh_in->b_frozen_data);
 		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
 	} else {
-		new_folio = jh2bh(jh_in)->b_folio;
-		new_offset = offset_in_folio(new_folio, jh2bh(jh_in)->b_data);
+		new_folio = bh_in->b_folio;
+		new_offset = offset_in_folio(new_folio, bh_in->b_data);
 	}
 
 	mapped_data = kmap_local_folio(new_folio, new_offset);
-- 
2.30.0

[PATCH v2 7/9] jbd2: remove dead equality check of j_commit_[sequence/request] in kjournald2

[Kemeng Shi]

The j_commit_[sequence/request] are updated with j_state_lock held during
runtime. In kjournald2, two equality checks of j_commit_[sequence/request]
are under the same j_state_lock, then the second check is unnecessary.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 71568cb82db7..4f1ab68cdb6f 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -224,8 +224,6 @@ static int kjournald2(void *arg)
 
 		prepare_to_wait(&journal->j_wait_commit, &wait,
 				TASK_INTERRUPTIBLE);
-		if (journal->j_commit_sequence != journal->j_commit_request)
-			should_sleep = 0;
 		transaction = journal->j_running_transaction;
 		if (transaction && time_after_eq(jiffies,
 						transaction->t_expires))
-- 
2.30.0

[PATCH v2 8/9] jbd2: remove dead check of JBD2_UNMOUNT in kjournald2

[Kemeng Shi]

We always set JBD2_UNMOUNT with j_state_lock held in journal_kill_thread.
In kjournald2, we check JBD2_UNMOUNT flag two times under the same
j_state_lock. Then the second check is unnecessary.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 4f1ab68cdb6f..59bff0b75ce7 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -228,8 +228,6 @@ static int kjournald2(void *arg)
 		if (transaction && time_after_eq(jiffies,
 						transaction->t_expires))
 			should_sleep = 0;
-		if (journal->j_flags & JBD2_UNMOUNT)
-			should_sleep = 0;
 		if (should_sleep) {
 			write_unlock(&journal->j_state_lock);
 			schedule();
-- 
2.30.0

[PATCH v2 9/9] jbd2: remove unnecessary "should_sleep" in kjournald2

[Kemeng Shi]

We only need to sleep if no running transaction is expired. Simply remove
unnecessary "should_sleep".

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/journal.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 59bff0b75ce7..bf3a425dc057 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -220,15 +220,12 @@ static int kjournald2(void *arg)
 		 * so we don't sleep
 		 */
 		DEFINE_WAIT(wait);
-		int should_sleep = 1;
 
 		prepare_to_wait(&journal->j_wait_commit, &wait,
 				TASK_INTERRUPTIBLE);
 		transaction = journal->j_running_transaction;
-		if (transaction && time_after_eq(jiffies,
-						transaction->t_expires))
-			should_sleep = 0;
-		if (should_sleep) {
+		if (transaction == NULL ||
+		    time_before(jiffies, transaction->t_expires)) {
 			write_unlock(&journal->j_state_lock);
 			schedule();
 			write_lock(&journal->j_state_lock);
-- 
2.30.0

Re: [PATCH v2 2/9] jbd2: remove unused return info from jbd2_journal_write_metadata_buffer

[Zhang Yi]

On 2024/5/14 19:24, Kemeng Shi wrote:
> The done_copy_out info from jbd2_journal_write_metadata_buffer is not
> used. Simply remove it.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Thanks, looks good to me.

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

> ---
>  fs/jbd2/commit.c  | 10 +++++-----
>  fs/jbd2/journal.c |  9 +++------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 5e122586e06e..67077308b56b 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -353,7 +353,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	struct buffer_head *descriptor;
>  	struct buffer_head **wbuf = journal->j_wbuf;
>  	int bufs;
> -	int flags;
> +	int escape;
>  	int err;
>  	unsigned long long blocknr;
>  	ktime_t start_time;
> @@ -661,10 +661,10 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		 */
>  		set_bit(BH_JWrite, &jh2bh(jh)->b_state);
>  		JBUFFER_TRACE(jh, "ph3: write metadata");
> -		flags = jbd2_journal_write_metadata_buffer(commit_transaction,
> +		escape = jbd2_journal_write_metadata_buffer(commit_transaction,
>  						jh, &wbuf[bufs], blocknr);
> -		if (flags < 0) {
> -			jbd2_journal_abort(journal, flags);
> +		if (escape < 0) {
> +			jbd2_journal_abort(journal, escape);
>  			continue;
>  		}
>  		jbd2_file_log_bh(&io_bufs, wbuf[bufs]);
> @@ -673,7 +673,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>                     buffer */
>  
>  		tag_flag = 0;
> -		if (flags & 1)
> +		if (escape)
>  			tag_flag |= JBD2_FLAG_ESCAPE;
>  		if (!first_tag)
>  			tag_flag |= JBD2_FLAG_SAME_UUID;
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 207b24e12ce9..2dca2f613a8e 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -316,11 +316,8 @@ static void journal_kill_thread(journal_t *journal)
>   *
>   * Return value:
>   *  <0: Error
> - * >=0: Finished OK
> - *
> - * On success:
> - * Bit 0 set == escape performed on the data
> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
> + *  =0: Finished OK without escape
> + *  =1: Finished OK with escape
>   */
>  
>  int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> @@ -455,7 +452,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	set_buffer_shadow(bh_in);
>  	spin_unlock(&jh_in->b_state_lock);
>  
> -	return do_escape | (done_copy_out << 1);
> +	return do_escape;
>  }
>  
>  /*
> 

Re: [PATCH v2 4/9] jbd2: jump to new copy_done tag when b_frozen_data is created concurrently

[Zhang Yi]

On 2024/5/14 19:24, Kemeng Shi wrote:
> If b_frozen_data is created concurrently, we can update new_folio and
> new_offset with b_frozen_data and then move forward
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

It's more straight forward, looks good to me.

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

> ---
>  fs/jbd2/journal.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 714e2ef0115a..5fb5062cf7ae 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -351,7 +351,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	atomic_set(&new_bh->b_count, 1);
>  
>  	spin_lock(&jh_in->b_state_lock);
> -repeat:
>  	/*
>  	 * If a new transaction has already done a buffer copy-out, then
>  	 * we use that version of the data for the commit.
> @@ -399,22 +398,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  		spin_lock(&jh_in->b_state_lock);
>  		if (jh_in->b_frozen_data) {
>  			jbd2_free(tmp, bh_in->b_size);
> -			goto repeat;
> +			goto copy_done;
>  		}
>  
>  		jh_in->b_frozen_data = tmp;
>  		memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
> -
> -		new_folio = virt_to_folio(tmp);
> -		new_offset = offset_in_folio(new_folio, tmp);
> -		done_copy_out = 1;
> -
>  		/*
>  		 * This isn't strictly necessary, as we're using frozen
>  		 * data for the escaping, but it keeps consistency with
>  		 * b_frozen_data usage.
>  		 */
>  		jh_in->b_frozen_triggers = jh_in->b_triggers;
> +
> +copy_done:
> +		new_folio = virt_to_folio(jh_in->b_frozen_data);
> +		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> +		done_copy_out = 1;
>  	}
>  
>  	/*
> 

Re: [PATCH v2 2/9] jbd2: remove unused return info from jbd2_journal_write_metadata_buffer

[Jan Kara]

On Tue 14-05-24 19:24:31, Kemeng Shi wrote:
> The done_copy_out info from jbd2_journal_write_metadata_buffer is not
> used. Simply remove it.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/commit.c  | 10 +++++-----
>  fs/jbd2/journal.c |  9 +++------
>  2 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 5e122586e06e..67077308b56b 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -353,7 +353,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  	struct buffer_head *descriptor;
>  	struct buffer_head **wbuf = journal->j_wbuf;
>  	int bufs;
> -	int flags;
> +	int escape;
>  	int err;
>  	unsigned long long blocknr;
>  	ktime_t start_time;
> @@ -661,10 +661,10 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		 */
>  		set_bit(BH_JWrite, &jh2bh(jh)->b_state);
>  		JBUFFER_TRACE(jh, "ph3: write metadata");
> -		flags = jbd2_journal_write_metadata_buffer(commit_transaction,
> +		escape = jbd2_journal_write_metadata_buffer(commit_transaction,
>  						jh, &wbuf[bufs], blocknr);
> -		if (flags < 0) {
> -			jbd2_journal_abort(journal, flags);
> +		if (escape < 0) {
> +			jbd2_journal_abort(journal, escape);
>  			continue;
>  		}
>  		jbd2_file_log_bh(&io_bufs, wbuf[bufs]);
> @@ -673,7 +673,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>                     buffer */
>  
>  		tag_flag = 0;
> -		if (flags & 1)
> +		if (escape)
>  			tag_flag |= JBD2_FLAG_ESCAPE;
>  		if (!first_tag)
>  			tag_flag |= JBD2_FLAG_SAME_UUID;
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 207b24e12ce9..2dca2f613a8e 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -316,11 +316,8 @@ static void journal_kill_thread(journal_t *journal)
>   *
>   * Return value:
>   *  <0: Error
> - * >=0: Finished OK
> - *
> - * On success:
> - * Bit 0 set == escape performed on the data
> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
> + *  =0: Finished OK without escape
> + *  =1: Finished OK with escape
>   */
>  
>  int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> @@ -455,7 +452,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	set_buffer_shadow(bh_in);
>  	spin_unlock(&jh_in->b_state_lock);
>  
> -	return do_escape | (done_copy_out << 1);
> +	return do_escape;
>  }
>  
>  /*
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 4/9] jbd2: jump to new copy_done tag when b_frozen_data is created concurrently

[Jan Kara]

On Tue 14-05-24 19:24:33, Kemeng Shi wrote:
> If b_frozen_data is created concurrently, we can update new_folio and
> new_offset with b_frozen_data and then move forward
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/journal.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 714e2ef0115a..5fb5062cf7ae 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -351,7 +351,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	atomic_set(&new_bh->b_count, 1);
>  
>  	spin_lock(&jh_in->b_state_lock);
> -repeat:
>  	/*
>  	 * If a new transaction has already done a buffer copy-out, then
>  	 * we use that version of the data for the commit.
> @@ -399,22 +398,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  		spin_lock(&jh_in->b_state_lock);
>  		if (jh_in->b_frozen_data) {
>  			jbd2_free(tmp, bh_in->b_size);
> -			goto repeat;
> +			goto copy_done;
>  		}
>  
>  		jh_in->b_frozen_data = tmp;
>  		memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
> -
> -		new_folio = virt_to_folio(tmp);
> -		new_offset = offset_in_folio(new_folio, tmp);
> -		done_copy_out = 1;
> -
>  		/*
>  		 * This isn't strictly necessary, as we're using frozen
>  		 * data for the escaping, but it keeps consistency with
>  		 * b_frozen_data usage.
>  		 */
>  		jh_in->b_frozen_triggers = jh_in->b_triggers;
> +
> +copy_done:
> +		new_folio = virt_to_folio(jh_in->b_frozen_data);
> +		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> +		done_copy_out = 1;
>  	}
>  
>  	/*
> -- 
> 2.30.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 0/9] A fix and some cleanups to jbd2

[Theodore Ts'o]

On Tue, 14 May 2024 19:24:29 +0800, Kemeng Shi wrote:
> v1->v2:
> -Collect RVB from Yi and Jan
> -remove rename "flags" to "escape" in patch 2/9
> -goto new copy_done tag in patch 4/9
> -add more comment in patch 5/9
> 
> Patch 1 fixes memleak in jbd2_journal_write_metadata_buffer.
> Patch 2-6 contain some cleanups to jbd2_journal_write_metadata_buffer().
> Patch 7-9 contain some cleanups to kjournald2()
> All tests in "kvm-xfstest smoke" survive. Please let me konw if more tests
> should be ran. Thanks.
> 
> [...]

Applied, thanks!

[1/9] jbd2: avoid memleak in jbd2_journal_write_metadata_buffer
      commit: cc102aa24638b90e04364d64e4f58a1fa91a1976
[2/9] jbd2: remove unused return info from jbd2_journal_write_metadata_buffer
      commit: abe48a52250a91f0d1f9b5052a246c63cb845827
[3/9] jbd2: remove unnedded "need_copy_out" in jbd2_journal_write_metadata_buffer
      commit: 5dd3e8c0758a7c7a8c8f3002b2632a3c9d31c808
[4/9] jbd2: jump to new copy_done tag when b_frozen_data is created concurrently
      commit: 4c15129aaad54af2df4665ddde9245788ee9fa9c
[5/9] jbd2: remove unneeded kmap to do escape in jbd2_journal_write_metadata_buffer
      commit: daabedd664021afa9e7e3225489888a4fee99bad
[6/9] jbd2: use bh_in instead of jh2bh(jh_in) to simplify code
      commit: 4eb9bd13eba3c168e91460972a76ddd53ac3b2b0
[7/9] jbd2: remove dead equality check of j_commit_[sequence/request] in kjournald2
      commit: d5c545735aa0d6443460ac407d35f8fa235d5102
[8/9] jbd2: remove dead check of JBD2_UNMOUNT in kjournald2
      commit: 136d3e0703e807b14ed6f9b8a5a544b6ba08d940
[9/9] jbd2: remove unnecessary "should_sleep" in kjournald2
      commit: b07855348b305c234d9fabb7ab9b50fa9b3a7759

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