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