[PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[Christoph Hellwig]

Call xfs_flush_unmap_range from xfs_free_file_space so that
xfs_file_fallocate doesn't have to predict which mode will call it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c |  8 ++++++++
 fs/xfs/xfs_file.c      | 21 ---------------------
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index fe2e2c93097550..187a0dbda24fc4 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -848,6 +848,14 @@ xfs_free_file_space(
 	if (len <= 0)	/* if nothing being freed */
 		return 0;
 
+	/*
+	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
+	 * the cached range over the first operation we are about to run.
+	 */
+	error = xfs_flush_unmap_range(ip, offset, len);
+	if (error)
+		return error;
+
 	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
 	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
 
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 4cdc54dc96862e..5b9e49da06013c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -890,27 +890,6 @@ xfs_file_fallocate(
 	 */
 	inode_dio_wait(inode);
 
-	/*
-	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
-	 * the cached range over the first operation we are about to run.
-	 *
-	 * We care about zero and collapse here because they both run a hole
-	 * punch over the range first. Because that can zero data, and the range
-	 * of invalidation for the shift operations is much larger, we still do
-	 * the required flush for collapse in xfs_prepare_shift().
-	 *
-	 * Insert has the same range requirements as collapse, and we extend the
-	 * file first which can zero data. Hence insert has the same
-	 * flush/invalidate requirements as collapse and so they are both
-	 * handled at the right time by xfs_prepare_shift().
-	 */
-	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE |
-		    FALLOC_FL_COLLAPSE_RANGE)) {
-		error = xfs_flush_unmap_range(ip, offset, len);
-		if (error)
-			goto out_unlock;
-	}
-
 	error = file_modified(file);
 	if (error)
 		goto out_unlock;
-- 
2.43.0

sort out the fallocate mode mess v2

[Christoph Hellwig]

Hi all,

I've recently been looking at the XFS fallocate implementation and got
upset about the messing parsing of the mode argument, which mixes modes
and an optional flag in a really confusing way.

This series tries to clean this up by better defining what is the
operation mode and what is an optional flag, so that both the core
code and file systems can use switch statements to switch on the mode.

Changes since v1:
 - fix the IS_APPEND check
 - ensure space is allocated after unshare

[PATCH 1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE

[Christoph Hellwig]

While the FALLOC_FL_NO_HIDE_STALE value has been registered, it has
always been rejected by vfs_fallocate before making it into
blkdev_fallocate because it isn't in the supported mask.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/fops.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/block/fops.c b/block/fops.c
index 9825c1713a49a9..7f48f03a62e9a8 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -771,7 +771,7 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 
 #define	BLKDEV_FALLOC_FL_SUPPORTED					\
 		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
-		 FALLOC_FL_ZERO_RANGE | FALLOC_FL_NO_HIDE_STALE)
+		 FALLOC_FL_ZERO_RANGE)
 
 static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 			     loff_t len)
@@ -830,14 +830,6 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 					     len >> SECTOR_SHIFT, GFP_KERNEL,
 					     BLKDEV_ZERO_NOFALLBACK);
 		break;
-	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE:
-		error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
-		if (error)
-			goto fail;
-
-		error = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
-					     len >> SECTOR_SHIFT, GFP_KERNEL);
-		break;
 	default:
 		error = -EOPNOTSUPP;
 	}
-- 
2.43.0

[PATCH 2/6] ext4: remove tracing for FALLOC_FL_NO_HIDE_STALE

[Christoph Hellwig]

FALLOC_FL_NO_HIDE_STALE can't make it past vfs_fallocate (and if the
flag does what the name implies that's a good thing as it would be
highly dangerous).  Remove the dead tracing code for it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/trace/events/ext4.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index cc5e9b7b2b44e7..156908641e68f1 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -91,7 +91,6 @@ TRACE_DEFINE_ENUM(ES_REFERENCED_B);
 #define show_falloc_mode(mode) __print_flags(mode, "|",		\
 	{ FALLOC_FL_KEEP_SIZE,		"KEEP_SIZE"},		\
 	{ FALLOC_FL_PUNCH_HOLE,		"PUNCH_HOLE"},		\
-	{ FALLOC_FL_NO_HIDE_STALE,	"NO_HIDE_STALE"},	\
 	{ FALLOC_FL_COLLAPSE_RANGE,	"COLLAPSE_RANGE"},	\
 	{ FALLOC_FL_ZERO_RANGE,		"ZERO_RANGE"})
 
-- 
2.43.0

[PATCH 3/6] fs: sort out the fallocate mode vs flag mess

[Christoph Hellwig]

The fallocate system call takes a mode argument, but that argument
contains a wild mix of exclusive modes and an optional flags.

Replace FALLOC_FL_SUPPORTED_MASK with FALLOC_FL_MODE_MASK, which excludes
the optional flag bit, so that we can use switch statement on the value
to easily enumerate the cases while getting the check for duplicate modes
for free.

