Re: [PATCH v2 10/10] ext4: factor out a common helper to lock and flush data before fallocate

[Jan Kara <jack@suse.cz>]

On Wed 04-09-24 14:29:25, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Now the beginning of the first four functions in ext4_fallocate() (punch
> hole, zero range, insert range and collapse range) are almost the same,
> they need to wait for the dio to finish, get filemap invalidate lock,
> write back dirty data and finally drop page cache. Factor out a common
> helper to do these work can reduce a lot of the redundant code.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

I like that we factor out this functionality in a common helper. But see
below:

> @@ -4731,6 +4707,52 @@ static int ext4_fallocate_check(struct inode *inode, int mode,
>  	return 0;
>  }
>  
> +int ext4_prepare_falloc(struct file *file, loff_t start, loff_t end, int mode)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct address_space *mapping = inode->i_mapping;
> +	int ret;
> +
> +	/* Wait all existing dio workers, newcomers will block on i_rwsem */
> +	inode_dio_wait(inode);
> +	ret = file_modified(file);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Prevent page faults from reinstantiating pages we have released
> +	 * from page cache.
> +	 */
> +	filemap_invalidate_lock(mapping);
> +
> +	ret = ext4_break_layouts(inode);
> +	if (ret)
> +		goto failed;
> +
> +	/*
> +	 * Write data that will be zeroed to preserve them when successfully
> +	 * discarding page cache below but fail to convert extents.
> +	 */
> +	ret = filemap_write_and_wait_range(mapping, start, end);

The comment is somewhat outdated now. Also the range is wrong for collapse
and insert range. There we need to writeout data upto the EOF because we
truncate it below.

								Honza

> +	if (ret)
> +		goto failed;
> +
> +	/*
> +	 * For insert range and collapse range, COWed private pages should
> +	 * be removed since the file's logical offset will be changed, but
> +	 * punch hole and zero range doesn't.
> +	 */
> +	if (mode & (FALLOC_FL_INSERT_RANGE | FALLOC_FL_COLLAPSE_RANGE))
> +		truncate_pagecache(inode, start);
> +	else
> +		truncate_pagecache_range(inode, start, end);
> +
> +	return 0;
> +failed:
> +	filemap_invalidate_unlock(mapping);
> +	return ret;
> +}

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR