linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems
@ 2018-10-10  0:10 Darrick J. Wong
  2018-10-10  0:10 ` [PATCH 01/25] xfs: add a per-xfs trace_printk macro Darrick J. Wong
                   ` (25 more replies)
  0 siblings, 26 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:10 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

Hi all,

Dave, Eric, and I have been chasing a stale data exposure bug in the XFS
reflink implementation, and tracked it down to reflink forgetting to do
some of the file-extending activities that must happen for regular
writes.

We then started auditing the clone, dedupe, and copyfile code and
realized that from a file contents perspective, clonerange isn't any
different from a regular file write.  Unfortunately, we also noticed
that *unlike* a regular write, clonerange skips a ton of overflow
checks, such as validating the ranges against s_maxbytes, MAX_NON_LFS,
and RLIMIT_FSIZE.  We also observed that cloning into a file did not
strip security privileges (suid, capabilities) like a regular write
would.  I also noticed that xfs and ocfs2 need to dump the page cache
before remapping blocks, not after.

In fixing the range checking problems I also realized that both dedupe
and copyfile tell userspace how much of the requested operation was
acted upon.  Since the range validation can shorten a clone request (or
we can ENOSPC midway through), we might as well plumb the short
operation reporting back through the VFS indirection code to userspace.

So, here's the whole giant pile of patches[1] that fix all the problems.
The patch "generic: test reflink side effects" recently sent to fstests
exercises the fixes in this series.  Tests are in [2].

--D

[1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=djwong-devel
[2] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=djwong-devel

^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH 01/25] xfs: add a per-xfs trace_printk macro
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
@ 2018-10-10  0:10 ` Darrick J. Wong
  2018-10-10  0:36   ` Dave Chinner
  2018-10-10 15:00   ` [PATCH v2 " Darrick J. Wong
  2018-10-10  0:10 ` [PATCH 02/25] xfs: refactor clonerange preparation into a separate helper Darrick J. Wong
                   ` (24 subsequent siblings)
  25 siblings, 2 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:10 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a "xfs_tprintk" macro so that developers can use trace_printk to
print out arbitrary debugging information with the XFS device name
attached to the trace output.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_error.h |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 246d3e989c6c..c3d9546b138c 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -99,4 +99,9 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
 #define		XFS_PTAG_SHUTDOWN_LOGERROR	0x00000040
 #define		XFS_PTAG_FSBLOCK_ZERO		0x00000080
 
+/* trace printk version of xfs_err and friends */
+#define xfs_tprintk(mp, fmt, args...) \
+	trace_printk("dev %d:%d " fmt, MAJOR((mp)->m_super->s_dev), \
+			MINOR((mp)->m_super->s_dev), ##args)
+
 #endif	/* __XFS_ERROR_H__ */

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 02/25] xfs: refactor clonerange preparation into a separate helper
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
  2018-10-10  0:10 ` [PATCH 01/25] xfs: add a per-xfs trace_printk macro Darrick J. Wong
@ 2018-10-10  0:10 ` Darrick J. Wong
  2018-10-10  0:10 ` [PATCH 03/25] xfs: zero posteof blocks when cloning above eof Darrick J. Wong
                   ` (23 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:10 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, Dave Chinner, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor all the reflink preparation steps into a separate helper that
we'll use to land all the upcoming fixes for insufficient input checks.

This rework also moves the invalidation of the destination range to the
prep function so that it is done before the range is remapped.  This
ensures that nobody can access the data in range being remapped until
the remap is complete.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c |  101 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 74 insertions(+), 27 deletions(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 5289e22cb081..ebd65d3e31f3 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1220,35 +1220,48 @@ xfs_iolock_two_inodes_and_break_layout(
 	return 0;
 }
 
+/* Unlock both inodes after they've been prepped for a range clone. */
+STATIC void
+xfs_reflink_remap_unlock(
+	struct file		*file_in,
+	struct file		*file_out)
+{
+	struct inode		*inode_in = file_inode(file_in);
+	struct xfs_inode	*src = XFS_I(inode_in);
+	struct inode		*inode_out = file_inode(file_out);
+	struct xfs_inode	*dest = XFS_I(inode_out);
+	bool			same_inode = (inode_in == inode_out);
+
+	xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
+	if (!same_inode)
+		xfs_iunlock(src, XFS_MMAPLOCK_SHARED);
+	inode_unlock(inode_out);
+	if (!same_inode)
+		inode_unlock_shared(inode_in);
+}
+
 /*
- * Link a range of blocks from one file to another.
+ * Prepare two files for range cloning.  Upon a successful return both inodes
+ * will have the iolock and mmaplock held, the page cache of the out file
+ * will be truncated, and any leases on the out file will have been broken.
+ * Returns negative for error, 0 for nothing to do, and 1 for success.
  */
-int
-xfs_reflink_remap_range(
+STATIC int
+xfs_reflink_remap_prep(
 	struct file		*file_in,
 	loff_t			pos_in,
 	struct file		*file_out,
 	loff_t			pos_out,
-	u64			len,
+	u64			*len,
 	bool			is_dedupe)
 {
 	struct inode		*inode_in = file_inode(file_in);
 	struct xfs_inode	*src = XFS_I(inode_in);
 	struct inode		*inode_out = file_inode(file_out);
 	struct xfs_inode	*dest = XFS_I(inode_out);
-	struct xfs_mount	*mp = src->i_mount;
 	bool			same_inode = (inode_in == inode_out);
-	xfs_fileoff_t		sfsbno, dfsbno;
-	xfs_filblks_t		fsblen;
-	xfs_extlen_t		cowextsize;
 	ssize_t			ret;
 
-	if (!xfs_sb_version_hasreflink(&mp->m_sb))
-		return -EOPNOTSUPP;
-
-	if (XFS_FORCED_SHUTDOWN(mp))
-		return -EIO;
-
 	/* Lock both files against IO */
 	ret = xfs_iolock_two_inodes_and_break_layout(inode_in, inode_out);
 	if (ret)
@@ -1270,7 +1283,7 @@ xfs_reflink_remap_range(
 		goto out_unlock;
 
 	ret = vfs_clone_file_prep_inodes(inode_in, pos_in, inode_out, pos_out,
-			&len, is_dedupe);
+			len, is_dedupe);
 	if (ret <= 0)
 		goto out_unlock;
 
@@ -1279,8 +1292,6 @@ xfs_reflink_remap_range(
 	if (ret)
 		goto out_unlock;
 
-	trace_xfs_reflink_remap_range(src, pos_in, len, dest, pos_out);
-
 	/*
 	 * Clear out post-eof preallocations because we don't have page cache
 	 * backing the delayed allocations and they'll never get freed on
@@ -1297,6 +1308,51 @@ xfs_reflink_remap_range(
 	if (ret)
 		goto out_unlock;
 
+	/* Zap any page cache for the destination file's range. */
+	truncate_inode_pages_range(&inode_out->i_data, pos_out,
+				   PAGE_ALIGN(pos_out + *len) - 1);
+	return 1;
+out_unlock:
+	xfs_reflink_remap_unlock(file_in, file_out);
+	return ret;
+}
+
+/*
+ * Link a range of blocks from one file to another.
+ */
+int
+xfs_reflink_remap_range(
+	struct file		*file_in,
+	loff_t			pos_in,
+	struct file		*file_out,
+	loff_t			pos_out,
+	u64			len,
+	bool			is_dedupe)
+{
+	struct inode		*inode_in = file_inode(file_in);
+	struct xfs_inode	*src = XFS_I(inode_in);
+	struct inode		*inode_out = file_inode(file_out);
+	struct xfs_inode	*dest = XFS_I(inode_out);
+	struct xfs_mount	*mp = src->i_mount;
+	xfs_fileoff_t		sfsbno, dfsbno;
+	xfs_filblks_t		fsblen;
+	xfs_extlen_t		cowextsize;
+	ssize_t			ret;
+
+	if (!xfs_sb_version_hasreflink(&mp->m_sb))
+		return -EOPNOTSUPP;
+
+	if (XFS_FORCED_SHUTDOWN(mp))
+		return -EIO;
+
+	/* Prepare and then clone file data. */
+	ret = xfs_reflink_remap_prep(file_in, pos_in, file_out, pos_out,
+			&len, is_dedupe);
+	if (ret <= 0)
+		return ret;
+
+	trace_xfs_reflink_remap_range(src, pos_in, len, dest, pos_out);
+
 	dfsbno = XFS_B_TO_FSBT(mp, pos_out);
 	sfsbno = XFS_B_TO_FSBT(mp, pos_in);
 	fsblen = XFS_B_TO_FSB(mp, len);
@@ -1305,10 +1361,6 @@ xfs_reflink_remap_range(
 	if (ret)
 		goto out_unlock;
 
-	/* Zap any page cache for the destination file's range. */
-	truncate_inode_pages_range(&inode_out->i_data, pos_out,
-				   PAGE_ALIGN(pos_out + len) - 1);
-
 	/*
 	 * Carry the cowextsize hint from src to dest if we're sharing the
 	 * entire source file to the entire destination file, the source file
@@ -1325,12 +1377,7 @@ xfs_reflink_remap_range(
 			is_dedupe);
 
 out_unlock:
-	xfs_iunlock(dest, XFS_MMAPLOCK_EXCL);
-	if (!same_inode)
-		xfs_iunlock(src, XFS_MMAPLOCK_SHARED);
-	inode_unlock(inode_out);
-	if (!same_inode)
-		inode_unlock_shared(inode_in);
+	xfs_reflink_remap_unlock(file_in, file_out);
 	if (ret)
 		trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
 	return ret;

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 03/25] xfs: zero posteof blocks when cloning above eof
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
  2018-10-10  0:10 ` [PATCH 01/25] xfs: add a per-xfs trace_printk macro Darrick J. Wong
  2018-10-10  0:10 ` [PATCH 02/25] xfs: refactor clonerange preparation into a separate helper Darrick J. Wong
@ 2018-10-10  0:10 ` Darrick J. Wong
  2018-10-10  0:11 ` [PATCH 04/25] xfs: update ctime and remove suid before cloning files Darrick J. Wong
                   ` (22 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:10 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, Zorro Lang, linux-unionfs,
	linux-xfs, linux-mm, linux-btrfs, Dave Chinner, linux-fsdevel,
	ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

When we're reflinking between two files and the destination file range
is well beyond the destination file's EOF marker, zero any posteof
speculative preallocations in the destination file so that we don't
expose stale disk contents.  The previous strategy of trying to clear
the preallocations does not work if the destination file has the
PREALLOC flag set.

Uncovered by shared/010.

Reported-by: Zorro Lang <zlang@redhat.com>
Bugzilla-id: https://bugzilla.kernel.org/show_bug.cgi?id=201259
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c |   33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index ebd65d3e31f3..cbb359e68a72 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1240,6 +1240,26 @@ xfs_reflink_remap_unlock(
 		inode_unlock_shared(inode_in);
 }
 
+/*
+ * If we're reflinking to a point past the destination file's EOF, we must
+ * zero any speculative post-EOF preallocations that sit between the old EOF
+ * and the destination file offset.
+ */
+static int
+xfs_reflink_zero_posteof(
+	struct xfs_inode	*ip,
+	loff_t			pos)
+{
+	loff_t			isize = i_size_read(VFS_I(ip));
+
+	if (pos <= isize)
+		return 0;
+
+	trace_xfs_zero_eof(ip, isize, pos - isize);
+	return iomap_zero_range(VFS_I(ip), isize, pos - isize, NULL,
+			&xfs_iomap_ops);
+}
+
 /*
  * Prepare two files for range cloning.  Upon a successful return both inodes
  * will have the iolock and mmaplock held, the page cache of the out file
@@ -1293,15 +1313,12 @@ xfs_reflink_remap_prep(
 		goto out_unlock;
 
 	/*
-	 * Clear out post-eof preallocations because we don't have page cache
-	 * backing the delayed allocations and they'll never get freed on
-	 * their own.
+	 * Zero existing post-eof speculative preallocations in the destination
+	 * file.
 	 */
-	if (xfs_can_free_eofblocks(dest, true)) {
-		ret = xfs_free_eofblocks(dest);
-		if (ret)
-			goto out_unlock;
-	}
+	ret = xfs_reflink_zero_posteof(dest, pos_out);
+	if (ret)
+		goto out_unlock;
 
 	/* Set flags and remap blocks. */
 	ret = xfs_reflink_set_inode_flag(src, dest);

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 04/25] xfs: update ctime and remove suid before cloning files
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (2 preceding siblings ...)
  2018-10-10  0:10 ` [PATCH 03/25] xfs: zero posteof blocks when cloning above eof Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10  0:11 ` [PATCH 05/25] vfs: check file ranges " Darrick J. Wong
                   ` (21 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, Dave Chinner, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Before cloning into a file, update the ctime and remove sensitive
attributes like suid, just like we'd do for a regular file write.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_reflink.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index cbb359e68a72..d4feaeba8542 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1264,6 +1264,7 @@ xfs_reflink_zero_posteof(
  * Prepare two files for range cloning.  Upon a successful return both inodes
  * will have the iolock and mmaplock held, the page cache of the out file
  * will be truncated, and any leases on the out file will have been broken.
+ * This function borrows heavily from xfs_file_aio_write_checks.
  * Returns negative for error, 0 for nothing to do, and 1 for success.
  */
 STATIC int
@@ -1328,6 +1329,30 @@ xfs_reflink_remap_prep(
 	/* Zap any page cache for the destination file's range. */
 	truncate_inode_pages_range(&inode_out->i_data, pos_out,
 				   PAGE_ALIGN(pos_out + *len) - 1);
+
+	/* If we're altering the file contents... */
+	if (!is_dedupe) {
+		/*
+		 * ...update the timestamps (which will grab the ilock again
+		 * from xfs_fs_dirty_inode, so we have to call it before we
+		 * take the ilock).
+		 */
+		if (!(file_out->f_mode & FMODE_NOCMTIME)) {
+			ret = file_update_time(file_out);
+			if (ret)
+				goto out_unlock;
+		}
+
+		/*
+		 * ...clear the security bits if the process is not being run
+		 * by root.  This keeps people from modifying setuid and setgid
+		 * binaries.
+		 */
+		ret = file_remove_privs(file_out);
+		if (ret)
+			goto out_unlock;
+	}
+
 	return 1;
 out_unlock:
 	xfs_reflink_remap_unlock(file_in, file_out);

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 05/25] vfs: check file ranges before cloning files
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (3 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 04/25] xfs: update ctime and remove suid before cloning files Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10 23:06   ` Dave Chinner
  2018-10-10  0:11 ` [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks Darrick J. Wong
                   ` (20 subsequent siblings)
  25 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, Christoph Hellwig,
	ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the file range checks from vfs_clone_file_prep into a separate
generic_remap_checks function so that all the checks are collected in a
central location.  This forms the basis for adding more checks from
generic_write_checks that will make cloning's input checking more
consistent with write input checking.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/ocfs2/refcounttree.c |    2 +
 fs/read_write.c         |   53 +++++++-----------------------------
 fs/xfs/xfs_reflink.c    |    2 +
 include/linux/fs.h      |    9 ++++--
 mm/filemap.c            |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 89 insertions(+), 47 deletions(-)


diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 7a5ee145c733..19e03936c5e1 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4850,7 +4850,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 	    (OCFS2_I(inode_out)->ip_flags & OCFS2_INODE_SYSTEM_FILE))
 		goto out_unlock;
 
-	ret = vfs_clone_file_prep_inodes(inode_in, pos_in, inode_out, pos_out,
+	ret = vfs_clone_file_prep(file_in, pos_in, file_out, pos_out,
 			&len, is_dedupe);
 	if (ret <= 0)
 		goto out_unlock;
diff --git a/fs/read_write.c b/fs/read_write.c
index 8a2737f0d61d..f7b728d4972f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1717,12 +1717,12 @@ static int clone_verify_area(struct file *file, loff_t pos, u64 len, bool write)
  * Returns: 0 for "nothing to clone", 1 for "something to clone", or
  * the usual negative error code.
  */
-int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
-			       struct inode *inode_out, loff_t pos_out,
-			       u64 *len, bool is_dedupe)
+int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
+			struct file *file_out, loff_t pos_out,
+			u64 *len, bool is_dedupe)
 {
-	loff_t bs = inode_out->i_sb->s_blocksize;
-	loff_t blen;
+	struct inode *inode_in = file_inode(file_in);
+	struct inode *inode_out = file_inode(file_out);
 	loff_t isize;
 	bool same_inode = (inode_in == inode_out);
 	int ret;
@@ -1740,10 +1740,7 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
 	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
 		return -EINVAL;
 
-	/* Are we going all the way to the end? */
 	isize = i_size_read(inode_in);
-	if (isize == 0)
-		return 0;
 
 	/* Zero length dedupe exits immediately; reflink goes to EOF. */
 	if (*len == 0) {
@@ -1754,36 +1751,11 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
 		*len = isize - pos_in;
 	}
 
-	/* Ensure offsets don't wrap and the input is inside i_size */
-	if (pos_in + *len < pos_in || pos_out + *len < pos_out ||
-	    pos_in + *len > isize)
-		return -EINVAL;
-
-	/* Don't allow dedupe past EOF in the dest file */
-	if (is_dedupe) {
-		loff_t	disize;
-
-		disize = i_size_read(inode_out);
-		if (pos_out >= disize || pos_out + *len > disize)
-			return -EINVAL;
-	}
-
-	/* If we're linking to EOF, continue to the block boundary. */
-	if (pos_in + *len == isize)
-		blen = ALIGN(isize, bs) - pos_in;
-	else
-		blen = *len;
-
-	/* Only reflink if we're aligned to block boundaries */
-	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_in + blen, bs) ||
-	    !IS_ALIGNED(pos_out, bs) || !IS_ALIGNED(pos_out + blen, bs))
-		return -EINVAL;
-
-	/* Don't allow overlapped reflink within the same file */
-	if (same_inode) {
-		if (pos_out + blen > pos_in && pos_out < pos_in + blen)
-			return -EINVAL;
-	}
+	/* Check that we don't violate system file offset limits. */
+	ret = generic_remap_checks(file_in, pos_in, file_out, pos_out, len,
+			is_dedupe);
+	if (ret)
+		return ret;
 
 	/* Wait for the completion of any pending IOs on both files */
 	inode_dio_wait(inode_in);
@@ -1816,7 +1788,7 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
 
 	return 1;
 }
-EXPORT_SYMBOL(vfs_clone_file_prep_inodes);
+EXPORT_SYMBOL(vfs_clone_file_prep);
 
 int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			struct file *file_out, loff_t pos_out, u64 len)
@@ -1854,9 +1826,6 @@ int do_clone_file_range(struct file *file_in, loff_t pos_in,
 	if (ret)
 		return ret;
 
-	if (pos_in + len > i_size_read(inode_in))
-		return -EINVAL;
-
 	ret = file_in->f_op->clone_file_range(file_in, pos_in,
 			file_out, pos_out, len);
 	if (!ret) {
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index d4feaeba8542..a4a7b2d9c8a1 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1303,7 +1303,7 @@ xfs_reflink_remap_prep(
 	if (IS_DAX(inode_in) || IS_DAX(inode_out))
 		goto out_unlock;
 
-	ret = vfs_clone_file_prep_inodes(inode_in, pos_in, inode_out, pos_out,
+	ret = vfs_clone_file_prep(file_in, pos_in, file_out, pos_out,
 			len, is_dedupe);
 	if (ret <= 0)
 		goto out_unlock;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 897eae8faee1..ba93a6e7dac4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1825,9 +1825,9 @@ extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *, rwf_t);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
-extern int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
-				      struct inode *inode_out, loff_t pos_out,
-				      u64 *len, bool is_dedupe);
+extern int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
+			       struct file *file_out, loff_t pos_out,
+			       u64 *count, bool is_dedupe);
 extern int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			       struct file *file_out, loff_t pos_out, u64 len);
 extern int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
@@ -2967,6 +2967,9 @@ extern int sb_min_blocksize(struct super_block *, int);
 extern int generic_file_mmap(struct file *, struct vm_area_struct *);
 extern int generic_file_readonly_mmap(struct file *, struct vm_area_struct *);
 extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
+extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
+				struct file *file_out, loff_t pos_out,
+				uint64_t *count, bool is_dedupe);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);
diff --git a/mm/filemap.c b/mm/filemap.c
index 52517f28e6f4..14041a8468ba 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2974,6 +2974,76 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 }
 EXPORT_SYMBOL(generic_write_checks);
 
+/*
+ * Performs necessary checks before doing a clone.
+ *
+ * Can adjust amount of bytes to clone.
+ * Returns appropriate error code that caller should return or
+ * zero in case the clone should be allowed.
+ */
+int generic_remap_checks(struct file *file_in, loff_t pos_in,
+			 struct file *file_out, loff_t pos_out,
+			 uint64_t *req_count, bool is_dedupe)
+{
+	struct inode *inode_in = file_in->f_mapping->host;
+	struct inode *inode_out = file_out->f_mapping->host;
+	unsigned long limit = rlimit(RLIMIT_FSIZE);
+	uint64_t count = *req_count;
+	uint64_t bcount;
+	loff_t size_in, size_out;
+	loff_t bs = inode_out->i_sb->s_blocksize;
+
+	/* The start of both ranges must be aligned to an fs block. */
+	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
+		return -EINVAL;
+
+	/* Ensure offsets don't wrap. */
+	if (pos_in + count < pos_in || pos_out + count < pos_out)
+		return -EINVAL;
+
+	size_in = i_size_read(inode_in);
+	size_out = i_size_read(inode_out);
+
+	/* Dedupe requires both ranges to be within EOF. */
+	if (is_dedupe &&
+	    (pos_in >= size_in || pos_in + count > size_in ||
+	     pos_out >= size_out || pos_out + count > size_out))
+		return -EINVAL;
+
+	/* Ensure the infile range is within the infile. */
+	if (pos_in >= size_in)
+		return -EINVAL;
+	count = min(count, size_in - (uint64_t)pos_in);
+
+	/*
+	 * If the user wanted us to link to the infile's EOF, round up to the
+	 * next block boundary for this check.
+	 *
+	 * Otherwise, make sure the count is also block-aligned, having
+	 * already confirmed the starting offsets' block alignment.
+	 */
+	if (pos_in + count == size_in) {
+		bcount = ALIGN(size_in, bs) - pos_in;
+	} else {
+		if (!IS_ALIGNED(count, bs))
+			return -EINVAL;
+
+		bcount = count;
+	}
+
+	/* Don't allow overlapped cloning within the same file. */
+	if (inode_in == inode_out &&
+	    pos_out + bcount > pos_in &&
+	    pos_out < pos_in + bcount)
+		return -EINVAL;
+
+	/* For now we don't support changing the length. */
+	if (*req_count != count)
+		return -EINVAL;
+
+	return 0;
+}
+
 int pagecache_write_begin(struct file *file, struct address_space *mapping,
 				loff_t pos, unsigned len, unsigned flags,
 				struct page **pagep, void **fsdata)

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (4 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 05/25] vfs: check file ranges " Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10  5:23   ` Amir Goldstein
  2018-10-10  0:11 ` [PATCH 07/25] vfs: skip zero-length dedupe requests Darrick J. Wong
                   ` (19 subsequent siblings)
  25 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

File range remapping, if allowed to run past the destination file's EOF,
is an optimization on a regular file write.  Regular file writes that
extend the file length are subject to various constraints which are not
checked by range cloning.

This is a correctness problem because we're never allowed to touch
ranges that the page cache can't support (s_maxbytes); we're not
supposed to deal with large offsets (MAX_NON_LFS) if O_LARGEFILE isn't
set; and we must obey resource limits (RLIMIT_FSIZE).

