diff mbox series

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

Message ID 20240904062925.716856-10-yi.zhang@huaweicloud.com (mailing list archive)
State New
Headers show
Series ext4: clean up and refactor fallocate | expand

Commit Message

Zhang Yi Sept. 4, 2024, 6:29 a.m. UTC
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>
---
 fs/ext4/extents.c | 132 ++++++++++++++++++----------------------------
 fs/ext4/inode.c   |  13 ++---
 2 files changed, 56 insertions(+), 89 deletions(-)

Comments

Jan Kara Sept. 23, 2024, 8:31 a.m. UTC | #1
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.

								Honza
Zhang Yi Sept. 24, 2024, 7:52 a.m. UTC | #2
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.
diff mbox series

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 06b2c1190181..91e509201915 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4548,23 +4548,14 @@  static long ext4_zero_range(struct file *file, loff_t offset,
 	int ret, flags, credits;
 
 	trace_ext4_zero_range(inode, offset, len, mode);
-
-	inode_lock(inode);
-
-	/*
-	 * Indirect files do not support unwritten extents
-	 */
-	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
+	WARN_ON_ONCE(!inode_is_locked(inode));
 
 	if (!(mode & FALLOC_FL_KEEP_SIZE) &&
 	    (end > inode->i_size || end > EXT4_I(inode)->i_disksize)) {
 		new_size = end;
 		ret = inode_newsize_ok(inode, new_size);
 		if (ret)
-			goto out;
+			return ret;
 	}
 
 	/* Wait all existing dio workers, newcomers will block on i_rwsem */
@@ -4572,7 +4563,7 @@  static long ext4_zero_range(struct file *file, loff_t offset,
 
 	ret = file_modified(file);
 	if (ret)
-		goto out;
+		return ret;
 
 	/*
 	 * Prevent page faults from reinstantiating pages we have released
@@ -4657,8 +4648,6 @@  static long ext4_zero_range(struct file *file, loff_t offset,
 	ext4_journal_stop(handle);
 out_invalidate_lock:
 	filemap_invalidate_unlock(mapping);
-out:
-	inode_unlock(inode);
 	return ret;
 }
 
@@ -4672,18 +4661,11 @@  static long ext4_do_fallocate(struct file *file, loff_t offset,
 	int ret;
 
 	trace_ext4_fallocate_enter(inode, offset, len, mode);
+	WARN_ON_ONCE(!inode_is_locked(inode));
 
 	start_lblk = offset >> inode->i_blkbits;
 	len_lblk = EXT4_MAX_BLOCKS(len, offset, inode->i_blkbits);
 
-	inode_lock(inode);
-
-	/* We only support preallocation for extent-based files only. */
-	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-
 	if (!(mode & FALLOC_FL_KEEP_SIZE) &&
 	    (end > inode->i_size || end > EXT4_I(inode)->i_disksize)) {
 		new_size = end;
@@ -4709,11 +4691,46 @@  static long ext4_do_fallocate(struct file *file, loff_t offset,
 					EXT4_I(inode)->i_sync_tid);
 	}
 out:
-	inode_unlock(inode);
 	trace_ext4_fallocate_exit(inode, offset, len_lblk, ret);
 	return ret;
 }
 
+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;
+}
+
 /*
  * preallocate space for a file. This implements ext4's fallocate file
  * operation, which gets called from sys_fallocate system call.
@@ -4744,9 +4761,12 @@  long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 
 	inode_lock(inode);
 	ret = ext4_convert_inline_data(inode);
-	inode_unlock(inode);
 	if (ret)
-		return ret;
+		goto out;
+
+	ret = ext4_fallocate_check(inode, mode, offset, len);
+	if (ret)
+		goto out;
 
 	if (mode & FALLOC_FL_PUNCH_HOLE)
 		ret = ext4_punch_hole(file, offset, len);
@@ -4758,7 +4778,8 @@  long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		ret = ext4_zero_range(file, offset, len, mode);
 	else
 		ret = ext4_do_fallocate(file, offset, len, mode);
-
+out:
+	inode_unlock(inode);
 	return ret;
 }
 
@@ -5267,36 +5288,14 @@  static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
 	int ret;
 
 	trace_ext4_collapse_range(inode, offset, len);
-
-	inode_lock(inode);
-
-	/* Currently just for extent based files */
-	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-
-	/* Collapse range works only on fs cluster size aligned regions. */
-	if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/*
-	 * 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) {
-		ret = -EINVAL;
-		goto out;
-	}
+	WARN_ON_ONCE(!inode_is_locked(inode));
 
 	/* Wait for existing dio to complete */
 	inode_dio_wait(inode);
 
 	ret = file_modified(file);
 	if (ret)
-		goto out;
+		return ret;
 
 	/*
 	 * Prevent page faults from reinstantiating pages we have released from
@@ -5365,8 +5364,6 @@  static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
 	ext4_journal_stop(handle);
 out_invalidate_lock:
 	filemap_invalidate_unlock(mapping);
-out:
-	inode_unlock(inode);
 	return ret;
 }
 
@@ -5392,39 +5389,14 @@  static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
 	loff_t start;
 
 	trace_ext4_insert_range(inode, offset, len);
-
-	inode_lock(inode);
-
-	/* Currently just for extent based files */
-	if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-
-	/* Insert range works only on fs cluster size aligned regions. */
-	if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/* Offset must be less than i_size */
-	if (offset >= inode->i_size) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/* Check whether the maximum file size would be exceeded */
-	if (len > inode->i_sb->s_maxbytes - inode->i_size) {
-		ret = -EFBIG;
-		goto out;
-	}
+	WARN_ON_ONCE(!inode_is_locked(inode));
 
 	/* Wait for existing dio to complete */
 	inode_dio_wait(inode);
 
 	ret = file_modified(file);
 	if (ret)
-		goto out;
+		return ret;
 
 	/*
 	 * Prevent page faults from reinstantiating pages we have released from
@@ -5525,8 +5497,6 @@  static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
 	ext4_journal_stop(handle);
 out_invalidate_lock:
 	filemap_invalidate_unlock(mapping);
-out:
-	inode_unlock(inode);
 	return ret;
 }
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index dfaf9e9d6ad8..57636c656fa5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3923,15 +3923,14 @@  int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 	unsigned long blocksize = i_blocksize(inode);
 	handle_t *handle;
 	unsigned int credits;
-	int ret = 0;
+	int ret;
 
 	trace_ext4_punch_hole(inode, offset, length, 0);
-
-	inode_lock(inode);
+	WARN_ON_ONCE(!inode_is_locked(inode));
 
 	/* No need to punch hole beyond i_size */
 	if (offset >= inode->i_size)
-		goto out;
+		return 0;
 
 	/*
 	 * If the hole extends beyond i_size, set the hole to end after
@@ -3951,7 +3950,7 @@  int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 	if (offset & (blocksize - 1) || end & (blocksize - 1)) {
 		ret = ext4_inode_attach_jinode(inode);
 		if (ret < 0)
-			goto out;
+			return ret;
 	}
 
 	/* Wait all existing dio workers, newcomers will block on i_rwsem */
@@ -3959,7 +3958,7 @@  int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 
 	ret = file_modified(file);
 	if (ret)
-		goto out;
+		return ret;
 
 	/*
 	 * Prevent page faults from reinstantiating pages we have released from
@@ -4036,8 +4035,6 @@  int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 	ext4_journal_stop(handle);
 out_invalidate_lock:
 	filemap_invalidate_unlock(mapping);
-out:
-	inode_unlock(inode);
 	return ret;
 }