To make this (and in the future the file system implementations) more
readable also add a symbolic name for the 0 mode used to allocate blocks.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/open.c                   | 51 ++++++++++++++++++-------------------
 include/linux/falloc.h      | 18 ++++++++-----
 include/uapi/linux/falloc.h |  1 +
 3 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 22adbef7ecc2a6..daf1b55ca8180b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -252,40 +252,39 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (offset < 0 || len <= 0)
 		return -EINVAL;
 
-	/* Return error if mode is not supported */
-	if (mode & ~FALLOC_FL_SUPPORTED_MASK)
+	if (mode & ~(FALLOC_FL_MODE_MASK | FALLOC_FL_KEEP_SIZE))
 		return -EOPNOTSUPP;
 
-	/* Punch hole and zero range are mutually exclusive */
-	if ((mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) ==
-	    (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE))
-		return -EOPNOTSUPP;
-
-	/* Punch hole must have keep size set */
-	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
-	    !(mode & FALLOC_FL_KEEP_SIZE))
+	/*
+	 * Modes are exclusive, even if that is not obvious from the encoding
+	 * as bit masks and the mix with the flag in the same namespace.
+	 *
+	 * To make things even more complicated, FALLOC_FL_ALLOCATE_RANGE is
+	 * encoded as no bit set.
+	 */
+	switch (mode & FALLOC_FL_MODE_MASK) {
+	case FALLOC_FL_ALLOCATE_RANGE:
+	case FALLOC_FL_UNSHARE_RANGE:
+	case FALLOC_FL_ZERO_RANGE:
+		break;
+	case FALLOC_FL_PUNCH_HOLE:
+		if (!(mode & FALLOC_FL_KEEP_SIZE))
+			return -EOPNOTSUPP;
+		break;
+	case FALLOC_FL_COLLAPSE_RANGE:
+	case FALLOC_FL_INSERT_RANGE:
+		if (mode & FALLOC_FL_KEEP_SIZE)
+			return -EOPNOTSUPP;
+		break;
+	default:
 		return -EOPNOTSUPP;
-
-	/* Collapse range should only be used exclusively. */
-	if ((mode & FALLOC_FL_COLLAPSE_RANGE) &&
-	    (mode & ~FALLOC_FL_COLLAPSE_RANGE))
-		return -EINVAL;
-
-	/* Insert range should only be used exclusively. */
-	if ((mode & FALLOC_FL_INSERT_RANGE) &&
-	    (mode & ~FALLOC_FL_INSERT_RANGE))
-		return -EINVAL;
-
-	/* Unshare range should only be used with allocate mode. */
-	if ((mode & FALLOC_FL_UNSHARE_RANGE) &&
-	    (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE)))
-		return -EINVAL;
+	}
 
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EBADF;
 
 	/*
-	 * We can only allow pure fallocate on append only files
+	 * On append-only files only space preallocation is supported.
 	 */
 	if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode))
 		return -EPERM;
diff --git a/include/linux/falloc.h b/include/linux/falloc.h
index f3f0b97b167579..3f49f3df6af5fb 100644
--- a/include/linux/falloc.h
+++ b/include/linux/falloc.h
@@ -25,12 +25,18 @@ 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 |		\
-					 FALLOC_FL_COLLAPSE_RANGE |	\
-					 FALLOC_FL_ZERO_RANGE |		\
-					 FALLOC_FL_INSERT_RANGE |	\
-					 FALLOC_FL_UNSHARE_RANGE)
+/*
+ * Mask of all supported fallocate modes.  Only one can be set at a time.
+ *
+ * In addition to the mode bit, the mode argument can also encode flags.
+ * FALLOC_FL_KEEP_SIZE is the only supported flag so far.
+ */
+#define FALLOC_FL_MODE_MASK	(FALLOC_FL_ALLOCATE_RANGE |	\
+				 FALLOC_FL_PUNCH_HOLE |		\
+				 FALLOC_FL_COLLAPSE_RANGE |	\
+				 FALLOC_FL_ZERO_RANGE |		\
+				 FALLOC_FL_INSERT_RANGE |	\
+				 FALLOC_FL_UNSHARE_RANGE)
 
 /* on ia32 l_start is on a 32-bit boundary */
 #if defined(CONFIG_X86_64)
diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
index 51398fa57f6cdf..5810371ed72bbd 100644
--- a/include/uapi/linux/falloc.h
+++ b/include/uapi/linux/falloc.h
@@ -2,6 +2,7 @@
 #ifndef _UAPI_FALLOC_H_
 #define _UAPI_FALLOC_H_
 
+#define FALLOC_FL_ALLOCATE_RANGE 0x00 /* allocate range */
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 #define FALLOC_FL_PUNCH_HOLE	0x02 /* de-allocates range */
 #define FALLOC_FL_NO_HIDE_STALE	0x04 /* reserved codepoint */
-- 
2.43.0

[PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[Christoph Hellwig]

Call xfs_flush_unmap_range from xfs_free_file_space so that
xfs_file_fallocate doesn't have to predict which mode will call it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c |  8 ++++++++
 fs/xfs/xfs_file.c      | 21 ---------------------
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index fe2e2c93097550..187a0dbda24fc4 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -848,6 +848,14 @@ xfs_free_file_space(
 	if (len <= 0)	/* if nothing being freed */
 		return 0;
 
+	/*
+	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
+	 * the cached range over the first operation we are about to run.
+	 */
+	error = xfs_flush_unmap_range(ip, offset, len);
+	if (error)
+		return error;
+
 	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
 	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
 
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 4cdc54dc96862e..5b9e49da06013c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -890,27 +890,6 @@ xfs_file_fallocate(
 	 */
 	inode_dio_wait(inode);
 
-	/*
-	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
-	 * the cached range over the first operation we are about to run.
-	 *
-	 * We care about zero and collapse here because they both run a hole
-	 * punch over the range first. Because that can zero data, and the range
-	 * of invalidation for the shift operations is much larger, we still do
-	 * the required flush for collapse in xfs_prepare_shift().
-	 *
-	 * Insert has the same range requirements as collapse, and we extend the
-	 * file first which can zero data. Hence insert has the same
-	 * flush/invalidate requirements as collapse and so they are both
-	 * handled at the right time by xfs_prepare_shift().
-	 */
-	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE |
-		    FALLOC_FL_COLLAPSE_RANGE)) {
-		error = xfs_flush_unmap_range(ip, offset, len);
-		if (error)
-			goto out_unlock;
-	}
-
 	error = file_modified(file);
 	if (error)
 		goto out_unlock;
-- 
2.43.0

[PATCH 5/6] xfs: move the xfs_is_always_cow_inode check into xfs_alloc_file_space

[Christoph Hellwig]

Move the xfs_is_always_cow_inode check from the caller into
xfs_alloc_file_space to prepare for refactoring of xfs_file_fallocate.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c | 3 +++
 fs/xfs/xfs_file.c      | 8 +++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 187a0dbda24fc4..e9fdebaa40ea59 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -653,6 +653,9 @@ xfs_alloc_file_space(
 	xfs_bmbt_irec_t		imaps[1], *imapp;
 	int			error;
 
+	if (xfs_is_always_cow_inode(ip))
+		return 0;
+
 	trace_xfs_alloc_file_space(ip);
 
 	if (xfs_is_shutdown(mp))
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 5b9e49da06013c..489bc1b173c268 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -987,11 +987,9 @@ xfs_file_fallocate(
 			}
 		}
 
-		if (!xfs_is_always_cow_inode(ip)) {
-			error = xfs_alloc_file_space(ip, offset, len);
-			if (error)
-				goto out_unlock;
-		}
+		error = xfs_alloc_file_space(ip, offset, len);
+		if (error)
+			goto out_unlock;
 	}
 
 	/* Change file size if needed */
-- 
2.43.0

[PATCH 6/6] xfs: refactor xfs_file_fallocate

[Christoph Hellwig]

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>
---
 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
+	 * 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

Re: [PATCH 3/6] fs: sort out the fallocate mode vs flag mess

[Darrick J. Wong]

On Tue, Aug 27, 2024 at 08:50:47AM +0200, Christoph Hellwig wrote:
> The fallocate system call takes a mode argument, but that argument
> contains a wild mix of exclusive modes and an optional flags.
> 
> Replace FALLOC_FL_SUPPORTED_MASK with FALLOC_FL_MODE_MASK, which excludes
> the optional flag bit, so that we can use switch statement on the value
> to easily enumerate the cases while getting the check for duplicate modes
> for free.
> 
> To make this (and in the future the file system implementations) more
> readable also add a symbolic name for the 0 mode used to allocate blocks.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

For a brief moment I wondered if it were possible to set more than one
mode bit but it looks like none of the implementations support that kind
of wackiness (e.g. COLLAPSE|INSERT_RANGE for magicks!) so we're good:

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

--D