Therefore, add these checks to the new generic_remap_checks function so
that we curtail unexpected behavior.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mm/filemap.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)


diff --git a/mm/filemap.c b/mm/filemap.c
index 14041a8468ba..59056bd9c58a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2974,6 +2974,27 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 }
 EXPORT_SYMBOL(generic_write_checks);
 
+static int
+generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
+{
+	struct inode *inode = file->f_mapping->host;
+
+	/* Don't exceed the LFS limits. */
+	if (unlikely(pos + *count > MAX_NON_LFS &&
+				!(file->f_flags & O_LARGEFILE))) {
+		if (pos >= MAX_NON_LFS)
+			return -EFBIG;
+		*count = min(*count, MAX_NON_LFS - (uint64_t)pos);
+	}
+
+	/* Don't operate on ranges the page cache doesn't support. */
+	if (unlikely(pos >= inode->i_sb->s_maxbytes))
+		return -EFBIG;
+
+	*count = min(*count, inode->i_sb->s_maxbytes - (uint64_t)pos);
+	return 0;
+}
+
 /*
  * Performs necessary checks before doing a clone.
  *
@@ -2992,6 +3013,7 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	uint64_t bcount;
 	loff_t size_in, size_out;
 	loff_t bs = inode_out->i_sb->s_blocksize;
+	int ret;
 
 	/* The start of both ranges must be aligned to an fs block. */
 	if (!IS_ALIGNED(pos_in, bs) || !IS_ALIGNED(pos_out, bs))
@@ -3015,6 +3037,23 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 		return -EINVAL;
 	count = min(count, size_in - (uint64_t)pos_in);
 
+	/* Don't exceed RLMIT_FSIZE in the file we're writing into. */
+	if (limit != RLIM_INFINITY) {
+		if (pos_out >= limit) {
+			send_sig(SIGXFSZ, current, 0);
+			return -EFBIG;
+		}
+		count = min(count, limit - (uint64_t)pos_out);
+	}
+
+	ret = generic_remap_check_limits(file_in, pos_in, &count);
+	if (ret)
+		return ret;
+
+	ret = generic_remap_check_limits(file_out, pos_out, &count);
+	if (ret)
+		return ret;
+
 	/*
 	 * If the user wanted us to link to the infile's EOF, round up to the
 	 * next block boundary for this check.

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 07/25] vfs: skip zero-length dedupe requests
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (5 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10  0:11 ` [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range Darrick J. Wong
                   ` (18 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, Christoph Hellwig,
	ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Don't bother calling the filesystem for a zero-length dedupe request;
we can return zero and exit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/read_write.c |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/fs/read_write.c b/fs/read_write.c
index f7b728d4972f..3ff90b3315fb 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1975,6 +1975,11 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 	if (!dst_file->f_op->dedupe_file_range)
 		goto out_drop_write;
 
+	if (len == 0) {
+		ret = 0;
+		goto out_drop_write;
+	}
+
 	ret = dst_file->f_op->dedupe_file_range(src_file, src_pos,
 						dst_file, dst_pos, len);
 out_drop_write:

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (6 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 07/25] vfs: skip zero-length dedupe requests Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10  5:54   ` Amir Goldstein
  2018-10-10  0:11 ` [PATCH 09/25] vfs: rename vfs_clone_file_prep to be more descriptive Darrick J. Wong
                   ` (17 subsequent siblings)
  25 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Combine the clone_file_range and dedupe_file_range operations into a
single remap_file_range file operation dispatch since they're
fundamentally the same operation.  The differences between the two can
be made in the prep functions.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 Documentation/filesystems/vfs.txt |   12 ++++-------
 fs/btrfs/ctree.h                  |    8 +++----
 fs/btrfs/file.c                   |    3 +--
 fs/btrfs/ioctl.c                  |   42 ++++++++++++++++++-------------------
 fs/cifs/cifsfs.c                  |   22 +++++++++++--------
 fs/nfs/nfs4file.c                 |   10 ++++++---
 fs/ocfs2/file.c                   |   23 +++++---------------
 fs/overlayfs/file.c               |   27 ++++++++++++------------
 fs/read_write.c                   |   18 ++++++++--------
 fs/xfs/xfs_file.c                 |   22 ++++---------------
 include/linux/fs.h                |   13 ++++++++---
 11 files changed, 92 insertions(+), 108 deletions(-)


diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index a6c6a8af48a2..120fd72c87be 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -883,8 +883,9 @@ struct file_operations {
 	unsigned (*mmap_capabilities)(struct file *);
 #endif
 	ssize_t (*copy_file_range)(struct file *, loff_t, struct file *, loff_t, size_t, unsigned int);
-	int (*clone_file_range)(struct file *, loff_t, struct file *, loff_t, u64);
-	int (*dedupe_file_range)(struct file *, loff_t, struct file *, loff_t, u64);
+	int (*remap_file_range)(struct file *file_in, loff_t pos_in,
+				struct file *file_out, loff_t pos_out,
+				u64 len, unsigned int flags);
 	int (*fadvise)(struct file *, loff_t, loff_t, int);
 };
 
@@ -960,11 +961,8 @@ otherwise noted.
 
   copy_file_range: called by the copy_file_range(2) system call.
 
-  clone_file_range: called by the ioctl(2) system call for FICLONERANGE and
-	FICLONE commands.
-
-  dedupe_file_range: called by the ioctl(2) system call for FIDEDUPERANGE
-	command.
+  remap_file_range: called by the ioctl(2) system call for FICLONERANGE and
+	FICLONE and FIDEDUPERANGE commands to remap file ranges.
 
   fadvise: possibly called by the fadvise64() system call.
 
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2cddfe7806a4..a79783a713f0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3218,9 +3218,6 @@ void btrfs_get_block_group_info(struct list_head *groups_list,
 				struct btrfs_ioctl_space_info *space);
 void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
 			       struct btrfs_ioctl_balance_args *bargs);
-int btrfs_dedupe_file_range(struct file *src_file, loff_t src_loff,
-			    struct file *dst_file, loff_t dst_loff,
-			    u64 olen);
 
 /* file.c */
 int __init btrfs_auto_defrag_init(void);
@@ -3250,8 +3247,9 @@ int btrfs_dirty_pages(struct inode *inode, struct page **pages,
 		      size_t num_pages, loff_t pos, size_t write_bytes,
 		      struct extent_state **cached);
 int btrfs_fdatawrite_range(struct inode *inode, loff_t start, loff_t end);
-int btrfs_clone_file_range(struct file *file_in, loff_t pos_in,
-			   struct file *file_out, loff_t pos_out, u64 len);
+int btrfs_remap_file_range(struct file *file_in, loff_t pos_in,
+			   struct file *file_out, loff_t pos_out, u64 len,
+			   unsigned int flags);
 
 /* tree-defrag.c */
 int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 2be00e873e92..9a963f061393 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3269,8 +3269,7 @@ const struct file_operations btrfs_file_operations = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= btrfs_compat_ioctl,
 #endif
-	.clone_file_range = btrfs_clone_file_range,
-	.dedupe_file_range = btrfs_dedupe_file_range,
+	.remap_file_range = btrfs_remap_file_range,
 };
 
 void __cold btrfs_auto_defrag_exit(void)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d60b6caf09e8..e22b294fa25b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3627,26 +3627,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
 	return ret;
 }
 
-int btrfs_dedupe_file_range(struct file *src_file, loff_t src_loff,
-			    struct file *dst_file, loff_t dst_loff,
-			    u64 olen)
-{
-	struct inode *src = file_inode(src_file);
-	struct inode *dst = file_inode(dst_file);
-	u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
-
-	if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
-		/*
-		 * Btrfs does not support blocksize < page_size. As a
-		 * result, btrfs_cmp_data() won't correctly handle
-		 * this situation without an update.
-		 */
-		return -EINVAL;
-	}
-
-	return btrfs_extent_same(src, src_loff, olen, dst, dst_loff);
-}
-
 static int clone_finish_inode_update(struct btrfs_trans_handle *trans,
 				     struct inode *inode,
 				     u64 endoff,
@@ -4348,9 +4328,27 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
 	return ret;
 }
 
-int btrfs_clone_file_range(struct file *src_file, loff_t off,
-		struct file *dst_file, loff_t destoff, u64 len)
+int btrfs_remap_file_range(struct file *src_file, loff_t off,
+		struct file *dst_file, loff_t destoff, u64 len,
+		unsigned int flags)
 {
+	if (flags & RFR_IDENTICAL_DATA) {
+		struct inode *src = file_inode(src_file);
+		struct inode *dst = file_inode(dst_file);
+		u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
+
+		if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
+			/*
+			 * Btrfs does not support blocksize < page_size. As a
+			 * result, btrfs_cmp_data() won't correctly handle
+			 * this situation without an update.
+			 */
+			return -EINVAL;
+		}
+
+		return btrfs_extent_same(src, off, len, dst, destoff);
+	}
+
 	return btrfs_clone_files(dst_file, src_file, off, len, destoff);
 }
 
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 7065426b3280..bf971fd7cab2 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -975,8 +975,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.listxattr = cifs_listxattr,
 };
 
-static int cifs_clone_file_range(struct file *src_file, loff_t off,
-		struct file *dst_file, loff_t destoff, u64 len)
+static int cifs_remap_file_range(struct file *src_file, loff_t off,
+		struct file *dst_file, loff_t destoff, u64 len,
+		unsigned int flags)
 {
 	struct inode *src_inode = file_inode(src_file);
 	struct inode *target_inode = file_inode(dst_file);
@@ -986,6 +987,9 @@ static int cifs_clone_file_range(struct file *src_file, loff_t off,
 	unsigned int xid;
 	int rc;
 
+	if (flags & RFR_IDENTICAL_DATA)
+		return -EOPNOTSUPP;
+
 	cifs_dbg(FYI, "clone range\n");
 
 	xid = get_xid();
@@ -1134,7 +1138,7 @@ const struct file_operations cifs_file_ops = {
 	.llseek = cifs_llseek,
 	.unlocked_ioctl	= cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
 };
@@ -1153,7 +1157,7 @@ const struct file_operations cifs_file_strict_ops = {
 	.llseek = cifs_llseek,
 	.unlocked_ioctl	= cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
 };
@@ -1172,7 +1176,7 @@ const struct file_operations cifs_file_direct_ops = {
 	.splice_write = iter_file_splice_write,
 	.unlocked_ioctl  = cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.llseek = cifs_llseek,
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
@@ -1191,7 +1195,7 @@ const struct file_operations cifs_file_nobrl_ops = {
 	.llseek = cifs_llseek,
 	.unlocked_ioctl	= cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
 };
@@ -1209,7 +1213,7 @@ const struct file_operations cifs_file_strict_nobrl_ops = {
 	.llseek = cifs_llseek,
 	.unlocked_ioctl	= cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
 };
@@ -1227,7 +1231,7 @@ const struct file_operations cifs_file_direct_nobrl_ops = {
 	.splice_write = iter_file_splice_write,
 	.unlocked_ioctl  = cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.llseek = cifs_llseek,
 	.setlease = cifs_setlease,
 	.fallocate = cifs_fallocate,
@@ -1239,7 +1243,7 @@ const struct file_operations cifs_dir_ops = {
 	.read    = generic_read_dir,
 	.unlocked_ioctl  = cifs_ioctl,
 	.copy_file_range = cifs_copy_file_range,
-	.clone_file_range = cifs_clone_file_range,
+	.remap_file_range = cifs_remap_file_range,
 	.llseek = generic_file_llseek,
 	.fsync = cifs_dir_fsync,
 };
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 4288a6ecaf75..a93aa225c454 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -180,8 +180,9 @@ static long nfs42_fallocate(struct file *filep, int mode, loff_t offset, loff_t
 	return nfs42_proc_allocate(filep, offset, len);
 }
 
-static int nfs42_clone_file_range(struct file *src_file, loff_t src_off,
-		struct file *dst_file, loff_t dst_off, u64 count)
+static int nfs42_remap_file_range(struct file *src_file, loff_t src_off,
+		struct file *dst_file, loff_t dst_off, u64 count,
+		unsigned int flags)
 {
 	struct inode *dst_inode = file_inode(dst_file);
 	struct nfs_server *server = NFS_SERVER(dst_inode);
@@ -190,6 +191,9 @@ static int nfs42_clone_file_range(struct file *src_file, loff_t src_off,
 	bool same_inode = false;
 	int ret;
 
+	if (flags & RFR_IDENTICAL_DATA)
+		return -EOPNOTSUPP;
+
 	/* check alignment w.r.t. clone_blksize */
 	ret = -EINVAL;
 	if (bs) {
@@ -262,7 +266,7 @@ const struct file_operations nfs4_file_operations = {
 	.copy_file_range = nfs4_copy_file_range,
 	.llseek		= nfs4_file_llseek,
 	.fallocate	= nfs42_fallocate,
-	.clone_file_range = nfs42_clone_file_range,
+	.remap_file_range = nfs42_remap_file_range,
 #else
 	.llseek		= nfs_file_llseek,
 #endif
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 9fa35cb6f6e0..5da278edf189 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2527,24 +2527,15 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)
 	return offset;
 }
 
-static int ocfs2_file_clone_range(struct file *file_in,
+static int ocfs2_remap_file_range(struct file *file_in,
 				  loff_t pos_in,
 				  struct file *file_out,
 				  loff_t pos_out,
-				  u64 len)
+				  u64 len,
+				  unsigned int flags)
 {
 	return ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-					 len, false);
-}
-
-static int ocfs2_file_dedupe_range(struct file *file_in,
-				   loff_t pos_in,
-				   struct file *file_out,
-				   loff_t pos_out,
-				   u64 len)
-{
-	return ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-					  len, true);
+					 len, flags & RFR_IDENTICAL_DATA);
 }
 
 const struct inode_operations ocfs2_file_iops = {
@@ -2586,8 +2577,7 @@ const struct file_operations ocfs2_fops = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ocfs2_fallocate,
-	.clone_file_range = ocfs2_file_clone_range,
-	.dedupe_file_range = ocfs2_file_dedupe_range,
+	.remap_file_range = ocfs2_remap_file_range,
 };
 
 const struct file_operations ocfs2_dops = {
@@ -2633,8 +2623,7 @@ const struct file_operations ocfs2_fops_no_plocks = {
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ocfs2_fallocate,
-	.clone_file_range = ocfs2_file_clone_range,
-	.dedupe_file_range = ocfs2_file_dedupe_range,
+	.remap_file_range = ocfs2_remap_file_range,
 };
 
 const struct file_operations ocfs2_dops_no_plocks = {
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 986313da0c88..693bd0620a81 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -489,26 +489,28 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
 			    OVL_COPY);
 }
 
-static int ovl_clone_file_range(struct file *file_in, loff_t pos_in,
-				struct file *file_out, loff_t pos_out, u64 len)
+static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
+				struct file *file_out, loff_t pos_out,
+				u64 len, unsigned int flags)
 {
-	return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
-			    OVL_CLONE);
-}
+	enum ovl_copyop op;
+
+	if (flags & RFR_IDENTICAL_DATA)
+		op = OVL_DEDUPE;
+	else
+		op = OVL_CLONE;
 
-static int ovl_dedupe_file_range(struct file *file_in, loff_t pos_in,
-				 struct file *file_out, loff_t pos_out, u64 len)
-{
 	/*
 	 * Don't copy up because of a dedupe request, this wouldn't make sense
 	 * most of the time (data would be duplicated instead of deduplicated).
 	 */
-	if (!ovl_inode_upper(file_inode(file_in)) ||
-	    !ovl_inode_upper(file_inode(file_out)))
+	if (op == OVL_DEDUPE &&
+	    (!ovl_inode_upper(file_inode(file_in)) ||
+	     !ovl_inode_upper(file_inode(file_out))))
 		return -EPERM;
 
 	return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
-			    OVL_DEDUPE);
+			    op);
 }
 
 const struct file_operations ovl_file_operations = {
@@ -525,6 +527,5 @@ const struct file_operations ovl_file_operations = {
 	.compat_ioctl	= ovl_compat_ioctl,
 
 	.copy_file_range	= ovl_copy_file_range,
-	.clone_file_range	= ovl_clone_file_range,
-	.dedupe_file_range	= ovl_dedupe_file_range,
+	.remap_file_range	= ovl_remap_file_range,
 };
diff --git a/fs/read_write.c b/fs/read_write.c
index 3ff90b3315fb..a33c8503f12e 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1588,9 +1588,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	 * Try cloning first, this is supported by more file systems, and
 	 * more efficient if both clone and copy are supported (e.g. NFS).
 	 */
-	if (file_in->f_op->clone_file_range) {
-		ret = file_in->f_op->clone_file_range(file_in, pos_in,
-				file_out, pos_out, len);
+	if (file_in->f_op->remap_file_range) {
+		ret = file_in->f_op->remap_file_range(file_in, pos_in,
+				file_out, pos_out, len, 0);
 		if (ret == 0) {
 			ret = len;
 			goto done;
@@ -1815,7 +1815,7 @@ int do_clone_file_range(struct file *file_in, loff_t pos_in,
 	    (file_out->f_flags & O_APPEND))
 		return -EBADF;
 
-	if (!file_in->f_op->clone_file_range)
+	if (!file_in->f_op->remap_file_range)
 		return -EOPNOTSUPP;
 
 	ret = clone_verify_area(file_in, pos_in, len, false);
@@ -1826,8 +1826,8 @@ int do_clone_file_range(struct file *file_in, loff_t pos_in,
 	if (ret)
 		return ret;
 
-	ret = file_in->f_op->clone_file_range(file_in, pos_in,
-			file_out, pos_out, len);
+	ret = file_in->f_op->remap_file_range(file_in, pos_in,
+			file_out, pos_out, len, 0);
 	if (!ret) {
 		fsnotify_access(file_in);
 		fsnotify_modify(file_out);
@@ -1972,7 +1972,7 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 		goto out_drop_write;
 
 	ret = -EINVAL;
-	if (!dst_file->f_op->dedupe_file_range)
+	if (!dst_file->f_op->remap_file_range)
 		goto out_drop_write;
 
 	if (len == 0) {
@@ -1980,8 +1980,8 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 		goto out_drop_write;
 	}
 
-	ret = dst_file->f_op->dedupe_file_range(src_file, src_pos,
-						dst_file, dst_pos, len);
+	ret = dst_file->f_op->remap_file_range(src_file, src_pos, dst_file,
+			dst_pos, len, RFR_IDENTICAL_DATA);
 out_drop_write:
 	mnt_drop_write_file(dst_file);
 
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 61a5ad2600e8..74ad73231ea4 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -920,27 +920,16 @@ xfs_file_fallocate(
 }
 
 STATIC int
-xfs_file_clone_range(
+xfs_file_remap_range(
 	struct file	*file_in,
 	loff_t		pos_in,
 	struct file	*file_out,
 	loff_t		pos_out,
-	u64		len)
+	u64		len,
+	unsigned int	flags)
 {
 	return xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-				     len, false);
-}
-
-STATIC int
-xfs_file_dedupe_range(
-	struct file	*file_in,
-	loff_t		pos_in,
-	struct file	*file_out,
-	loff_t		pos_out,
-	u64		len)
-{
-	return xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-				     len, true);
+			len, flags & RFR_IDENTICAL_DATA);
 }
 
 STATIC int
@@ -1175,8 +1164,7 @@ const struct file_operations xfs_file_operations = {
 	.fsync		= xfs_file_fsync,
 	.get_unmapped_area = thp_get_unmapped_area,
 	.fallocate	= xfs_file_fallocate,
-	.clone_file_range = xfs_file_clone_range,
-	.dedupe_file_range = xfs_file_dedupe_range,
+	.remap_file_range = xfs_file_remap_range,
 };
 
 const struct file_operations xfs_dir_file_operations = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ba93a6e7dac4..6fedfe4fb5ef 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1721,6 +1721,12 @@ struct block_device_operations;
 #define NOMMU_VMFLAGS \
 	(NOMMU_MAP_READ | NOMMU_MAP_WRITE | NOMMU_MAP_EXEC)
 
+/*
+ * These flags control the behavior of the remap_file_range function pointer.
+ *
+ * RFR_IDENTICAL_DATA: only remap if contents identical (i.e. deduplicate)
+ */
+#define RFR_IDENTICAL_DATA	(1 << 0)
 
 struct iov_iter;
 
@@ -1759,10 +1765,9 @@ struct file_operations {
 #endif
 	ssize_t (*copy_file_range)(struct file *, loff_t, struct file *,
 			loff_t, size_t, unsigned int);
-	int (*clone_file_range)(struct file *, loff_t, struct file *, loff_t,
-			u64);
-	int (*dedupe_file_range)(struct file *, loff_t, struct file *, loff_t,
-			u64);
+	int (*remap_file_range)(struct file *file_in, loff_t pos_in,
+				struct file *file_out, loff_t pos_out,
+				u64 len, unsigned int flags);
 	int (*fadvise)(struct file *, loff_t, loff_t, int);
 } __randomize_layout;
 

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 09/25] vfs: rename vfs_clone_file_prep to be more descriptive
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (7 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10  0:11 ` [PATCH 10/25] vfs: rename clone_verify_area to remap_verify_area Darrick J. Wong
                   ` (16 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

The vfs_clone_file_prep is a generic function to be called by filesystem
implementations only.  Rename the prefix to generic_ and make it more
clear that it applies to remap operations, not just clones.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ocfs2/refcounttree.c |    2 +-
 fs/read_write.c         |    8 ++++----
 fs/xfs/xfs_reflink.c    |    2 +-
 include/linux/fs.h      |    6 +++---
 4 files changed, 9 insertions(+), 9 deletions(-)


diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 19e03936c5e1..36c56dfbe485 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4850,7 +4850,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 	    (OCFS2_I(inode_out)->ip_flags & OCFS2_INODE_SYSTEM_FILE))
 		goto out_unlock;
 
-	ret = vfs_clone_file_prep(file_in, pos_in, file_out, pos_out,
+	ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
 			&len, is_dedupe);
 	if (ret <= 0)
 		goto out_unlock;
diff --git a/fs/read_write.c b/fs/read_write.c
index a33c8503f12e..4ea81ea7d78d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1717,9 +1717,9 @@ static int clone_verify_area(struct file *file, loff_t pos, u64 len, bool write)
  * Returns: 0 for "nothing to clone", 1 for "something to clone", or
  * the usual negative error code.
  */
-int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
-			struct file *file_out, loff_t pos_out,
-			u64 *len, bool is_dedupe)
+int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
+				  struct file *file_out, loff_t pos_out,
+				  u64 *len, bool is_dedupe)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
@@ -1788,7 +1788,7 @@ int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
 
 	return 1;
 }
-EXPORT_SYMBOL(vfs_clone_file_prep);
+EXPORT_SYMBOL(generic_remap_file_range_prep);
 
 int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			struct file *file_out, loff_t pos_out, u64 len)
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index a4a7b2d9c8a1..4cf1e52efbff 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1303,7 +1303,7 @@ xfs_reflink_remap_prep(
 	if (IS_DAX(inode_in) || IS_DAX(inode_out))
 		goto out_unlock;
 
-	ret = vfs_clone_file_prep(file_in, pos_in, file_out, pos_out,
+	ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
 			len, is_dedupe);
 	if (ret <= 0)
 		goto out_unlock;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6fedfe4fb5ef..d8f90bdd34e2 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1830,9 +1830,9 @@ extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *, rwf_t);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
