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.
> + unlock_page(page);
> + put_page(page);
>
> - return copied;
> + /*
> + * Don't mark the inode dirty under page lock. First, it unnecessarily
> + * makes the holding time of page lock longer. Second, it forces lock
> + * ordering of page lock and transaction start for journaling
> + * filesystems.
> + */
> + if (likely(copied) || i_size_changed)
> + mark_inode_dirty(inode);
And then it is obvious here that (copied == 0) => !i_size_changed so we can
just remove the i_size_changed term from the condition.
> +out:
> + /*
> + * If we have allocated more blocks and copied less. We will have
> + * blocks allocated outside inode->i_size, so truncate them.
> + */
> + if (pos + len > inode->i_size && ext4_can_truncate(inode))
> + ext4_orphan_add(handle, inode);
> +
> + ret2 = ext4_journal_stop(handle);
> + if (!ret)
> + ret = ret2;
> + if (pos + len > inode->i_size) {
> + ext4_truncate_failed_write(inode);
> + /*
> + * If truncate failed early the inode might still be
> + * on the orphan list; we need to make sure the inode
> + * is removed from the orphan list in that case.
> + */
> + if (inode->i_nlink)
> + ext4_orphan_del(NULL, inode);
> + }
> + return ret ? ret : copied;
> }
Honza
---
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
