Re: [PATCH 6/6] xfs: refactor xfs_file_fallocate

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Aug 27, 2024 at 08:50:50AM +0200, Christoph Hellwig wrote:
> Refactor xfs_file_fallocate into separate helpers for each mode,
> two factors for i_size handling and a single switch statement over the
> supported modes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Much less complicated now! :)

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c | 330 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 208 insertions(+), 122 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 489bc1b173c268..f6e4912769a0d5 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -852,6 +852,192 @@ static inline bool xfs_file_sync_writes(struct file *filp)
>  	return false;
>  }
>  
> +static int
> +xfs_falloc_newsize(
> +	struct file		*file,
> +	int			mode,
> +	loff_t			offset,
> +	loff_t			len,
> +	loff_t			*new_size)
> +{
> +	struct inode		*inode = file_inode(file);
> +
> +	if ((mode & FALLOC_FL_KEEP_SIZE) || offset + len <= i_size_read(inode))
> +		return 0;
> +	*new_size = offset + len;
> +	return inode_newsize_ok(inode, *new_size);
> +}
> +
> +static int
> +xfs_falloc_setsize(
> +	struct file		*file,
> +	loff_t			new_size)
> +{
> +	struct iattr iattr = {
> +		.ia_valid	= ATTR_SIZE,
> +		.ia_size	= new_size,
> +	};
> +
> +	if (!new_size)
> +		return 0;
> +	return xfs_vn_setattr_size(file_mnt_idmap(file), file_dentry(file),
> +			&iattr);
> +}
> +
> +static int
> +xfs_falloc_collapse_range(
> +	struct file		*file,
> +	loff_t			offset,
> +	loff_t			len)
> +{
> +	struct inode		*inode = file_inode(file);
> +	loff_t			new_size = i_size_read(inode) - len;
> +	int			error;
> +
> +	if (!xfs_is_falloc_aligned(XFS_I(inode), offset, len))
> +		return -EINVAL;
> +
> +	/*
> +	 * There is no need to overlap collapse range with EOF, in which case it
> +	 * is effectively a truncate operation
> +	 */
> +	if (offset + len >= i_size_read(inode))
> +		return -EINVAL;
> +
> +	error = xfs_collapse_file_space(XFS_I(inode), offset, len);
> +	if (error)
> +		return error;
> +	return xfs_falloc_setsize(file, new_size);
> +}
> +
> +static int
> +xfs_falloc_insert_range(
> +	struct file		*file,
> +	loff_t			offset,
> +	loff_t			len)
> +{
> +	struct inode		*inode = file_inode(file);
> +	loff_t			isize = i_size_read(inode);
> +	int			error;
> +
> +	if (!xfs_is_falloc_aligned(XFS_I(inode), offset, len))
> +		return -EINVAL;
> +
> +	/*
> +	 * New inode size must not exceed ->s_maxbytes, accounting for
> +	 * possible signed overflow.
> +	 */
> +	if (inode->i_sb->s_maxbytes - isize < len)
> +		return -EFBIG;
> +
> +	/* Offset should be less than i_size */
> +	if (offset >= isize)
> +		return -EINVAL;
> +
> +	error = xfs_falloc_setsize(file, isize + len);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Perform hole insertion now that the file size has been updated so
> +	 * that if we crash during the operation we don't leave shifted extents
> +	 * past EOF and hence losing access to the data that is contained within

"...and hence lose access to the data..."

> +	 * them.
> +	 */
> +	return xfs_insert_file_space(XFS_I(inode), offset, len);
> +}
> +
> +/*
> + * Punch a hole and prealloc the range.  We use a hole punch rather than
> + * unwritten extent conversion for two reasons:
> + *
> + *   1.) Hole punch handles partial block zeroing for us.
> + *   2.) If prealloc returns ENOSPC, the file range is still zero-valued by
> + *	 virtue of the hole punch.
> + */
> +static int
> +xfs_falloc_zero_range(
> +	struct file		*file,
> +	int			mode,
> +	loff_t			offset,
> +	loff_t			len)
> +{
> +	struct inode		*inode = file_inode(file);
> +	unsigned int		blksize = i_blocksize(inode);
> +	loff_t			new_size = 0;
> +	int			error;
> +
> +	trace_xfs_zero_file_space(XFS_I(inode));
> +
> +	error = xfs_falloc_newsize(file, mode, offset, len, &new_size);
> +	if (error)
> +		return error;
> +
> +	error = xfs_free_file_space(XFS_I(inode), offset, len);
> +	if (error)
> +		return error;
> +
> +	len = round_up(offset + len, blksize) - round_down(offset, blksize);
> +	offset = round_down(offset, blksize);
> +	error = xfs_alloc_file_space(XFS_I(inode), offset, len);
> +	if (error)
> +		return error;
> +	return xfs_falloc_setsize(file, new_size);
> +}
> +
> +static int
> +xfs_falloc_unshare_range(
> +	struct file		*file,
> +	int			mode,
> +	loff_t			offset,
> +	loff_t			len)
> +{
> +	struct inode		*inode = file_inode(file);
> +	loff_t			new_size = 0;
> +	int			error;
> +
> +	error = xfs_falloc_newsize(file, mode, offset, len, &new_size);
> +	if (error)
> +		return error;
> +
> +	error = xfs_reflink_unshare(XFS_I(inode), offset, len);
> +	if (error)
> +		return error;
> +
> +	error = xfs_alloc_file_space(XFS_I(inode), offset, len);
> +	if (error)
> +		return error;
> +	return xfs_falloc_setsize(file, new_size);
> +}
> +
> +static int
> +xfs_falloc_allocate_range(
> +	struct file		*file,
> +	int			mode,
> +	loff_t			offset,
> +	loff_t			len)
> +{
> +	struct inode		*inode = file_inode(file);
> +	loff_t			new_size = 0;
> +	int			error;
> +
> +	/*
> +	 * If always_cow mode we can't use preallocations and thus should not
> +	 * create them.
> +	 */
> +	if (xfs_is_always_cow_inode(XFS_I(inode)))
> +		return -EOPNOTSUPP;
> +
> +	error = xfs_falloc_newsize(file, mode, offset, len, &new_size);
> +	if (error)
> +		return error;
> +
> +	error = xfs_alloc_file_space(XFS_I(inode), offset, len);
> +	if (error)
> +		return error;
> +	return xfs_falloc_setsize(file, new_size);
> +}
> +
>  #define	XFS_FALLOC_FL_SUPPORTED						\
>  		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
>  		 FALLOC_FL_COLLAPSE_RANGE | FALLOC_FL_ZERO_RANGE |	\
> @@ -868,8 +1054,6 @@ xfs_file_fallocate(
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	long			error;
>  	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
> -	loff_t			new_size = 0;
> -	bool			do_file_insert = false;
>  
>  	if (!S_ISREG(inode->i_mode))
>  		return -EINVAL;
> @@ -894,129 +1078,31 @@ xfs_file_fallocate(
>  	if (error)
>  		goto out_unlock;
>  
> -	if (mode & FALLOC_FL_PUNCH_HOLE) {
> +	switch (mode & FALLOC_FL_MODE_MASK) {
> +	case FALLOC_FL_PUNCH_HOLE:
>  		error = xfs_free_file_space(ip, offset, len);
> -		if (error)
> -			goto out_unlock;
> -	} else if (mode & FALLOC_FL_COLLAPSE_RANGE) {
> -		if (!xfs_is_falloc_aligned(ip, offset, len)) {
> -			error = -EINVAL;
> -			goto out_unlock;
> -		}
> -
> -		/*
> -		 * There is no need to overlap collapse range with EOF,
> -		 * in which case it is effectively a truncate operation
> -		 */
> -		if (offset + len >= i_size_read(inode)) {
> -			error = -EINVAL;
> -			goto out_unlock;
> -		}
> -
> -		new_size = i_size_read(inode) - len;
> -
> -		error = xfs_collapse_file_space(ip, offset, len);
> -		if (error)
> -			goto out_unlock;
> -	} else if (mode & FALLOC_FL_INSERT_RANGE) {
> -		loff_t		isize = i_size_read(inode);
> -
> -		if (!xfs_is_falloc_aligned(ip, offset, len)) {
> -			error = -EINVAL;
> -			goto out_unlock;
> -		}
> -
> -		/*
> -		 * New inode size must not exceed ->s_maxbytes, accounting for
> -		 * possible signed overflow.
> -		 */
> -		if (inode->i_sb->s_maxbytes - isize < len) {
> -			error = -EFBIG;
> -			goto out_unlock;
> -		}
> -		new_size = isize + len;
> -
> -		/* Offset should be less than i_size */
> -		if (offset >= isize) {
> -			error = -EINVAL;
> -			goto out_unlock;
> -		}
> -		do_file_insert = true;
> -	} else {
> -		if (!(mode & FALLOC_FL_KEEP_SIZE) &&
> -		    offset + len > i_size_read(inode)) {
> -			new_size = offset + len;
> -			error = inode_newsize_ok(inode, new_size);
> -			if (error)
> -				goto out_unlock;
> -		}
> -
> -		if (mode & FALLOC_FL_ZERO_RANGE) {
> -			/*
> -			 * Punch a hole and prealloc the range.  We use a hole
> -			 * punch rather than unwritten extent conversion for two
> -			 * reasons:
> -			 *
> -			 *   1.) Hole punch handles partial block zeroing for us.
> -			 *   2.) If prealloc returns ENOSPC, the file range is
> -			 *       still zero-valued by virtue of the hole punch.
> -			 */
> -			unsigned int blksize = i_blocksize(inode);
> -
> -			trace_xfs_zero_file_space(ip);
> -
> -			error = xfs_free_file_space(ip, offset, len);
> -			if (error)
> -				goto out_unlock;
> -
> -			len = round_up(offset + len, blksize) -
> -			      round_down(offset, blksize);
> -			offset = round_down(offset, blksize);
> -		} else if (mode & FALLOC_FL_UNSHARE_RANGE) {
> -			error = xfs_reflink_unshare(ip, offset, len);
> -			if (error)
> -				goto out_unlock;
> -		} else {
> -			/*
> -			 * If always_cow mode we can't use preallocations and
> -			 * thus should not create them.
> -			 */
> -			if (xfs_is_always_cow_inode(ip)) {
> -				error = -EOPNOTSUPP;
> -				goto out_unlock;
> -			}
> -		}
> -
> -		error = xfs_alloc_file_space(ip, offset, len);
> -		if (error)
> -			goto out_unlock;
> -	}
> -
> -	/* Change file size if needed */
> -	if (new_size) {
> -		struct iattr iattr;
> -
> -		iattr.ia_valid = ATTR_SIZE;
> -		iattr.ia_size = new_size;
> -		error = xfs_vn_setattr_size(file_mnt_idmap(file),
> -					    file_dentry(file), &iattr);
> -		if (error)
> -			goto out_unlock;
> -	}
> -
> -	/*
> -	 * Perform hole insertion now that the file size has been
> -	 * updated so that if we crash during the operation we don't
> -	 * leave shifted extents past EOF and hence losing access to
> -	 * the data that is contained within them.
> -	 */
> -	if (do_file_insert) {
> -		error = xfs_insert_file_space(ip, offset, len);
> -		if (error)
> -			goto out_unlock;
> +		break;
> +	case FALLOC_FL_COLLAPSE_RANGE:
> +		error = xfs_falloc_collapse_range(file, offset, len);
> +		break;
> +	case FALLOC_FL_INSERT_RANGE:
> +		error = xfs_falloc_insert_range(file, offset, len);
> +		break;
> +	case FALLOC_FL_ZERO_RANGE:
> +		error = xfs_falloc_zero_range(file, mode, offset, len);
> +		break;
> +	case FALLOC_FL_UNSHARE_RANGE:
> +		error = xfs_falloc_unshare_range(file, mode, offset, len);
> +		break;
> +	case FALLOC_FL_ALLOCATE_RANGE:
> +		error = xfs_falloc_allocate_range(file, mode, offset, len);
> +		break;
> +	default:
> +		error = -EOPNOTSUPP;
> +		break;
>  	}
>  
> -	if (xfs_file_sync_writes(file))
> +	if (!error && xfs_file_sync_writes(file))
>  		error = xfs_log_force_inode(ip);
>  
>  out_unlock:
> -- 
> 2.43.0
> 
>