-extern int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
-			       struct file *file_out, loff_t pos_out,
-			       u64 *count, bool is_dedupe);
+extern int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
+					 struct file *file_out, loff_t pos_out,
+					 u64 *count, bool is_dedupe);
 extern int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			       struct file *file_out, loff_t pos_out, u64 len);
 extern int vfs_clone_file_range(struct file *file_in, loff_t pos_in,

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 10/25] vfs: rename clone_verify_area to remap_verify_area
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (8 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 09/25] vfs: rename vfs_clone_file_prep to be more descriptive Darrick J. Wong
@ 2018-10-10  0:11 ` Darrick J. Wong
  2018-10-10  0:13 ` [PATCH 11/25] vfs: create generic_remap_file_range_touch to update inode metadata Darrick J. Wong
                   ` (15 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:11 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Since we use clone_verify_area for both clone and dedupe range checks,
rename the function to make it clear that it's for both.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/read_write.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


diff --git a/fs/read_write.c b/fs/read_write.c
index 4ea81ea7d78d..b4acfb45d916 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1686,7 +1686,7 @@ SYSCALL_DEFINE6(copy_file_range, int, fd_in, loff_t __user *, off_in,
 	return ret;
 }
 
-static int clone_verify_area(struct file *file, loff_t pos, u64 len, bool write)
+static int remap_verify_area(struct file *file, loff_t pos, u64 len, bool write)
 {
 	struct inode *inode = file_inode(file);
 
@@ -1818,11 +1818,11 @@ int do_clone_file_range(struct file *file_in, loff_t pos_in,
 	if (!file_in->f_op->remap_file_range)
 		return -EOPNOTSUPP;
 
-	ret = clone_verify_area(file_in, pos_in, len, false);
+	ret = remap_verify_area(file_in, pos_in, len, false);
 	if (ret)
 		return ret;
 
-	ret = clone_verify_area(file_out, pos_out, len, true);
+	ret = remap_verify_area(file_out, pos_out, len, true);
 	if (ret)
 		return ret;
 
@@ -1955,7 +1955,7 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 	if (ret)
 		return ret;
 
-	ret = clone_verify_area(dst_file, dst_pos, len, true);
+	ret = remap_verify_area(dst_file, dst_pos, len, true);
 	if (ret < 0)
 		goto out_drop_write;
 
@@ -2017,7 +2017,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	if (!S_ISREG(src->i_mode))
 		goto out;
 
-	ret = clone_verify_area(file, off, len, false);
+	ret = remap_verify_area(file, off, len, false);
 	if (ret < 0)
 		goto out;
 	ret = 0;

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 11/25] vfs: create generic_remap_file_range_touch to update inode metadata
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (9 preceding siblings ...)
  2018-10-10  0:11 ` [PATCH 10/25] vfs: rename clone_verify_area to remap_verify_area Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10  0:13 ` [PATCH 12/25] vfs: pass remap flags to generic_remap_file_range_prep Darrick J. Wong
                   ` (14 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a new VFS helper to handle inode metadata updates when remapping
into a file.  If the operation can possibly alter the file contents, we
must update the ctime and mtime and remove security privileges, just
like we do for regular file writes.  Wire up ocfs2 to ensure consistent
behavior.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ocfs2/refcounttree.c |    8 ++++++++
 fs/read_write.c         |   24 ++++++++++++++++++++++++
 fs/xfs/xfs_reflink.c    |   29 +++++++----------------------
 include/linux/fs.h      |    1 +
 4 files changed, 40 insertions(+), 22 deletions(-)


diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 36c56dfbe485..ee1ed11379b3 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4855,6 +4855,14 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 	if (ret <= 0)
 		goto out_unlock;
 
+	/*
+	 * Update inode timestamps and remove security privileges before we
+	 * take the ilock.
+	 */
+	ret = generic_remap_file_range_touch(file_out, is_dedupe);
+	if (ret)
+		goto out_unlock;
+
 	/* Lock out changes to the allocation maps and remap. */
 	down_write(&OCFS2_I(inode_in)->ip_alloc_sem);
 	if (!same_inode)
diff --git a/fs/read_write.c b/fs/read_write.c
index b4acfb45d916..020bb7fdf431 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1790,6 +1790,30 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 }
 EXPORT_SYMBOL(generic_remap_file_range_prep);
 
+/* Update inode timestamps and remove security privileges when remapping. */
+int generic_remap_file_range_touch(struct file *file, bool is_dedupe)
+{
+	int ret;
+
+	/* If can't alter the file contents, we're done. */
+	if (is_dedupe)
+		return 0;
+
+	/* Update the timestamps, since we can alter file contents. */
+	if (!(file->f_mode & FMODE_NOCMTIME)) {
+		ret = file_update_time(file);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Clear the security bits if the process is not being run by root.
+	 * This keeps people from modifying setuid and setgid binaries.
+	 */
+	return file_remove_privs(file);
+}
+EXPORT_SYMBOL(generic_remap_file_range_touch);
+
 int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			struct file *file_out, loff_t pos_out, u64 len)
 {
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 4cf1e52efbff..0d67b2d0b3d4 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1330,28 +1330,13 @@ xfs_reflink_remap_prep(
 	truncate_inode_pages_range(&inode_out->i_data, pos_out,
 				   PAGE_ALIGN(pos_out + *len) - 1);
 
-	/* If we're altering the file contents... */
-	if (!is_dedupe) {
-		/*
-		 * ...update the timestamps (which will grab the ilock again
-		 * from xfs_fs_dirty_inode, so we have to call it before we
-		 * take the ilock).
-		 */
-		if (!(file_out->f_mode & FMODE_NOCMTIME)) {
-			ret = file_update_time(file_out);
-			if (ret)
-				goto out_unlock;
-		}
-
-		/*
-		 * ...clear the security bits if the process is not being run
-		 * by root.  This keeps people from modifying setuid and setgid
-		 * binaries.
-		 */
-		ret = file_remove_privs(file_out);
-		if (ret)
-			goto out_unlock;
-	}
+	/*
+	 * Update inode timestamps and remove security privileges before we
+	 * take the ilock.
+	 */
+	ret = generic_remap_file_range_touch(file_out, is_dedupe);
+	if (ret)
+		goto out_unlock;
 
 	return 1;
 out_unlock:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d8f90bdd34e2..661b9ef32d2b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1833,6 +1833,7 @@ extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 extern int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 					 struct file *file_out, loff_t pos_out,
 					 u64 *count, bool is_dedupe);
+extern int generic_remap_file_range_touch(struct file *file, bool is_dedupe);
 extern int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			       struct file *file_out, loff_t pos_out, u64 len);
 extern int vfs_clone_file_range(struct file *file_in, loff_t pos_in,

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 12/25] vfs: pass remap flags to generic_remap_file_range_prep
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (10 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 11/25] vfs: create generic_remap_file_range_touch to update inode metadata Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10  0:13 ` [PATCH 13/25] vfs: pass remap flags to generic_remap_checks Darrick J. Wong
                   ` (13 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Plumb the remap flags through the filesystem from the vfs function
dispatcher all the way to the prep function to prepare for behavior
changes in subsequent patches.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ocfs2/file.c         |    2 +-
 fs/ocfs2/refcounttree.c |    6 +++---
 fs/ocfs2/refcounttree.h |    2 +-
 fs/read_write.c         |   10 ++++++----
 fs/xfs/xfs_file.c       |    2 +-
 fs/xfs/xfs_reflink.c    |   15 ++++++++-------
 fs/xfs/xfs_reflink.h    |    3 ++-
 include/linux/fs.h      |    5 +++--
 8 files changed, 25 insertions(+), 20 deletions(-)


diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 5da278edf189..ca41e69c8e68 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2535,7 +2535,7 @@ static int ocfs2_remap_file_range(struct file *file_in,
 				  unsigned int flags)
 {
 	return ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-					 len, flags & RFR_IDENTICAL_DATA);
+					 len, flags);
 }
 
 const struct inode_operations ocfs2_file_iops = {
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index ee1ed11379b3..270a5b1919f6 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4825,7 +4825,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 			      struct file *file_out,
 			      loff_t pos_out,
 			      u64 len,
-			      bool is_dedupe)
+			      unsigned int remap_flags)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
@@ -4851,7 +4851,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 		goto out_unlock;
 
 	ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
-			&len, is_dedupe);
+			&len, remap_flags);
 	if (ret <= 0)
 		goto out_unlock;
 
@@ -4859,7 +4859,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 	 * Update inode timestamps and remove security privileges before we
 	 * take the ilock.
 	 */
-	ret = generic_remap_file_range_touch(file_out, is_dedupe);
+	ret = generic_remap_file_range_touch(file_out, remap_flags);
 	if (ret)
 		goto out_unlock;
 
diff --git a/fs/ocfs2/refcounttree.h b/fs/ocfs2/refcounttree.h
index 4af55bf4b35b..d2c5f526edff 100644
--- a/fs/ocfs2/refcounttree.h
+++ b/fs/ocfs2/refcounttree.h
@@ -120,6 +120,6 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 			      struct file *file_out,
 			      loff_t pos_out,
 			      u64 len,
-			      bool is_dedupe);
+			      unsigned int remap_flags);
 
 #endif /* OCFS2_REFCOUNTTREE_H */
diff --git a/fs/read_write.c b/fs/read_write.c
index 020bb7fdf431..47174785a94f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1712,18 +1712,20 @@ static int remap_verify_area(struct file *file, loff_t pos, u64 len, bool write)
 /*
  * Check that the two inodes are eligible for cloning, the ranges make
  * sense, and then flush all dirty data.  Caller must ensure that the
- * inodes have been locked against any other modifications.
+ * inodes have been locked against any other modifications.  This function
+ * takes RFR_* flags in remap_flags.
  *
  * Returns: 0 for "nothing to clone", 1 for "something to clone", or
  * the usual negative error code.
  */
 int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 				  struct file *file_out, loff_t pos_out,
-				  u64 *len, bool is_dedupe)
+				  u64 *len, unsigned int remap_flags)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
 	loff_t isize;
+	bool is_dedupe = (remap_flags & RFR_IDENTICAL_DATA);
 	bool same_inode = (inode_in == inode_out);
 	int ret;
 
@@ -1791,12 +1793,12 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 EXPORT_SYMBOL(generic_remap_file_range_prep);
 
 /* Update inode timestamps and remove security privileges when remapping. */
-int generic_remap_file_range_touch(struct file *file, bool is_dedupe)
+int generic_remap_file_range_touch(struct file *file, unsigned int remap_flags)
 {
 	int ret;
 
 	/* If can't alter the file contents, we're done. */
-	if (is_dedupe)
+	if (remap_flags & RFR_IDENTICAL_DATA)
 		return 0;
 
 	/* Update the timestamps, since we can alter file contents. */
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 74ad73231ea4..4c0ae1efb63f 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -929,7 +929,7 @@ xfs_file_remap_range(
 	unsigned int	flags)
 {
 	return xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-			len, flags & RFR_IDENTICAL_DATA);
+			len, flags);
 }
 
 STATIC int
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0d67b2d0b3d4..4f5e3923d9c2 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -921,10 +921,11 @@ xfs_reflink_update_dest(
 	struct xfs_inode	*dest,
 	xfs_off_t		newlen,
 	xfs_extlen_t		cowextsize,
-	bool			is_dedupe)
+	unsigned int		remap_flags)
 {
 	struct xfs_mount	*mp = dest->i_mount;
 	struct xfs_trans	*tp;
+	bool			is_dedupe = (remap_flags & RFR_IDENTICAL_DATA);
 	int			error;
 
 	if (is_dedupe && newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0)
@@ -1274,7 +1275,7 @@ xfs_reflink_remap_prep(
 	struct file		*file_out,
 	loff_t			pos_out,
 	u64			*len,
-	bool			is_dedupe)
+	unsigned int		remap_flags)
 {
 	struct inode		*inode_in = file_inode(file_in);
 	struct xfs_inode	*src = XFS_I(inode_in);
@@ -1304,7 +1305,7 @@ xfs_reflink_remap_prep(
 		goto out_unlock;
 
 	ret = generic_remap_file_range_prep(file_in, pos_in, file_out, pos_out,
-			len, is_dedupe);
+			len, remap_flags);
 	if (ret <= 0)
 		goto out_unlock;
 
@@ -1334,7 +1335,7 @@ xfs_reflink_remap_prep(
 	 * Update inode timestamps and remove security privileges before we
 	 * take the ilock.
 	 */
-	ret = generic_remap_file_range_touch(file_out, is_dedupe);
+	ret = generic_remap_file_range_touch(file_out, remap_flags);
 	if (ret)
 		goto out_unlock;
 
@@ -1354,7 +1355,7 @@ xfs_reflink_remap_range(
 	struct file		*file_out,
 	loff_t			pos_out,
 	u64			len,
-	bool			is_dedupe)
+	unsigned int		remap_flags)
 {
 	struct inode		*inode_in = file_inode(file_in);
 	struct xfs_inode	*src = XFS_I(inode_in);
@@ -1374,7 +1375,7 @@ xfs_reflink_remap_range(
 
 	/* Prepare and then clone file data. */
 	ret = xfs_reflink_remap_prep(file_in, pos_in, file_out, pos_out,
-			&len, is_dedupe);
+			&len, remap_flags);
 	if (ret <= 0)
 		return ret;
 
@@ -1401,7 +1402,7 @@ xfs_reflink_remap_range(
 		cowextsize = src->i_d.di_cowextsize;
 
 	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize,
-			is_dedupe);
+			remap_flags);
 
 out_unlock:
 	xfs_reflink_remap_unlock(file_in, file_out);
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index c585ad9552b2..6f82d628bf17 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -28,7 +28,8 @@ extern int xfs_reflink_end_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);
 extern int xfs_reflink_recover_cow(struct xfs_mount *mp);
 extern int xfs_reflink_remap_range(struct file *file_in, loff_t pos_in,
-		struct file *file_out, loff_t pos_out, u64 len, bool is_dedupe);
+		struct file *file_out, loff_t pos_out, u64 len,
+		unsigned int remap_flags);
 extern int xfs_reflink_inode_has_shared_extents(struct xfs_trans *tp,
 		struct xfs_inode *ip, bool *has_shared);
 extern int xfs_reflink_clear_inode_flag(struct xfs_inode *ip,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 661b9ef32d2b..0d9bdef0b4ea 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1832,8 +1832,9 @@ extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
 extern int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 					 struct file *file_out, loff_t pos_out,
-					 u64 *count, bool is_dedupe);
-extern int generic_remap_file_range_touch(struct file *file, bool is_dedupe);
+					 u64 *count, unsigned int remap_flags);
+extern int generic_remap_file_range_touch(struct file *file,
+					  unsigned int remap_flags);
 extern int do_clone_file_range(struct file *file_in, loff_t pos_in,
 			       struct file *file_out, loff_t pos_out, u64 len);
 extern int vfs_clone_file_range(struct file *file_in, loff_t pos_in,

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 13/25] vfs: pass remap flags to generic_remap_checks
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (11 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 12/25] vfs: pass remap flags to generic_remap_file_range_prep Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10  0:13 ` [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed Darrick J. Wong
                   ` (12 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Pass the same remap flags to generic_remap_checks for consistency.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/read_write.c    |    2 +-
 include/linux/fs.h |    2 +-
 mm/filemap.c       |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)


diff --git a/fs/read_write.c b/fs/read_write.c
index 47174785a94f..917934770b08 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1755,7 +1755,7 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 
 	/* Check that we don't violate system file offset limits. */
 	ret = generic_remap_checks(file_in, pos_in, file_out, pos_out, len,
-			is_dedupe);
+			remap_flags);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0d9bdef0b4ea..56167b10cbad 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2976,7 +2976,7 @@ extern int generic_file_readonly_mmap(struct file *, struct vm_area_struct *);
 extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
 extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