> ---
>  fs/open.c                   | 51 ++++++++++++++++++-------------------
>  include/linux/falloc.h      | 18 ++++++++-----
>  include/uapi/linux/falloc.h |  1 +
>  3 files changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 22adbef7ecc2a6..daf1b55ca8180b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -252,40 +252,39 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  	if (offset < 0 || len <= 0)
>  		return -EINVAL;
>  
> -	/* Return error if mode is not supported */
> -	if (mode & ~FALLOC_FL_SUPPORTED_MASK)
> +	if (mode & ~(FALLOC_FL_MODE_MASK | FALLOC_FL_KEEP_SIZE))
>  		return -EOPNOTSUPP;
>  
> -	/* Punch hole and zero range are mutually exclusive */
> -	if ((mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) ==
> -	    (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE))
> -		return -EOPNOTSUPP;
> -
> -	/* Punch hole must have keep size set */
> -	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
> -	    !(mode & FALLOC_FL_KEEP_SIZE))
> +	/*
> +	 * Modes are exclusive, even if that is not obvious from the encoding
> +	 * as bit masks and the mix with the flag in the same namespace.
> +	 *
> +	 * To make things even more complicated, FALLOC_FL_ALLOCATE_RANGE is
> +	 * encoded as no bit set.
> +	 */
> +	switch (mode & FALLOC_FL_MODE_MASK) {
> +	case FALLOC_FL_ALLOCATE_RANGE:
> +	case FALLOC_FL_UNSHARE_RANGE:
> +	case FALLOC_FL_ZERO_RANGE:
> +		break;
> +	case FALLOC_FL_PUNCH_HOLE:
> +		if (!(mode & FALLOC_FL_KEEP_SIZE))
> +			return -EOPNOTSUPP;
> +		break;
> +	case FALLOC_FL_COLLAPSE_RANGE:
> +	case FALLOC_FL_INSERT_RANGE:
> +		if (mode & FALLOC_FL_KEEP_SIZE)
> +			return -EOPNOTSUPP;
> +		break;
> +	default:
>  		return -EOPNOTSUPP;
> -
> -	/* Collapse range should only be used exclusively. */
> -	if ((mode & FALLOC_FL_COLLAPSE_RANGE) &&
> -	    (mode & ~FALLOC_FL_COLLAPSE_RANGE))
> -		return -EINVAL;
> -
> -	/* Insert range should only be used exclusively. */
> -	if ((mode & FALLOC_FL_INSERT_RANGE) &&
> -	    (mode & ~FALLOC_FL_INSERT_RANGE))
> -		return -EINVAL;
> -
> -	/* Unshare range should only be used with allocate mode. */
> -	if ((mode & FALLOC_FL_UNSHARE_RANGE) &&
> -	    (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE)))
> -		return -EINVAL;
> +	}
>  
>  	if (!(file->f_mode & FMODE_WRITE))
>  		return -EBADF;
>  
>  	/*
> -	 * We can only allow pure fallocate on append only files
> +	 * On append-only files only space preallocation is supported.
>  	 */
>  	if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode))
>  		return -EPERM;
> diff --git a/include/linux/falloc.h b/include/linux/falloc.h
> index f3f0b97b167579..3f49f3df6af5fb 100644
> --- a/include/linux/falloc.h
> +++ b/include/linux/falloc.h
> @@ -25,12 +25,18 @@ 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 |		\
> -					 FALLOC_FL_COLLAPSE_RANGE |	\
> -					 FALLOC_FL_ZERO_RANGE |		\
> -					 FALLOC_FL_INSERT_RANGE |	\
> -					 FALLOC_FL_UNSHARE_RANGE)
> +/*
> + * Mask of all supported fallocate modes.  Only one can be set at a time.
> + *
> + * In addition to the mode bit, the mode argument can also encode flags.
> + * FALLOC_FL_KEEP_SIZE is the only supported flag so far.
> + */
> +#define FALLOC_FL_MODE_MASK	(FALLOC_FL_ALLOCATE_RANGE |	\
> +				 FALLOC_FL_PUNCH_HOLE |		\
> +				 FALLOC_FL_COLLAPSE_RANGE |	\
> +				 FALLOC_FL_ZERO_RANGE |		\
> +				 FALLOC_FL_INSERT_RANGE |	\
> +				 FALLOC_FL_UNSHARE_RANGE)
>  
>  /* on ia32 l_start is on a 32-bit boundary */
>  #if defined(CONFIG_X86_64)
> diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
> index 51398fa57f6cdf..5810371ed72bbd 100644
> --- a/include/uapi/linux/falloc.h
> +++ b/include/uapi/linux/falloc.h
> @@ -2,6 +2,7 @@
>  #ifndef _UAPI_FALLOC_H_
>  #define _UAPI_FALLOC_H_
>  
> +#define FALLOC_FL_ALLOCATE_RANGE 0x00 /* allocate range */
>  #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
>  #define FALLOC_FL_PUNCH_HOLE	0x02 /* de-allocates range */
>  #define FALLOC_FL_NO_HIDE_STALE	0x04 /* reserved codepoint */
> -- 
> 2.43.0
> 
> 

Re: [PATCH 1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE

[Darrick J. Wong]

On Tue, Aug 27, 2024 at 08:50:45AM +0200, Christoph Hellwig wrote:
> While the FALLOC_FL_NO_HIDE_STALE value has been registered, it has
> always been rejected by vfs_fallocate before making it into
> blkdev_fallocate because it isn't in the supported mask.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yeah, I guess that never worked.  Oh well.  BLKDISCARD it is.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  block/fops.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index 9825c1713a49a9..7f48f03a62e9a8 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -771,7 +771,7 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  
>  #define	BLKDEV_FALLOC_FL_SUPPORTED					\
>  		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
> -		 FALLOC_FL_ZERO_RANGE | FALLOC_FL_NO_HIDE_STALE)
> +		 FALLOC_FL_ZERO_RANGE)
>  
>  static long blkdev_fallocate(struct file *file, int mode, loff_t start,
>  			     loff_t len)
> @@ -830,14 +830,6 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
>  					     len >> SECTOR_SHIFT, GFP_KERNEL,
>  					     BLKDEV_ZERO_NOFALLBACK);
>  		break;
> -	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE:
> -		error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
> -		if (error)
> -			goto fail;
> -
> -		error = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
> -					     len >> SECTOR_SHIFT, GFP_KERNEL);
> -		break;
>  	default:
>  		error = -EOPNOTSUPP;
>  	}
> -- 
> 2.43.0
> 
> 

