: dedupe/copy_file_range fixes

From: Dave Chinner <[email protected]>

It returns EINVAL when the operation is not supported by the filesystem. Fix it to return EOPNOTSUPP to be consistent with the man page and clone_file_range().

Clean up the inconsistent error return handling while I'm there. (I know, lipstick on a pig, but every little bit helps...)

Signed-off-by Dave Chinner <[email protected]>

---
 fs/read_write.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

fs/read_write.c +7 -8

index bfcb4ced5664..aa43224bcec6 100644
@@ -2094,17 +2094,18 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	off = same->src_offset;
 	len = same->src_length;
 
-	ret = -EISDIR;
+	if (!file->f_op->remap_file_range)
+		return -EOPNOTSUPP;
+
 	if (S_ISDIR(src->i_mode))
-		goto out;
+		return -EISDIR;
 
-	ret = -EINVAL;
 	if (!S_ISREG(src->i_mode))
-		goto out;
+		return -EINVAL;
 
 	ret = remap_verify_area(file, off, len, false);
 	if (ret < 0)
-		goto out;
+		return ret;
 	ret = 0;
 
 	if (off + len > i_size_read(src))

@@ -2147,10 +2148,8 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 		fdput(dst_fd);
 next_loop:
 		if (fatal_signal_pending(current))
-			goto out;
+			break;
 	}
-
-out:
 	return ret;
 }
 EXPORT_SYMBOL(vfs_dedupe_file_range);

Signature · 1 line

2.19.1

Quoted 10 lines · Dave Chinner

From: Dave Chinner <[email protected]>

It returns EINVAL when the operation is not supported by the filesystem. Fix it to return EOPNOTSUPP to be consistent with the man page and clone_file_range().

Clean up the inconsistent error return handling while I'm there. (I know, lipstick on a pig, but every little bit helps...)

Signed-off-by Dave Chinner <[email protected]>

Looks ok, would rather just shred this ioctl and make a better one. :P

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 12 lines

---
 fs/read_write.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

fs/read_write.c +0 -0

index bfcb4ced5664..aa43224bcec6 100644
@@ -2094,17 +2094,18 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	off = same->src_offset;
 	len = same->src_length;
 

fs/read_write.c int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) · line 2098

Quoted 3 lines

fs/read_write.c +2 -1

-	ret = -EISDIR;
+	if (!file->f_op->remap_file_range)
+		return -EOPNOTSUPP;

Quoted 32 lines

fs/read_write.c +5 -7

+
 	if (S_ISDIR(src->i_mode))
-		goto out;
+		return -EISDIR;
 
-	ret = -EINVAL;
 	if (!S_ISREG(src->i_mode))
-		goto out;
+		return -EINVAL;
 
 	ret = remap_verify_area(file, off, len, false);
 	if (ret < 0)
-		goto out;
+		return ret;
 	ret = 0;
 
 	if (off + len > i_size_read(src))

@@ -2147,10 +2148,8 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 		fdput(dst_fd);
 next_loop:
 		if (fatal_signal_pending(current))
-			goto out;
+			break;
 	}
-
-out:
 	return ret;
 }
 EXPORT_SYMBOL(vfs_dedupe_file_range);

Signature · 1 line

2.19.1

Quoted 30 lines · Darrick J. Wong

Quoted 10 lines · Dave Chinner

From: Dave Chinner <[email protected]>

It returns EINVAL when the operation is not supported by the filesystem. Fix it to return EOPNOTSUPP to be consistent with the man page and clone_file_range().

Clean up the inconsistent error return handling while I'm there. (I know, lipstick on a pig, but every little bit helps...)

Signed-off-by Dave Chinner <[email protected]>

Looks ok, would rather just shred this ioctl and make a better one. :P

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 12 lines

---
 fs/read_write.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

fs/read_write.c +0 -0

index bfcb4ced5664..aa43224bcec6 100644
@@ -2094,17 +2094,18 @@ int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same)
 	off = same->src_offset;
 	len = same->src_length;
 

Quoted 3 lines

fs/read_write.c int vfs_dedupe_file_range(struct file *file, struct file_dedupe_range *same) · line 2098

Quoted 3 lines

fs/read_write.c +2 -1

-	ret = -EISDIR;
+	if (!file->f_op->remap_file_range)
+		return -EOPNOTSUPP;

Minor downside: this causes regressions in generic/158 because directories and device files don't have a ->remap_file_range implementation. I think it can be solved by moving this check after the !S_ISREG check below.

From: Dave Chinner <[email protected]>

When doing direct IO to a pipe for do_splice_direct(), then pipe is trivial to fill up and overflow as it can only hold 16 pages. At this point bio_iov_iter_get_pages() then returns -EFAULT, and we abort the IO submission process. Unfortunately, iomap_dio_rw() propagates the error back up the stack.

The error is converted from the EFAULT to EAGAIN in generic_file_splice_read() to tell the splice layers that the pipe is full. do_splice_direct() completely fails to handle EAGAIN errors (it aborts on error) and returns EAGAIN to the caller.