-				uint64_t *count, bool is_dedupe);
+				uint64_t *count, unsigned int remap_flags);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);
diff --git a/mm/filemap.c b/mm/filemap.c
index 59056bd9c58a..e099c8880863 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3004,7 +3004,7 @@ generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
  */
 int generic_remap_checks(struct file *file_in, loff_t pos_in,
 			 struct file *file_out, loff_t pos_out,
-			 uint64_t *req_count, bool is_dedupe)
+			 uint64_t *req_count, unsigned int remap_flags)
 {
 	struct inode *inode_in = file_in->f_mapping->host;
 	struct inode *inode_out = file_out->f_mapping->host;
@@ -3027,7 +3027,7 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	size_out = i_size_read(inode_out);
 
 	/* Dedupe requires both ranges to be within EOF. */
-	if (is_dedupe &&
+	if ((remap_flags & RFR_IDENTICAL_DATA) &&
 	    (pos_in >= size_in || pos_in + count > size_in ||
 	     pos_out >= size_out || pos_out + count > size_out))
 		return -EINVAL;

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (12 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 13/25] vfs: pass remap flags to generic_remap_checks Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10  6:47   ` Amir Goldstein
  2018-10-10  0:13 ` [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions Darrick J. Wong
                   ` (11 subsequent siblings)
  25 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Change the remap_file_range functions to take a number of bytes to
operate upon and return the number of bytes they operated on.  This is a
requirement for allowing fs implementations to return short clone/dedupe
results to the user, which will enable us to obey resource limits in a
graceful manner.

A subsequent patch will enable copy_file_range to signal to the
->clone_file_range implementation that it can handle a short length,
which will be returned in the function's return value.  Neither clone
ioctl can take advantage of this, alas.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/btrfs/ctree.h        |    6 +++---
 fs/btrfs/ioctl.c        |   13 ++++++++----
 fs/cifs/cifsfs.c        |    6 +++---
 fs/ioctl.c              |   10 +++++++++-
 fs/nfs/nfs4file.c       |    6 +++---
 fs/nfsd/vfs.c           |    8 ++++++--
 fs/ocfs2/file.c         |   16 ++++++++-------
 fs/ocfs2/refcounttree.c |    2 +-
 fs/ocfs2/refcounttree.h |    2 +-
 fs/overlayfs/copy_up.c  |    5 +++--
 fs/overlayfs/file.c     |   12 ++++++------
 fs/read_write.c         |   49 ++++++++++++++++++++++++++---------------------
 fs/xfs/xfs_file.c       |   11 +++++++----
 fs/xfs/xfs_reflink.c    |    4 ++--
 fs/xfs/xfs_reflink.h    |    2 +-
 include/linux/fs.h      |   27 ++++++++++++++------------
 mm/filemap.c            |    8 ++++----
 17 files changed, 107 insertions(+), 80 deletions(-)


diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a79783a713f0..d09af171d185 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3247,9 +3247,9 @@ int btrfs_dirty_pages(struct inode *inode, struct page **pages,
 		      size_t num_pages, loff_t pos, size_t write_bytes,
 		      struct extent_state **cached);
 int btrfs_fdatawrite_range(struct inode *inode, loff_t start, loff_t end);
-int btrfs_remap_file_range(struct file *file_in, loff_t pos_in,
-			   struct file *file_out, loff_t pos_out, u64 len,
-			   unsigned int flags);
+loff_t btrfs_remap_file_range(struct file *file_in, loff_t pos_in,
+			      struct file *file_out, loff_t pos_out,
+			      loff_t len, unsigned int flags);
 
 /* tree-defrag.c */
 int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e22b294fa25b..3488bc90e5eb 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4328,10 +4328,12 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
 	return ret;
 }
 
-int btrfs_remap_file_range(struct file *src_file, loff_t off,
-		struct file *dst_file, loff_t destoff, u64 len,
+loff_t btrfs_remap_file_range(struct file *src_file, loff_t off,
+		struct file *dst_file, loff_t destoff, loff_t len,
 		unsigned int flags)
 {
+	int ret;
+
 	if (flags & RFR_IDENTICAL_DATA) {
 		struct inode *src = file_inode(src_file);
 		struct inode *dst = file_inode(dst_file);
@@ -4346,10 +4348,11 @@ int btrfs_remap_file_range(struct file *src_file, loff_t off,
 			return -EINVAL;
 		}
 
-		return btrfs_extent_same(src, off, len, dst, destoff);
+		ret = btrfs_extent_same(src, off, len, dst, destoff);
+	} else {
+		ret = btrfs_clone_files(dst_file, src_file, off, len, destoff);
 	}
-
-	return btrfs_clone_files(dst_file, src_file, off, len, destoff);
+	return ret < 0 ? ret : len;
 }
 
 static long btrfs_ioctl_default_subvol(struct file *file, void __user *argp)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index bf971fd7cab2..c6ea368ea920 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -975,8 +975,8 @@ const struct inode_operations cifs_symlink_inode_ops = {
 	.listxattr = cifs_listxattr,
 };
 
-static int cifs_remap_file_range(struct file *src_file, loff_t off,
-		struct file *dst_file, loff_t destoff, u64 len,
+static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
+		struct file *dst_file, loff_t destoff, loff_t len,
 		unsigned int flags)
 {
 	struct inode *src_inode = file_inode(src_file);
@@ -1029,7 +1029,7 @@ static int cifs_remap_file_range(struct file *src_file, loff_t off,
 	unlock_two_nondirectories(src_inode, target_inode);
 out:
 	free_xid(xid);
-	return rc;
+	return rc < 0 ? rc : len;
 }
 
 ssize_t cifs_file_copychunk_range(unsigned int xid,
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2005529af560..72537b68c272 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -223,6 +223,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 			     u64 off, u64 olen, u64 destoff)
 {
 	struct fd src_file = fdget(srcfd);
+	loff_t cloned;
 	int ret;
 
 	if (!src_file.file)
@@ -230,7 +231,14 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 	ret = -EXDEV;
 	if (src_file.file->f_path.mnt != dst_file->f_path.mnt)
 		goto fdput;
-	ret = vfs_clone_file_range(src_file.file, off, dst_file, destoff, olen);
+	cloned = vfs_clone_file_range(src_file.file, off, dst_file, destoff,
+				      olen);
+	if (cloned < 0)
+		ret = cloned;
+	else if (olen && cloned != olen)
+		ret = -EINVAL;
+	else
+		ret = 0;
 fdput:
 	fdput(src_file);
 	return ret;
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index a93aa225c454..0c56d1acac18 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -180,8 +180,8 @@ static long nfs42_fallocate(struct file *filep, int mode, loff_t offset, loff_t
 	return nfs42_proc_allocate(filep, offset, len);
 }
 
-static int nfs42_remap_file_range(struct file *src_file, loff_t src_off,
-		struct file *dst_file, loff_t dst_off, u64 count,
+static loff_t nfs42_remap_file_range(struct file *src_file, loff_t src_off,
+		struct file *dst_file, loff_t dst_off, loff_t count,
 		unsigned int flags)
 {
 	struct inode *dst_inode = file_inode(dst_file);
@@ -244,7 +244,7 @@ static int nfs42_remap_file_range(struct file *src_file, loff_t src_off,
 		inode_unlock(src_inode);
 	}
 out:
-	return ret;
+	return ret < 0 ? ret : count;
 }
 #endif /* CONFIG_NFS_V4_2 */
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index b53e76391e52..ac6cb6101cbe 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -541,8 +541,12 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
 __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
 		u64 dst_pos, u64 count)
 {
-	return nfserrno(vfs_clone_file_range(src, src_pos, dst, dst_pos,
-					     count));
+	loff_t cloned;
+
+	cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count);
+	if (count && cloned != count)
+		cloned = -EINVAL;
+	return nfserrno(cloned < 0 ? cloned : 0);
 }
 
 ssize_t nfsd_copy_file_range(struct file *src, u64 src_pos, struct file *dst,
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index ca41e69c8e68..c018605ac279 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2527,15 +2527,15 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)
 	return offset;
 }
 
-static int ocfs2_remap_file_range(struct file *file_in,
-				  loff_t pos_in,
-				  struct file *file_out,
-				  loff_t pos_out,
-				  u64 len,
-				  unsigned int flags)
+static loff_t ocfs2_remap_file_range(struct file *file_in, loff_t pos_in,
+				     struct file *file_out, loff_t pos_out,
+				     loff_t len, unsigned int flags)
 {
-	return ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-					 len, flags);
+	int ret;
+
+	ret = ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
+					len, flags);
+	return ret < 0 ? ret : len;
 }
 
 const struct inode_operations ocfs2_file_iops = {
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 270a5b1919f6..a3df118bf3b9 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4824,7 +4824,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 			      loff_t pos_in,
 			      struct file *file_out,
 			      loff_t pos_out,
-			      u64 len,
+			      loff_t len,
 			      unsigned int remap_flags)
 {
 	struct inode *inode_in = file_inode(file_in);
diff --git a/fs/ocfs2/refcounttree.h b/fs/ocfs2/refcounttree.h
index d2c5f526edff..eb65c1d0843c 100644
--- a/fs/ocfs2/refcounttree.h
+++ b/fs/ocfs2/refcounttree.h
@@ -119,7 +119,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 			      loff_t pos_in,
 			      struct file *file_out,
 			      loff_t pos_out,
-			      u64 len,
+			      loff_t len,
 			      unsigned int remap_flags);
 
 #endif /* OCFS2_REFCOUNTTREE_H */
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 1cc797a08a5b..c2b53509e7f1 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -125,6 +125,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	struct file *new_file;
 	loff_t old_pos = 0;
 	loff_t new_pos = 0;
+	loff_t cloned;
 	int error = 0;
 
 	if (len == 0)
@@ -141,8 +142,8 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	}
 
 	/* Try to use clone_file_range to clone up within the same fs */
-	error = do_clone_file_range(old_file, 0, new_file, 0, len);
-	if (!error)
+	cloned = do_clone_file_range(old_file, 0, new_file, 0, len);
+	if (cloned == len)
 		goto out;
 	/* Couldn't clone, so now we try to copy the data */
 	error = 0;
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 693bd0620a81..c8c890c22898 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -434,14 +434,14 @@ enum ovl_copyop {
 	OVL_DEDUPE,
 };
 
-static ssize_t ovl_copyfile(struct file *file_in, loff_t pos_in,
+static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 			    struct file *file_out, loff_t pos_out,
-			    u64 len, unsigned int flags, enum ovl_copyop op)
+			    loff_t len, unsigned int flags, enum ovl_copyop op)
 {
 	struct inode *inode_out = file_inode(file_out);
 	struct fd real_in, real_out;
 	const struct cred *old_cred;
-	ssize_t ret;
+	loff_t ret;
 
 	ret = ovl_real_fdget(file_out, &real_out);
 	if (ret)
@@ -489,9 +489,9 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
 			    OVL_COPY);
 }
 
-static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
-				struct file *file_out, loff_t pos_out,
-				u64 len, unsigned int flags)
+static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
+				   struct file *file_out, loff_t pos_out,
+				   loff_t len, unsigned int flags)
 {
 	enum ovl_copyop op;
 
diff --git a/fs/read_write.c b/fs/read_write.c
index 917934770b08..f43b0620afd4 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1589,10 +1589,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	 * more efficient if both clone and copy are supported (e.g. NFS).
 	 */
 	if (file_in->f_op->remap_file_range) {
-		ret = file_in->f_op->remap_file_range(file_in, pos_in,
-				file_out, pos_out, len, 0);
-		if (ret == 0) {
-			ret = len;
+		s64 cloned;
+
+		cloned = file_in->f_op->remap_file_range(file_in, pos_in,
+				file_out, pos_out,
+				min_t(loff_t, MAX_RW_COUNT, len), 0);
+		if (cloned >= 0) {
+			ret = cloned;
 			goto done;
 		}
 	}
@@ -1686,11 +1689,12 @@ SYSCALL_DEFINE6(copy_file_range, int, fd_in, loff_t __user *, off_in,
 	return ret;
 }
 
-static int remap_verify_area(struct file *file, loff_t pos, u64 len, bool write)
+static int remap_verify_area(struct file *file, loff_t pos, loff_t len,
+			     bool write)
 {
 	struct inode *inode = file_inode(file);
 
-	if (unlikely(pos < 0))
+	if (unlikely(pos < 0 || len < 0))
 		return -EINVAL;
 
 	 if (unlikely((loff_t) (pos + len) < 0))
@@ -1720,7 +1724,7 @@ static int remap_verify_area(struct file *file, loff_t pos, u64 len, bool write)
  */
 int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 				  struct file *file_out, loff_t pos_out,
-				  u64 *len, unsigned int remap_flags)
+				  loff_t *len, unsigned int remap_flags)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
@@ -1816,12 +1820,12 @@ int generic_remap_file_range_touch(struct file *file, unsigned int remap_flags)
 }
 EXPORT_SYMBOL(generic_remap_file_range_touch);
 
-int do_clone_file_range(struct file *file_in, loff_t pos_in,
-			struct file *file_out, loff_t pos_out, u64 len)
+loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
+			   struct file *file_out, loff_t pos_out, loff_t len)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
-	int ret;
+	loff_t ret;
 
 	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
 		return -EISDIR;
@@ -1854,19 +1858,19 @@ int do_clone_file_range(struct file *file_in, loff_t pos_in,
 
 	ret = file_in->f_op->remap_file_range(file_in, pos_in,
 			file_out, pos_out, len, 0);
-	if (!ret) {
-		fsnotify_access(file_in);
-		fsnotify_modify(file_out);
-	}
+	if (ret < 0)
+		return ret;
 
+	fsnotify_access(file_in);
+	fsnotify_modify(file_out);
 	return ret;
 }
 EXPORT_SYMBOL(do_clone_file_range);
 
-int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
-			 struct file *file_out, loff_t pos_out, u64 len)
+loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
+			    struct file *file_out, loff_t pos_out, loff_t len)
 {
-	int ret;
+	loff_t ret;
 
 	file_start_write(file_out);
 	ret = do_clone_file_range(file_in, pos_in, file_out, pos_out, len);
@@ -1972,10 +1976,11 @@ int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 }
 EXPORT_SYMBOL(vfs_dedupe_file_range_compare);
 
-int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
-			      struct file *dst_file, loff_t dst_pos, u64 len)
+loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
+				 struct file *dst_file, loff_t dst_pos,
+				 loff_t len)
 {
-	s64 ret;
+	loff_t ret;
 
 	ret = mnt_want_write_file(dst_file);
 	if (ret)
@@ -2024,7 +2029,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	int i;
 	int ret;
 	u16 count = same->dest_count;
-	int deduped;
+	loff_t deduped;
 
 	if (!(file->f_mode & FMODE_READ))
 		return -EINVAL;
@@ -2081,7 +2086,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 		else if (deduped < 0)
 			info->status = deduped;
 		else
-			info->bytes_deduped = len;
+			info->bytes_deduped = deduped;
 
 next_fdput:
 		fdput(dst_fd);
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 4c0ae1efb63f..6f4205846451 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -919,17 +919,20 @@ xfs_file_fallocate(
 	return error;
 }
 
-STATIC int
+STATIC loff_t
 xfs_file_remap_range(
 	struct file	*file_in,
 	loff_t		pos_in,
 	struct file	*file_out,
 	loff_t		pos_out,
-	u64		len,
+	loff_t		len,
 	unsigned int	flags)
 {
-	return xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-			len, flags);
+	int		ret;
+
+	ret = xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
+			len, false);
+	return ret < 0 ? ret : len;
 }
 
 STATIC int
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 4f5e3923d9c2..ec09e2783afe 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1274,7 +1274,7 @@ xfs_reflink_remap_prep(
 	loff_t			pos_in,
 	struct file		*file_out,
 	loff_t			pos_out,
-	u64			*len,
+	loff_t			*len,
 	unsigned int		remap_flags)
 {
 	struct inode		*inode_in = file_inode(file_in);
@@ -1354,7 +1354,7 @@ xfs_reflink_remap_range(
 	loff_t			pos_in,
 	struct file		*file_out,
 	loff_t			pos_out,
-	u64			len,
+	loff_t			len,
 	unsigned int		remap_flags)
 {
 	struct inode		*inode_in = file_inode(file_in);
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index 6f82d628bf17..c3c46c276fe1 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -28,7 +28,7 @@ extern int xfs_reflink_end_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);
 extern int xfs_reflink_recover_cow(struct xfs_mount *mp);
 extern int xfs_reflink_remap_range(struct file *file_in, loff_t pos_in,
-		struct file *file_out, loff_t pos_out, u64 len,
+		struct file *file_out, loff_t pos_out, loff_t len,
 		unsigned int remap_flags);
 extern int xfs_reflink_inode_has_shared_extents(struct xfs_trans *tp,
 		struct xfs_inode *ip, bool *has_shared);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 56167b10cbad..1ee09152ecd5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1765,9 +1765,9 @@ struct file_operations {
 #endif
 	ssize_t (*copy_file_range)(struct file *, loff_t, struct file *,
 			loff_t, size_t, unsigned int);
-	int (*remap_file_range)(struct file *file_in, loff_t pos_in,
-				struct file *file_out, loff_t pos_out,
-				u64 len, unsigned int flags);
+	loff_t (*remap_file_range)(struct file *file_in, loff_t pos_in,
+				   struct file *file_out, loff_t pos_out,
+				   loff_t len, unsigned int flags);
 	int (*fadvise)(struct file *, loff_t, loff_t, int);
 } __randomize_layout;
 
@@ -1832,21 +1832,24 @@ extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
 extern int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 					 struct file *file_out, loff_t pos_out,
-					 u64 *count, unsigned int remap_flags);
+					 loff_t *count,
+					 unsigned int remap_flags);
 extern int generic_remap_file_range_touch(struct file *file,
 					  unsigned int remap_flags);
-extern int do_clone_file_range(struct file *file_in, loff_t pos_in,
-			       struct file *file_out, loff_t pos_out, u64 len);
-extern int vfs_clone_file_range(struct file *file_in, loff_t pos_in,
-				struct file *file_out, loff_t pos_out, u64 len);
+extern loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
+				  struct file *file_out, loff_t pos_out,
+				  loff_t len);
+extern loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
+				   struct file *file_out, loff_t pos_out,
+				   loff_t len);
 extern int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 					 struct inode *dest, loff_t destoff,
 					 loff_t len, bool *is_same);
 extern int vfs_dedupe_file_range(struct file *file,
 				 struct file_dedupe_range *same);
-extern int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
-				     struct file *dst_file, loff_t dst_pos,
-				     u64 len);
+extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
+					struct file *dst_file, loff_t dst_pos,
+					loff_t len);
 
 
 struct super_operations {
@@ -2976,7 +2979,7 @@ extern int generic_file_readonly_mmap(struct file *, struct vm_area_struct *);
 extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
 extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
-				uint64_t *count, unsigned int remap_flags);
+				loff_t *count, unsigned int remap_flags);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);
diff --git a/mm/filemap.c b/mm/filemap.c
index e099c8880863..2522737483de 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2975,7 +2975,7 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
 EXPORT_SYMBOL(generic_write_checks);
 
 static int
-generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
+generic_remap_check_limits(struct file *file, loff_t pos, loff_t *count)
 {
 	struct inode *inode = file->f_mapping->host;
 
@@ -2984,14 +2984,14 @@ generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
 				!(file->f_flags & O_LARGEFILE))) {
 		if (pos >= MAX_NON_LFS)
 			return -EFBIG;
-		*count = min(*count, MAX_NON_LFS - (uint64_t)pos);
+		*count = min(*count, (loff_t)MAX_NON_LFS - pos);
 	}
 
 	/* Don't operate on ranges the page cache doesn't support. */
 	if (unlikely(pos >= inode->i_sb->s_maxbytes))
 		return -EFBIG;
 
-	*count = min(*count, inode->i_sb->s_maxbytes - (uint64_t)pos);
+	*count = min(*count, inode->i_sb->s_maxbytes - pos);
 	return 0;
 }
 
@@ -3004,7 +3004,7 @@ generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
  */
 int generic_remap_checks(struct file *file_in, loff_t pos_in,
 			 struct file *file_out, loff_t pos_out,
-			 uint64_t *req_count, unsigned int remap_flags)
+			 loff_t *req_count, unsigned int remap_flags)
 {
 	struct inode *inode_in = file_in->f_mapping->host;
 	struct inode *inode_out = file_out->f_mapping->host;

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (13 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10  6:22   ` Amir Goldstein
  2018-10-10  0:13 ` [PATCH 16/25] vfs: plumb RFR_* remap flags through the vfs dedupe functions Darrick J. Wong
                   ` (10 subsequent siblings)
  25 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Plumb a remap_flags argument through the {do,vfs}_clone_file_range
functions so that clone can take advantage of it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ioctl.c             |    2 +-
 fs/nfsd/vfs.c          |    2 +-
 fs/overlayfs/copy_up.c |    2 +-
 fs/overlayfs/file.c    |    4 ++--
 fs/read_write.c        |   13 +++++++++----
 include/linux/fs.h     |    4 ++--
 6 files changed, 16 insertions(+), 11 deletions(-)


diff --git a/fs/ioctl.c b/fs/ioctl.c
index 72537b68c272..505275ec5596 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -232,7 +232,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 	if (src_file.file->f_path.mnt != dst_file->f_path.mnt)
 		goto fdput;
 	cloned = vfs_clone_file_range(src_file.file, off, dst_file, destoff,
-				      olen);
+				      olen, 0);
 	if (cloned < 0)
 		ret = cloned;
 	else if (olen && cloned != olen)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index ac6cb6101cbe..726fc5b2b27a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -543,7 +543,7 @@ __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
 {
 	loff_t cloned;
 
-	cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count);
+	cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, 0);
 	if (count && cloned != count)
 		cloned = -EINVAL;
 	return nfserrno(cloned < 0 ? cloned : 0);
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index c2b53509e7f1..bf60a3386c11 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -142,7 +142,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	}
 
 	/* Try to use clone_file_range to clone up within the same fs */
-	cloned = do_clone_file_range(old_file, 0, new_file, 0, len);
+	cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0);
 	if (cloned == len)
 		goto out;
 	/* Couldn't clone, so now we try to copy the data */
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index c8c890c22898..8b22035af4d7 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -462,7 +462,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 
 	case OVL_CLONE:
 		ret = vfs_clone_file_range(real_in.file, pos_in,
-					   real_out.file, pos_out, len);
+					   real_out.file, pos_out, len, flags);
 		break;
 
 	case OVL_DEDUPE:
@@ -509,7 +509,7 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
 	     !ovl_inode_upper(file_inode(file_out))))
 		return -EPERM;
 
-	return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
+	return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
 			    op);
 }
 
diff --git a/fs/read_write.c b/fs/read_write.c
index f43b0620afd4..cf81d1f68b69 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1821,12 +1821,15 @@ int generic_remap_file_range_touch(struct file *file, unsigned int remap_flags)
 EXPORT_SYMBOL(generic_remap_file_range_touch);
 
 loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
-			   struct file *file_out, loff_t pos_out, loff_t len)
+			   struct file *file_out, loff_t pos_out,
+			   loff_t len, unsigned int remap_flags)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
 	loff_t ret;
 
+	WARN_ON_ONCE(remap_flags);
+
 	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
 		return -EISDIR;
 	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
@@ -1857,7 +1860,7 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 		return ret;
 
 	ret = file_in->f_op->remap_file_range(file_in, pos_in,
-			file_out, pos_out, len, 0);
+			file_out, pos_out, len, remap_flags);
 	if (ret < 0)
 		return ret;
 
@@ -1868,12 +1871,14 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 EXPORT_SYMBOL(do_clone_file_range);
 
 loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
-			    struct file *file_out, loff_t pos_out, loff_t len)
+			    struct file *file_out, loff_t pos_out,
+			    loff_t len, unsigned int remap_flags)
 {
 	loff_t ret;
 
 	file_start_write(file_out);
-	ret = do_clone_file_range(file_in, pos_in, file_out, pos_out, len);
+	ret = do_clone_file_range(file_in, pos_in, file_out, pos_out, len,
+				  remap_flags);
 	file_end_write(file_out);
 
 	return ret;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1ee09152ecd5..21f947b20f37 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1838,10 +1838,10 @@ extern int generic_remap_file_range_touch(struct file *file,
 					  unsigned int remap_flags);
 extern loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 				  struct file *file_out, loff_t pos_out,
-				  loff_t len);
+				  loff_t len, unsigned int remap_flags);
 extern loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
 				   struct file *file_out, loff_t pos_out,
-				   loff_t len);
+				   loff_t len, unsigned int remap_flags);
 extern int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 					 struct inode *dest, loff_t destoff,
 					 loff_t len, bool *is_same);

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 16/25] vfs: plumb RFR_* remap flags through the vfs dedupe functions
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (14 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10  0:13 ` [PATCH 17/25] vfs: make remapping to source file eof more explicit Darrick J. Wong
                   ` (9 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Plumb a remap_flags argument through the vfs_dedupe_file_range_one
functions so that dedupe can take advantage of it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/overlayfs/file.c |    3 ++-
 fs/read_write.c     |    9 ++++++---
 include/linux/fs.h  |    2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)


diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 8b22035af4d7..8b804e86ed4d 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -467,7 +467,8 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 
 	case OVL_DEDUPE:
 		ret = vfs_dedupe_file_range_one(real_in.file, pos_in,
-						real_out.file, pos_out, len);
+						real_out.file, pos_out, len,
+						flags);
 		break;
 	}
 	revert_creds(old_cred);
diff --git a/fs/read_write.c b/fs/read_write.c
index cf81d1f68b69..479eb810c8e6 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1983,10 +1983,12 @@ EXPORT_SYMBOL(vfs_dedupe_file_range_compare);
 
 loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 				 struct file *dst_file, loff_t dst_pos,
-				 loff_t len)
+				 loff_t len, unsigned int remap_flags)
 {
 	loff_t ret;
 
+	WARN_ON_ONCE(remap_flags & ~(RFR_IDENTICAL_DATA));
+
 	ret = mnt_want_write_file(dst_file);
 	if (ret)
 		return ret;
@@ -2017,7 +2019,7 @@ loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 	}
 
 	ret = dst_file->f_op->remap_file_range(src_file, src_pos, dst_file,
-			dst_pos, len, RFR_IDENTICAL_DATA);
+			dst_pos, len, remap_flags | RFR_IDENTICAL_DATA);
 out_drop_write:
 	mnt_drop_write_file(dst_file);
 
@@ -2085,7 +2087,8 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 		}
 
 		deduped = vfs_dedupe_file_range_one(file, off, dst_file,
-						    info->dest_offset, len);
+						    info->dest_offset, len,
+						    0);
 		if (deduped == -EBADE)
 			info->status = FILE_DEDUPE_RANGE_DIFFERS;
 		else if (deduped < 0)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21f947b20f37..5c1bf1c35bc6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1849,7 +1849,7 @@ extern int vfs_dedupe_file_range(struct file *file,
 				 struct file_dedupe_range *same);
 extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 					struct file *dst_file, loff_t dst_pos,
-					loff_t len);
+					loff_t len, unsigned int remap_flags);
 
 
 struct super_operations {

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 17/25] vfs: make remapping to source file eof more explicit
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (15 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 16/25] vfs: plumb RFR_* remap flags through the vfs dedupe functions Darrick J. Wong
@ 2018-10-10  0:13 ` Darrick J. Wong
  2018-10-10 12:29   ` Amir Goldstein
  2018-10-10  0:14 ` [PATCH 18/25] vfs: enable remap callers that can handle short operations Darrick J. Wong
                   ` (8 subsequent siblings)
  25 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:13 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a RFR_TO_SRC_EOF flag to explicitly declare that the caller wants
the remap implementation to remap to the end of the source file, once
the files are locked.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ioctl.c         |    3 ++-
 fs/nfsd/vfs.c      |    3 ++-
 fs/read_write.c    |   13 +++++++------
 include/linux/fs.h |    2 ++
 4 files changed, 13 insertions(+), 8 deletions(-)


diff --git a/fs/ioctl.c b/fs/ioctl.c
index 505275ec5596..7fec997abd2f 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -224,6 +224,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 {
 	struct fd src_file = fdget(srcfd);
 	loff_t cloned;
+	unsigned int flags = olen == 0 ? RFR_TO_SRC_EOF : 0;
 	int ret;
 
 	if (!src_file.file)
@@ -232,7 +233,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
 	if (src_file.file->f_path.mnt != dst_file->f_path.mnt)
 		goto fdput;
 	cloned = vfs_clone_file_range(src_file.file, off, dst_file, destoff,
-				      olen, 0);
+				      olen, flags);
 	if (cloned < 0)
 		ret = cloned;
 	else if (olen && cloned != olen)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 726fc5b2b27a..d1f2ae08adf6 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -542,8 +542,9 @@ __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
 		u64 dst_pos, u64 count)
 {
 	loff_t cloned;
+	unsigned int flags = count == 0 ? RFR_TO_SRC_EOF : 0;
 
-	cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, 0);
+	cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, flags);
 	if (count && cloned != count)
 		cloned = -EINVAL;
 	return nfserrno(cloned < 0 ? cloned : 0);
diff --git a/fs/read_write.c b/fs/read_write.c
index 479eb810c8e6..a628fd9a47cf 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1748,11 +1748,12 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 
 	isize = i_size_read(inode_in);
 
-	/* Zero length dedupe exits immediately; reflink goes to EOF. */
-	if (*len == 0) {
-		if (is_dedupe || pos_in == isize)
-			return 0;
-		if (pos_in > isize)
+	/*
+	 * If the caller asked to go all the way to the end of the source file,
+	 * set *len now that we have the file locked.
+	 */
+	if (remap_flags & RFR_TO_SRC_EOF) {
+		if (pos_in >= isize)
 			return -EINVAL;
 		*len = isize - pos_in;
 	}
@@ -1828,7 +1829,7 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 	struct inode *inode_out = file_inode(file_out);
 	loff_t ret;
 
-	WARN_ON_ONCE(remap_flags);
+	WARN_ON_ONCE(remap_flags & ~(RFR_TO_SRC_EOF));
 
 	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
 		return -EISDIR;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5c1bf1c35bc6..9f90dcd4df3b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1725,8 +1725,10 @@ struct block_device_operations;
  * These flags control the behavior of the remap_file_range function pointer.
  *
  * RFR_IDENTICAL_DATA: only remap if contents identical (i.e. deduplicate)
+ * RFR_TO_SRC_EOF: remap to the end of the source file
  */
 #define RFR_IDENTICAL_DATA	(1 << 0)
+#define RFR_TO_SRC_EOF		(1 << 1)
 
 struct iov_iter;
 

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 18/25] vfs: enable remap callers that can handle short operations
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (16 preceding siblings ...)
  2018-10-10  0:13 ` [PATCH 17/25] vfs: make remapping to source file eof more explicit Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 19/25] vfs: hide file range comparison function Darrick J. Wong
                   ` (7 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Plumb in a remap flag that enables the filesystem remap handler to
shorten remapping requests for callers that can handle it.  Now
copy_file_range can report partial success (in case we run up against
alignment problems, resource limits, etc.).

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/read_write.c    |    3 ++-
 include/linux/fs.h |    2 ++
 mm/filemap.c       |   16 ++++++++++++----
 3 files changed, 16 insertions(+), 5 deletions(-)


diff --git a/fs/read_write.c b/fs/read_write.c
index a628fd9a47cf..8ed0aed81649 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1593,7 +1593,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 
 		cloned = file_in->f_op->remap_file_range(file_in, pos_in,
 				file_out, pos_out,
-				min_t(loff_t, MAX_RW_COUNT, len), 0);
+				min_t(loff_t, MAX_RW_COUNT, len),
+				RFR_CAN_SHORTEN);
 		if (cloned >= 0) {
 			ret = cloned;
 			goto done;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9f90dcd4df3b..e0494d719ebc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1726,9 +1726,11 @@ struct block_device_operations;
  *
  * RFR_IDENTICAL_DATA: only remap if contents identical (i.e. deduplicate)
  * RFR_TO_SRC_EOF: remap to the end of the source file
+ * RFR_CAN_SHORTEN: caller can handle a shortened request
  */
 #define RFR_IDENTICAL_DATA	(1 << 0)
 #define RFR_TO_SRC_EOF		(1 << 1)
+#define RFR_CAN_SHORTEN		(1 << 2)
 
 struct iov_iter;
 
diff --git a/mm/filemap.c b/mm/filemap.c
index 2522737483de..2179d0204ee6 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3064,8 +3064,12 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	if (pos_in + count == size_in) {
 		bcount = ALIGN(size_in, bs) - pos_in;
 	} else {
-		if (!IS_ALIGNED(count, bs))
-			return -EINVAL;
+		if (!IS_ALIGNED(count, bs)) {
+			if (remap_flags & RFR_CAN_SHORTEN)
+				count = ALIGN_DOWN(count, bs);
+			else
+				return -EINVAL;
+		}
 
 		bcount = count;
 	}
@@ -3076,10 +3080,14 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	    pos_out < pos_in + bcount)
 		return -EINVAL;
 
