Re: [RFC PATCH 1/2] ext4: add a version of convert_inline_data_nolock() for regular files

[Eric Biggers <ebiggers3@gmail.com>]

Hi Ted,

On Mon, Jun 05, 2017 at 08:03:58PM -0400, Theodore Ts'o wrote:
> The current version of ext4_convert_inline_data_nolock() should not be
> used for regular files, since it does the conversion via using jbd2,
> and this if we are not using data journalling, this is unsafe.  If the
> data block is updated after the inode is converted from using inline
> data to using a data block, when the journal is replayed, the new
> version of the data block can get overwritten by the old version of
> the data block.

Thanks for trying to fix this mess!  I'm not sure I know enough to fully review
it, but here are some things I'm questioning...

> 
> The ext4_convert_inline_data_nolock() also doesn't handle races with
> Direct I/O correctly.
> 

Are you sure direct I/O is an issue?   ext4_direct_IO() doesn't support files
with inline data; it returns 0 if 'ext4_has_inline_data(inode)'.

>  /*
> + * Only used for regular files, not directories!
> + *
> + * The inode and page must be locked when this function is called.
> + */
> +static int reg_convert_inline_data_nolock(handle_t *handle,
> +					  struct inode *inode,
> +					  struct page *page,
> +					  struct ext4_iloc *iloc)
> +{
> +	int error;
> +	struct buffer_head *data_bh = NULL;
> +	int inline_size = ext4_get_inline_size(inode);
> +
> +	if (!(inode->i_state & (I_NEW|I_FREEING)))
> +		WARN_ON(!inode_is_locked(inode));

I don't think the inode lock is guaranteed... ext4_convert_inline_data() is
called from ext4_fallocate() and ext4_page_mkwrite() without out.  It seems the
xattr_sem will always be taken for write, though.

> +
> +	/* If some one has already done this for us, just exit. */
> +	if (!ext4_has_inline_data(inode))
> +		return 0;
> +
> +	if (!PageUptodate(page)) {
> +		error = ext4_read_inline_page(inode, page);
> +		if (error < 0)
> +			return error;
> +	}
> +
> +	/* Avoid races with DIO */
> +	ext4_inode_block_unlocked_dio(inode);

What is ext4_inode_block_unlocked_dio() even supposed to do?  It sets
EXT4_STATE_DIOREAD_LOCK but nothing seems to check it.


> +	inode_dio_wait(inode);
> +
> +	error = ext4_destroy_inline_data_nolock(handle, inode);
> +	if (error)
> +		return error;
> +
> +	error = __block_write_begin(page, 0, inline_size, ext4_get_block);
> +	if (error)
> +		goto out_restore;
> +
> +	data_bh = page_buffers(page);
> +	BUG_ON(data_bh == NULL);
> +
> +	if (ext4_should_journal_data(inode)) {
> +		lock_buffer(data_bh);
> +		error = ext4_journal_get_create_access(handle, data_bh);
> +		if (error) {
> +			unlock_buffer(data_bh);
> +			error = -EIO;
> +			goto out_restore;
> +		}
> +		error = ext4_handle_dirty_metadata(handle, inode, data_bh);
> +		unlock_buffer(data_bh);
> +	} else {
> +		__set_page_dirty_buffers(page);
> +		if (ext4_should_order_data(inode))
> +			error = ext4_jbd2_inode_add_write(handle, inode);
> +	}
> +
> +out_restore:
> +	if (error) {
> +		void *kaddr = kmap_atomic(page);

kmap(), not kmap_atomic(), since the stuff in between can block.

>  
> +	if (S_ISREG(inode->i_mode)) {
> +		page = grab_cache_page_write_begin(inode->i_mapping, 0, 0);
> +		if (!page)
> +			return -ENOMEM;
> +	}
> +
>  	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, needed_blocks);
>  	if (IS_ERR(handle)) {
>  		error = PTR_ERR(handle);
> @@ -2027,11 +2121,21 @@ int ext4_convert_inline_data(struct inode *inode)
>  	}

Doesn't the page lock rank below transaction start?

Eric