clean up xfs space management interfaces

clean up xfs space management interfaces

[Christoph Hellwig]

Hi all,

this series cleans up the XFS space management interfaces, and also
lifts two more of XFS-specific ioclts to common code, reusing the
existing infrastructure to map the XFS ioctls to ->fallocate.

[PATCH 1/4] xfs: don't implement XFS_IOC_RESVSP / XFS_IOC_RESVSP64 directly

[Christoph Hellwig]

These ioctls are implemented by the VFS and mapped to ->fallocate now,
so this code won't ever be reached.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c   | 10 ----------
 fs/xfs/xfs_ioctl32.c |  2 --
 2 files changed, 12 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0fed56d3175c..da4aaa75cfd3 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -643,8 +643,6 @@ xfs_ioc_space(
 	 */
 	switch (cmd) {
 	case XFS_IOC_ZERO_RANGE:
-	case XFS_IOC_RESVSP:
-	case XFS_IOC_RESVSP64:
 	case XFS_IOC_UNRESVSP:
 	case XFS_IOC_UNRESVSP64:
 		if (bf->l_len <= 0) {
@@ -670,12 +668,6 @@ xfs_ioc_space(
 		flags |= XFS_PREALLOC_SET;
 		error = xfs_zero_file_space(ip, bf->l_start, bf->l_len);
 		break;
-	case XFS_IOC_RESVSP:
-	case XFS_IOC_RESVSP64:
-		flags |= XFS_PREALLOC_SET;
-		error = xfs_alloc_file_space(ip, bf->l_start, bf->l_len,
-						XFS_BMAPI_PREALLOC);
-		break;
 	case XFS_IOC_UNRESVSP:
 	case XFS_IOC_UNRESVSP64:
 		error = xfs_free_file_space(ip, bf->l_start, bf->l_len);
@@ -2121,11 +2113,9 @@ xfs_file_ioctl(
 		return xfs_ioc_setlabel(filp, mp, arg);
 	case XFS_IOC_ALLOCSP:
 	case XFS_IOC_FREESP:
-	case XFS_IOC_RESVSP:
 	case XFS_IOC_UNRESVSP:
 	case XFS_IOC_ALLOCSP64:
 	case XFS_IOC_FREESP64:
-	case XFS_IOC_RESVSP64:
 	case XFS_IOC_UNRESVSP64:
 	case XFS_IOC_ZERO_RANGE: {
 		xfs_flock64_t		bf;
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 1e08bf79b478..257b7caf7fed 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -558,8 +558,6 @@ xfs_file_compat_ioctl(
 	case XFS_IOC_FREESP_32:
 	case XFS_IOC_ALLOCSP64_32:
 	case XFS_IOC_FREESP64_32:
-	case XFS_IOC_RESVSP_32:
-	case XFS_IOC_UNRESVSP_32:
 	case XFS_IOC_RESVSP64_32:
 	case XFS_IOC_UNRESVSP64_32:
 	case XFS_IOC_ZERO_RANGE_32: {
-- 
2.20.1

Re: [PATCH 1/4] xfs: don't implement XFS_IOC_RESVSP / XFS_IOC_RESVSP64 directly

[Darrick J. Wong]

On Fri, Oct 25, 2019 at 11:36:06AM +0900, Christoph Hellwig wrote:
> These ioctls are implemented by the VFS and mapped to ->fallocate now,
> so this code won't ever be reached.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_ioctl.c   | 10 ----------
>  fs/xfs/xfs_ioctl32.c |  2 --
>  2 files changed, 12 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0fed56d3175c..da4aaa75cfd3 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -643,8 +643,6 @@ xfs_ioc_space(
>  	 */
>  	switch (cmd) {
>  	case XFS_IOC_ZERO_RANGE:
> -	case XFS_IOC_RESVSP:
> -	case XFS_IOC_RESVSP64:
>  	case XFS_IOC_UNRESVSP:
>  	case XFS_IOC_UNRESVSP64:
>  		if (bf->l_len <= 0) {
> @@ -670,12 +668,6 @@ xfs_ioc_space(
>  		flags |= XFS_PREALLOC_SET;
>  		error = xfs_zero_file_space(ip, bf->l_start, bf->l_len);
>  		break;
> -	case XFS_IOC_RESVSP:
> -	case XFS_IOC_RESVSP64:
> -		flags |= XFS_PREALLOC_SET;
> -		error = xfs_alloc_file_space(ip, bf->l_start, bf->l_len,
> -						XFS_BMAPI_PREALLOC);
> -		break;
>  	case XFS_IOC_UNRESVSP:
>  	case XFS_IOC_UNRESVSP64:
>  		error = xfs_free_file_space(ip, bf->l_start, bf->l_len);
> @@ -2121,11 +2113,9 @@ xfs_file_ioctl(
>  		return xfs_ioc_setlabel(filp, mp, arg);
>  	case XFS_IOC_ALLOCSP:
>  	case XFS_IOC_FREESP:
> -	case XFS_IOC_RESVSP:
>  	case XFS_IOC_UNRESVSP:
>  	case XFS_IOC_ALLOCSP64:
>  	case XFS_IOC_FREESP64:
> -	case XFS_IOC_RESVSP64:
>  	case XFS_IOC_UNRESVSP64:
>  	case XFS_IOC_ZERO_RANGE: {
>  		xfs_flock64_t		bf;
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 1e08bf79b478..257b7caf7fed 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -558,8 +558,6 @@ xfs_file_compat_ioctl(
>  	case XFS_IOC_FREESP_32:
>  	case XFS_IOC_ALLOCSP64_32:
>  	case XFS_IOC_FREESP64_32:
> -	case XFS_IOC_RESVSP_32:
> -	case XFS_IOC_UNRESVSP_32:
>  	case XFS_IOC_RESVSP64_32:
>  	case XFS_IOC_UNRESVSP64_32:
>  	case XFS_IOC_ZERO_RANGE_32: {
> -- 
> 2.20.1
> 

[PATCH 2/4] fs: add generic UNRESVSP and ZERO_RANGE ioctl handlers

[Christoph Hellwig]

These use the same scheme as the pre-existing mapping of the XFS
RESVP ioctls to ->falloc, so just extend it and remove the XFS
implementation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/compat_ioctl.c      | 32 ++++++++++++++++---
 fs/ioctl.c             | 12 +++++--
 fs/xfs/xfs_ioctl.c     | 72 +++++++-----------------------------------
 fs/xfs/xfs_ioctl.h     |  1 -
 fs/xfs/xfs_ioctl32.c   |  7 ++--
 include/linux/falloc.h |  3 ++
 include/linux/fs.h     |  2 +-
 7 files changed, 54 insertions(+), 75 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index a7ec2d3dff92..7d920dda2811 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -480,11 +480,14 @@ struct space_resv_32 {
 	__s32		l_pad[4];	/* reserve area */
 };
 
-#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP_32	_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_UNRESVSP_32	_IOW ('X', 41, struct space_resv_32)
 #define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+#define FS_IOC_UNRESVSP64_32	_IOW ('X', 43, struct space_resv_32)
+#define FS_IOC_ZERO_RANGE_32	_IOW ('X', 57, struct space_resv_32)
 
 /* just account for different alignment */
-static int compat_ioctl_preallocate(struct file *file,
+static int compat_ioctl_preallocate(struct file *file, int mode,
 			struct space_resv_32    __user *p32)
 {
 	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
@@ -498,7 +501,7 @@ static int compat_ioctl_preallocate(struct file *file,
 	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
 		return -EFAULT;
 
-	return ioctl_preallocate(file, p);
+	return ioctl_preallocate(file, mode, p);
 }
 #endif
 
@@ -1022,13 +1025,32 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
 #if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
 	case FS_IOC_RESVSP_32:
 	case FS_IOC_RESVSP64_32:
-		error = compat_ioctl_preallocate(f.file, compat_ptr(arg));
+		error = compat_ioctl_preallocate(f.file, 0, compat_ptr(arg));
+		goto out_fput;
+	case FS_IOC_UNRESVSP_32:
+	case FS_IOC_UNRESVSP64_32:
+		error = compat_ioctl_preallocate(f.file, FALLOC_FL_PUNCH_HOLE,
+				compat_ptr(arg));
+		goto out_fput;
+	case FS_IOC_ZERO_RANGE_32:
+		error = compat_ioctl_preallocate(f.file, FALLOC_FL_ZERO_RANGE,
+				compat_ptr(arg));
 		goto out_fput;
 #else
 	case FS_IOC_RESVSP:
 	case FS_IOC_RESVSP64:
-		error = ioctl_preallocate(f.file, compat_ptr(arg));
+		error = ioctl_preallocate(f.file, 0, compat_ptr(arg));
+		goto out_fput;
+	case FS_IOC_UNRESVSP:
+	case FS_IOC_UNRESVSP64:
+		error = ioctl_preallocate(f.file, FALLOC_FL_PUNCH_HOLE,
+				compat_ptr(arg));
 		goto out_fput;
+	case FS_IOC_ZERO_RANGE:
+		error = ioctl_preallocate(f.file, FALLOC_FL_ZERO_RANGE,
+				compat_ptr(arg));
+		goto out_fput;
+	}
 #endif
 
 	case FICLONE:
diff --git a/fs/ioctl.c b/fs/ioctl.c
index fef3a6bf7c78..55c7cfb0e599 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -466,7 +466,7 @@ EXPORT_SYMBOL(generic_block_fiemap);
  * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
  * are used here, rest are ignored.
  */
-int ioctl_preallocate(struct file *filp, void __user *argp)
+int ioctl_preallocate(struct file *filp, int mode, void __user *argp)
 {
 	struct inode *inode = file_inode(filp);
 	struct space_resv sr;
@@ -487,7 +487,8 @@ int ioctl_preallocate(struct file *filp, void __user *argp)
 		return -EINVAL;
 	}
 
-	return vfs_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+	return vfs_fallocate(filp, mode | FALLOC_FL_KEEP_SIZE, sr.l_start,
+			sr.l_len);
 }
 
 static int file_ioctl(struct file *filp, unsigned int cmd,
@@ -503,7 +504,12 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
 		return put_user(i_size_read(inode) - filp->f_pos, p);
 	case FS_IOC_RESVSP:
 	case FS_IOC_RESVSP64:
-		return ioctl_preallocate(filp, p);
+		return ioctl_preallocate(filp, 0, p);
+	case FS_IOC_UNRESVSP:
+	case FS_IOC_UNRESVSP64:
+		return ioctl_preallocate(filp, FALLOC_FL_PUNCH_HOLE, p);
+	case FS_IOC_ZERO_RANGE:
+		return ioctl_preallocate(filp, FALLOC_FL_ZERO_RANGE, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index da4aaa75cfd3..3fe1543f9f02 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -588,13 +588,12 @@ xfs_attrmulti_by_handle(
 int
 xfs_ioc_space(
 	struct file		*filp,
-	unsigned int		cmd,
 	xfs_flock64_t		*bf)
 {
 	struct inode		*inode = file_inode(filp);
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct iattr		iattr;
-	enum xfs_prealloc_flags	flags = 0;
+	enum xfs_prealloc_flags	flags = XFS_PREALLOC_CLEAR;
 	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
 	int			error;
 
@@ -635,65 +634,21 @@ xfs_ioc_space(
 		goto out_unlock;
 	}
 
-	/*
-	 * length of <= 0 for resv/unresv/zero is invalid.  length for
-	 * alloc/free is ignored completely and we have no idea what userspace
-	 * might have set it to, so set it to zero to allow range
-	 * checks to pass.
-	 */
-	switch (cmd) {
-	case XFS_IOC_ZERO_RANGE:
-	case XFS_IOC_UNRESVSP:
-	case XFS_IOC_UNRESVSP64:
-		if (bf->l_len <= 0) {
-			error = -EINVAL;
-			goto out_unlock;
-		}
-		break;
-	default:
-		bf->l_len = 0;
-		break;
-	}
-
-	if (bf->l_start < 0 ||
-	    bf->l_start > inode->i_sb->s_maxbytes ||
-	    bf->l_start + bf->l_len < 0 ||
-	    bf->l_start + bf->l_len >= inode->i_sb->s_maxbytes) {
+	if (bf->l_start < 0 || bf->l_start > inode->i_sb->s_maxbytes) {
 		error = -EINVAL;
 		goto out_unlock;
 	}
 
-	switch (cmd) {
-	case XFS_IOC_ZERO_RANGE:
-		flags |= XFS_PREALLOC_SET;
-		error = xfs_zero_file_space(ip, bf->l_start, bf->l_len);
-		break;
-	case XFS_IOC_UNRESVSP:
-	case XFS_IOC_UNRESVSP64:
-		error = xfs_free_file_space(ip, bf->l_start, bf->l_len);
-		break;
-	case XFS_IOC_ALLOCSP:
-	case XFS_IOC_ALLOCSP64:
-	case XFS_IOC_FREESP:
-	case XFS_IOC_FREESP64:
-		flags |= XFS_PREALLOC_CLEAR;
-		if (bf->l_start > XFS_ISIZE(ip)) {
-			error = xfs_alloc_file_space(ip, XFS_ISIZE(ip),
-					bf->l_start - XFS_ISIZE(ip), 0);
-			if (error)
-				goto out_unlock;
-		}
-
-		iattr.ia_valid = ATTR_SIZE;
-		iattr.ia_size = bf->l_start;
-
-		error = xfs_vn_setattr_size(file_dentry(filp), &iattr);
-		break;
-	default:
-		ASSERT(0);
-		error = -EINVAL;
+	if (bf->l_start > XFS_ISIZE(ip)) {
+		error = xfs_alloc_file_space(ip, XFS_ISIZE(ip),
+				bf->l_start - XFS_ISIZE(ip), 0);
+		if (error)
+			goto out_unlock;
 	}
 
+	iattr.ia_valid = ATTR_SIZE;
+	iattr.ia_size = bf->l_start;
+	error = xfs_vn_setattr_size(file_dentry(filp), &iattr);
 	if (error)
 		goto out_unlock;
 
@@ -2113,16 +2068,13 @@ xfs_file_ioctl(
 		return xfs_ioc_setlabel(filp, mp, arg);
 	case XFS_IOC_ALLOCSP:
 	case XFS_IOC_FREESP:
-	case XFS_IOC_UNRESVSP:
 	case XFS_IOC_ALLOCSP64:
-	case XFS_IOC_FREESP64:
-	case XFS_IOC_UNRESVSP64:
-	case XFS_IOC_ZERO_RANGE: {
+	case XFS_IOC_FREESP64: {
 		xfs_flock64_t		bf;
 
 		if (copy_from_user(&bf, arg, sizeof(bf)))
 			return -EFAULT;
-		return xfs_ioc_space(filp, cmd, &bf);
+		return xfs_ioc_space(filp, &bf);
 	}
 	case XFS_IOC_DIOINFO: {
 		struct dioattr		da;
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index 654c0bb1bcf8..25ef178cbb74 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -9,7 +9,6 @@
 extern int
 xfs_ioc_space(
 	struct file		*filp,
-	unsigned int		cmd,
 	xfs_flock64_t		*bf);
 
 int
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 257b7caf7fed..3c0d518e1039 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -557,16 +557,13 @@ xfs_file_compat_ioctl(
 	case XFS_IOC_ALLOCSP_32:
 	case XFS_IOC_FREESP_32:
 	case XFS_IOC_ALLOCSP64_32:
-	case XFS_IOC_FREESP64_32:
-	case XFS_IOC_RESVSP64_32:
-	case XFS_IOC_UNRESVSP64_32:
-	case XFS_IOC_ZERO_RANGE_32: {
+	case XFS_IOC_FREESP64_32: {
 		struct xfs_flock64	bf;
 
 		if (xfs_compat_flock64_copyin(&bf, arg))
 			return -EFAULT;
 		cmd = _NATIVE_IOC(cmd, struct xfs_flock64);
-		return xfs_ioc_space(filp, cmd, &bf);
+		return xfs_ioc_space(filp, &bf);
 	}
 	case XFS_IOC_FSGEOMETRY_V1_32:
 		return xfs_compat_ioc_fsgeometry_v1(mp, arg);
diff --git a/include/linux/falloc.h b/include/linux/falloc.h
index 674d59f4d6ce..f5c73f0ec22d 100644
--- a/include/linux/falloc.h
+++ b/include/linux/falloc.h
@@ -20,7 +20,10 @@ struct space_resv {
 };
 
 #define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_UNRESVSP		_IOW('X', 41, struct space_resv)
 #define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+#define FS_IOC_UNRESVSP64	_IOW('X', 43, struct space_resv)
+#define FS_IOC_ZERO_RANGE	_IOW('X', 57, struct space_resv)
 
 #define	FALLOC_FL_SUPPORTED_MASK	(FALLOC_FL_KEEP_SIZE |		\
 					 FALLOC_FL_PUNCH_HOLE |		\
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0d909d35763..2b5692207c1d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2547,7 +2547,7 @@ extern int finish_no_open(struct file *file, struct dentry *dentry);
 
 /* fs/ioctl.c */
 
-extern int ioctl_preallocate(struct file *filp, void __user *argp);
+extern int ioctl_preallocate(struct file *filp, int mode, void __user *argp);
 
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
-- 
2.20.1

Re: [PATCH 2/4] fs: add generic UNRESVSP and ZERO_RANGE ioctl handlers

[Darrick J. Wong]

On Fri, Oct 25, 2019 at 11:36:07AM +0900, Christoph Hellwig wrote:
> These use the same scheme as the pre-existing mapping of the XFS
> RESVP ioctls to ->falloc, so just extend it and remove the XFS
> implementation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/compat_ioctl.c      | 32 ++++++++++++++++---
>  fs/ioctl.c             | 12 +++++--
>  fs/xfs/xfs_ioctl.c     | 72 +++++++-----------------------------------
>  fs/xfs/xfs_ioctl.h     |  1 -
>  fs/xfs/xfs_ioctl32.c   |  7 ++--
>  include/linux/falloc.h |  3 ++
>  include/linux/fs.h     |  2 +-
>  7 files changed, 54 insertions(+), 75 deletions(-)
> 
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index a7ec2d3dff92..7d920dda2811 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -480,11 +480,14 @@ struct space_resv_32 {
>  	__s32		l_pad[4];	/* reserve area */
>  };
>  
> -#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
> +#define FS_IOC_RESVSP_32	_IOW ('X', 40, struct space_resv_32)
> +#define FS_IOC_UNRESVSP_32	_IOW ('X', 41, struct space_resv_32)
>  #define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
> +#define FS_IOC_UNRESVSP64_32	_IOW ('X', 43, struct space_resv_32)
> +#define FS_IOC_ZERO_RANGE_32	_IOW ('X', 57, struct space_resv_32)
>  
>  /* just account for different alignment */
> -static int compat_ioctl_preallocate(struct file *file,
> +static int compat_ioctl_preallocate(struct file *file, int mode,
>  			struct space_resv_32    __user *p32)
>  {
>  	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
> @@ -498,7 +501,7 @@ static int compat_ioctl_preallocate(struct file *file,
>  	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
>  		return -EFAULT;
>  
> -	return ioctl_preallocate(file, p);
> +	return ioctl_preallocate(file, mode, p);
>  }
>  #endif
>  
> @@ -1022,13 +1025,32 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd,
>  #if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
>  	case FS_IOC_RESVSP_32:
>  	case FS_IOC_RESVSP64_32:
> -		error = compat_ioctl_preallocate(f.file, compat_ptr(arg));
> +		error = compat_ioctl_preallocate(f.file, 0, compat_ptr(arg));
> +		goto out_fput;
> +	case FS_IOC_UNRESVSP_32:
> +	case FS_IOC_UNRESVSP64_32:
> +		error = compat_ioctl_preallocate(f.file, FALLOC_FL_PUNCH_HOLE,
> +				compat_ptr(arg));
> +		goto out_fput;
> +	case FS_IOC_ZERO_RANGE_32:
> +		error = compat_ioctl_preallocate(f.file, FALLOC_FL_ZERO_RANGE,
> +				compat_ptr(arg));
>  		goto out_fput;
>  #else
>  	case FS_IOC_RESVSP:
>  	case FS_IOC_RESVSP64:
> -		error = ioctl_preallocate(f.file, compat_ptr(arg));
> +		error = ioctl_preallocate(f.file, 0, compat_ptr(arg));
> +		goto out_fput;
> +	case FS_IOC_UNRESVSP:
> +	case FS_IOC_UNRESVSP64:
> +		error = ioctl_preallocate(f.file, FALLOC_FL_PUNCH_HOLE,
> +				compat_ptr(arg));
>  		goto out_fput;
> +	case FS_IOC_ZERO_RANGE:
> +		error = ioctl_preallocate(f.file, FALLOC_FL_ZERO_RANGE,
> +				compat_ptr(arg));
> +		goto out_fput;
> +	}
>  #endif
>  
>  	case FICLONE:
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index fef3a6bf7c78..55c7cfb0e599 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -466,7 +466,7 @@ EXPORT_SYMBOL(generic_block_fiemap);
>   * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
>   * are used here, rest are ignored.
>   */
> -int ioctl_preallocate(struct file *filp, void __user *argp)
> +int ioctl_preallocate(struct file *filp, int mode, void __user *argp)
>  {
>  	struct inode *inode = file_inode(filp);
>  	struct space_resv sr;
> @@ -487,7 +487,8 @@ int ioctl_preallocate(struct file *filp, void __user *argp)
>  		return -EINVAL;
>  	}
>  
> -	return vfs_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
> +	return vfs_fallocate(filp, mode | FALLOC_FL_KEEP_SIZE, sr.l_start,
> +			sr.l_len);
>  }
>  
>  static int file_ioctl(struct file *filp, unsigned int cmd,
> @@ -503,7 +504,12 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
>  		return put_user(i_size_read(inode) - filp->f_pos, p);
>  	case FS_IOC_RESVSP:
>  	case FS_IOC_RESVSP64:
> -		return ioctl_preallocate(filp, p);
> +		return ioctl_preallocate(filp, 0, p);
> +	case FS_IOC_UNRESVSP:
> +	case FS_IOC_UNRESVSP64:
> +		return ioctl_preallocate(filp, FALLOC_FL_PUNCH_HOLE, p);
> +	case FS_IOC_ZERO_RANGE:
> +		return ioctl_preallocate(filp, FALLOC_FL_ZERO_RANGE, p);
>  	}
>  
>  	return vfs_ioctl(filp, cmd, arg);
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index da4aaa75cfd3..3fe1543f9f02 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -588,13 +588,12 @@ xfs_attrmulti_by_handle(
>  int
>  xfs_ioc_space(
>  	struct file		*filp,
> -	unsigned int		cmd,
>  	xfs_flock64_t		*bf)
>  {
>  	struct inode		*inode = file_inode(filp);
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	struct iattr		iattr;
> -	enum xfs_prealloc_flags	flags = 0;
> +	enum xfs_prealloc_flags	flags = XFS_PREALLOC_CLEAR;
>  	uint			iolock = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
>  	int			error;
>  
> @@ -635,65 +634,21 @@ xfs_ioc_space(
>  		goto out_unlock;
>  	}
>  
> -	/*
> -	 * length of <= 0 for resv/unresv/zero is invalid.  length for
> -	 * alloc/free is ignored completely and we have no idea what userspace
> -	 * might have set it to, so set it to zero to allow range
> -	 * checks to pass.
> -	 */
> -	switch (cmd) {
> -	case XFS_IOC_ZERO_RANGE:
> -	case XFS_IOC_UNRESVSP:
> -	case XFS_IOC_UNRESVSP64:
> -		if (bf->l_len <= 0) {
> -			error = -EINVAL;
> -			goto out_unlock;
> -		}
> -		break;
> -	default:
> -		bf->l_len = 0;
> -		break;
> -	}
> -
> -	if (bf->l_start < 0 ||
> -	    bf->l_start > inode->i_sb->s_maxbytes ||
> -	    bf->l_start + bf->l_len < 0 ||
> -	    bf->l_start + bf->l_len >= inode->i_sb->s_maxbytes) {
> +	if (bf->l_start < 0 || bf->l_start > inode->i_sb->s_maxbytes) {
>  		error = -EINVAL;
>  		goto out_unlock;
>  	}
>  
> -	switch (cmd) {
> -	case XFS_IOC_ZERO_RANGE:
> -		flags |= XFS_PREALLOC_SET;
> -		error = xfs_zero_file_space(ip, bf->l_start, bf->l_len);
> -		break;
> -	case XFS_IOC_UNRESVSP:
> -	case XFS_IOC_UNRESVSP64:
> -		error = xfs_free_file_space(ip, bf->l_start, bf->l_len);
> -		break;
> -	case XFS_IOC_ALLOCSP:
> -	case XFS_IOC_ALLOCSP64:
> -	case XFS_IOC_FREESP:
> -	case XFS_IOC_FREESP64:
> -		flags |= XFS_PREALLOC_CLEAR;
> -		if (bf->l_start > XFS_ISIZE(ip)) {
> -			error = xfs_alloc_file_space(ip, XFS_ISIZE(ip),
> -					bf->l_start - XFS_ISIZE(ip), 0);
> -			if (error)
> -				goto out_unlock;
> -		}
> -
> -		iattr.ia_valid = ATTR_SIZE;
> -		iattr.ia_size = bf->l_start;
> -
> -		error = xfs_vn_setattr_size(file_dentry(filp), &iattr);
> -		break;
> -	default:
> -		ASSERT(0);
> -		error = -EINVAL;
> +	if (bf->l_start > XFS_ISIZE(ip)) {
> +		error = xfs_alloc_file_space(ip, XFS_ISIZE(ip),
> +				bf->l_start - XFS_ISIZE(ip), 0);
> +		if (error)
> +			goto out_unlock;
>  	}
>  
> +	iattr.ia_valid = ATTR_SIZE;
> +	iattr.ia_size = bf->l_start;
> +	error = xfs_vn_setattr_size(file_dentry(filp), &iattr);
>  	if (error)
>  		goto out_unlock;
>  
> @@ -2113,16 +2068,13 @@ xfs_file_ioctl(
>  		return xfs_ioc_setlabel(filp, mp, arg);
>  	case XFS_IOC_ALLOCSP:
>  	case XFS_IOC_FREESP:
> -	case XFS_IOC_UNRESVSP:
>  	case XFS_IOC_ALLOCSP64:
> -	case XFS_IOC_FREESP64:
> -	case XFS_IOC_UNRESVSP64:
> -	case XFS_IOC_ZERO_RANGE: {
> +	case XFS_IOC_FREESP64: {

Ok, so this hoists everything to the vfs except for ALLOCSP and FREESP,
which seems to be ... "set new size; allocate between old and new EOF if
appropriate"?

I'm asking because I was never really clear on what those things are
supposed to do. :)

--D

>  		xfs_flock64_t		bf;
>  
>  		if (copy_from_user(&bf, arg, sizeof(bf)))
>  			return -EFAULT;
> -		return xfs_ioc_space(filp, cmd, &bf);
> +		return xfs_ioc_space(filp, &bf);
>  	}
>  	case XFS_IOC_DIOINFO: {
>  		struct dioattr		da;
> diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
> index 654c0bb1bcf8..25ef178cbb74 100644
> --- a/fs/xfs/xfs_ioctl.h
> +++ b/fs/xfs/xfs_ioctl.h
> @@ -9,7 +9,6 @@
>  extern int
>  xfs_ioc_space(
>  	struct file		*filp,
> -	unsigned int		cmd,
>  	xfs_flock64_t		*bf);
>  
>  int
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index 257b7caf7fed..3c0d518e1039 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -557,16 +557,13 @@ xfs_file_compat_ioctl(
>  	case XFS_IOC_ALLOCSP_32:
>  	case XFS_IOC_FREESP_32:
>  	case XFS_IOC_ALLOCSP64_32:
> -	case XFS_IOC_FREESP64_32:
> -	case XFS_IOC_RESVSP64_32:
> -	case XFS_IOC_UNRESVSP64_32:
> -	case XFS_IOC_ZERO_RANGE_32: {
> +	case XFS_IOC_FREESP64_32: {
>  		struct xfs_flock64	bf;
>  
>  		if (xfs_compat_flock64_copyin(&bf, arg))
>  			return -EFAULT;
>  		cmd = _NATIVE_IOC(cmd, struct xfs_flock64);
> -		return xfs_ioc_space(filp, cmd, &bf);
> +		return xfs_ioc_space(filp, &bf);
>  	}
>  	case XFS_IOC_FSGEOMETRY_V1_32:
>  		return xfs_compat_ioc_fsgeometry_v1(mp, arg);
> diff --git a/include/linux/falloc.h b/include/linux/falloc.h
> index 674d59f4d6ce..f5c73f0ec22d 100644
> --- a/include/linux/falloc.h
> +++ b/include/linux/falloc.h
> @@ -20,7 +20,10 @@ struct space_resv {
>  };
>  
>  #define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> +#define FS_IOC_UNRESVSP		_IOW('X', 41, struct space_resv)
>  #define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> +#define FS_IOC_UNRESVSP64	_IOW('X', 43, struct space_resv)
> +#define FS_IOC_ZERO_RANGE	_IOW('X', 57, struct space_resv)
>  
>  #define	FALLOC_FL_SUPPORTED_MASK	(FALLOC_FL_KEEP_SIZE |		\
>  					 FALLOC_FL_PUNCH_HOLE |		\
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e0d909d35763..2b5692207c1d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2547,7 +2547,7 @@ extern int finish_no_open(struct file *file, struct dentry *dentry);
>  
>  /* fs/ioctl.c */
>  
> -extern int ioctl_preallocate(struct file *filp, void __user *argp);
> +extern int ioctl_preallocate(struct file *filp, int mode, void __user *argp);
>  
>  /* fs/dcache.c */
>  extern void __init vfs_caches_init_early(void);
> -- 
> 2.20.1
> 

Re: [PATCH 2/4] fs: add generic UNRESVSP and ZERO_RANGE ioctl handlers

[Christoph Hellwig]

On Thu, Oct 24, 2019 at 10:44:52PM -0700, Darrick J. Wong wrote:
> >  	case XFS_IOC_FREESP:
> > -	case XFS_IOC_UNRESVSP:
> >  	case XFS_IOC_ALLOCSP64:
> > -	case XFS_IOC_FREESP64:
> > -	case XFS_IOC_UNRESVSP64:
> > -	case XFS_IOC_ZERO_RANGE: {
> > +	case XFS_IOC_FREESP64: {
> 
> Ok, so this hoists everything to the vfs except for ALLOCSP and FREESP,
> which seems to be ... "set new size; allocate between old and new EOF if
> appropriate"?
> 
> I'm asking because I was never really clear on what those things are
> supposed to do. :)

Yes. ALLOCSP/FREESP have so weird semantics that we never added the
equivalent functionality to fallocate.

Re: [PATCH 2/4] fs: add generic UNRESVSP and ZERO_RANGE ioctl handlers

[Dave Chinner]

On Fri, Oct 25, 2019 at 11:50:05AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 24, 2019 at 10:44:52PM -0700, Darrick J. Wong wrote:
> > >  	case XFS_IOC_FREESP:
> > > -	case XFS_IOC_UNRESVSP:
> > >  	case XFS_IOC_ALLOCSP64:
> > > -	case XFS_IOC_FREESP64:
> > > -	case XFS_IOC_UNRESVSP64:
> > > -	case XFS_IOC_ZERO_RANGE: {
> > > +	case XFS_IOC_FREESP64: {
> > 
> > Ok, so this hoists everything to the vfs except for ALLOCSP and FREESP,
> > which seems to be ... "set new size; allocate between old and new EOF if
> > appropriate"?
> > 
> > I'm asking because I was never really clear on what those things are
> > supposed to do. :)
> 
> Yes. ALLOCSP/FREESP have so weird semantics that we never added the
> equivalent functionality to fallocate.

We should plan to deprecate and remove ALLOCSP/FREESP - they just
aren't useful APIs anymore, and nobody has used them in preference
to the RESVSP/UNRESVSP ioctls since they were introduced in ~1998
with unwritten extents. We probably should have deprecated then 10
years ago....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] fs: add generic UNRESVSP and ZERO_RANGE ioctl handlers

[Christoph Hellwig]

On Sun, Oct 27, 2019 at 07:56:09AM +1100, Dave Chinner wrote:
> We should plan to deprecate and remove ALLOCSP/FREESP - they just
> aren't useful APIs anymore, and nobody has used them in preference
> to the RESVSP/UNRESVSP ioctls since they were introduced in ~1998
> with unwritten extents. We probably should have deprecated then 10
> years ago....

I vaguely remember an actually reported bug beeing fixed in the code
just a few years ago, which suggests actual users.  That being said
I'm all for throwing in a deprecation warnings and then see if anyone
screams.  With this series the code becomes more self-contained, and
I have another patch that moves the IOC_RESVP / fallocate implementation
over to use iomap, at which point it is entirely standalone.

[PATCH 3/4] xfs: disable xfs_ioc_space for always COW inodes

[Christoph Hellwig]

If we always have to write out of place preallocating blocks is
pointless.  We already check for this in the normal falloc path, but
the check was missig in the legacy ALLOCSP path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 3fe1543f9f02..552034325991 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -33,6 +33,7 @@
 #include "xfs_sb.h"
 #include "xfs_ag.h"
 #include "xfs_health.h"
+#include "xfs_reflink.h"
 
 #include <linux/mount.h>
 #include <linux/namei.h>
@@ -606,6 +607,9 @@ xfs_ioc_space(
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 
+	if (xfs_is_always_cow_inode(ip))
+		return -EOPNOTSUPP;
+
 	if (filp->f_flags & O_DSYNC)
 		flags |= XFS_PREALLOC_SYNC;
 	if (filp->f_mode & FMODE_NOCMTIME)
-- 
2.20.1

Re: [PATCH 3/4] xfs: disable xfs_ioc_space for always COW inodes

[Darrick J. Wong]

On Fri, Oct 25, 2019 at 11:36:08AM +0900, Christoph Hellwig wrote:
> If we always have to write out of place preallocating blocks is
> pointless.  We already check for this in the normal falloc path, but
> the check was missig in the legacy ALLOCSP path.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_ioctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 3fe1543f9f02..552034325991 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -33,6 +33,7 @@
>  #include "xfs_sb.h"
>  #include "xfs_ag.h"
>  #include "xfs_health.h"
> +#include "xfs_reflink.h"
>  
>  #include <linux/mount.h>
>  #include <linux/namei.h>
> @@ -606,6 +607,9 @@ xfs_ioc_space(
>  	if (!S_ISREG(inode->i_mode))
>  		return -EINVAL;
>  
> +	if (xfs_is_always_cow_inode(ip))
> +		return -EOPNOTSUPP;
> +
>  	if (filp->f_flags & O_DSYNC)
>  		flags |= XFS_PREALLOC_SYNC;
>  	if (filp->f_mode & FMODE_NOCMTIME)
> -- 
> 2.20.1
> 

[PATCH 4/4] xfs: consolidate preallocation in xfs_file_fallocate

[Christoph Hellwig]

Remove xfs_zero_file_space and reorganize xfs_file_fallocate so that a
single call to xfs_alloc_file_space covers all modes that preallocate
blocks.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c | 37 -------------------------------------
 fs/xfs/xfs_bmap_util.h |  2 --
 fs/xfs/xfs_file.c      | 32 ++++++++++++++++++++++++--------
 3 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 9b0572a7b03a..11658da40640 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1133,43 +1133,6 @@ xfs_free_file_space(
 	return error;
 }
 
-/*
- * Preallocate and zero a range of a file. This mechanism has the allocation
- * semantics of fallocate and in addition converts data in the range to zeroes.
- */
-int
-xfs_zero_file_space(
-	struct xfs_inode	*ip,
-	xfs_off_t		offset,
-	xfs_off_t		len)
-{
-	struct xfs_mount	*mp = ip->i_mount;
-	uint			blksize;
-	int			error;
-
-	trace_xfs_zero_file_space(ip);
-
-	blksize = 1 << mp->m_sb.sb_blocklog;
-
-	/*
-	 * Punch a hole and prealloc the range. We use 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.
-	 */
-	error = xfs_free_file_space(ip, offset, len);
-	if (error || xfs_is_always_cow_inode(ip))
-		return error;
-
-	return xfs_alloc_file_space(ip, round_down(offset, blksize),
-				     round_up(offset + len, blksize) -
-				     round_down(offset, blksize),
-				     XFS_BMAPI_PREALLOC);
-}
-
 static int
 xfs_prepare_shift(
 	struct xfs_inode	*ip,
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index 7a78229cf1a7..3e0fa0d363d1 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -59,8 +59,6 @@ int	xfs_alloc_file_space(struct xfs_inode *ip, xfs_off_t offset,
 			     xfs_off_t len, int alloc_type);
 int	xfs_free_file_space(struct xfs_inode *ip, xfs_off_t offset,
 			    xfs_off_t len);
-int	xfs_zero_file_space(struct xfs_inode *ip, xfs_off_t offset,
-			    xfs_off_t len);
 int	xfs_collapse_file_space(struct xfs_inode *, xfs_off_t offset,
 				xfs_off_t len);
 int	xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 156238d5af19..525b29b99116 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -880,16 +880,30 @@ xfs_file_fallocate(
 		}
 
 		if (mode & FALLOC_FL_ZERO_RANGE) {
-			error = xfs_zero_file_space(ip, 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.
+			 */
+			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;
-
-			if (!xfs_is_always_cow_inode(ip)) {
-				error = xfs_alloc_file_space(ip, offset, len,
-						XFS_BMAPI_PREALLOC);
-			}
 		} else {
 			/*
 			 * If always_cow mode we can't use preallocations and
@@ -899,12 +913,14 @@ xfs_file_fallocate(
 				error = -EOPNOTSUPP;
 				goto out_unlock;
 			}
+		}
 
+		if (!xfs_is_always_cow_inode(ip)) {
 			error = xfs_alloc_file_space(ip, offset, len,
 						     XFS_BMAPI_PREALLOC);
+			if (error)
+				goto out_unlock;
 		}
-		if (error)
-			goto out_unlock;
 	}
 
 	if (file->f_flags & O_DSYNC)
-- 
2.20.1

Re: [PATCH 4/4] xfs: consolidate preallocation in xfs_file_fallocate

[Darrick J. Wong]

On Fri, Oct 25, 2019 at 11:36:09AM +0900, Christoph Hellwig wrote:
> Remove xfs_zero_file_space and reorganize xfs_file_fallocate so that a
> single call to xfs_alloc_file_space covers all modes that preallocate
> blocks.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems pretty straightforward.

On a side note I think Dave and Eric might have noticed that fallocate
doesn't drain directio which could lead to incorrect file size.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_bmap_util.c | 37 -------------------------------------
>  fs/xfs/xfs_bmap_util.h |  2 --
>  fs/xfs/xfs_file.c      | 32 ++++++++++++++++++++++++--------
>  3 files changed, 24 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 9b0572a7b03a..11658da40640 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1133,43 +1133,6 @@ xfs_free_file_space(
>  	return error;
>  }
>  
> -/*
> - * Preallocate and zero a range of a file. This mechanism has the allocation
> - * semantics of fallocate and in addition converts data in the range to zeroes.
> - */
> -int
> -xfs_zero_file_space(
> -	struct xfs_inode	*ip,
> -	xfs_off_t		offset,
> -	xfs_off_t		len)
> -{
> -	struct xfs_mount	*mp = ip->i_mount;
> -	uint			blksize;
> -	int			error;
> -
> -	trace_xfs_zero_file_space(ip);
> -
> -	blksize = 1 << mp->m_sb.sb_blocklog;
> -
> -	/*
> -	 * Punch a hole and prealloc the range. We use 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.
> -	 */
> -	error = xfs_free_file_space(ip, offset, len);
> -	if (error || xfs_is_always_cow_inode(ip))
> -		return error;
> -
> -	return xfs_alloc_file_space(ip, round_down(offset, blksize),
> -				     round_up(offset + len, blksize) -
> -				     round_down(offset, blksize),
> -				     XFS_BMAPI_PREALLOC);
> -}
> -
>  static int
>  xfs_prepare_shift(
>  	struct xfs_inode	*ip,
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 7a78229cf1a7..3e0fa0d363d1 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -59,8 +59,6 @@ int	xfs_alloc_file_space(struct xfs_inode *ip, xfs_off_t offset,
>  			     xfs_off_t len, int alloc_type);
>  int	xfs_free_file_space(struct xfs_inode *ip, xfs_off_t offset,
>  			    xfs_off_t len);
> -int	xfs_zero_file_space(struct xfs_inode *ip, xfs_off_t offset,
> -			    xfs_off_t len);
>  int	xfs_collapse_file_space(struct xfs_inode *, xfs_off_t offset,
>  				xfs_off_t len);
>  int	xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 156238d5af19..525b29b99116 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -880,16 +880,30 @@ xfs_file_fallocate(
>  		}
>  
>  		if (mode & FALLOC_FL_ZERO_RANGE) {
> -			error = xfs_zero_file_space(ip, 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.
> +			 */
> +			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;
> -
> -			if (!xfs_is_always_cow_inode(ip)) {
> -				error = xfs_alloc_file_space(ip, offset, len,
> -						XFS_BMAPI_PREALLOC);
> -			}
>  		} else {
>  			/*
>  			 * If always_cow mode we can't use preallocations and
> @@ -899,12 +913,14 @@ xfs_file_fallocate(
>  				error = -EOPNOTSUPP;
>  				goto out_unlock;
>  			}
> +		}
>  
> +		if (!xfs_is_always_cow_inode(ip)) {
>  			error = xfs_alloc_file_space(ip, offset, len,
>  						     XFS_BMAPI_PREALLOC);
> +			if (error)
> +				goto out_unlock;
>  		}
> -		if (error)
> -			goto out_unlock;
>  	}
>  
>  	if (file->f_flags & O_DSYNC)
> -- 
> 2.20.1
>