linux-ext4 - Re: [RFC PATCH 3/4] ext4: factor out write end code of inline file

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] [day] [month] [year] [list]

Date:   Sat, 10 Jul 2021 16:13:29 +0800
From:   Zhang Yi <yi.zhang@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <linux-ext4@...r.kernel.org>, <tytso@....edu>,
        <adilger.kernel@...ger.ca>, <yukuai3@...wei.com>
Subject: Re: [RFC PATCH 3/4] ext4: factor out write end code of inline file

On 2021/7/8 0:49, Jan Kara wrote:
> On Tue 06-07-21 10:42:09, Zhang Yi wrote:
>> Now that the inline_data file write end procedure are falled into the
>> common write end functions, it is not clear. Factor them out and do
>> some cleanup. This patch also drop ext4_da_write_inline_data_end()
>> and switch to use ext4_write_inline_data_end() instead because we also
>> need to do the same error processing if we failed to write data into
>> inline entry.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> 
> Looks good. Just two nits below.
>  
>> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
>> index 28b666f25ac2..8fbf8ec05bd5 100644
>> --- a/fs/ext4/inline.c
>> +++ b/fs/ext4/inline.c
>> @@ -729,34 +729,80 @@ int ext4_try_to_write_inline_data(struct address_space *mapping,
>>  int ext4_write_inline_data_end(struct inode *inode, loff_t pos, unsigned len,
>>  			       unsigned copied, struct page *page)
>>  {
>> -	int ret, no_expand;
>> +	handle_t *handle = ext4_journal_current_handle();
>> +	int i_size_changed = 0;
>> +	int no_expand;
>>  	void *kaddr;
>>  	struct ext4_iloc iloc;
>> +	int ret, ret2;
>>  
>>  	if (unlikely(copied < len) && !PageUptodate(page))
>> -		return 0;
>> +		copied = 0;
>>  
>> -	ret = ext4_get_inode_loc(inode, &iloc);
>> -	if (ret) {
>> -		ext4_std_error(inode->i_sb, ret);
>> -		return ret;
>> -	}
>> +	if (likely(copied)) {
>> +		ret = ext4_get_inode_loc(inode, &iloc);
>> +		if (ret) {
>> +			unlock_page(page);
>> +			put_page(page);
>> +			ext4_std_error(inode->i_sb, ret);
>> +			goto out;
>> +		}
>> +		ext4_write_lock_xattr(inode, &no_expand);
>> +		BUG_ON(!ext4_has_inline_data(inode));
>>  
>> -	ext4_write_lock_xattr(inode, &no_expand);
>> -	BUG_ON(!ext4_has_inline_data(inode));
>> +		kaddr = kmap_atomic(page);
>> +		ext4_write_inline_data(inode, &iloc, kaddr, pos, copied);
>> +		kunmap_atomic(kaddr);
>> +		SetPageUptodate(page);
>> +		/* clear page dirty so that writepages wouldn't work for us. */
>> +		ClearPageDirty(page);
>>  
>> -	kaddr = kmap_atomic(page);
>> -	ext4_write_inline_data(inode, &iloc, kaddr, pos, copied);
>> -	kunmap_atomic(kaddr);
>> -	SetPageUptodate(page);
>> -	/* clear page dirty so that writepages wouldn't work for us. */
>> -	ClearPageDirty(page);
>> +		ext4_write_unlock_xattr(inode, &no_expand);
>> +		brelse(iloc.bh);
>> +	}
>>  
>> -	ext4_write_unlock_xattr(inode, &no_expand);
>> -	brelse(iloc.bh);
>> -	mark_inode_dirty(inode);
>> +	/*
>> +	 * It's important to update i_size while still holding page lock:
>> +	 * page writeout could otherwise come in and zero beyond i_size.
>> +	 */
>> +	i_size_changed = ext4_update_inode_size(inode, pos + copied);
>> +	if (ext4_should_journal_data(inode)) {
>> +		ext4_set_inode_state(inode, EXT4_STATE_JDATA);
>> +		EXT4_I(inode)->i_datasync_tid = handle->h_transaction->t_tid;
>> +	}
> 
> I think this hunk should also go into the "if (copied)" block. There's no
> point in changing i_size or i_disksize when nothing was written.
> 

Yeah, I will put ext4_update_inode_size() into the "if (copied)" block.
Thinking about it again, IIUC, the hunk in "if (ext4_should_journal_data(inode))"
also seems useless for inline inode, and could be dropped.

Thanks,
Yi.