Re: [PATCH 2/6] ext4: remove tracing for FALLOC_FL_NO_HIDE_STALE

[Darrick J. Wong]

On Tue, Aug 27, 2024 at 08:50:46AM +0200, Christoph Hellwig wrote:
> FALLOC_FL_NO_HIDE_STALE can't make it past vfs_fallocate (and if the
> flag does what the name implies that's a good thing as it would be
> highly dangerous).  Remove the dead tracing code for it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/trace/events/ext4.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index cc5e9b7b2b44e7..156908641e68f1 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -91,7 +91,6 @@ TRACE_DEFINE_ENUM(ES_REFERENCED_B);
>  #define show_falloc_mode(mode) __print_flags(mode, "|",		\
>  	{ FALLOC_FL_KEEP_SIZE,		"KEEP_SIZE"},		\
>  	{ FALLOC_FL_PUNCH_HOLE,		"PUNCH_HOLE"},		\
> -	{ FALLOC_FL_NO_HIDE_STALE,	"NO_HIDE_STALE"},	\

If we're going to drop this flag that has never been accepted by any of
the fallocate implementations, then can we remove it from the uapi?

--D

>  	{ FALLOC_FL_COLLAPSE_RANGE,	"COLLAPSE_RANGE"},	\
>  	{ FALLOC_FL_ZERO_RANGE,		"ZERO_RANGE"})
>  
> -- 
> 2.43.0
> 
> 

Re: [PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[Darrick J. Wong]

On Tue, Aug 27, 2024 at 08:50:48AM +0200, Christoph Hellwig wrote:
> Call xfs_flush_unmap_range from xfs_free_file_space so that
> xfs_file_fallocate doesn't have to predict which mode will call it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hmm.  I /think/ it's ok to shift the xfs_flush_unmap_range after the
file_modified and some of the other EINVAL bailouts that can happen
before xfs_free_file_space gets called.  Effectively that means that we
can fail faster now? :)

Later on this means that the cow-around code that the rtreflink patchset
introduces will also get flushed to disk before we start
collapsing/zeroing/punching.  AFAICT that should be fine.

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

--D

> ---
>  fs/xfs/xfs_bmap_util.c |  8 ++++++++
>  fs/xfs/xfs_file.c      | 21 ---------------------
>  2 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index fe2e2c93097550..187a0dbda24fc4 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -848,6 +848,14 @@ xfs_free_file_space(
>  	if (len <= 0)	/* if nothing being freed */
>  		return 0;
>  
> +	/*
> +	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
> +	 * the cached range over the first operation we are about to run.
> +	 */
> +	error = xfs_flush_unmap_range(ip, offset, len);
> +	if (error)
> +		return error;
> +
>  	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
>  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
>  
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 4cdc54dc96862e..5b9e49da06013c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -890,27 +890,6 @@ xfs_file_fallocate(
>  	 */
>  	inode_dio_wait(inode);
>  
> -	/*
> -	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
> -	 * the cached range over the first operation we are about to run.
> -	 *
> -	 * We care about zero and collapse here because they both run a hole
> -	 * punch over the range first. Because that can zero data, and the range
> -	 * of invalidation for the shift operations is much larger, we still do
> -	 * the required flush for collapse in xfs_prepare_shift().
> -	 *
> -	 * Insert has the same range requirements as collapse, and we extend the
> -	 * file first which can zero data. Hence insert has the same
> -	 * flush/invalidate requirements as collapse and so they are both
> -	 * handled at the right time by xfs_prepare_shift().
> -	 */
> -	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE |
> -		    FALLOC_FL_COLLAPSE_RANGE)) {
> -		error = xfs_flush_unmap_range(ip, offset, len);
> -		if (error)
> -			goto out_unlock;
> -	}
> -
>  	error = file_modified(file);
>  	if (error)
>  		goto out_unlock;
> -- 
> 2.43.0
> 
> 

Re: [PATCH 5/6] xfs: move the xfs_is_always_cow_inode check into xfs_alloc_file_space

[Darrick J. Wong]

On Tue, Aug 27, 2024 at 08:50:49AM +0200, Christoph Hellwig wrote:
> Move the xfs_is_always_cow_inode check from the caller into
> xfs_alloc_file_space to prepare for refactoring of xfs_file_fallocate.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

--D

> ---
>  fs/xfs/xfs_bmap_util.c | 3 +++
>  fs/xfs/xfs_file.c      | 8 +++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 187a0dbda24fc4..e9fdebaa40ea59 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -653,6 +653,9 @@ xfs_alloc_file_space(
>  	xfs_bmbt_irec_t		imaps[1], *imapp;
>  	int			error;
>  
> +	if (xfs_is_always_cow_inode(ip))
> +		return 0;
> +
>  	trace_xfs_alloc_file_space(ip);
>  
>  	if (xfs_is_shutdown(mp))
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 5b9e49da06013c..489bc1b173c268 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -987,11 +987,9 @@ xfs_file_fallocate(
>  			}
>  		}
>  
> -		if (!xfs_is_always_cow_inode(ip)) {
> -			error = xfs_alloc_file_space(ip, offset, len);
> -			if (error)
> -				goto out_unlock;
> -		}
> +		error = xfs_alloc_file_space(ip, offset, len);
> +		if (error)
> +			goto out_unlock;
>  	}
>  
>  	/* Change file size if needed */
> -- 
> 2.43.0
> 
> 

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