-	/* For now we don't support changing the length. */
-	if (*req_count != count)
+	/*
+	 * We shortened the request but the caller can't deal with that, so
+	 * bounce the request back to userspace.
+	 */
+	if (*req_count != count && !(remap_flags & RFR_CAN_SHORTEN))
 		return -EINVAL;
 
+	*req_count = count;
 	return 0;
 }
 

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 19/25] vfs: hide file range comparison function
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (17 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 18/25] vfs: enable remap callers that can handle short operations Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 20/25] vfs: implement opportunistic short dedupe Darrick J. Wong
                   ` (6 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

There are no callers of vfs_dedupe_file_range_compare, so we might as
well make it a static helper and remove the export.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/read_write.c    |  191 ++++++++++++++++++++++++++--------------------------
 include/linux/fs.h |    3 -
 2 files changed, 95 insertions(+), 99 deletions(-)


diff --git a/fs/read_write.c b/fs/read_write.c
index 8ed0aed81649..57627202bd50 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1714,6 +1714,101 @@ static int remap_verify_area(struct file *file, loff_t pos, loff_t len,
 	return security_file_permission(file, write ? MAY_WRITE : MAY_READ);
 }
 
+/*
+ * Read a page's worth of file data into the page cache.  Return the page
+ * locked.
+ */
+static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)
+{
+	struct address_space *mapping;
+	struct page *page;
+	pgoff_t n;
+
+	n = offset >> PAGE_SHIFT;
+	mapping = inode->i_mapping;
+	page = read_mapping_page(mapping, n, NULL);
+	if (IS_ERR(page))
+		return page;
+	if (!PageUptodate(page)) {
+		put_page(page);
+		return ERR_PTR(-EIO);
+	}
+	lock_page(page);
+	return page;
+}
+
+/*
+ * Compare extents of two files to see if they are the same.
+ * Caller must have locked both inodes to prevent write races.
+ */
+static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
+					 struct inode *dest, loff_t destoff,
+					 loff_t len, bool *is_same)
+{
+	loff_t src_poff;
+	loff_t dest_poff;
+	void *src_addr;
+	void *dest_addr;
+	struct page *src_page;
+	struct page *dest_page;
+	loff_t cmp_len;
+	bool same;
+	int error;
+
+	error = -EINVAL;
+	same = true;
+	while (len) {
+		src_poff = srcoff & (PAGE_SIZE - 1);
+		dest_poff = destoff & (PAGE_SIZE - 1);
+		cmp_len = min(PAGE_SIZE - src_poff,
+			      PAGE_SIZE - dest_poff);
+		cmp_len = min(cmp_len, len);
+		if (cmp_len <= 0)
+			goto out_error;
+
+		src_page = vfs_dedupe_get_page(src, srcoff);
+		if (IS_ERR(src_page)) {
+			error = PTR_ERR(src_page);
+			goto out_error;
+		}
+		dest_page = vfs_dedupe_get_page(dest, destoff);
+		if (IS_ERR(dest_page)) {
+			error = PTR_ERR(dest_page);
+			unlock_page(src_page);
+			put_page(src_page);
+			goto out_error;
+		}
+		src_addr = kmap_atomic(src_page);
+		dest_addr = kmap_atomic(dest_page);
+
+		flush_dcache_page(src_page);
+		flush_dcache_page(dest_page);
+
+		if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len))
+			same = false;
+
+		kunmap_atomic(dest_addr);
+		kunmap_atomic(src_addr);
+		unlock_page(dest_page);
+		unlock_page(src_page);
+		put_page(dest_page);
+		put_page(src_page);
+
+		if (!same)
+			break;
+
+		srcoff += cmp_len;
+		destoff += cmp_len;
+		len -= cmp_len;
+	}
+
+	*is_same = same;
+	return 0;
+
+out_error:
+	return error;
+}
+
 /*
  * Check that the two inodes are eligible for cloning, the ranges make
  * sense, and then flush all dirty data.  Caller must ensure that the
@@ -1887,102 +1982,6 @@ loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
 }
 EXPORT_SYMBOL(vfs_clone_file_range);
 
-/*
- * Read a page's worth of file data into the page cache.  Return the page
- * locked.
- */
-static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)
-{
-	struct address_space *mapping;
-	struct page *page;
-	pgoff_t n;
-
-	n = offset >> PAGE_SHIFT;
-	mapping = inode->i_mapping;
-	page = read_mapping_page(mapping, n, NULL);
-	if (IS_ERR(page))
-		return page;
-	if (!PageUptodate(page)) {
-		put_page(page);
-		return ERR_PTR(-EIO);
-	}
-	lock_page(page);
-	return page;
-}
-
-/*
- * Compare extents of two files to see if they are the same.
- * Caller must have locked both inodes to prevent write races.
- */
-int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
-				  struct inode *dest, loff_t destoff,
-				  loff_t len, bool *is_same)
-{
-	loff_t src_poff;
-	loff_t dest_poff;
-	void *src_addr;
-	void *dest_addr;
-	struct page *src_page;
-	struct page *dest_page;
-	loff_t cmp_len;
-	bool same;
-	int error;
-
-	error = -EINVAL;
-	same = true;
-	while (len) {
-		src_poff = srcoff & (PAGE_SIZE - 1);
-		dest_poff = destoff & (PAGE_SIZE - 1);
-		cmp_len = min(PAGE_SIZE - src_poff,
-			      PAGE_SIZE - dest_poff);
-		cmp_len = min(cmp_len, len);
-		if (cmp_len <= 0)
-			goto out_error;
-
-		src_page = vfs_dedupe_get_page(src, srcoff);
-		if (IS_ERR(src_page)) {
-			error = PTR_ERR(src_page);
-			goto out_error;
-		}
-		dest_page = vfs_dedupe_get_page(dest, destoff);
-		if (IS_ERR(dest_page)) {
-			error = PTR_ERR(dest_page);
-			unlock_page(src_page);
-			put_page(src_page);
-			goto out_error;
-		}
-		src_addr = kmap_atomic(src_page);
-		dest_addr = kmap_atomic(dest_page);
-
-		flush_dcache_page(src_page);
-		flush_dcache_page(dest_page);
-
-		if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len))
-			same = false;
-
-		kunmap_atomic(dest_addr);
-		kunmap_atomic(src_addr);
-		unlock_page(dest_page);
-		unlock_page(src_page);
-		put_page(dest_page);
-		put_page(src_page);
-
-		if (!same)
-			break;
-
-		srcoff += cmp_len;
-		destoff += cmp_len;
-		len -= cmp_len;
-	}
-
-	*is_same = same;
-	return 0;
-
-out_error:
-	return error;
-}
-EXPORT_SYMBOL(vfs_dedupe_file_range_compare);
-
 loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 				 struct file *dst_file, loff_t dst_pos,
 				 loff_t len, unsigned int remap_flags)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0494d719ebc..9c53276979bb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1846,9 +1846,6 @@ extern loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 extern loff_t vfs_clone_file_range(struct file *file_in, loff_t pos_in,
 				   struct file *file_out, loff_t pos_out,
 				   loff_t len, unsigned int remap_flags);
-extern int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
-					 struct inode *dest, loff_t destoff,
-					 loff_t len, bool *is_same);
 extern int vfs_dedupe_file_range(struct file *file,
 				 struct file_dedupe_range *same);
 extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 20/25] vfs: implement opportunistic short dedupe
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (18 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 19/25] vfs: hide file range comparison function Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 21/25] ocfs2: truncate page cache for clone destination file before remapping Darrick J. Wong
                   ` (5 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

For a given dedupe request, the bytes_deduped field in the control
structure tells userspace if we managed to deduplicate some, but not all
of, the requested regions starting from the file offsets supplied.
However, due to sloppy coding, the current dedupe code returns
FILE_DEDUPE_RANGE_DIFFERS if any part of the range is different.
Fix this so that we can actually support partial request completion.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/read_write.c |   59 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 11 deletions(-)


diff --git a/fs/read_write.c b/fs/read_write.c
index 57627202bd50..8be3c3add030 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1737,13 +1737,26 @@ static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)
 	return page;
 }
 
+static unsigned int vfs_dedupe_memcmp(const char *s1, const char *s2,
+				      unsigned int len)
+{
+	const char *orig_s1;
+
+	for (orig_s1 = s1; len > 0; s1++, s2++, len--)
+		if (*s1 != *s2)
+			break;
+
+	return s1 - orig_s1;
+}
+
 /*
  * Compare extents of two files to see if they are the same.
  * Caller must have locked both inodes to prevent write races.
  */
 static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 					 struct inode *dest, loff_t destoff,
-					 loff_t len, bool *is_same)
+					 loff_t *req_len,
+					 unsigned int remap_flags)
 {
 	loff_t src_poff;
 	loff_t dest_poff;
@@ -1751,8 +1764,11 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 	void *dest_addr;
 	struct page *src_page;
 	struct page *dest_page;
-	loff_t cmp_len;
+	loff_t len = *req_len;
+	loff_t same_len = 0;
 	bool same;
+	unsigned int cmp_len;
+	unsigned int cmp_same;
 	int error;
 
 	error = -EINVAL;
@@ -1762,7 +1778,7 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 		dest_poff = destoff & (PAGE_SIZE - 1);
 		cmp_len = min(PAGE_SIZE - src_poff,
 			      PAGE_SIZE - dest_poff);
-		cmp_len = min(cmp_len, len);
+		cmp_len = min_t(loff_t, cmp_len, len);
 		if (cmp_len <= 0)
 			goto out_error;
 
@@ -1784,7 +1800,10 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 		flush_dcache_page(src_page);
 		flush_dcache_page(dest_page);
 
-		if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len))
+		cmp_same = vfs_dedupe_memcmp(src_addr + src_poff,
+					     dest_addr + dest_poff, cmp_len);
+		same_len += cmp_same;
+		if (cmp_same != cmp_len)
 			same = false;
 
 		kunmap_atomic(dest_addr);
@@ -1802,7 +1821,17 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
 		len -= cmp_len;
 	}
 
-	*is_same = same;
+	/*
+	 * If less than the whole range matched, we have to back down to the
+	 * nearest block boundary.
+	 */
+	if (*req_len != same_len) {
+		if (!(remap_flags & RFR_CAN_SHORTEN))
+			return -EINVAL;
+
+		*req_len = ALIGN_DOWN(same_len, dest->i_sb->s_blocksize);
+	}
+
 	return 0;
 
 out_error:
@@ -1879,13 +1908,11 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 	 * Check that the extents are the same.
 	 */
 	if (is_dedupe) {
-		bool		is_same = false;
-
 		ret = vfs_dedupe_file_range_compare(inode_in, pos_in,
-				inode_out, pos_out, *len, &is_same);
+				inode_out, pos_out, len, remap_flags);
 		if (ret)
 			return ret;
-		if (!is_same)
+		if (*len == 0)
 			return -EBADE;
 	}
 
@@ -1988,7 +2015,7 @@ loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
 {
 	loff_t ret;
 
-	WARN_ON_ONCE(remap_flags & ~(RFR_IDENTICAL_DATA));
+	WARN_ON_ONCE(remap_flags & ~(RFR_IDENTICAL_DATA | RFR_CAN_SHORTEN));
 
 	ret = mnt_want_write_file(dst_file);
 	if (ret)
@@ -2037,6 +2064,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	int i;
 	int ret;
 	u16 count = same->dest_count;
+	unsigned int remap_flags = 0;
 	loff_t deduped;
 
 	if (!(file->f_mode & FMODE_READ))
@@ -2073,6 +2101,15 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 		same->info[i].status = FILE_DEDUPE_RANGE_SAME;
 	}
 
+	/*
+	 * We can't allow the dedupe implementation to shorten the request if
+	 * there are multiple dedupe candidates because each candidate might
+	 * shorten the request by a different amount due to EOF and allocation
+	 * block size mismatches.
+	 */
+	if (count == 1)
+		remap_flags |= RFR_CAN_SHORTEN;
+
 	for (i = 0, info = same->info; i < count; i++, info++) {
 		struct fd dst_fd = fdget(info->dest_fd);
 		struct file *dst_file = dst_fd.file;
@@ -2089,7 +2126,7 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 
 		deduped = vfs_dedupe_file_range_one(file, off, dst_file,
 						    info->dest_offset, len,
-						    0);
+						    remap_flags);
 		if (deduped == -EBADE)
 			info->status = FILE_DEDUPE_RANGE_DIFFERS;
 		else if (deduped < 0)

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 21/25] ocfs2: truncate page cache for clone destination file before remapping
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (19 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 20/25] vfs: implement opportunistic short dedupe Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 22/25] ocfs2: fix pagecache truncation prior to reflink Darrick J. Wong
                   ` (4 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

When cloning blocks into another file, truncate the page cache before we
start remapping blocks so that concurrent reads wait for us to finish.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ocfs2/refcounttree.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)


diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index a3df118bf3b9..851ba3ae7ce8 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4869,14 +4869,12 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 		down_write_nested(&OCFS2_I(inode_out)->ip_alloc_sem,
 				  SINGLE_DEPTH_NESTING);
 
-	ret = ocfs2_reflink_remap_blocks(inode_in, in_bh, pos_in, inode_out,
-					 out_bh, pos_out, len);
-
 	/* Zap any page cache for the destination file's range. */
-	if (!ret)
-		truncate_inode_pages_range(&inode_out->i_data, pos_out,
-					   PAGE_ALIGN(pos_out + len) - 1);
+	truncate_inode_pages_range(&inode_out->i_data, pos_out,
+				   PAGE_ALIGN(pos_out + len) - 1);
 
+	ret = ocfs2_reflink_remap_blocks(inode_in, in_bh, pos_in, inode_out,
+					 out_bh, pos_out, len);
 	up_write(&OCFS2_I(inode_in)->ip_alloc_sem);
 	if (!same_inode)
 		up_write(&OCFS2_I(inode_out)->ip_alloc_sem);

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 22/25] ocfs2: fix pagecache truncation prior to reflink
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (20 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 21/25] ocfs2: truncate page cache for clone destination file before remapping Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 23/25] ocfs2: support partial clone range and dedupe range Darrick J. Wong
                   ` (3 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Prior to remapping blocks, it is necessary to remove pages from the
destination file's page cache.  Unfortunately, the truncation is not
aggressive enough -- if page size > block size, we'll end up zeroing
subpage blocks instead of removing them.  So, round the start offset
down and the end offset up.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ocfs2/refcounttree.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index 851ba3ae7ce8..b9e0418a1974 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4870,8 +4870,9 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 				  SINGLE_DEPTH_NESTING);
 
 	/* Zap any page cache for the destination file's range. */
-	truncate_inode_pages_range(&inode_out->i_data, pos_out,
-				   PAGE_ALIGN(pos_out + len) - 1);
+	truncate_inode_pages_range(&inode_out->i_data,
+				   round_down(pos_out, PAGE_SIZE),
+				   round_up(pos_out + len, PAGE_SIZE) - 1);
 
 	ret = ocfs2_reflink_remap_blocks(inode_in, in_bh, pos_in, inode_out,
 					 out_bh, pos_out, len);

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 23/25] ocfs2: support partial clone range and dedupe range
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (21 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 22/25] ocfs2: fix pagecache truncation prior to reflink Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 24/25] xfs: fix pagecache truncation prior to reflink Darrick J. Wong
                   ` (2 subsequent siblings)
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Change the ocfs2 remap code to allow for returning partial results.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ocfs2/file.c         |    7 +----
 fs/ocfs2/refcounttree.c |   73 ++++++++++++++++++++++++++---------------------
 fs/ocfs2/refcounttree.h |   12 ++++----
 3 files changed, 48 insertions(+), 44 deletions(-)


diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index c018605ac279..f9e65ff89a8a 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2531,11 +2531,8 @@ static loff_t ocfs2_remap_file_range(struct file *file_in, loff_t pos_in,
 				     struct file *file_out, loff_t pos_out,
 				     loff_t len, unsigned int flags)
 {
-	int ret;
-
-	ret = ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-					len, flags);
-	return ret < 0 ? ret : len;
+	return ocfs2_reflink_remap_range(file_in, pos_in, file_out, pos_out,
+			len, flags);
 }
 
 const struct inode_operations ocfs2_file_iops = {
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c
index b9e0418a1974..4eacdd703874 100644
--- a/fs/ocfs2/refcounttree.c
+++ b/fs/ocfs2/refcounttree.c
@@ -4507,14 +4507,14 @@ static int ocfs2_reflink_update_dest(struct inode *dest,
 }
 
 /* Remap the range pos_in:len in s_inode to pos_out:len in t_inode. */
-static int ocfs2_reflink_remap_extent(struct inode *s_inode,
-				      struct buffer_head *s_bh,
-				      loff_t pos_in,
-				      struct inode *t_inode,
-				      struct buffer_head *t_bh,
-				      loff_t pos_out,
-				      loff_t len,
-				      struct ocfs2_cached_dealloc_ctxt *dealloc)
+static loff_t ocfs2_reflink_remap_extent(struct inode *s_inode,
+					 struct buffer_head *s_bh,
+					 loff_t pos_in,
+					 struct inode *t_inode,
+					 struct buffer_head *t_bh,
+					 loff_t pos_out,
+					 loff_t len,
+					 struct ocfs2_cached_dealloc_ctxt *dealloc)
 {
 	struct ocfs2_extent_tree s_et;
 	struct ocfs2_extent_tree t_et;
@@ -4522,6 +4522,7 @@ static int ocfs2_reflink_remap_extent(struct inode *s_inode,
 	struct buffer_head *ref_root_bh = NULL;
 	struct ocfs2_refcount_tree *ref_tree;
 	struct ocfs2_super *osb;
+	loff_t remapped = 0;
 	loff_t pstart, plen;
 	u32 p_cluster, num_clusters, slast, spos, tpos;
 	unsigned int ext_flags;
@@ -4605,30 +4606,32 @@ static int ocfs2_reflink_remap_extent(struct inode *s_inode,
 next_loop:
 		spos += num_clusters;
 		tpos += num_clusters;
+		remapped += ocfs2_clusters_to_bytes(t_inode->i_sb,
+				num_clusters);
 	}
 
-out:
-	return ret;
+	return remapped;
 out_unlock_refcount:
 	ocfs2_unlock_refcount_tree(osb, ref_tree, 1);
 	brelse(ref_root_bh);
-	return ret;
+out:
+	return remapped > 0 ? remapped : ret;
 }
 
 /* Set up refcount tree and remap s_inode to t_inode. */
-static int ocfs2_reflink_remap_blocks(struct inode *s_inode,
-				      struct buffer_head *s_bh,
-				      loff_t pos_in,
-				      struct inode *t_inode,
-				      struct buffer_head *t_bh,
-				      loff_t pos_out,
-				      loff_t len)
+static loff_t ocfs2_reflink_remap_blocks(struct inode *s_inode,
+					 struct buffer_head *s_bh,
+					 loff_t pos_in,
+					 struct inode *t_inode,
+					 struct buffer_head *t_bh,
+					 loff_t pos_out,
+					 loff_t len)
 {
 	struct ocfs2_cached_dealloc_ctxt dealloc;
 	struct ocfs2_super *osb;
 	struct ocfs2_dinode *dis;
 	struct ocfs2_dinode *dit;
-	int ret;
+	loff_t ret;
 
 	osb = OCFS2_SB(s_inode->i_sb);
 	dis = (struct ocfs2_dinode *)s_bh->b_data;
@@ -4700,7 +4703,7 @@ static int ocfs2_reflink_remap_blocks(struct inode *s_inode,
 	/* Actually remap extents now. */
 	ret = ocfs2_reflink_remap_extent(s_inode, s_bh, pos_in, t_inode, t_bh,
 					 pos_out, len, &dealloc);
-	if (ret) {
+	if (ret < 0) {
 		mlog_errno(ret);
 		goto out;
 	}
@@ -4820,18 +4823,19 @@ static void ocfs2_reflink_inodes_unlock(struct inode *s_inode,
 }
 
 /* Link a range of blocks from one file to another. */
-int ocfs2_reflink_remap_range(struct file *file_in,
-			      loff_t pos_in,
-			      struct file *file_out,
-			      loff_t pos_out,
-			      loff_t len,
-			      unsigned int remap_flags)
+loff_t ocfs2_reflink_remap_range(struct file *file_in,
+				 loff_t pos_in,
+				 struct file *file_out,
+				 loff_t pos_out,
+				 loff_t len,
+				 unsigned int remap_flags)
 {
 	struct inode *inode_in = file_inode(file_in);
 	struct inode *inode_out = file_inode(file_out);
 	struct ocfs2_super *osb = OCFS2_SB(inode_in->i_sb);
 	struct buffer_head *in_bh = NULL, *out_bh = NULL;
 	bool same_inode = (inode_in == inode_out);
+	loff_t remapped = 0;
 	ssize_t ret;
 
 	if (!ocfs2_refcount_tree(osb))
@@ -4855,6 +4859,11 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 	if (ret <= 0)
 		goto out_unlock;
 
+	if (len == 0) {
+		ret = 0;
+		goto out_unlock;
+	}
+
 	/*
 	 * Update inode timestamps and remove security privileges before we
 	 * take the ilock.
@@ -4874,12 +4883,13 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 				   round_down(pos_out, PAGE_SIZE),
 				   round_up(pos_out + len, PAGE_SIZE) - 1);
 
-	ret = ocfs2_reflink_remap_blocks(inode_in, in_bh, pos_in, inode_out,
-					 out_bh, pos_out, len);
+	remapped = ocfs2_reflink_remap_blocks(inode_in, in_bh, pos_in,
+			inode_out, out_bh, pos_out, len);
 	up_write(&OCFS2_I(inode_in)->ip_alloc_sem);
 	if (!same_inode)
 		up_write(&OCFS2_I(inode_out)->ip_alloc_sem);
-	if (ret) {
+	if (remapped < 0) {
+		ret = remapped;
 		mlog_errno(ret);
 		goto out_unlock;
 	}
@@ -4897,10 +4907,7 @@ int ocfs2_reflink_remap_range(struct file *file_in,
 		goto out_unlock;
 	}
 
-	ocfs2_reflink_inodes_unlock(inode_in, in_bh, inode_out, out_bh);
-	return 0;
-
 out_unlock:
 	ocfs2_reflink_inodes_unlock(inode_in, in_bh, inode_out, out_bh);
-	return ret;
+	return remapped > 0 ? remapped : ret;
 }
diff --git a/fs/ocfs2/refcounttree.h b/fs/ocfs2/refcounttree.h
index eb65c1d0843c..9e64daba395d 100644
--- a/fs/ocfs2/refcounttree.h
+++ b/fs/ocfs2/refcounttree.h
@@ -115,11 +115,11 @@ int ocfs2_reflink_ioctl(struct inode *inode,
 			const char __user *oldname,
 			const char __user *newname,
 			bool preserve);
-int ocfs2_reflink_remap_range(struct file *file_in,
-			      loff_t pos_in,
-			      struct file *file_out,
-			      loff_t pos_out,
-			      loff_t len,
-			      unsigned int remap_flags);
+loff_t ocfs2_reflink_remap_range(struct file *file_in,
+				 loff_t pos_in,
+				 struct file *file_out,
+				 loff_t pos_out,
+				 loff_t len,
+				 unsigned int remap_flags);
 
 #endif /* OCFS2_REFCOUNTTREE_H */

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 24/25] xfs: fix pagecache truncation prior to reflink
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (22 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 23/25] ocfs2: support partial clone range and dedupe range Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  0:14 ` [PATCH 25/25] xfs: support returning partial reflink results Darrick J. Wong
  2018-10-10  1:02 ` [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Dave Chinner
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Prior to remapping blocks, it is necessary to remove pages from the
destination file's page cache.  Unfortunately, the truncation is not
aggressive enough -- if page size > block size, we'll end up zeroing
subpage blocks instead of removing them.  So, round the start offset
down and the end offset up.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_reflink.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index ec09e2783afe..0f4678920240 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1328,8 +1328,9 @@ xfs_reflink_remap_prep(
 		goto out_unlock;
 
 	/* Zap any page cache for the destination file's range. */
-	truncate_inode_pages_range(&inode_out->i_data, pos_out,
-				   PAGE_ALIGN(pos_out + *len) - 1);
+	truncate_inode_pages_range(&inode_out->i_data,
+			round_down(pos_out, PAGE_SIZE),
+			round_up(pos_out + *len, PAGE_SIZE) - 1);
 
 	/*
 	 * Update inode timestamps and remove security privileges before we

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* [PATCH 25/25] xfs: support returning partial reflink results
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (23 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 24/25] xfs: fix pagecache truncation prior to reflink Darrick J. Wong
@ 2018-10-10  0:14 ` Darrick J. Wong
  2018-10-10  1:02 ` [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Dave Chinner
  25 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  0:14 UTC (permalink / raw)
  To: david, darrick.wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Back when the XFS reflink code only supported clone_file_range, we were
only able to return zero or negative error codes to userspace.  However,
now that copy_file_range (which returns bytes copied) can use XFS'
clone_file_range, we have the opportunity to return partial results.
For example, if userspace sends a 1GB clone request and we run out of
space halfway through, we at least can tell userspace that we completed
512M of that request like a regular write.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_file.c    |    7 ++-----
 fs/xfs/xfs_reflink.c |   19 ++++++++++++++-----
 fs/xfs/xfs_reflink.h |    2 +-
 3 files changed, 17 insertions(+), 11 deletions(-)


diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 6f4205846451..a15057be1994 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -928,11 +928,8 @@ xfs_file_remap_range(
 	loff_t		len,
 	unsigned int	flags)
 {
-	int		ret;
-
-	ret = xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
-			len, false);
-	return ret < 0 ? ret : len;
+	return xfs_reflink_remap_range(file_in, pos_in, file_out, pos_out,
+			len, flags);
 }
 
 STATIC int
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index 0f4678920240..b33107a35330 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -1123,6 +1123,7 @@ xfs_reflink_remap_blocks(
 	struct xfs_inode	*dest,
 	xfs_fileoff_t		destoff,
 	xfs_filblks_t		len,
+	xfs_filblks_t		*remapped,
 	xfs_off_t		new_isize)
 {
 	struct xfs_bmbt_irec	imap;
@@ -1130,6 +1131,7 @@ xfs_reflink_remap_blocks(
 	int			error = 0;
 	xfs_filblks_t		range_len;
 
+	*remapped = 0;
 	/* drange = (destoff, destoff + len); srange = (srcoff, srcoff + len) */
 	while (len) {
 		uint		lock_mode;
@@ -1168,6 +1170,7 @@ xfs_reflink_remap_blocks(
 		srcoff += range_len;
 		destoff += range_len;
 		len -= range_len;
+		*remapped += range_len;
 	}
 
 	return 0;
@@ -1349,7 +1352,7 @@ xfs_reflink_remap_prep(
 /*
  * Link a range of blocks from one file to another.
  */
-int
+loff_t
 xfs_reflink_remap_range(
 	struct file		*file_in,
 	loff_t			pos_in,
@@ -1364,9 +1367,9 @@ xfs_reflink_remap_range(
 	struct xfs_inode	*dest = XFS_I(inode_out);
 	struct xfs_mount	*mp = src->i_mount;
 	xfs_fileoff_t		sfsbno, dfsbno;
-	xfs_filblks_t		fsblen;
+	xfs_filblks_t		fsblen, remapped = 0;
 	xfs_extlen_t		cowextsize;
-	ssize_t			ret;
+	int			ret;
 
 	if (!xfs_sb_version_hasreflink(&mp->m_sb))
 		return -EOPNOTSUPP;
@@ -1382,11 +1385,17 @@ xfs_reflink_remap_range(
 
 	trace_xfs_reflink_remap_range(src, pos_in, len, dest, pos_out);
 
+	if (len == 0) {
+		ret = 0;
+		goto out_unlock;
+	}
+
 	dfsbno = XFS_B_TO_FSBT(mp, pos_out);
 	sfsbno = XFS_B_TO_FSBT(mp, pos_in);
 	fsblen = XFS_B_TO_FSB(mp, len);
 	ret = xfs_reflink_remap_blocks(src, sfsbno, dest, dfsbno, fsblen,
-			pos_out + len);
+			&remapped, pos_out + len);
+	remapped = min_t(int64_t, len, XFS_FSB_TO_B(mp, remapped));
 	if (ret)
 		goto out_unlock;
 
@@ -1409,7 +1418,7 @@ xfs_reflink_remap_range(
 	xfs_reflink_remap_unlock(file_in, file_out);
 	if (ret)
 		trace_xfs_reflink_remap_range_error(dest, ret, _RET_IP_);
-	return ret;
+	return remapped > 0 ? remapped : ret;
 }
 
 /*
diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
index c3c46c276fe1..cbc26ff79a8f 100644
--- a/fs/xfs/xfs_reflink.h
+++ b/fs/xfs/xfs_reflink.h
@@ -27,7 +27,7 @@ extern int xfs_reflink_cancel_cow_range(struct xfs_inode *ip, xfs_off_t offset,
 extern int xfs_reflink_end_cow(struct xfs_inode *ip, xfs_off_t offset,
 		xfs_off_t count);
 extern int xfs_reflink_recover_cow(struct xfs_mount *mp);
-extern int xfs_reflink_remap_range(struct file *file_in, loff_t pos_in,
+extern loff_t xfs_reflink_remap_range(struct file *file_in, loff_t pos_in,
 		struct file *file_out, loff_t pos_out, loff_t len,
 		unsigned int remap_flags);
 extern int xfs_reflink_inode_has_shared_extents(struct xfs_trans *tp,

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH 01/25] xfs: add a per-xfs trace_printk macro
  2018-10-10  0:10 ` [PATCH 01/25] xfs: add a per-xfs trace_printk macro Darrick J. Wong