copy_file_write() then compeltely fails to handle EAGAIN as well,

and so returns EAGAIN to userspace, having failed to copy the data it was asked to.

Avoid this whole steaming pile of fail by having iomap_dio_rw() silently swallow EFAULT errors and so do short reads.

To make matters worse, iomap_dio_actor() has a stale data exposure bug bio_iov_iter_get_pages() fails - it does not zero the tail block that it may have been left uncovered by partial IO. Fix the error handling case to drop to the sub-block zeroing rather than immmediately returning the -EFAULT error.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/iomap.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

fs/iomap.c +19 -3

index 7aacd48c593e..2fda13bc37d8 100644
@@ -1775,7 +1775,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	struct bio *bio;
 	bool need_zeroout = false;
 	bool use_fua = false;
-	int nr_pages, ret;
+	int nr_pages, ret = 0;
 	size_t copied = 0;
 
 	if ((pos | length | align) & ((1 << blkbits) - 1))

@@ -1839,8 +1839,14 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 
 		ret = bio_iov_iter_get_pages(bio, &iter);
 		if (unlikely(ret)) {
+			/*
+			 * We have to stop part way through an IO. We must fall through
+			 * to the sub-block tail zeroing here, otherwise this
+			 * short IO may expose stale data in the tail of the
+			 * block we haven't written data to.
+			 */
 			bio_put(bio);
-			return copied ? copied : ret;
+			goto zero_tail;
 		}
 
 		n = bio->bi_iter.bi_size;

@@ -1877,6 +1883,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	 * the block tail in the latter case, we can expose stale data via mmap
 	 * reads of the EOF block.
 	 */
+zero_tail:
 	if (need_zeroout ||
 	    ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) {
 		/* zero out from the end of the write to the end of the block */

@@ -1884,7 +1891,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		if (pad)
 			iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
 	}
-	return copied;
+	return copied ? copied : ret;
 }
 
 static loff_t

@@ -2079,6 +2086,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 				dio->wait_for_completion = true;
 				ret = 0;
 			}
+
+			/*
+			 * splicing to pipes can fail on a full pipe. We have to
+			 * swallow this to make it look like a short IO
+			 * otherwise the higher splice layers will completely
+			 * mishandle the error and stop moving data.
+			 */
+			if (ret == -EFAULT)
+				ret = 0;
 			break;
 		}
 		pos += ret;

Signature · 1 line

2.19.1

From: Dave Chinner <[email protected]>

When doing direct IO to a pipe for do_splice_direct(), then pipe is trivial to fill up and overflow as it can only hold 16 pages. At this point bio_iov_iter_get_pages() then returns -EFAULT, and we abort the IO submission process. Unfortunately, iomap_dio_rw() propagates the error back up the stack.

The error is converted from the EFAULT to EAGAIN in generic_file_splice_read() to tell the splice layers that the pipe is full. do_splice_direct() completely fails to handle EAGAIN errors (it aborts on error) and returns EAGAIN to the caller.

copy_file_write() then compeltely fails to handle EAGAIN as well, and so returns EAGAIN to userspace, having failed to copy the data it was asked to.

Avoid this whole steaming pile of fail by having iomap_dio_rw() silently swallow EFAULT errors and so do short reads.

To make matters worse, iomap_dio_actor() has a stale data exposure bug bio_iov_iter_get_pages() fails - it does not zero the tail block that it may have been left uncovered by partial IO. Fix the error handling case to drop to the sub-block zeroing rather than immmediately returning the -EFAULT error.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/iomap.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

fs/iomap.c +1 -1

index 7aacd48c593e..2fda13bc37d8 100644
@@ -1775,7 +1775,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	struct bio *bio;
 	bool need_zeroout = false;
 	bool use_fua = false;
-	int nr_pages, ret;
+	int nr_pages, ret = 0;
 	size_t copied = 0;
 
 	if ((pos | length | align) & ((1 << blkbits) - 1))

fs/iomap.c +7 -2

+			 * to the sub-block tail zeroing here, otherwise this
+			 * short IO may expose stale data in the tail of the
+			 * block we haven't written data to.
+			 */
 			bio_put(bio);
-			return copied ? copied : ret;
+			goto zero_tail;
 		}
 
 		n = bio->bi_iter.bi_size;

@@ -1877,6 +1883,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 	 * the block tail in the latter case, we can expose stale data via mmap
 	 * reads of the EOF block.
 	 */
+zero_tail:
 	if (need_zeroout ||
 	    ((dio->flags & IOMAP_DIO_WRITE) && pos >= i_size_read(inode))) {
 		/* zero out from the end of the write to the end of the block */

@@ -1884,7 +1891,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		if (pad)
 			iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
 	}
-	return copied;
+	return copied ? copied : ret;
 }
 
 static loff_t

From: Dave Chinner <[email protected]>