[Darrick J. Wong]

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
> 
> 

Re: [PATCH 2/6] ext4: remove tracing for FALLOC_FL_NO_HIDE_STALE

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 07:56:34AM -0700, Darrick J. Wong wrote:
> If we're going to drop this flag that has never been accepted by any of
> the fallocate implementations, then can we remove it from the uapi?

We usually don't really remove code point allocations once added,
but converting the define to a comment might be a good idea.

Re: [PATCH 3/6] fs: sort out the fallocate mode vs flag mess

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 07:55:02AM -0700, Darrick J. Wong wrote:
> For a brief moment I wondered if it were possible to set more than one
> mode bit but it looks like none of the implementations support that kind
> of wackiness (e.g. COLLAPSE|INSERT_RANGE for magicks!) so we're good:

Yes, we check for all possible mode combinations and reject them already,
just in had to read an not extensible way.

Re: [PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 09:03:23AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 27, 2024 at 08:50:48AM +0200, Christoph Hellwig wrote:
> > Call xfs_flush_unmap_range from xfs_free_file_space so that
> > xfs_file_fallocate doesn't have to predict which mode will call it.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Hmm.  I /think/ it's ok to shift the xfs_flush_unmap_range after the
> file_modified and some of the other EINVAL bailouts that can happen
> before xfs_free_file_space gets called.  Effectively that means that we
> can fail faster now? :)

Yes, failing faster has always been my personal benchmark :)

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

[Christoph Hellwig]

On Tue, Aug 27, 2024 at 09:07:03AM -0700, Darrick J. Wong wrote:
> 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! :)

A little more LOC, though.  But I think that's worth the tradeoff.

Re: [PATCH 1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE

[Christian Brauner]

On Tue, 27 Aug 2024 08:50:45 +0200, Christoph Hellwig wrote:
> While the FALLOC_FL_NO_HIDE_STALE value has been registered, it has
> always been rejected by vfs_fallocate before making it into
> blkdev_fallocate because it isn't in the supported mask.
> 
> 

Applied to the vfs.fallocate branch of the vfs/vfs.git tree.
Patches in the vfs.fallocate branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fallocate

[1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE
      https://git.kernel.org/vfs/vfs/c/a24dfa515642
[2/6] ext4: remove tracing for FALLOC_FL_NO_HIDE_STALE
      https://git.kernel.org/vfs/vfs/c/c5a8e5423301
[3/6] fs: sort out the fallocate mode vs flag mess
      https://git.kernel.org/vfs/vfs/c/2f6369068139
[4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space
      https://git.kernel.org/vfs/vfs/c/2764727be269
[5/6] xfs: move the xfs_is_always_cow_inode check into xfs_alloc_file_space
      https://git.kernel.org/vfs/vfs/c/12206b1c423b
[6/6] xfs: refactor xfs_file_fallocate
      https://git.kernel.org/vfs/vfs/c/a0c3802f87a2

Re: [PATCH 2/6] ext4: remove tracing for FALLOC_FL_NO_HIDE_STALE

[Jan Kara]

On Tue 27-08-24 08:50:46, Christoph Hellwig wrote:
> FALLOC_FL_NO_HIDE_STALE can't make it past vfs_fallocate (and if the
> flag does what the name implies that's a good thing as it would be
> highly dangerous).  Remove the dead tracing code for it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/trace/events/ext4.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index cc5e9b7b2b44e7..156908641e68f1 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -91,7 +91,6 @@ TRACE_DEFINE_ENUM(ES_REFERENCED_B);
>  #define show_falloc_mode(mode) __print_flags(mode, "|",		\
>  	{ FALLOC_FL_KEEP_SIZE,		"KEEP_SIZE"},		\
>  	{ FALLOC_FL_PUNCH_HOLE,		"PUNCH_HOLE"},		\
> -	{ FALLOC_FL_NO_HIDE_STALE,	"NO_HIDE_STALE"},	\
>  	{ FALLOC_FL_COLLAPSE_RANGE,	"COLLAPSE_RANGE"},	\
>  	{ FALLOC_FL_ZERO_RANGE,		"ZERO_RANGE"})
>  
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/6] block: remove checks for FALLOC_FL_NO_HIDE_STALE

[Jan Kara]

