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

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

[Kemeng Shi]

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: move repeat tag around to remove a repeat check of b_frozen_data
  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/journal.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

-- 
2.30.0

[PATCH 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>
---
 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 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/journal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 207b24e12ce9..068031f35aea 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
  *
  * On success:
  * Bit 0 set == escape performed on the data
- * Bit 1 set == buffer copy-out performed (kfree the data after IO)
  */
 
 int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
@@ -455,7 +454,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 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>
---
 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 068031f35aea..9a35d0c5b38c 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -327,7 +327,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;
@@ -382,16 +381,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 4/9] jbd2: move repeat tag around to remove a repeat check of b_frozen_data

[Kemeng Shi]

We make sure b_frozen_data is not NULL before jump to "repeat" tag, move
"repeat" tag around to remove repeat check of b_frozen_data.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/jbd2/journal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 9a35d0c5b38c..77fcdc76fdfd 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -353,12 +353,12 @@ 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.
 	 */
 	if (jh_in->b_frozen_data) {
+repeat:
 		done_copy_out = 1;
 		new_folio = virt_to_folio(jh_in->b_frozen_data);
 		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
-- 
2.30.0

[PATCH 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>
---
 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 77fcdc76fdfd..87f558bd2e8a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -423,11 +423,8 @@ 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.
 	 */
-	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 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>
---
 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 87f558bd2e8a..01e33b643e4d 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -363,8 +363,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 7/9] jbd2: remove dead equality check of j_commit_[sequence/request] in kjournald2

[Kemeng Shi]

In kjournald2, two equality checks of j_commit_[sequence/request] are
under the same j_state_lock. As j_commit_[sequence/request] are updated
concurrently with j_state_lock held during runtime, the second check is
unnecessary.
The j_commit_sequence is only updated concurrently in
jbd2_journal_commit_transaction with j_state_lock held.
The j_commit_request is only updated concurrently in
__jbd2_log_start_commit with j_state_lock held.
Also see comment in struct journal_s about lock rule of j_commit_sequence
and j_commit_request.

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

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 01e33b643e4d..e8f592fbd6e1 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 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.
Also see comment in struct journal_s about lock rule of j_flags.

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

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index e8f592fbd6e1..ce9004f40ffb 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 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>
---
 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 ce9004f40ffb..65c6cfce9d92 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 1/9] jbd2: avoid memleak in jbd2_journal_write_metadata_buffer

[Zhang Yi]

On 2024/5/6 22:17, Kemeng Shi wrote:
> The new_bh is from alloc_buffer_head, we should call free_buffer_head to
> free it in error case.

Oh, yeah, this is a separate bh, so it should be freed explicitly, it
looks good to me.

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

> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  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);
> 

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

[Zhang Yi]

On 2024/5/6 22:17, 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>
> ---
>  fs/jbd2/journal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 207b24e12ce9..068031f35aea 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
>   *
>   * On success:
>   * Bit 0 set == escape performed on the data
> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
>   */
>  
>  int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> @@ -455,7 +454,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;
>  }
>  

I checked the history, it seems that this bit has not been used since
the very beginning when the jbd code was merged in, I suppose we could
drop it. Since there is only one flag that is still in use, why not just
drop the flag and pass out do_escape through an output parameter, or
directly pass tag_flag, after that we could also drop the weird
"if (flags & 1)" check in jbd2_journal_commit_transaction()?

Thanks,
Yi.

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

[Zhang Yi]

On 2024/5/6 22:17, Kemeng Shi wrote:
> 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>

Make sense, looks good to me.

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

> ---
>  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 068031f35aea..9a35d0c5b38c 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -327,7 +327,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;
> @@ -382,16 +381,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);
> 

Re: [PATCH 4/9] jbd2: move repeat tag around to remove a repeat check of b_frozen_data

[Zhang Yi]

