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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e6cceeee-9ff1-4638-8521-19ab40593693@huaweicloud.com>
Date: Tue, 24 Sep 2024 16:11:08 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 ritesh.list@...il.com, yi.zhang@...wei.com, chengzhihao1@...wei.com,
 yukuai3@...wei.com
Subject: Re: [PATCH v2 10/10] ext4: factor out a common helper to lock and
 flush data before fallocate

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@...wei.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@...wei.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;
>> +}
>