When copy_file_range() is called on files that have been opened with O_DIRECT, do_splice_direct() does a manual copy of the range one pipe buffer at a time. The default is 16 pages, which means on x86_64 it is limited to 64kB IO. This is extremely slow - 64k synchrnous read/write will run at maybe 5-10MB/s on a spinning disk and be seek bound. It will be faster on SSDs, but still very inefficient.

Increase the pipe size to the maximum allowed user size so that we can get decent throughput for this highly sub-optimal copy loop. Add a new function to the pipe code that lets us set the pipe size to the maximum allowed without root permissions to keep things really simple. We also don't care if changing the pipe size fails - that will just result in a slower copy.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/pipe.c                 | 10 ++++++++++
 fs/splice.c               |  7 +++++++
 include/linux/pipe_fs_i.h |  1 +
 3 files changed, 18 insertions(+)

fs/pipe.c +10 -0

index bdc5d3c0977d..436bc0464569 100644
@@ -1109,6 +1109,16 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	return ret;
 }
 
+/*
+ * Set the pipe to the maximum allowable user size. Advisory only, will
+ * swallow any errors and return the resultant pipe size.
+ */
+long pipe_set_max_safe_size(struct pipe_inode_info *pipe)
+{
+	pipe_set_size(pipe, pipe_max_size);
+	return pipe->buffers * PAGE_SIZE;
+}
+
 /*
  * After the inode slimming patch, i_pipe/i_bdev/i_cdev share the same
  * location, so checking ->i_pipe is not enough to verify that this is a

fs/splice.c +0 -0

index 3553f1956508..9749139da731 100644

fs/splice.c +1 -0

+
 	/*
 	 * Do the splice.
 	 */

include/linux/pipe_fs_i.h +1 -0

index 5a3bb3b7c9ad..962ba4cfcb74 100644
@@ -191,5 +191,6 @@ struct pipe_inode_info *get_pipe_info(struct file *file);
 
 int create_pipe_files(struct file **, int);
 unsigned int round_pipe_size(unsigned long size);
+long pipe_set_max_safe_size(struct pipe_inode_info *pipe);
 
 #endif

Quoted 51 lines · Dave Chinner

From: Dave Chinner <[email protected]>

When copy_file_range() is called on files that have been opened with O_DIRECT, do_splice_direct() does a manual copy of the range one pipe buffer at a time. The default is 16 pages, which means on x86_64 it is limited to 64kB IO. This is extremely slow - 64k synchrnous read/write will run at maybe 5-10MB/s on a spinning disk and be seek bound. It will be faster on SSDs, but still very inefficient.

Increase the pipe size to the maximum allowed user size so that we can get decent throughput for this highly sub-optimal copy loop. Add a new function to the pipe code that lets us set the pipe size to the maximum allowed without root permissions to keep things really simple. We also don't care if changing the pipe size fails - that will just result in a slower copy.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/pipe.c                 | 10 ++++++++++
 fs/splice.c               |  7 +++++++
 include/linux/pipe_fs_i.h |  1 +
 3 files changed, 18 insertions(+)

fs/pipe.c +10 -0

index bdc5d3c0977d..436bc0464569 100644
@@ -1109,6 +1109,16 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg)
 	return ret;
 }
 
+/*
+ * Set the pipe to the maximum allowable user size. Advisory only, will
+ * swallow any errors and return the resultant pipe size.
+ */
+long pipe_set_max_safe_size(struct pipe_inode_info *pipe)
+{
+	pipe_set_size(pipe, pipe_max_size);
+	return pipe->buffers * PAGE_SIZE;
+}
+
 /*
  * After the inode slimming patch, i_pipe/i_bdev/i_cdev share the same
  * location, so checking ->i_pipe is not enough to verify that this is a

fs/splice.c +0 -0

index 3553f1956508..9749139da731 100644

fs/splice.c ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd, · line 939

Quoted 9 lines

fs/splice.c +6 -0

@@ -931,6 +931,13 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
 		current->splice_pipe = pipe;
 	}
 
+	/*
+	 * Try to increase the data holding capacity of the pipe so we can do
+	 * larger IOs. This may not increase the size at all because maximum
+	 * user pipe size is administrator controlled, but we still should try.
+	 */
+	pipe_set_max_safe_size(pipe);

I get where you're going with this, but I have two questions:

- Is it safe to be enlarging the pipe buffer size unconditionally?

- Especially if we didn't just create the splice pipe? Suppose someone comes along later trying to splice things and doesn't realize the pipe is now 1MB...

Then I started wondering about the splice_pipe lifetime and couldn't figure out if it ever gets detached from current prior to do_exit. I don't think it does, which means that we're stuck with the 1MB kernel memory allocation until the process dies.

fs/pipe.c static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long arg) · line 1120

Quoted 5 lines

fs/pipe.c +5 -0

+long pipe_set_max_safe_size(struct pipe_inode_info *pipe)
+{
+	pipe_set_size(pipe, pipe_max_size);
+	return pipe->buffers * PAGE_SIZE;
+}

This should probably return an unsigned value, given that we don't return errors. Then again the callers ignores the return value entirely. Wouldn't it be easier to just call pipe_set_size from splice.c after removing the static marker?