On Tue 27-08-24 08:50:45, Christoph Hellwig wrote:
> While the FALLOC_FL_NO_HIDE_STALE value has been registered, it has
> always been rejected by vfs_fallocate before making it into
> blkdev_fallocate because it isn't in the supported mask.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/fops.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index 9825c1713a49a9..7f48f03a62e9a8 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -771,7 +771,7 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  
>  #define	BLKDEV_FALLOC_FL_SUPPORTED					\
>  		(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE |		\
> -		 FALLOC_FL_ZERO_RANGE | FALLOC_FL_NO_HIDE_STALE)
> +		 FALLOC_FL_ZERO_RANGE)
>  
>  static long blkdev_fallocate(struct file *file, int mode, loff_t start,
>  			     loff_t len)
> @@ -830,14 +830,6 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
>  					     len >> SECTOR_SHIFT, GFP_KERNEL,
>  					     BLKDEV_ZERO_NOFALLBACK);
>  		break;
> -	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE:
> -		error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
> -		if (error)
> -			goto fail;
> -
> -		error = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
> -					     len >> SECTOR_SHIFT, GFP_KERNEL);
> -		break;
>  	default:
>  		error = -EOPNOTSUPP;
>  	}
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/6] fs: sort out the fallocate mode vs flag mess

[Jan Kara]

On Tue 27-08-24 08:50:47, Christoph Hellwig wrote:
> The fallocate system call takes a mode argument, but that argument
> contains a wild mix of exclusive modes and an optional flags.
> 
> Replace FALLOC_FL_SUPPORTED_MASK with FALLOC_FL_MODE_MASK, which excludes
> the optional flag bit, so that we can use switch statement on the value
> to easily enumerate the cases while getting the check for duplicate modes
> for free.
> 
> To make this (and in the future the file system implementations) more
> readable also add a symbolic name for the 0 mode used to allocate blocks.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/open.c                   | 51 ++++++++++++++++++-------------------
>  include/linux/falloc.h      | 18 ++++++++-----
>  include/uapi/linux/falloc.h |  1 +
>  3 files changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 22adbef7ecc2a6..daf1b55ca8180b 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -252,40 +252,39 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  	if (offset < 0 || len <= 0)
>  		return -EINVAL;
>  
> -	/* Return error if mode is not supported */
> -	if (mode & ~FALLOC_FL_SUPPORTED_MASK)
> +	if (mode & ~(FALLOC_FL_MODE_MASK | FALLOC_FL_KEEP_SIZE))
>  		return -EOPNOTSUPP;
>  
> -	/* Punch hole and zero range are mutually exclusive */
> -	if ((mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE)) ==
> -	    (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_ZERO_RANGE))
> -		return -EOPNOTSUPP;
> -
> -	/* Punch hole must have keep size set */
> -	if ((mode & FALLOC_FL_PUNCH_HOLE) &&
> -	    !(mode & FALLOC_FL_KEEP_SIZE))
> +	/*
> +	 * Modes are exclusive, even if that is not obvious from the encoding
> +	 * as bit masks and the mix with the flag in the same namespace.
> +	 *
> +	 * To make things even more complicated, FALLOC_FL_ALLOCATE_RANGE is
> +	 * encoded as no bit set.
> +	 */
> +	switch (mode & FALLOC_FL_MODE_MASK) {
> +	case FALLOC_FL_ALLOCATE_RANGE:
> +	case FALLOC_FL_UNSHARE_RANGE:
> +	case FALLOC_FL_ZERO_RANGE:
> +		break;
> +	case FALLOC_FL_PUNCH_HOLE:
> +		if (!(mode & FALLOC_FL_KEEP_SIZE))
> +			return -EOPNOTSUPP;
> +		break;
> +	case FALLOC_FL_COLLAPSE_RANGE:
> +	case FALLOC_FL_INSERT_RANGE:
> +		if (mode & FALLOC_FL_KEEP_SIZE)
> +			return -EOPNOTSUPP;
> +		break;
> +	default:
>  		return -EOPNOTSUPP;
> -
> -	/* Collapse range should only be used exclusively. */
> -	if ((mode & FALLOC_FL_COLLAPSE_RANGE) &&
> -	    (mode & ~FALLOC_FL_COLLAPSE_RANGE))
> -		return -EINVAL;
> -
> -	/* Insert range should only be used exclusively. */
> -	if ((mode & FALLOC_FL_INSERT_RANGE) &&
> -	    (mode & ~FALLOC_FL_INSERT_RANGE))
> -		return -EINVAL;
> -
> -	/* Unshare range should only be used with allocate mode. */
> -	if ((mode & FALLOC_FL_UNSHARE_RANGE) &&
> -	    (mode & ~(FALLOC_FL_UNSHARE_RANGE | FALLOC_FL_KEEP_SIZE)))
> -		return -EINVAL;
> +	}
>  
>  	if (!(file->f_mode & FMODE_WRITE))
>  		return -EBADF;
>  
>  	/*
> -	 * We can only allow pure fallocate on append only files
> +	 * On append-only files only space preallocation is supported.
>  	 */
>  	if ((mode & ~FALLOC_FL_KEEP_SIZE) && IS_APPEND(inode))
>  		return -EPERM;
> diff --git a/include/linux/falloc.h b/include/linux/falloc.h
> index f3f0b97b167579..3f49f3df6af5fb 100644
> --- a/include/linux/falloc.h
> +++ b/include/linux/falloc.h
> @@ -25,12 +25,18 @@ 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 |		\
> -					 FALLOC_FL_COLLAPSE_RANGE |	\
> -					 FALLOC_FL_ZERO_RANGE |		\
> -					 FALLOC_FL_INSERT_RANGE |	\
> -					 FALLOC_FL_UNSHARE_RANGE)
> +/*
> + * Mask of all supported fallocate modes.  Only one can be set at a time.
> + *
> + * In addition to the mode bit, the mode argument can also encode flags.
> + * FALLOC_FL_KEEP_SIZE is the only supported flag so far.
> + */
> +#define FALLOC_FL_MODE_MASK	(FALLOC_FL_ALLOCATE_RANGE |	\
> +				 FALLOC_FL_PUNCH_HOLE |		\
> +				 FALLOC_FL_COLLAPSE_RANGE |	\
> +				 FALLOC_FL_ZERO_RANGE |		\
> +				 FALLOC_FL_INSERT_RANGE |	\
> +				 FALLOC_FL_UNSHARE_RANGE)
>  
>  /* on ia32 l_start is on a 32-bit boundary */
>  #if defined(CONFIG_X86_64)
> diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
> index 51398fa57f6cdf..5810371ed72bbd 100644
> --- a/include/uapi/linux/falloc.h
> +++ b/include/uapi/linux/falloc.h
> @@ -2,6 +2,7 @@
>  #ifndef _UAPI_FALLOC_H_
>  #define _UAPI_FALLOC_H_
>  
> +#define FALLOC_FL_ALLOCATE_RANGE 0x00 /* allocate range */
>  #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
>  #define FALLOC_FL_PUNCH_HOLE	0x02 /* de-allocates range */
>  #define FALLOC_FL_NO_HIDE_STALE	0x04 /* reserved codepoint */
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[Dave Chinner]

