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

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 2024/9/23 16:54, Jan Kara wrote:
> 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.

Sure, will update it in next iteration.

> Also the range is wrong for collapse
> and insert range. There we need to writeout data upto the EOF because we
> truncate it below.
> 

For collapse and insert range, I passed the length LLONG_MAX, which is
the same as before, this should've upto the EOF, so I think it's
right, or am I missing something?

ext4_collapse_range():

-	start = round_down(offset, PAGE_SIZE);
-	/* Write out all dirty pages */
-	ret = filemap_write_and_wait_range(mapping, start, LLONG_MAX);
+	ret = ext4_prepare_falloc(file, round_down(offset, PAGE_SIZE),
+				  LLONG_MAX, FALLOC_FL_COLLAPSE_RANGE);


ext4_insert_range():

-	start = round_down(offset, PAGE_SIZE);
-	/* Write out all dirty pages */
-	ret = filemap_write_and_wait_range(mapping, start, LLONG_MAX);
+	ret = ext4_prepare_falloc(file, round_down(offset, PAGE_SIZE),
+				  LLONG_MAX, FALLOC_FL_INSERT_RANGE);

Thanks,
Yi.

> 
>> +	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;
>> +}
>