On 2024/5/6 22:17, Kemeng Shi wrote:
> We make sure b_frozen_data is not NULL before jump to "repeat" tag, move
> "repeat" tag around to remove repeat check of b_frozen_data.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/jbd2/journal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 9a35d0c5b38c..77fcdc76fdfd 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -353,12 +353,12 @@ 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.
>  	 */
>  	if (jh_in->b_frozen_data) {
> +repeat:
>  		done_copy_out = 1;
>  		new_folio = virt_to_folio(jh_in->b_frozen_data);
>  		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> 

I suppose we could drop the repeat tag entirely, just set the new_folio and
new_offset, and then goto handle do_escape. We don't need to call
jbd2_buffer_frozen_trigger() and check for escaping again, is that right?

Thanks,
Yi.

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

[Zhang Yi]

On 2024/5/6 22:17, Kemeng Shi wrote:
> 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>

Looks good to me.

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

> ---
>  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 77fcdc76fdfd..87f558bd2e8a 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -423,11 +423,8 @@ 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.
>  	 */
> -	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;
> 

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

[Zhang Yi]

On 2024/5/6 22:17, Kemeng Shi wrote:
> 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>

Looks good to me.

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

> ---
>  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 87f558bd2e8a..01e33b643e4d 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -363,8 +363,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);
> 

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

[Kemeng Shi]

on 5/7/2024 7:51 PM, Zhang Yi wrote:
> On 2024/5/6 22:17, 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>
>> ---
>>  fs/jbd2/journal.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 207b24e12ce9..068031f35aea 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
>>   *
>>   * On success:
>>   * Bit 0 set == escape performed on the data
>> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
>>   */
>>  
>>  int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>> @@ -455,7 +454,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;
>>  }
>>  
> 
> I checked the history, it seems that this bit has not been used since
> the very beginning when the jbd code was merged in, I suppose we could
> drop it. Since there is only one flag that is still in use, why not just
> drop the flag and pass out do_escape through an output parameter, or
> directly pass tag_flag, after that we could also drop the weird
> "if (flags & 1)" check in jbd2_journal_commit_transaction()?
Thanks for looking into this. I agree that the "flags & 1" is wired, I
wonder if we could further remove "flags & 1" with following change:

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 08c59197c5bd..c8a1eca5caab 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -309,10 +309,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
+ *  =0: Finished OK without escape
+ *  =1: Finished OK with escape
  */

Look forward to your reply.
Thanks.
Kemeng

> 
> Thanks,
> Yi.
> 

Re: [PATCH 4/9] jbd2: move repeat tag around to remove a repeat check of b_frozen_data

[Kemeng Shi]

on 5/7/2024 8:41 PM, Zhang Yi wrote:
> On 2024/5/6 22:17, Kemeng Shi wrote:
>> We make sure b_frozen_data is not NULL before jump to "repeat" tag, move
>> "repeat" tag around to remove repeat check of b_frozen_data.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/jbd2/journal.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 9a35d0c5b38c..77fcdc76fdfd 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -353,12 +353,12 @@ 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.
>>  	 */
>>  	if (jh_in->b_frozen_data) {
>> +repeat:
>>  		done_copy_out = 1;
>>  		new_folio = virt_to_folio(jh_in->b_frozen_data);
>>  		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
>>
> 
> I suppose we could drop the repeat tag entirely, just set the new_folio and
> new_offset, and then goto handle do_escape. We don't need to call
> jbd2_buffer_frozen_trigger() and check for escaping again, is that right?
Sure, sounds reasonable to me. Will do it in next version. Thanks
> 
> Thanks,
> Yi.
> 

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

[Zhang Yi]

On 2024/5/6 22:17, Kemeng Shi wrote:
> In kjournald2, two equality checks of j_commit_[sequence/request] are
> under the same j_state_lock. As j_commit_[sequence/request] are updated
> concurrently with j_state_lock held during runtime, the second check is
> unnecessary.
> The j_commit_sequence is only updated concurrently in
> jbd2_journal_commit_transaction with j_state_lock held.
> The j_commit_request is only updated concurrently in
> __jbd2_log_start_commit with j_state_lock held.
> Also see comment in struct journal_s about lock rule of j_commit_sequence
> and j_commit_request.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks reasonable to me.

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

> ---
>  fs/jbd2/journal.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 01e33b643e4d..e8f592fbd6e1 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))
> 

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

[Zhang Yi]

On 2024/5/6 22:18, Kemeng Shi wrote:
> 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.
> Also see comment in struct journal_s about lock rule of j_flags.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Looks good to me.

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

> ---
>  fs/jbd2/journal.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index e8f592fbd6e1..ce9004f40ffb 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();
> 

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

[Zhang Yi]

On 2024/5/6 22:18, Kemeng Shi wrote:
> We only need to sleep if no running transaction is expired. Simply remove
> unnecessary "should_sleep".
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

It looks much clearer now.

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

> ---
>  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 ce9004f40ffb..65c6cfce9d92 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);
> 

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

[Jan Kara]

On Mon 06-05-24 22:17:53, Kemeng Shi wrote:
> 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>

Looks good. Feel free to add:

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

								Honza

> ---
>  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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Wed 08-05-24 09:42:30, Kemeng Shi wrote:
> 
> 
> on 5/7/2024 7:51 PM, Zhang Yi wrote:
> > On 2024/5/6 22:17, 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>
> >> ---
> >>  fs/jbd2/journal.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> >> index 207b24e12ce9..068031f35aea 100644
> >> --- a/fs/jbd2/journal.c
> >> +++ b/fs/jbd2/journal.c
> >> @@ -320,7 +320,6 @@ static void journal_kill_thread(journal_t *journal)
> >>   *
> >>   * On success:
> >>   * Bit 0 set == escape performed on the data
> >> - * Bit 1 set == buffer copy-out performed (kfree the data after IO)
> >>   */
> >>  
> >>  int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >> @@ -455,7 +454,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;
> >>  }
> >>  
> > 
> > I checked the history, it seems that this bit has not been used since
> > the very beginning when the jbd code was merged in, I suppose we could
> > drop it. Since there is only one flag that is still in use, why not just
> > drop the flag and pass out do_escape through an output parameter, or
> > directly pass tag_flag, after that we could also drop the weird
> > "if (flags & 1)" check in jbd2_journal_commit_transaction()?
> Thanks for looking into this. I agree that the "flags & 1" is wired, I
> wonder if we could further remove "flags & 1" with following change:
> 
> 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 08c59197c5bd..c8a1eca5caab 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -309,10 +309,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
> + *  =0: Finished OK without escape
> + *  =1: Finished OK with escape
>   */
> 
> Look forward to your reply.

Yep, looks like a fine solution to me.

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

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

[Jan Kara]

On Mon 06-05-24 22:17:55, Kemeng Shi wrote:
> 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>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  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 068031f35aea..9a35d0c5b38c 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -327,7 +327,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;
> @@ -382,16 +381,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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 06-05-24 22:17:57, Kemeng Shi wrote:
> 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>

Ah, good point. But this deserves a comment at that site that jbd2_alloc()
always provides an address from the direct kernel mapping. Otherwise feel
free to add:

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

								Honza

> ---
>  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 77fcdc76fdfd..87f558bd2e8a 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -423,11 +423,8 @@ 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.
>  	 */
> -	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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 06-05-24 22:17:58, Kemeng Shi wrote:
> 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>

Looks good. Feel free to add:

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

								Honza

> ---
>  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 87f558bd2e8a..01e33b643e4d 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -363,8 +363,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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 06-05-24 22:17:59, Kemeng Shi wrote:
> In kjournald2, two equality checks of j_commit_[sequence/request] are
> under the same j_state_lock. As j_commit_[sequence/request] are updated
> concurrently with j_state_lock held during runtime, the second check is
> unnecessary.
> The j_commit_sequence is only updated concurrently in
> jbd2_journal_commit_transaction with j_state_lock held.
> The j_commit_request is only updated concurrently in
> __jbd2_log_start_commit with j_state_lock held.
> Also see comment in struct journal_s about lock rule of j_commit_sequence
> and j_commit_request.
> 
> 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 | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 01e33b643e4d..e8f592fbd6e1 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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 06-05-24 22:18:00, Kemeng Shi wrote:
> 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.
> Also see comment in struct journal_s about lock rule of j_flags.
> 
> 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 | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index e8f592fbd6e1..ce9004f40ffb 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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 06-05-24 22:18:01, Kemeng Shi wrote:
> We only need to sleep if no running transaction is expired. Simply remove
> unnecessary "should_sleep".
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Nice! Feel free to add:

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

								Honza

> ---
>  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 ce9004f40ffb..65c6cfce9d92 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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR