Re: [PATCH v2 09/10] ext4: factor out the common checking part of all fallocate operations

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

On 2024/9/23 16:31, Jan Kara wrote:
> On Wed 04-09-24 14:29:24, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Now the beginning of all the five functions in ext4_fallocate() (punch
>> hole, zero range, insert range, collapse range and normal fallocate) are
>> almost the same, they need to hold i_rwsem and check the validity of
>> input parameters, so move the holding of i_rwsem to ext4_fallocate()
>> and factor out a common helper to check the input parameters can make
>> the code more clear.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ...
>> +static int ext4_fallocate_check(struct inode *inode, int mode,
>> +				loff_t offset, loff_t len)
>> +{
>> +	/* Currently except punch_hole, just for extent based files. */
>> +	if (!(mode & FALLOC_FL_PUNCH_HOLE) &&
>> +	    !ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * Insert range and collapse range works only on fs cluster size
>> +	 * aligned regions.
>> +	 */
>> +	if (mode & (FALLOC_FL_INSERT_RANGE | FALLOC_FL_COLLAPSE_RANGE) &&
>> +	    !IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(inode->i_sb)))
>> +		return -EINVAL;
>> +
>> +	if (mode & FALLOC_FL_INSERT_RANGE) {
>> +		/* Collapse range, offset must be less than i_size */
>> +		if (offset >= inode->i_size)
>> +			return -EINVAL;
>> +		/* Check whether the maximum file size would be exceeded */
>> +		if (len > inode->i_sb->s_maxbytes - inode->i_size)
>> +			return -EFBIG;
>> +	} else if (mode & FALLOC_FL_COLLAPSE_RANGE) {
>> +		/*
>> +		 * Insert range, there is no need to overlap collapse
>> +		 * range with EOF, in which case it is effectively a
>> +		 * truncate operation.
>> +		 */
>> +		if (offset + len >= inode->i_size)
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I don't think this helps. If the code is really shared, then the
> factorization is good but here you have to do various checks what operation
> we perform and in that case I don't think it really helps readability to
> factor out checks into a common function.
> 

Yeah, I think you are right, this is just move out the checks and
may increase the reading difficulty, it should be easier to understand
if they're still in their original places.

Thanks,
Yi.