On Tue, Aug 27, 2024 at 08:50:48AM +0200, Christoph Hellwig wrote:
> Call xfs_flush_unmap_range from xfs_free_file_space so that
> xfs_file_fallocate doesn't have to predict which mode will call it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_bmap_util.c |  8 ++++++++
>  fs/xfs/xfs_file.c      | 21 ---------------------
>  2 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index fe2e2c93097550..187a0dbda24fc4 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -848,6 +848,14 @@ xfs_free_file_space(
>  	if (len <= 0)	/* if nothing being freed */
>  		return 0;
>  
> +	/*
> +	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
> +	 * the cached range over the first operation we are about to run.
> +	 */
> +	error = xfs_flush_unmap_range(ip, offset, len);
> +	if (error)
> +		return error;
> +
>  	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
>  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);

Ok, so if we ever change the zeroing implementation to not punch a
hole first, we're going to have to make sure we add this to whatever
new zeroing implementation we have.

Can you leave a comment in the FALLOC_FL_ZERO_RANGE implementation
code that notes it currently relies on the xfs_flush_unmap_range()
in xfs_free_file_space() for correct operation?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Dave Chinner]

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>
> ---
>  fs/xfs/xfs_file.c | 330 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 208 insertions(+), 122 deletions(-)

Much nicer. :)

And it made an existing issue in the code quite obvious, too:

> +/*
> + * 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);
> +}

Our zeroing operation always does preallocation, but....


> +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;

... our preallocation operation always returns -EOPNOTSUPP for
COW mode.

Should the zeroing code also have this COW mode check in it after
the hole punch has run so we don't do unnecessary prealloc there?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/6] xfs: call xfs_flush_unmap_range from xfs_free_file_space

[Christoph Hellwig]

On Thu, Aug 29, 2024 at 01:03:31PM +1000, Dave Chinner wrote:
> > @@ -848,6 +848,14 @@ xfs_free_file_space(
> >  	if (len <= 0)	/* if nothing being freed */
> >  		return 0;
> >  
> > +	/*
> > +	 * Now AIO and DIO has drained we flush and (if necessary) invalidate
> > +	 * the cached range over the first operation we are about to run.
> > +	 */
> > +	error = xfs_flush_unmap_range(ip, offset, len);
> > +	if (error)
> > +		return error;
> > +
> >  	startoffset_fsb = XFS_B_TO_FSB(mp, offset);
> >  	endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
> 
> Ok, so if we ever change the zeroing implementation to not punch a
> hole first, we're going to have to make sure we add this to whatever
> new zeroing implementation we have.
> 
> Can you leave a comment in the FALLOC_FL_ZERO_RANGE implementation
> code that notes it currently relies on the xfs_flush_unmap_range()
> in xfs_free_file_space() for correct operation?

Sure.

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

[Christoph Hellwig]

On Thu, Aug 29, 2024 at 01:11:11PM +1000, Dave Chinner wrote:
> > +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;
> 
> ... our preallocation operation always returns -EOPNOTSUPP for
> COW mode.
> 
> Should the zeroing code also have this COW mode check in it after
> the hole punch has run so we don't do unnecessary prealloc there?

The low-level block allocation helper just returns early without
doing work (move a bit, but not changed in behavior earlier in the
series).  So it won't actually do the prealloc.

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
---end quoted text---