1. Patchew
  2. linux
  3. [PATCH 0/7] Fix and cleanups to jbd2
  4. View patch

[PATCH 5/7] jbd2: remove unneeded done_copy_out variable in jbd2_journal_write_metadata_buffer

Kemeng Shi posted 7 patches 1 year, 6 months ago
There is a newer version of this series
[PATCH 5/7] jbd2: remove unneeded done_copy_out variable in jbd2_journal_write_metadata_buffer
Posted by Kemeng Shi 1 year, 6 months ago 
It's more intuitive to use jh_in->b_frozen_data directly instead of
done_copy_out variable. Simply remove unneeded done_copy_out variable
and use b_frozen_data instead.

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

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 9c1ffb0dc740..f17d05bc61df 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -318,7 +318,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 				  struct buffer_head **bh_out,
 				  sector_t blocknr)
 {
-	int done_copy_out = 0;
 	int do_escape = 0;
 	char *mapped_data;
 	struct buffer_head *new_bh;
@@ -349,7 +348,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	 * we use that version of the data for the commit.
 	 */
 	if (jh_in->b_frozen_data) {
-		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);
 		mapped_data = jh_in->b_frozen_data;
@@ -357,17 +355,15 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		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);
-	}
-
-	/*
-	 * Fire data frozen trigger if data already wasn't frozen.  Do this
-	 * before checking for escaping, as the trigger may modify the magic
-	 * offset.  If a copy-out happens afterwards, it will have the correct
-	 * data in the buffer.
-	 */
-	if (!done_copy_out)
+		/*
+		 * Fire data frozen trigger if data already wasn't frozen. Do
+		 * this before checking for escaping, as the trigger may modify
+		 * the magic offset.  If a copy-out happens afterwards, it will
+		 * have the correct data in the buffer.
+		 */
 		jbd2_buffer_frozen_trigger(jh_in, mapped_data,
 					   jh_in->b_triggers);
+	}
 
 	/*
 	 * Check for escaping
@@ -380,7 +376,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	/*
 	 * Do we need to do a data copy?
 	 */
-	if (do_escape && !done_copy_out) {
+	if (do_escape && !jh_in->b_frozen_data) {
 		char *tmp;
 
 		spin_unlock(&jh_in->b_state_lock);
@@ -408,7 +404,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 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
Re: [PATCH 5/7] jbd2: remove unneeded done_copy_out variable in jbd2_journal_write_metadata_buffer
Posted by Jan Kara 1 year, 6 months ago 
On Tue 30-07-24 19:33:33, Kemeng Shi wrote:
> It's more intuitive to use jh_in->b_frozen_data directly instead of
> done_copy_out variable. Simply remove unneeded done_copy_out variable
> and use b_frozen_data instead.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

> @@ -357,17 +355,15 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  		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);
> -	}
> -
> -	/*
> -	 * Fire data frozen trigger if data already wasn't frozen.  Do this
> -	 * before checking for escaping, as the trigger may modify the magic
> -	 * offset.  If a copy-out happens afterwards, it will have the correct
> -	 * data in the buffer.
> -	 */
> -	if (!done_copy_out)
> +		/*
> +		 * Fire data frozen trigger if data already wasn't frozen. Do
> +		 * this before checking for escaping, as the trigger may modify
> +		 * the magic offset.  If a copy-out happens afterwards, it will
> +		 * have the correct data in the buffer.
> +		 */
>  		jbd2_buffer_frozen_trigger(jh_in, mapped_data,
>  					   jh_in->b_triggers);
> +	}

I like how you've got rid of the conditional. But I'd go further and also
move the escaping check and thus unmap & possible frozen buffer creation
into the branch. Like:

		do_escape = jbd2_data_needs_escaping(mapped_data);
			- create this trivial helper
		kunmap_local(mapped_data);
		if (do_escape) {
			handle b_frozen_data creation
		}

								Honza

>  
>  	/*
>  	 * Check for escaping
> @@ -380,7 +376,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  	/*
>  	 * Do we need to do a data copy?
>  	 */
> -	if (do_escape && !done_copy_out) {
> +	if (do_escape && !jh_in->b_frozen_data) {
>  		char *tmp;
>  
>  		spin_unlock(&jh_in->b_state_lock);
> @@ -408,7 +404,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>  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 5/7] jbd2: remove unneeded done_copy_out variable in jbd2_journal_write_metadata_buffer
Posted by Kemeng Shi 1 year, 6 months ago 

on 7/30/2024 9:49 PM, Jan Kara wrote:
> On Tue 30-07-24 19:33:33, Kemeng Shi wrote:
>> It's more intuitive to use jh_in->b_frozen_data directly instead of
>> done_copy_out variable. Simply remove unneeded done_copy_out variable
>> and use b_frozen_data instead.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
>> @@ -357,17 +355,15 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>>  		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);
>> -	}
>> -
>> -	/*
>> -	 * Fire data frozen trigger if data already wasn't frozen.  Do this
>> -	 * before checking for escaping, as the trigger may modify the magic
>> -	 * offset.  If a copy-out happens afterwards, it will have the correct
>> -	 * data in the buffer.
>> -	 */
>> -	if (!done_copy_out)
>> +		/*
>> +		 * Fire data frozen trigger if data already wasn't frozen. Do
>> +		 * this before checking for escaping, as the trigger may modify
>> +		 * the magic offset.  If a copy-out happens afterwards, it will
>> +		 * have the correct data in the buffer.
>> +		 */
>>  		jbd2_buffer_frozen_trigger(jh_in, mapped_data,
>>  					   jh_in->b_triggers);
>> +	}
> 
> I like how you've got rid of the conditional. But I'd go further and also
> move the escaping check and thus unmap & possible frozen buffer creation
> into the branch. Like:
> 
> 		do_escape = jbd2_data_needs_escaping(mapped_data);
> 			- create this trivial helper
> 		kunmap_local(mapped_data);
> 		if (do_escape) {
> 			handle b_frozen_data creation
> 		}
Thanks Jan. It does look better this way, I will do it in next version.

Kemeng.
> 
> 								Honza
> 
>>  
>>  	/*
>>  	 * Check for escaping
>> @@ -380,7 +376,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>>  	/*
>>  	 * Do we need to do a data copy?
>>  	 */
>> -	if (do_escape && !done_copy_out) {
>> +	if (do_escape && !jh_in->b_frozen_data) {
>>  		char *tmp;
>>  
>>  		spin_unlock(&jh_in->b_state_lock);
>> @@ -408,7 +404,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
>>  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
>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.