@ 2018-10-10  0:36   ` Dave Chinner
  2018-10-10 15:00   ` [PATCH v2 " Darrick J. Wong
  1 sibling, 0 replies; 47+ messages in thread
From: Dave Chinner @ 2018-10-10  0:36 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

On Tue, Oct 09, 2018 at 05:10:45PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a "xfs_tprintk" macro so that developers can use trace_printk to
> print out arbitrary debugging information with the XFS device name
> attached to the trace output.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_error.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> 
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index 246d3e989c6c..c3d9546b138c 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -99,4 +99,9 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
>  #define		XFS_PTAG_SHUTDOWN_LOGERROR	0x00000040
>  #define		XFS_PTAG_FSBLOCK_ZERO		0x00000080
>  
> +/* trace printk version of xfs_err and friends */
> +#define xfs_tprintk(mp, fmt, args...) \
> +	trace_printk("dev %d:%d " fmt, MAJOR((mp)->m_super->s_dev), \
> +			MINOR((mp)->m_super->s_dev), ##args)
> +
>  #endif	/* __XFS_ERROR_H__ */

Not convinced this is a good idea.  How are you going to ensure code
calling this trace point is not committed?

If we decide to add this, it needs to be a CONFIG_XFS_DEBUG=y only
definition because trace_printk() is only for temporary debugging
code and has substantial performance overheads even when these trace
points are not being traced.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems
  2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
                   ` (24 preceding siblings ...)
  2018-10-10  0:14 ` [PATCH 25/25] xfs: support returning partial reflink results Darrick J. Wong
@ 2018-10-10  1:02 ` Dave Chinner
  2018-10-10  1:06   ` Darrick J. Wong
  25 siblings, 1 reply; 47+ messages in thread
From: Dave Chinner @ 2018-10-10  1:02 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

On Tue, Oct 09, 2018 at 05:10:38PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> Dave, Eric, and I have been chasing a stale data exposure bug in the XFS
> reflink implementation, and tracked it down to reflink forgetting to do
> some of the file-extending activities that must happen for regular
> writes.
> 
> We then started auditing the clone, dedupe, and copyfile code and
> realized that from a file contents perspective, clonerange isn't any
> different from a regular file write.  Unfortunately, we also noticed
> that *unlike* a regular write, clonerange skips a ton of overflow
> checks, such as validating the ranges against s_maxbytes, MAX_NON_LFS,
> and RLIMIT_FSIZE.  We also observed that cloning into a file did not
> strip security privileges (suid, capabilities) like a regular write
> would.  I also noticed that xfs and ocfs2 need to dump the page cache
> before remapping blocks, not after.
> 
> In fixing the range checking problems I also realized that both dedupe
> and copyfile tell userspace how much of the requested operation was
> acted upon.  Since the range validation can shorten a clone request (or
> we can ENOSPC midway through), we might as well plumb the short
> operation reporting back through the VFS indirection code to userspace.
> 
> So, here's the whole giant pile of patches[1] that fix all the problems.
> The patch "generic: test reflink side effects" recently sent to fstests
> exercises the fixes in this series.  Tests are in [2].

Can you rebase this on the for-next branch on the xfs tree which
already contains some of the initial fixes in the series and a
couple of other reflink/dedupe data corruption fixes? I'm planning
on pushing them to Greg tomorrow, so you'll have to do this soon
anyway....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems
  2018-10-10  1:02 ` [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Dave Chinner
@ 2018-10-10  1:06   ` Darrick J. Wong
  0 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10  1:06 UTC (permalink / raw)
  To: Dave Chinner
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

On Wed, Oct 10, 2018 at 12:02:08PM +1100, Dave Chinner wrote:
> On Tue, Oct 09, 2018 at 05:10:38PM -0700, Darrick J. Wong wrote:
> > Hi all,
> > 
> > Dave, Eric, and I have been chasing a stale data exposure bug in the XFS
> > reflink implementation, and tracked it down to reflink forgetting to do
> > some of the file-extending activities that must happen for regular
> > writes.
> > 
> > We then started auditing the clone, dedupe, and copyfile code and
> > realized that from a file contents perspective, clonerange isn't any
> > different from a regular file write.  Unfortunately, we also noticed
> > that *unlike* a regular write, clonerange skips a ton of overflow
> > checks, such as validating the ranges against s_maxbytes, MAX_NON_LFS,
> > and RLIMIT_FSIZE.  We also observed that cloning into a file did not
> > strip security privileges (suid, capabilities) like a regular write
> > would.  I also noticed that xfs and ocfs2 need to dump the page cache
> > before remapping blocks, not after.
> > 
> > In fixing the range checking problems I also realized that both dedupe
> > and copyfile tell userspace how much of the requested operation was
> > acted upon.  Since the range validation can shorten a clone request (or
> > we can ENOSPC midway through), we might as well plumb the short
> > operation reporting back through the VFS indirection code to userspace.
> > 
> > So, here's the whole giant pile of patches[1] that fix all the problems.
> > The patch "generic: test reflink side effects" recently sent to fstests
> > exercises the fixes in this series.  Tests are in [2].
> 
> Can you rebase this on the for-next branch on the xfs tree which
> already contains some of the initial fixes in the series and a
> couple of other reflink/dedupe data corruption fixes? I'm planning
> on pushing them to Greg tomorrow, so you'll have to do this soon
> anyway....

<nod> I was planning to do that tomorrow, but figured I might as well
scrape for review comments in the mean time.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks
  2018-10-10  0:11 ` [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks Darrick J. Wong
@ 2018-10-10  5:23   ` Amir Goldstein
  2018-10-10 17:01     ` Darrick J. Wong
  0 siblings, 1 reply; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10  5:23 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 3:11 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> File range remapping, if allowed to run past the destination file's EOF,
> is an optimization on a regular file write.  Regular file writes that
> extend the file length are subject to various constraints which are not
> checked by range cloning.
>
> This is a correctness problem because we're never allowed to touch
> ranges that the page cache can't support (s_maxbytes); we're not
> supposed to deal with large offsets (MAX_NON_LFS) if O_LARGEFILE isn't
> set; and we must obey resource limits (RLIMIT_FSIZE).
>
> Therefore, add these checks to the new generic_remap_checks function so
> that we curtail unexpected behavior.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  mm/filemap.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 14041a8468ba..59056bd9c58a 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2974,6 +2974,27 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
>  }
>  EXPORT_SYMBOL(generic_write_checks);
>
> +static int
> +generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
> +{
> +       struct inode *inode = file->f_mapping->host;
> +
> +       /* Don't exceed the LFS limits. */
> +       if (unlikely(pos + *count > MAX_NON_LFS &&
> +                               !(file->f_flags & O_LARGEFILE))) {
> +               if (pos >= MAX_NON_LFS)
> +                       return -EFBIG;
> +               *count = min(*count, MAX_NON_LFS - (uint64_t)pos);
> +       }
> +
> +       /* Don't operate on ranges the page cache doesn't support. */
> +       if (unlikely(pos >= inode->i_sb->s_maxbytes))
> +               return -EFBIG;
> +
> +       *count = min(*count, inode->i_sb->s_maxbytes - (uint64_t)pos);
> +       return 0;
> +}
> +

Sorry. I haven't explained myself properly last time.
What I meant is that it hurts my eyes to see generic_write_checks() and
generic_remap_check_limits() which from the line of (limit != RLIM_INFINITY)
are exactly the same thing. Yes, generic_remap_check_limits() uses
iov_iter_truncate(), but that's a minor semantic change - it can be easily
resolved by creating a dummy iter in generic_remap_checks() instead of
passing int *count.

You could say that this is nit picking, but the very reason this patch
set exists
it because clone/dedup implementation did not use the same range checks
of write to begin with, so it just seems wrong to diverge them at this point.

So to be clear, I suggest that generic_write_checks() should use your
generic_remap_check_limits() helper.
If you disagree and others can live with this minor duplication, fine by me.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range
  2018-10-10  0:11 ` [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range Darrick J. Wong
@ 2018-10-10  5:54   ` Amir Goldstein
  2018-10-10 15:13     ` Darrick J. Wong
  0 siblings, 1 reply; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10  5:54 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 3:12 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Combine the clone_file_range and dedupe_file_range operations into a
> single remap_file_range file operation dispatch since they're
> fundamentally the same operation.  The differences between the two can
> be made in the prep functions.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I like this. Nits below.

[...]

> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d60b6caf09e8..e22b294fa25b 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3627,26 +3627,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
>         return ret;
>  }
>
> -int btrfs_dedupe_file_range(struct file *src_file, loff_t src_loff,
> -                           struct file *dst_file, loff_t dst_loff,
> -                           u64 olen)
> -{
> -       struct inode *src = file_inode(src_file);
> -       struct inode *dst = file_inode(dst_file);
> -       u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> -
> -       if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
> -               /*
> -                * Btrfs does not support blocksize < page_size. As a
> -                * result, btrfs_cmp_data() won't correctly handle
> -                * this situation without an update.
> -                */
> -               return -EINVAL;
> -       }
> -
> -       return btrfs_extent_same(src, src_loff, olen, dst, dst_loff);
> -}
> -
>  static int clone_finish_inode_update(struct btrfs_trans_handle *trans,
>                                      struct inode *inode,
>                                      u64 endoff,
> @@ -4348,9 +4328,27 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
>         return ret;
>  }
>
> -int btrfs_clone_file_range(struct file *src_file, loff_t off,
> -               struct file *dst_file, loff_t destoff, u64 len)
> +int btrfs_remap_file_range(struct file *src_file, loff_t off,
> +               struct file *dst_file, loff_t destoff, u64 len,
> +               unsigned int flags)
>  {
> +       if (flags & RFR_IDENTICAL_DATA) {
> +               struct inode *src = file_inode(src_file);
> +               struct inode *dst = file_inode(dst_file);
> +               u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> +
> +               if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
> +                       /*
> +                        * Btrfs does not support blocksize < page_size. As a
> +                        * result, btrfs_cmp_data() won't correctly handle
> +                        * this situation without an update.
> +                        */
> +                       return -EINVAL;
> +               }
> +
> +               return btrfs_extent_same(src, off, len, dst, destoff);
> +       }
> +

Seems weird that you would do that instead of:

+    if (flags & ~RFR_IDENTICAL_DATA)
+        return -EINVAL;
+    if (flags & RFR_IDENTICAL_DATA)
+        return btrfs_dedupe_file_range(src, off, dst, destoff, len);

>         return btrfs_clone_files(dst_file, src_file, off, len, destoff);
>  }
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 7065426b3280..bf971fd7cab2 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -975,8 +975,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
>         .listxattr = cifs_listxattr,
>  };
>
> -static int cifs_clone_file_range(struct file *src_file, loff_t off,
> -               struct file *dst_file, loff_t destoff, u64 len)
> +static int cifs_remap_file_range(struct file *src_file, loff_t off,
> +               struct file *dst_file, loff_t destoff, u64 len,
> +               unsigned int flags)
>  {
>         struct inode *src_inode = file_inode(src_file);
>         struct inode *target_inode = file_inode(dst_file);
> @@ -986,6 +987,9 @@ static int cifs_clone_file_range(struct file *src_file, loff_t off,
>         unsigned int xid;
>         int rc;
>
> +       if (flags & RFR_IDENTICAL_DATA)
> +               return -EOPNOTSUPP;
> +

I think everyone would be better off with:
+       if (flags)
+               return -EINVAL;

This way you won't need to change all filesystem implementations
every time that you add a new RFR flag.
Lucky for us, dedup already return -EINVAL if (!f_op->dedupe_file_range)
(and not -EOPNOTSUPP).

[...]
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 986313da0c88..693bd0620a81 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -489,26 +489,28 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
>                             OVL_COPY);
>  }
>
> -static int ovl_clone_file_range(struct file *file_in, loff_t pos_in,
> -                               struct file *file_out, loff_t pos_out, u64 len)
> +static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> +                               struct file *file_out, loff_t pos_out,
> +                               u64 len, unsigned int flags)
>  {
> -       return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> -                           OVL_CLONE);
> -}
> +       enum ovl_copyop op;
> +
> +       if (flags & RFR_IDENTICAL_DATA)
> +               op = OVL_DEDUPE;
> +       else
> +               op = OVL_CLONE;
>
> -static int ovl_dedupe_file_range(struct file *file_in, loff_t pos_in,
> -                                struct file *file_out, loff_t pos_out, u64 len)
> -{
>         /*
>          * Don't copy up because of a dedupe request, this wouldn't make sense
>          * most of the time (data would be duplicated instead of deduplicated).
>          */
> -       if (!ovl_inode_upper(file_inode(file_in)) ||
> -           !ovl_inode_upper(file_inode(file_out)))
> +       if (op == OVL_DEDUPE &&
> +           (!ovl_inode_upper(file_inode(file_in)) ||
> +            !ovl_inode_upper(file_inode(file_out))))
>                 return -EPERM;
>
>         return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> -                           OVL_DEDUPE);
> +                           op);
>  }
>

Apart from the generic check invalid flags comment - ACK on ovl part.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions
  2018-10-10  0:13 ` [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions Darrick J. Wong
@ 2018-10-10  6:22   ` Amir Goldstein
  2018-10-10  6:39     ` Amir Goldstein
  0 siblings, 1 reply; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10  6:22 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Plumb a remap_flags argument through the {do,vfs}_clone_file_range
> functions so that clone can take advantage of it.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
[...]
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index c8c890c22898..8b22035af4d7 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -462,7 +462,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
>
>         case OVL_CLONE:
>                 ret = vfs_clone_file_range(real_in.file, pos_in,
> -                                          real_out.file, pos_out, len);
> +                                          real_out.file, pos_out, len, flags);
>                 break;
>
>         case OVL_DEDUPE:
> @@ -509,7 +509,7 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
>              !ovl_inode_upper(file_inode(file_out))))
>                 return -EPERM;
>
> -       return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> +       return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
>                             op);
>  }

This patch forgets to change args in ovl_copyfile() function definition.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions
  2018-10-10  6:22   ` Amir Goldstein
@ 2018-10-10  6:39     ` Amir Goldstein
  0 siblings, 0 replies; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10  6:39 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 9:22 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Plumb a remap_flags argument through the {do,vfs}_clone_file_range
> > functions so that clone can take advantage of it.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> [...]
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index c8c890c22898..8b22035af4d7 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -462,7 +462,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> >
> >         case OVL_CLONE:
> >                 ret = vfs_clone_file_range(real_in.file, pos_in,
> > -                                          real_out.file, pos_out, len);
> > +                                          real_out.file, pos_out, len, flags);
> >                 break;
> >
> >         case OVL_DEDUPE:
> > @@ -509,7 +509,7 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> >              !ovl_inode_upper(file_inode(file_out))))
> >                 return -EPERM;
> >
> > -       return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> > +       return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
> >                             op);
> >  }
>
> This patch forgets to change args in ovl_copyfile() function definition.
>

Sorry, it was already there.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed
  2018-10-10  0:13 ` [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed Darrick J. Wong
@ 2018-10-10  6:47   ` Amir Goldstein
  2018-10-10 15:50     ` Darrick J. Wong
  0 siblings, 1 reply; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10  6:47 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Change the remap_file_range functions to take a number of bytes to
> operate upon and return the number of bytes they operated on.  This is a
> requirement for allowing fs implementations to return short clone/dedupe
> results to the user, which will enable us to obey resource limits in a
> graceful manner.
>
> A subsequent patch will enable copy_file_range to signal to the
> ->clone_file_range implementation that it can handle a short length,
> which will be returned in the function's return value.  Neither clone
> ioctl can take advantage of this, alas.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
[...]
> @@ -141,8 +142,8 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>         }
>
>         /* Try to use clone_file_range to clone up within the same fs */
> -       error = do_clone_file_range(old_file, 0, new_file, 0, len);
> -       if (!error)
> +       cloned = do_clone_file_range(old_file, 0, new_file, 0, len);
> +       if (cloned == len)
>                 goto out;
>         /* Couldn't clone, so now we try to copy the data */
>         error = 0;

This error = 0 not needed anymore, but not a big deal...

> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 693bd0620a81..c8c890c22898 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -434,14 +434,14 @@ enum ovl_copyop {
>         OVL_DEDUPE,
>  };
>
> -static ssize_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> +static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
>                             struct file *file_out, loff_t pos_out,
> -                           u64 len, unsigned int flags, enum ovl_copyop op)
> +                           loff_t len, unsigned int flags, enum ovl_copyop op)
>  {
>         struct inode *inode_out = file_inode(file_out);
>         struct fd real_in, real_out;
>         const struct cred *old_cred;
> -       ssize_t ret;
> +       loff_t ret;
>
>         ret = ovl_real_fdget(file_out, &real_out);
>         if (ret)
> @@ -489,9 +489,9 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
>                             OVL_COPY);
>  }
>
> -static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> -                               struct file *file_out, loff_t pos_out,
> -                               u64 len, unsigned int flags)
> +static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> +                                  struct file *file_out, loff_t pos_out,
> +                                  loff_t len, unsigned int flags)
>  {
>         enum ovl_copyop op;
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 917934770b08..f43b0620afd4 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1589,10 +1589,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>          * more efficient if both clone and copy are supported (e.g. NFS).
>          */
>         if (file_in->f_op->remap_file_range) {
> -               ret = file_in->f_op->remap_file_range(file_in, pos_in,
> -                               file_out, pos_out, len, 0);
> -               if (ret == 0) {
> -                       ret = len;
> +               s64 cloned;

loff_t?

> +
> +               cloned = file_in->f_op->remap_file_range(file_in, pos_in,
> +                               file_out, pos_out,
> +                               min_t(loff_t, MAX_RW_COUNT, len), 0);
> +               if (cloned >= 0) {
> +                       ret = cloned;
>                         goto done;
>                 }
>         }

Commit message wasn't clear enough on the behavior of copy_file_range()
before and after the patch IMO. Maybe it would be better to pospone this
semantic change to the RFR_SHORTEN patch and keep if (cloned == len)
in this patch?

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 17/25] vfs: make remapping to source file eof more explicit
  2018-10-10  0:13 ` [PATCH 17/25] vfs: make remapping to source file eof more explicit Darrick J. Wong
@ 2018-10-10 12:29   ` Amir Goldstein
  2018-10-10 16:29     ` Darrick J. Wong
  0 siblings, 1 reply; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10 12:29 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Create a RFR_TO_SRC_EOF flag to explicitly declare that the caller wants
> the remap implementation to remap to the end of the source file, once
> the files are locked.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/ioctl.c         |    3 ++-
>  fs/nfsd/vfs.c      |    3 ++-
>  fs/read_write.c    |   13 +++++++------
>  include/linux/fs.h |    2 ++
>  4 files changed, 13 insertions(+), 8 deletions(-)
>
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 505275ec5596..7fec997abd2f 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -224,6 +224,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
>  {
>         struct fd src_file = fdget(srcfd);
>         loff_t cloned;
> +       unsigned int flags = olen == 0 ? RFR_TO_SRC_EOF : 0;
>         int ret;
>
>         if (!src_file.file)
> @@ -232,7 +233,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
>         if (src_file.file->f_path.mnt != dst_file->f_path.mnt)
>                 goto fdput;
>         cloned = vfs_clone_file_range(src_file.file, off, dst_file, destoff,
> -                                     olen, 0);
> +                                     olen, flags);
>         if (cloned < 0)
>                 ret = cloned;
>         else if (olen && cloned != olen)
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 726fc5b2b27a..d1f2ae08adf6 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -542,8 +542,9 @@ __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
>                 u64 dst_pos, u64 count)
>  {
>         loff_t cloned;
> +       unsigned int flags = count == 0 ? RFR_TO_SRC_EOF : 0;
>
> -       cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, 0);
> +       cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, flags);
>         if (count && cloned != count)
>                 cloned = -EINVAL;
>         return nfserrno(cloned < 0 ? cloned : 0);
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 479eb810c8e6..a628fd9a47cf 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1748,11 +1748,12 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
>
>         isize = i_size_read(inode_in);
>
> -       /* Zero length dedupe exits immediately; reflink goes to EOF. */
> -       if (*len == 0) {
> -               if (is_dedupe || pos_in == isize)
> -                       return 0;
> -               if (pos_in > isize)
> +       /*
> +        * If the caller asked to go all the way to the end of the source file,
> +        * set *len now that we have the file locked.
> +        */
> +       if (remap_flags & RFR_TO_SRC_EOF) {
> +               if (pos_in >= isize)
>                         return -EINVAL;
>                 *len = isize - pos_in;
>         }
> @@ -1828,7 +1829,7 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
>         struct inode *inode_out = file_inode(file_out);
>         loff_t ret;
>
> -       WARN_ON_ONCE(remap_flags);
> +       WARN_ON_ONCE(remap_flags & ~(RFR_TO_SRC_EOF));
>
>         if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
>                 return -EISDIR;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 5c1bf1c35bc6..9f90dcd4df3b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1725,8 +1725,10 @@ struct block_device_operations;
>   * These flags control the behavior of the remap_file_range function pointer.
>   *
>   * RFR_IDENTICAL_DATA: only remap if contents identical (i.e. deduplicate)

<bikeshedding> not that I care so much, but is there any reason you chose
to use _IDENTICAL_ vs. _SAME_? The latter is shorter and already engraved
in the dedup uapi. </bikeshedding>

> + * RFR_TO_SRC_EOF: remap to the end of the source file
>   */
>  #define RFR_IDENTICAL_DATA     (1 << 0)
> +#define RFR_TO_SRC_EOF         (1 << 1)
>

So what is the best way to make sure that all filesystems can
properly handle this flag? and the RFR_CAN_SHORTEN flag?

The way that your patches took is to not check for invalid flags
at all in filesystems, but I don't think that is a viable option.

Another way would be to individually add those flags to invalid
flags check in all relevant filesystems.

Another way would be to follow a pattern similar to
fiemap_check_flags(), except in case filesystem does not declare
to support the RFR_ "advisory" flags, it will not fail the operation.

Comparing to FIEMAP_ flags, no filesystem would have needed to declare
support for FIEMAP_FLAG_SYNC, because vfs dealt with it anyway
before calling into the filesystem. So de-facto, any filesystem supports
FIEMAP_FLAG_SYNC without doing anything, but it is still worth passing
the flag into filesystem in case it matter (it does for overlayfs).

I am not sure if giving fiemap_check_flags() as an example for sorting
out VFS API flags is a good idea, because I completely don't understand
what is going on in ext4_fiemap(). Why do FIEMAP_FLAG_CACHE and
the case of  !EXT4_INODE_EXTENTS bypass fiemap_check_flags()?
Is there a rational behind all this or just plain old API bit rot?
If bit rot, then perhaps the fiemap_check_flags() wasn't a good enough
coding pattern?

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* [PATCH v2 01/25] xfs: add a per-xfs trace_printk macro
  2018-10-10  0:10 ` [PATCH 01/25] xfs: add a per-xfs trace_printk macro Darrick J. Wong
  2018-10-10  0:36   ` Dave Chinner
@ 2018-10-10 15:00   ` Darrick J. Wong
  1 sibling, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 15:00 UTC (permalink / raw)
  To: david
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, ocfs2-devel

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a "xfs_tprintk" macro so that developers can use trace_printk to
print out arbitrary debugging information with the XFS device name
attached to the trace output.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_error.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
index 246d3e989c6c..5caa8bdf6c38 100644
--- a/fs/xfs/xfs_error.h
+++ b/fs/xfs/xfs_error.h
@@ -76,6 +76,11 @@ extern int xfs_errortag_set(struct xfs_mount *mp, unsigned int error_tag,
 		unsigned int tag_value);
 extern int xfs_errortag_add(struct xfs_mount *mp, unsigned int error_tag);
 extern int xfs_errortag_clearall(struct xfs_mount *mp);
+
+/* trace printk version of xfs_err and friends */
+#define xfs_tprintk(mp, fmt, args...) \
+	trace_printk("dev %d:%d " fmt, MAJOR((mp)->m_super->s_dev), \
+			MINOR((mp)->m_super->s_dev), ##args)
 #else
 #define xfs_errortag_init(mp)			(0)
 #define xfs_errortag_del(mp)
@@ -83,6 +88,7 @@ extern int xfs_errortag_clearall(struct xfs_mount *mp);
 #define xfs_errortag_set(mp, tag, val)		(ENOSYS)
 #define xfs_errortag_add(mp, tag)		(ENOSYS)
 #define xfs_errortag_clearall(mp)		(ENOSYS)
+#define xfs_tprintk(mp, fmt, args...)		do { } while (0)
 #endif /* DEBUG */
 
 /*

^ permalink raw reply related	[flat|nested] 47+ messages in thread

* Re: [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range
  2018-10-10  5:54   ` Amir Goldstein
@ 2018-10-10 15:13     ` Darrick J. Wong
  2018-10-10 15:23       ` Amir Goldstein
  0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 15:13 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 08:54:44AM +0300, Amir Goldstein wrote:
> On Wed, Oct 10, 2018 at 3:12 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Combine the clone_file_range and dedupe_file_range operations into a
> > single remap_file_range file operation dispatch since they're
> > fundamentally the same operation.  The differences between the two can
> > be made in the prep functions.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> I like this. Nits below.
> 
> [...]
> 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index d60b6caf09e8..e22b294fa25b 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -3627,26 +3627,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen,
> >         return ret;
> >  }
> >
> > -int btrfs_dedupe_file_range(struct file *src_file, loff_t src_loff,
> > -                           struct file *dst_file, loff_t dst_loff,
> > -                           u64 olen)
> > -{
> > -       struct inode *src = file_inode(src_file);
> > -       struct inode *dst = file_inode(dst_file);
> > -       u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> > -
> > -       if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
> > -               /*
> > -                * Btrfs does not support blocksize < page_size. As a
> > -                * result, btrfs_cmp_data() won't correctly handle
> > -                * this situation without an update.
> > -                */
> > -               return -EINVAL;
> > -       }
> > -
> > -       return btrfs_extent_same(src, src_loff, olen, dst, dst_loff);
> > -}
> > -
> >  static int clone_finish_inode_update(struct btrfs_trans_handle *trans,
> >                                      struct inode *inode,
> >                                      u64 endoff,
> > @@ -4348,9 +4328,27 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
> >         return ret;
> >  }
> >
> > -int btrfs_clone_file_range(struct file *src_file, loff_t off,
> > -               struct file *dst_file, loff_t destoff, u64 len)
> > +int btrfs_remap_file_range(struct file *src_file, loff_t off,
> > +               struct file *dst_file, loff_t destoff, u64 len,
> > +               unsigned int flags)
> >  {
> > +       if (flags & RFR_IDENTICAL_DATA) {
> > +               struct inode *src = file_inode(src_file);
> > +               struct inode *dst = file_inode(dst_file);
> > +               u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
> > +
> > +               if (WARN_ON_ONCE(bs < PAGE_SIZE)) {
> > +                       /*
> > +                        * Btrfs does not support blocksize < page_size. As a
> > +                        * result, btrfs_cmp_data() won't correctly handle
> > +                        * this situation without an update.
> > +                        */
> > +                       return -EINVAL;
> > +               }
> > +
> > +               return btrfs_extent_same(src, off, len, dst, destoff);
> > +       }
> > +
> 
> Seems weird that you would do that instead of:
> 
> +    if (flags & ~RFR_IDENTICAL_DATA)
> +        return -EINVAL;
> +    if (flags & RFR_IDENTICAL_DATA)
> +        return btrfs_dedupe_file_range(src, off, dst, destoff, len);

Hmm.  The flags validation thing is kind of a mess here.  There should be a:

#define RFR_VALID_FLAGS	(RFR_IDENTICAL_DATA | /* add other RFR flags */)

And all these functions should also gate on:

if (remap_flags & ~RFR_VALID_FLAGS) {
	WARN_ON(...);
	return -EINVAL;
}

Though FWIW the btrfs implementation actually will support all three
flags, so the particular structure of these checks here are correct if
you add in my self-criticism above.

> 
> >         return btrfs_clone_files(dst_file, src_file, off, len, destoff);
> >  }
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 7065426b3280..bf971fd7cab2 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -975,8 +975,9 @@ const struct inode_operations cifs_symlink_inode_ops = {
> >         .listxattr = cifs_listxattr,
> >  };
> >
> > -static int cifs_clone_file_range(struct file *src_file, loff_t off,
> > -               struct file *dst_file, loff_t destoff, u64 len)
> > +static int cifs_remap_file_range(struct file *src_file, loff_t off,
> > +               struct file *dst_file, loff_t destoff, u64 len,
> > +               unsigned int flags)
> >  {
> >         struct inode *src_inode = file_inode(src_file);
> >         struct inode *target_inode = file_inode(dst_file);
> > @@ -986,6 +987,9 @@ static int cifs_clone_file_range(struct file *src_file, loff_t off,
> >         unsigned int xid;
> >         int rc;
> >
> > +       if (flags & RFR_IDENTICAL_DATA)
> > +               return -EOPNOTSUPP;
> > +
> 
> I think everyone would be better off with:
> +       if (flags)
> +               return -EINVAL;
> 
> This way you won't need to change all filesystem implementations
> every time that you add a new RFR flag.
> Lucky for us, dedup already return -EINVAL if (!f_op->dedupe_file_range)
> (and not -EOPNOTSUPP).

Ugh, right, I forgot about that, um, quirk of the interface. :(

> [...]
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 986313da0c88..693bd0620a81 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -489,26 +489,28 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
> >                             OVL_COPY);
> >  }
> >
> > -static int ovl_clone_file_range(struct file *file_in, loff_t pos_in,
> > -                               struct file *file_out, loff_t pos_out, u64 len)
> > +static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> > +                               struct file *file_out, loff_t pos_out,
> > +                               u64 len, unsigned int flags)
> >  {
> > -       return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> > -                           OVL_CLONE);
> > -}
> > +       enum ovl_copyop op;
> > +
> > +       if (flags & RFR_IDENTICAL_DATA)
> > +               op = OVL_DEDUPE;
> > +       else
> > +               op = OVL_CLONE;
> >
> > -static int ovl_dedupe_file_range(struct file *file_in, loff_t pos_in,
> > -                                struct file *file_out, loff_t pos_out, u64 len)
> > -{
> >         /*
> >          * Don't copy up because of a dedupe request, this wouldn't make sense
> >          * most of the time (data would be duplicated instead of deduplicated).
> >          */
> > -       if (!ovl_inode_upper(file_inode(file_in)) ||
> > -           !ovl_inode_upper(file_inode(file_out)))
> > +       if (op == OVL_DEDUPE &&
> > +           (!ovl_inode_upper(file_inode(file_in)) ||
> > +            !ovl_inode_upper(file_inode(file_out))))
> >                 return -EPERM;
> >
> >         return ovl_copyfile(file_in, pos_in, file_out, pos_out, len, 0,
> > -                           OVL_DEDUPE);
> > +                           op);
> >  }
> >
> 
> Apart from the generic check invalid flags comment - ACK on ovl part.

Thanks for the review!  Is that an official Acked-by to add to the
commit message, or an informal ACK?

--D

> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range
  2018-10-10 15:13     ` Darrick J. Wong
@ 2018-10-10 15:23       ` Amir Goldstein
  0 siblings, 0 replies; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10 15:23 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 6:13 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Oct 10, 2018 at 08:54:44AM +0300, Amir Goldstein wrote:
> > On Wed, Oct 10, 2018 at 3:12 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > Combine the clone_file_range and dedupe_file_range operations into a
> > > single remap_file_range file operation dispatch since they're
> > > fundamentally the same operation.  The differences between the two can
> > > be made in the prep functions.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> >

> >
> > Apart from the generic check invalid flags comment - ACK on ovl part.
>
> Thanks for the review!  Is that an official Acked-by to add to the
> commit message, or an informal ACK?
>

I would offer my Acked-by for whole of the vfs patches
if we agree on the correct way to handle invalid flags
(see more comments up the series regarding the "advisory" flags).

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed
  2018-10-10  6:47   ` Amir Goldstein
@ 2018-10-10 15:50     ` Darrick J. Wong
  2018-10-10 18:28       ` Amir Goldstein
  0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 15:50 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 09:47:00AM +0300, Amir Goldstein wrote:
> On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Change the remap_file_range functions to take a number of bytes to
> > operate upon and return the number of bytes they operated on.  This is a
> > requirement for allowing fs implementations to return short clone/dedupe
> > results to the user, which will enable us to obey resource limits in a
> > graceful manner.
> >
> > A subsequent patch will enable copy_file_range to signal to the
> > ->clone_file_range implementation that it can handle a short length,
> > which will be returned in the function's return value.  Neither clone
> > ioctl can take advantage of this, alas.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> [...]
> > @@ -141,8 +142,8 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> >         }
> >
> >         /* Try to use clone_file_range to clone up within the same fs */
> > -       error = do_clone_file_range(old_file, 0, new_file, 0, len);
> > -       if (!error)
> > +       cloned = do_clone_file_range(old_file, 0, new_file, 0, len);
> > +       if (cloned == len)
> >                 goto out;
> >         /* Couldn't clone, so now we try to copy the data */
> >         error = 0;
> 
> This error = 0 not needed anymore, but not a big deal...

Fixed.

> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 693bd0620a81..c8c890c22898 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -434,14 +434,14 @@ enum ovl_copyop {
> >         OVL_DEDUPE,
> >  };
> >
> > -static ssize_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> > +static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> >                             struct file *file_out, loff_t pos_out,
> > -                           u64 len, unsigned int flags, enum ovl_copyop op)
> > +                           loff_t len, unsigned int flags, enum ovl_copyop op)
> >  {
> >         struct inode *inode_out = file_inode(file_out);
> >         struct fd real_in, real_out;
> >         const struct cred *old_cred;
> > -       ssize_t ret;
> > +       loff_t ret;
> >
> >         ret = ovl_real_fdget(file_out, &real_out);
> >         if (ret)
> > @@ -489,9 +489,9 @@ static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in,
> >                             OVL_COPY);
> >  }
> >
> > -static int ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> > -                               struct file *file_out, loff_t pos_out,
> > -                               u64 len, unsigned int flags)
> > +static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
> > +                                  struct file *file_out, loff_t pos_out,
> > +                                  loff_t len, unsigned int flags)
> >  {
> >         enum ovl_copyop op;
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 917934770b08..f43b0620afd4 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1589,10 +1589,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >          * more efficient if both clone and copy are supported (e.g. NFS).
> >          */
> >         if (file_in->f_op->remap_file_range) {
> > -               ret = file_in->f_op->remap_file_range(file_in, pos_in,
> > -                               file_out, pos_out, len, 0);
> > -               if (ret == 0) {
> > -                       ret = len;
> > +               s64 cloned;
> 
> loff_t?

Fixed.

> > +
> > +               cloned = file_in->f_op->remap_file_range(file_in, pos_in,
> > +                               file_out, pos_out,
> > +                               min_t(loff_t, MAX_RW_COUNT, len), 0);
> > +               if (cloned >= 0) {
> > +                       ret = cloned;
> >                         goto done;
> >                 }
> >         }
> 
> Commit message wasn't clear enough on the behavior of copy_file_range()
> before and after the patch IMO. Maybe it would be better to pospone this
> semantic change to the RFR_SHORTEN patch and keep if (cloned == len)
> in this patch?

There shouldn't be any behavior change here -- all implementations
return a negative error code or the length that was passed in.  I'll
clarify that in the commit message.

--D

> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 17/25] vfs: make remapping to source file eof more explicit
  2018-10-10 12:29   ` Amir Goldstein
@ 2018-10-10 16:29     ` Darrick J. Wong
  2018-10-10 17:31       ` Amir Goldstein
  0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 16:29 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 03:29:06PM +0300, Amir Goldstein wrote:
