linux-kernel - Re: [PATCH 5/7] jbd2: remove unneeded done_copy_out variable in jbd2_journal_write_metadata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0f83f5e5-97d1-fc63-f611-21cd08e9e70b@huaweicloud.com>
Date: Wed, 31 Jul 2024 09:34:49 +0800
From: Kemeng Shi <shikemeng@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: tytso@....edu, jack@...e.com, linux-ext4@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] jbd2: remove unneeded done_copy_out variable in
 jbd2_journal_write_metadata_buffer



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@...weicloud.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
>>