> On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Create a RFR_TO_SRC_EOF flag to explicitly declare that the caller wants
> > the remap implementation to remap to the end of the source file, once
> > the files are locked.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/ioctl.c         |    3 ++-
> >  fs/nfsd/vfs.c      |    3 ++-
> >  fs/read_write.c    |   13 +++++++------
> >  include/linux/fs.h |    2 ++
> >  4 files changed, 13 insertions(+), 8 deletions(-)
> >
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 505275ec5596..7fec997abd2f 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -224,6 +224,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
> >  {
> >         struct fd src_file = fdget(srcfd);
> >         loff_t cloned;
> > +       unsigned int flags = olen == 0 ? RFR_TO_SRC_EOF : 0;
> >         int ret;
> >
> >         if (!src_file.file)
> > @@ -232,7 +233,7 @@ static long ioctl_file_clone(struct file *dst_file, unsigned long srcfd,
> >         if (src_file.file->f_path.mnt != dst_file->f_path.mnt)
> >                 goto fdput;
> >         cloned = vfs_clone_file_range(src_file.file, off, dst_file, destoff,
> > -                                     olen, 0);
> > +                                     olen, flags);
> >         if (cloned < 0)
> >                 ret = cloned;
> >         else if (olen && cloned != olen)
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 726fc5b2b27a..d1f2ae08adf6 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -542,8 +542,9 @@ __be32 nfsd4_clone_file_range(struct file *src, u64 src_pos, struct file *dst,
> >                 u64 dst_pos, u64 count)
> >  {
> >         loff_t cloned;
> > +       unsigned int flags = count == 0 ? RFR_TO_SRC_EOF : 0;
> >
> > -       cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, 0);
> > +       cloned = vfs_clone_file_range(src, src_pos, dst, dst_pos, count, flags);
> >         if (count && cloned != count)
> >                 cloned = -EINVAL;
> >         return nfserrno(cloned < 0 ? cloned : 0);
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 479eb810c8e6..a628fd9a47cf 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1748,11 +1748,12 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
> >
> >         isize = i_size_read(inode_in);
> >
> > -       /* Zero length dedupe exits immediately; reflink goes to EOF. */
> > -       if (*len == 0) {
> > -               if (is_dedupe || pos_in == isize)
> > -                       return 0;
> > -               if (pos_in > isize)
> > +       /*
> > +        * If the caller asked to go all the way to the end of the source file,
> > +        * set *len now that we have the file locked.
> > +        */
> > +       if (remap_flags & RFR_TO_SRC_EOF) {
> > +               if (pos_in >= isize)
> >                         return -EINVAL;
> >                 *len = isize - pos_in;
> >         }
> > @@ -1828,7 +1829,7 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
> >         struct inode *inode_out = file_inode(file_out);
> >         loff_t ret;
> >
> > -       WARN_ON_ONCE(remap_flags);
> > +       WARN_ON_ONCE(remap_flags & ~(RFR_TO_SRC_EOF));
> >
> >         if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
> >                 return -EISDIR;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 5c1bf1c35bc6..9f90dcd4df3b 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1725,8 +1725,10 @@ struct block_device_operations;
> >   * These flags control the behavior of the remap_file_range function pointer.
> >   *
> >   * RFR_IDENTICAL_DATA: only remap if contents identical (i.e. deduplicate)
> 
> <bikeshedding> not that I care so much, but is there any reason you chose
> to use _IDENTICAL_ vs. _SAME_? The latter is shorter and already engraved
> in the dedup uapi. </bikeshedding>

I picked 'identical' because it seemed (to me anyway) to hint that yes
we do memcmp the contents.  Some people I've met seem to use 'same' to
describe the much looser statement that hash(a) == hash(b), e.g.

  "We use crc32c to find out if the blocks are the same and then we of
   course do a byte by byte comparison to find out if they're really the
   same."

No, you're using crc32c to find block-match candidates and then memcmp
to decide if they're really identical. :)

Anyway, I suppose we already use 'same' elsewhere in the kernel to
describe this, so I should reduce the cognitive dissonance and use that
too.  Fixed.

> > + * RFR_TO_SRC_EOF: remap to the end of the source file
> >   */
> >  #define RFR_IDENTICAL_DATA     (1 << 0)
> > +#define RFR_TO_SRC_EOF         (1 << 1)
> >
> 
> So what is the best way to make sure that all filesystems can
> properly handle this flag? and the RFR_CAN_SHORTEN flag?
> 
> The way that your patches took is to not check for invalid flags
> at all in filesystems, but I don't think that is a viable option.

The RFR flags are internal APIs, so we don't need to be quite as strict
as fiemap does...

> Another way would be to individually add those flags to invalid
> flags check in all relevant filesystems.
> 
> Another way would be to follow a pattern similar to
> fiemap_check_flags(), except in case filesystem does not declare
> to support the RFR_ "advisory" flags, it will not fail the operation
> 
> Comparing to FIEMAP_ flags, no filesystem would have needed to declare
> support for FIEMAP_FLAG_SYNC, because vfs dealt with it anyway
> before calling into the filesystem. So de-facto, any filesystem supports
> FIEMAP_FLAG_SYNC without doing anything, but it is still worth passing
> the flag into filesystem in case it matter (it does for overlayfs).

...but I think you have a good point that we could help filesystem
writers distinguish between advisory flags that are taken care of by the
VFS but passed to the fs for full disclosure; and mandatory flags that
the fs for which the fs must advertise support.

IOWs,

int remap_check_flags(unsigned int remap_flags, unsigned int supported_flags)
{
	/* VFS already took care of these */
	remap_flags &= ~(RFR_TO_EOF | RFR_CAN_SHORTEN);

	if (remap_flags & ~supported_flags) {
		WARN_ONCE(1, "Internal API misuse at %pS", __return_address);
		return -EINVAL;
	}

	return 0;
}

> I am not sure if giving fiemap_check_flags() as an example for sorting
> out VFS API flags is a good idea, because I completely don't understand
> what is going on in ext4_fiemap(). Why do FIEMAP_FLAG_CACHE and
> the case of  !EXT4_INODE_EXTENTS bypass fiemap_check_flags()?

generic_block_fiemap calls fiemap_check_flags.

> Is there a rational behind all this or just plain old API bit rot?
> If bit rot, then perhaps the fiemap_check_flags() wasn't a good enough
> coding pattern?

Hooooo... I don't even want to get into what FIEMAP_FLAG_CACHE does in
ext4.

--D

> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks
  2018-10-10  5:23   ` Amir Goldstein
@ 2018-10-10 17:01     ` Darrick J. Wong
  2018-10-10 17:26       ` Amir Goldstein
  0 siblings, 1 reply; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 17:01 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 08:23:27AM +0300, Amir Goldstein wrote:
> On Wed, Oct 10, 2018 at 3:11 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > File range remapping, if allowed to run past the destination file's EOF,
> > is an optimization on a regular file write.  Regular file writes that
> > extend the file length are subject to various constraints which are not
> > checked by range cloning.
> >
> > This is a correctness problem because we're never allowed to touch
> > ranges that the page cache can't support (s_maxbytes); we're not
> > supposed to deal with large offsets (MAX_NON_LFS) if O_LARGEFILE isn't
> > set; and we must obey resource limits (RLIMIT_FSIZE).
> >
> > Therefore, add these checks to the new generic_remap_checks function so
> > that we curtail unexpected behavior.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  mm/filemap.c |   39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 14041a8468ba..59056bd9c58a 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -2974,6 +2974,27 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
> >  }
> >  EXPORT_SYMBOL(generic_write_checks);
> >
> > +static int
> > +generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
> > +{
> > +       struct inode *inode = file->f_mapping->host;
> > +
> > +       /* Don't exceed the LFS limits. */
> > +       if (unlikely(pos + *count > MAX_NON_LFS &&
> > +                               !(file->f_flags & O_LARGEFILE))) {
> > +               if (pos >= MAX_NON_LFS)
> > +                       return -EFBIG;
> > +               *count = min(*count, MAX_NON_LFS - (uint64_t)pos);
> > +       }
> > +
> > +       /* Don't operate on ranges the page cache doesn't support. */
> > +       if (unlikely(pos >= inode->i_sb->s_maxbytes))
> > +               return -EFBIG;
> > +
> > +       *count = min(*count, inode->i_sb->s_maxbytes - (uint64_t)pos);
> > +       return 0;
> > +}
> > +
> 
> Sorry. I haven't explained myself properly last time.
> What I meant is that it hurts my eyes to see generic_write_checks() and
> generic_remap_check_limits() which from the line of (limit != RLIM_INFINITY)
> are exactly the same thing. Yes, generic_remap_check_limits() uses
> iov_iter_truncate(), but that's a minor semantic change - it can be easily
> resolved by creating a dummy iter in generic_remap_checks() instead of
> passing int *count.

Making a fake kiocb and iterator seem like a terribly fragile idea.

How about I make the common helper take a pos and *count, and
generic_write_checks can translate that into iov_iter_truncate?

> You could say that this is nit picking, but the very reason this patch
> set exists
> it because clone/dedup implementation did not use the same range checks
> of write to begin with, so it just seems wrong to diverge them at this point.
> 
> So to be clear, I suggest that generic_write_checks() should use your
> generic_remap_check_limits() helper.
> If you disagree and others can live with this minor duplication, fine by me.

Nah, I think I misunderstood you the first time, sorry about that.

--D

> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks
  2018-10-10 17:01     ` Darrick J. Wong
@ 2018-10-10 17:26       ` Amir Goldstein
  0 siblings, 0 replies; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10 17:26 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 8:01 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Oct 10, 2018 at 08:23:27AM +0300, Amir Goldstein wrote:
> > On Wed, Oct 10, 2018 at 3:11 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > File range remapping, if allowed to run past the destination file's EOF,
> > > is an optimization on a regular file write.  Regular file writes that
> > > extend the file length are subject to various constraints which are not
> > > checked by range cloning.
> > >
> > > This is a correctness problem because we're never allowed to touch
> > > ranges that the page cache can't support (s_maxbytes); we're not
> > > supposed to deal with large offsets (MAX_NON_LFS) if O_LARGEFILE isn't
> > > set; and we must obey resource limits (RLIMIT_FSIZE).
> > >
> > > Therefore, add these checks to the new generic_remap_checks function so
> > > that we curtail unexpected behavior.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  mm/filemap.c |   39 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > >
> > >
> > > diff --git a/mm/filemap.c b/mm/filemap.c
> > > index 14041a8468ba..59056bd9c58a 100644
> > > --- a/mm/filemap.c
> > > +++ b/mm/filemap.c
> > > @@ -2974,6 +2974,27 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from)
> > >  }
> > >  EXPORT_SYMBOL(generic_write_checks);
> > >
> > > +static int
> > > +generic_remap_check_limits(struct file *file, loff_t pos, uint64_t *count)
> > > +{
> > > +       struct inode *inode = file->f_mapping->host;
> > > +
> > > +       /* Don't exceed the LFS limits. */
> > > +       if (unlikely(pos + *count > MAX_NON_LFS &&
> > > +                               !(file->f_flags & O_LARGEFILE))) {
> > > +               if (pos >= MAX_NON_LFS)
> > > +                       return -EFBIG;
> > > +               *count = min(*count, MAX_NON_LFS - (uint64_t)pos);
> > > +       }
> > > +
> > > +       /* Don't operate on ranges the page cache doesn't support. */
> > > +       if (unlikely(pos >= inode->i_sb->s_maxbytes))
> > > +               return -EFBIG;
> > > +
> > > +       *count = min(*count, inode->i_sb->s_maxbytes - (uint64_t)pos);
> > > +       return 0;
> > > +}
> > > +
> >
> > Sorry. I haven't explained myself properly last time.
> > What I meant is that it hurts my eyes to see generic_write_checks() and
> > generic_remap_check_limits() which from the line of (limit != RLIM_INFINITY)
> > are exactly the same thing. Yes, generic_remap_check_limits() uses
> > iov_iter_truncate(), but that's a minor semantic change - it can be easily
> > resolved by creating a dummy iter in generic_remap_checks() instead of
> > passing int *count.
>
> Making a fake kiocb and iterator seem like a terribly fragile idea.
>
> How about I make the common helper take a pos and *count, and
> generic_write_checks can translate that into iov_iter_truncate?
>

Seems good to me.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 17/25] vfs: make remapping to source file eof more explicit
  2018-10-10 16:29     ` Darrick J. Wong
@ 2018-10-10 17:31       ` Amir Goldstein
  0 siblings, 0 replies; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10 17:31 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 7:29 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Oct 10, 2018 at 03:29:06PM +0300, Amir Goldstein wrote:
> > On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > Create a RFR_TO_SRC_EOF flag to explicitly declare that the caller wants
> > > the remap implementation to remap to the end of the source file, once
> > > the files are locked.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
[...]

> > > + * RFR_TO_SRC_EOF: remap to the end of the source file
> > >   */
> > >  #define RFR_IDENTICAL_DATA     (1 << 0)
> > > +#define RFR_TO_SRC_EOF         (1 << 1)
> > >
> >
> > So what is the best way to make sure that all filesystems can
> > properly handle this flag? and the RFR_CAN_SHORTEN flag?
> >
> > The way that your patches took is to not check for invalid flags
> > at all in filesystems, but I don't think that is a viable option.
>
> The RFR flags are internal APIs, so we don't need to be quite as strict
> as fiemap does...
>

That's true.

> > Another way would be to individually add those flags to invalid
> > flags check in all relevant filesystems.
> >
> > Another way would be to follow a pattern similar to
> > fiemap_check_flags(), except in case filesystem does not declare
> > to support the RFR_ "advisory" flags, it will not fail the operation
> >
> > Comparing to FIEMAP_ flags, no filesystem would have needed to declare
> > support for FIEMAP_FLAG_SYNC, because vfs dealt with it anyway
> > before calling into the filesystem. So de-facto, any filesystem supports
> > FIEMAP_FLAG_SYNC without doing anything, but it is still worth passing
> > the flag into filesystem in case it matter (it does for overlayfs).
>
> ...but I think you have a good point that we could help filesystem
> writers distinguish between advisory flags that are taken care of by the
> VFS but passed to the fs for full disclosure; and mandatory flags that
> the fs for which the fs must advertise support.
>
> IOWs,
>
> int remap_check_flags(unsigned int remap_flags, unsigned int supported_flags)
> {
>         /* VFS already took care of these */
>         remap_flags &= ~(RFR_TO_EOF | RFR_CAN_SHORTEN);
>
>         if (remap_flags & ~supported_flags) {
>                 WARN_ONCE(1, "Internal API misuse at %pS", __return_address);
>                 return -EINVAL;
>         }
>
>         return 0;
> }
>

With that in place, you can add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

on the vfs patches.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed
  2018-10-10 15:50     ` Darrick J. Wong
@ 2018-10-10 18:28       ` Amir Goldstein
  2018-10-10 18:32         ` Darrick J. Wong
  0 siblings, 1 reply; 47+ messages in thread
From: Amir Goldstein @ 2018-10-10 18:28 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 6:51 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Oct 10, 2018 at 09:47:00AM +0300, Amir Goldstein wrote:
> > On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > Change the remap_file_range functions to take a number of bytes to
> > > operate upon and return the number of bytes they operated on.  This is a
> > > requirement for allowing fs implementations to return short clone/dedupe
> > > results to the user, which will enable us to obey resource limits in a
> > > graceful manner.
> > >
> > > A subsequent patch will enable copy_file_range to signal to the
> > > ->clone_file_range implementation that it can handle a short length,
> > > which will be returned in the function's return value.  Neither clone
> > > ioctl can take advantage of this, alas.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
[...]
> > Commit message wasn't clear enough on the behavior of copy_file_range()
> > before and after the patch IMO. Maybe it would be better to pospone this
> > semantic change to the RFR_SHORTEN patch and keep if (cloned == len)
> > in this patch?
>
> There shouldn't be any behavior change here -- all implementations
> return a negative error code or the length that was passed in.  I'll
> clarify that in the commit message.
>

OK. BTW, you forgot to update Documentation/filesystems/vfs.txt.

Also since this series has a potential to break clone/dedup on
overlayfs, it would be great if you could run some of the clone/dedupe
xfstests with overlay over xfs.

For the simple case of running ./check with a local.config file that is
not multi section, this just means running ./check -overlay after the
first ./check run (-overlay doesn't mkfs the base fs).

If this is a problem, let me know once new devel branch is ready
and I'll pull it for testing. If I need to pull in extra xfstests, please
mention that as well.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed
  2018-10-10 18:28       ` Amir Goldstein
@ 2018-10-10 18:32         ` Darrick J. Wong
  0 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 18:32 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Dave Chinner, Eric Sandeen, Linux NFS Mailing List, linux-cifs,
	overlayfs, linux-xfs, Linux MM, Linux Btrfs, linux-fsdevel,
	ocfs2-devel

On Wed, Oct 10, 2018 at 09:28:34PM +0300, Amir Goldstein wrote:
> On Wed, Oct 10, 2018 at 6:51 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > On Wed, Oct 10, 2018 at 09:47:00AM +0300, Amir Goldstein wrote:
> > > On Wed, Oct 10, 2018 at 3:14 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > > >
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > >
> > > > Change the remap_file_range functions to take a number of bytes to
> > > > operate upon and return the number of bytes they operated on.  This is a
> > > > requirement for allowing fs implementations to return short clone/dedupe
> > > > results to the user, which will enable us to obey resource limits in a
> > > > graceful manner.
> > > >
> > > > A subsequent patch will enable copy_file_range to signal to the
> > > > ->clone_file_range implementation that it can handle a short length,
> > > > which will be returned in the function's return value.  Neither clone
> > > > ioctl can take advantage of this, alas.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> [...]
> > > Commit message wasn't clear enough on the behavior of copy_file_range()
> > > before and after the patch IMO. Maybe it would be better to pospone this
> > > semantic change to the RFR_SHORTEN patch and keep if (cloned == len)
> > > in this patch?
> >
> > There shouldn't be any behavior change here -- all implementations
> > return a negative error code or the length that was passed in.  I'll
> > clarify that in the commit message.
> >
> 
> OK. BTW, you forgot to update Documentation/filesystems/vfs.txt.

Yeah, I noticed that and updated it too.

> Also since this series has a potential to break clone/dedup on
> overlayfs, it would be great if you could run some of the clone/dedupe
> xfstests with overlay over xfs.
> 
> For the simple case of running ./check with a local.config file that is
> not multi section, this just means running ./check -overlay after the
> first ./check run (-overlay doesn't mkfs the base fs).

I'll give it a try, though we should probably both run '-g clone' just
to make sure everything is working...

> If this is a problem, let me know once new devel branch is ready

Yeah, Dave asked me to merge the xfs for-next branch so I'm working on
that too.

> and I'll pull it for testing. If I need to pull in extra xfstests, please
> mention that as well.

You'll probably want "generic: test creation time recovery after power
failure" that I sent to the fstests lists a few days ago.

--D

> 
> Thanks,
> Amir.

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 05/25] vfs: check file ranges before cloning files
  2018-10-10  0:11 ` [PATCH 05/25] vfs: check file ranges " Darrick J. Wong
@ 2018-10-10 23:06   ` Dave Chinner
  2018-10-10 23:13     ` Darrick J. Wong
  0 siblings, 1 reply; 47+ messages in thread
From: Dave Chinner @ 2018-10-10 23:06 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, Christoph Hellwig,
	ocfs2-devel

On Tue, Oct 09, 2018 at 05:11:13PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the file range checks from vfs_clone_file_prep into a separate
> generic_remap_checks function so that all the checks are collected in a
> central location.  This forms the basis for adding more checks from
> generic_write_checks that will make cloning's input checking more
> consistent with write input checking.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
....
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1717,12 +1717,12 @@ static int clone_verify_area(struct file *file, loff_t pos, u64 len, bool write)
>   * Returns: 0 for "nothing to clone", 1 for "something to clone", or
>   * the usual negative error code.
>   */
> -int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
> -			       struct inode *inode_out, loff_t pos_out,
> -			       u64 *len, bool is_dedupe)
> +int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
> +			struct file *file_out, loff_t pos_out,
> +			u64 *len, bool is_dedupe)
>  {
> -	loff_t bs = inode_out->i_sb->s_blocksize;
> -	loff_t blen;
> +	struct inode *inode_in = file_inode(file_in);
> +	struct inode *inode_out = file_inode(file_out);
>  	loff_t isize;
>  	bool same_inode = (inode_in == inode_out);
>  	int ret;
> @@ -1740,10 +1740,7 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
>  	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
>  		return -EINVAL;
>  
> -	/* Are we going all the way to the end? */
>  	isize = i_size_read(inode_in);
> -	if (isize == 0)
> -		return 0;

This looks like a change of behaviour. Instead of skipping zero
legnth source files and returning success, this will now return
-EINVAL as other checks fail? That needs to be documented in the
commit message if it's intentional and a valid change to make...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 47+ messages in thread

* Re: [PATCH 05/25] vfs: check file ranges before cloning files
  2018-10-10 23:06   ` Dave Chinner
@ 2018-10-10 23:13     ` Darrick J. Wong
  0 siblings, 0 replies; 47+ messages in thread
From: Darrick J. Wong @ 2018-10-10 23:13 UTC (permalink / raw)
  To: Dave Chinner
  Cc: sandeen, linux-nfs, linux-cifs, linux-unionfs, linux-xfs,
	linux-mm, linux-btrfs, linux-fsdevel, Christoph Hellwig,
	ocfs2-devel

On Thu, Oct 11, 2018 at 10:06:39AM +1100, Dave Chinner wrote:
> On Tue, Oct 09, 2018 at 05:11:13PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move the file range checks from vfs_clone_file_prep into a separate
> > generic_remap_checks function so that all the checks are collected in a
> > central location.  This forms the basis for adding more checks from
> > generic_write_checks that will make cloning's input checking more
> > consistent with write input checking.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> ....
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1717,12 +1717,12 @@ static int clone_verify_area(struct file *file, loff_t pos, u64 len, bool write)
> >   * Returns: 0 for "nothing to clone", 1 for "something to clone", or
> >   * the usual negative error code.
> >   */
> > -int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
> > -			       struct inode *inode_out, loff_t pos_out,
> > -			       u64 *len, bool is_dedupe)
> > +int vfs_clone_file_prep(struct file *file_in, loff_t pos_in,
> > +			struct file *file_out, loff_t pos_out,
> > +			u64 *len, bool is_dedupe)
> >  {
> > -	loff_t bs = inode_out->i_sb->s_blocksize;
> > -	loff_t blen;
> > +	struct inode *inode_in = file_inode(file_in);
> > +	struct inode *inode_out = file_inode(file_out);
> >  	loff_t isize;
> >  	bool same_inode = (inode_in == inode_out);
> >  	int ret;
> > @@ -1740,10 +1740,7 @@ int vfs_clone_file_prep_inodes(struct inode *inode_in, loff_t pos_in,
> >  	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
> >  		return -EINVAL;
> >  
> > -	/* Are we going all the way to the end? */
> >  	isize = i_size_read(inode_in);
> > -	if (isize == 0)
> > -		return 0;
> 
> This looks like a change of behaviour. Instead of skipping zero
> legnth source files and returning success, this will now return
> -EINVAL as other checks fail? That needs to be documented in the
> commit message if it's intentional and a valid change to make...

Meh, I'll make another patch.  btrfs has never had this behavior.

$ ls /mnt/a
-rw-r--r-- 1 root root 0 Oct 10 16:10 /mnt/a
$ xfs_io -f -c 'reflink /mnt/a 1000000 0 4096' /mnt/b
XFS_IOC_CLONE_RANGE: Invalid argument

So it's a bug in the vfs prep functions.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

^ permalink raw reply	[flat|nested] 47+ messages in thread

end of thread, other threads:[~2018-10-10 23:13 UTC | newest]

Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10  0:10 [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Darrick J. Wong
2018-10-10  0:10 ` [PATCH 01/25] xfs: add a per-xfs trace_printk macro Darrick J. Wong
2018-10-10  0:36   ` Dave Chinner
2018-10-10 15:00   ` [PATCH v2 " Darrick J. Wong
2018-10-10  0:10 ` [PATCH 02/25] xfs: refactor clonerange preparation into a separate helper Darrick J. Wong
2018-10-10  0:10 ` [PATCH 03/25] xfs: zero posteof blocks when cloning above eof Darrick J. Wong
2018-10-10  0:11 ` [PATCH 04/25] xfs: update ctime and remove suid before cloning files Darrick J. Wong
2018-10-10  0:11 ` [PATCH 05/25] vfs: check file ranges " Darrick J. Wong
2018-10-10 23:06   ` Dave Chinner
2018-10-10 23:13     ` Darrick J. Wong
2018-10-10  0:11 ` [PATCH 06/25] vfs: strengthen checking of file range inputs to generic_remap_checks Darrick J. Wong
2018-10-10  5:23   ` Amir Goldstein
2018-10-10 17:01     ` Darrick J. Wong
2018-10-10 17:26       ` Amir Goldstein
2018-10-10  0:11 ` [PATCH 07/25] vfs: skip zero-length dedupe requests Darrick J. Wong
2018-10-10  0:11 ` [PATCH 08/25] vfs: combine the clone and dedupe into a single remap_file_range Darrick J. Wong
2018-10-10  5:54   ` Amir Goldstein
2018-10-10 15:13     ` Darrick J. Wong
2018-10-10 15:23       ` Amir Goldstein
2018-10-10  0:11 ` [PATCH 09/25] vfs: rename vfs_clone_file_prep to be more descriptive Darrick J. Wong
2018-10-10  0:11 ` [PATCH 10/25] vfs: rename clone_verify_area to remap_verify_area Darrick J. Wong
2018-10-10  0:13 ` [PATCH 11/25] vfs: create generic_remap_file_range_touch to update inode metadata Darrick J. Wong
2018-10-10  0:13 ` [PATCH 12/25] vfs: pass remap flags to generic_remap_file_range_prep Darrick J. Wong
2018-10-10  0:13 ` [PATCH 13/25] vfs: pass remap flags to generic_remap_checks Darrick J. Wong
2018-10-10  0:13 ` [PATCH 14/25] vfs: make remap_file_range functions take and return bytes completed Darrick J. Wong
2018-10-10  6:47   ` Amir Goldstein
2018-10-10 15:50     ` Darrick J. Wong
2018-10-10 18:28       ` Amir Goldstein
2018-10-10 18:32         ` Darrick J. Wong
2018-10-10  0:13 ` [PATCH 15/25] vfs: plumb RFR_* remap flags through the vfs clone functions Darrick J. Wong
2018-10-10  6:22   ` Amir Goldstein
2018-10-10  6:39     ` Amir Goldstein
2018-10-10  0:13 ` [PATCH 16/25] vfs: plumb RFR_* remap flags through the vfs dedupe functions Darrick J. Wong
2018-10-10  0:13 ` [PATCH 17/25] vfs: make remapping to source file eof more explicit Darrick J. Wong
2018-10-10 12:29   ` Amir Goldstein
2018-10-10 16:29     ` Darrick J. Wong
2018-10-10 17:31       ` Amir Goldstein
2018-10-10  0:14 ` [PATCH 18/25] vfs: enable remap callers that can handle short operations Darrick J. Wong
2018-10-10  0:14 ` [PATCH 19/25] vfs: hide file range comparison function Darrick J. Wong
2018-10-10  0:14 ` [PATCH 20/25] vfs: implement opportunistic short dedupe Darrick J. Wong
2018-10-10  0:14 ` [PATCH 21/25] ocfs2: truncate page cache for clone destination file before remapping Darrick J. Wong
2018-10-10  0:14 ` [PATCH 22/25] ocfs2: fix pagecache truncation prior to reflink Darrick J. Wong
2018-10-10  0:14 ` [PATCH 23/25] ocfs2: support partial clone range and dedupe range Darrick J. Wong
2018-10-10  0:14 ` [PATCH 24/25] xfs: fix pagecache truncation prior to reflink Darrick J. Wong
2018-10-10  0:14 ` [PATCH 25/25] xfs: support returning partial reflink results Darrick J. Wong
2018-10-10  1:02 ` [PATCH v2 00/25] fs: fixes for serious clone/dedupe problems Dave Chinner
2018-10-10  1:06   ` Darrick J. Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).