[PATCH 0/3] Holey splice! copy_file_range() with holes

[PATCH 0/3] Holey splice! copy_file_range() with holes

[Goldwyn Rodrigues]

If copy_file_range performs a copy using splice, it converts holes
to zeros. This effort primarily changes this behavior to create
holes when it is possible. I am not sure if we should put it under
a flag, but I think this should be the default behavior as opposed
to converting holes to allocated zeros. We can use a flag to instruct
copy_file_range() to convert holes to allocated zeros. Let me know..

We should be able to splice files if they do not belong the same
super_block.

overlay copy_up can benefit from this.

This could make it ready for coreutils/cp to use copy_file_range(),
primarily to deal with --sparse=WHEN option.

-- 
Goldwyn

[PATCH 1/3] Perform splice in copy_file_range if in/out SB are not same

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

While performing copy_file_range(), if superblocks of file_in and
file_out don't match, instead of returning -EXDEV, perform
splice for a faster copy.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/read_write.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index c4eabbfc90df..e71270033402 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1575,15 +1575,14 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	    (file_out->f_flags & O_APPEND))
 		return -EBADF;
 
-	/* this could be relaxed once a method supports cross-fs copies */
-	if (inode_in->i_sb != inode_out->i_sb)
-		return -EXDEV;
-
 	if (len == 0)
 		return 0;
 
 	file_start_write(file_out);
 
+	if (inode_in->i_sb != inode_out->i_sb)
+		goto do_splice;
+
 	/*
 	 * Try cloning first, this is supported by more file systems, and
 	 * more efficient if both clone and copy are supported (e.g. NFS).
@@ -1604,6 +1603,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 			goto done;
 	}
 
+do_splice:
 	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
 			len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
 
-- 
2.16.3

[PATCH 2/3] copy_file_range: splice with holes

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

copy_file_range calls do_splice_direct() if fs->clone_file_range
or fs->copy_file_range() is not available. However, do_splice_direct()
converts holes to zeros. Detect holes in the file_in range, and
create them in the corresponding file_out range.

If there is already data present at the offset in file_out, attempt
to punch a hole there. If the operation is not supported, fall
back to performing splice on the whole range.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/read_write.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index e71270033402..6866e2a27594 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -20,6 +20,7 @@
 #include <linux/compat.h>
 #include <linux/mount.h>
 #include <linux/fs.h>
+#include <linux/falloc.h>
 #include "internal.h"
 
 #include <linux/uaccess.h>
@@ -1541,6 +1542,63 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
 }
 #endif
 
+
+static ssize_t splice_with_holes(struct file *file_in, loff_t *pos_in,
+		struct file *file_out, loff_t *pos_out,	size_t len)
+{
+	ssize_t ret = 0, total = 0;
+	loff_t size, end;
+
+	while (total < len) {
+		end = vfs_llseek(file_in, *pos_in, SEEK_HOLE);
+		if (end == *pos_in)
+			goto hole;
+		size = end - *pos_in;
+splice:
+		ret = do_splice_direct(file_in, pos_in, file_out, pos_out,
+				size, 0);
+		if (ret < 0)
+			goto out;
+		total += ret;
+		if (total == len)
+			break;
+hole:
+		end = vfs_llseek(file_in, *pos_in, SEEK_DATA);
+		if (end < 0) {
+			ret = end;
+			goto out;
+		}
+		size = end - *pos_in;
+		/* For files already containing data, punch holes */
+		if (i_size_read(file_out->f_inode) > *pos_out) {
+			ret = vfs_fallocate(file_out,
+					FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+					*pos_out, size);
+			if (ret < 0) {
+				/*
+				 * The filesystem does not support punching
+				 * holes. Perform splice on the entire range.
+				 */
+				if (ret == -EOPNOTSUPP) {
+					size = len - total;
+					goto splice;
+				}
+				goto out;
+			}
+		}
+		if (ret < 0) {
+			ret = end;
+			goto out;
+		}
+		*pos_out += size;
+		*pos_in = end;
+		total += size;
+	}
+
+out:
+	return total ? total : ret;
+}
+
 /*
  * copy_file_range() differs from regular file read and write in that it
  * specifically allows return partial success.  When it does so is up to
@@ -1604,8 +1662,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	}
 
 do_splice:
-	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
-			len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
+	ret = splice_with_holes(file_in, &pos_in, file_out, &pos_out,
+			len > MAX_RW_COUNT ? MAX_RW_COUNT : len);
 
 done:
 	if (ret > 0) {
-- 
2.16.3

[PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/overlayfs/copy_up.c |  2 +-
 fs/read_write.c        | 10 ++++++----
 include/linux/fs.h     |  2 ++
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8bede0742619..6634a85255ae 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 			break;
 		}
 
-		bytes = do_splice_direct(old_file, &old_pos,
+		bytes = splice_with_holes(old_file, &old_pos,
 					 new_file, &new_pos,
 					 this_len, SPLICE_F_MOVE);
 		if (bytes <= 0) {
diff --git a/fs/read_write.c b/fs/read_write.c
index 6866e2a27594..0aa8390d4d39 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1543,8 +1543,9 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
 #endif
 
 
-static ssize_t splice_with_holes(struct file *file_in, loff_t *pos_in,
-		struct file *file_out, loff_t *pos_out,	size_t len)
+ssize_t splice_with_holes(struct file *file_in, loff_t *pos_in,
+		struct file *file_out, loff_t *pos_out,	size_t len,
+		unsigned int flags)
 {
 	ssize_t ret = 0, total = 0;
 	loff_t size, end;
@@ -1556,7 +1557,7 @@ static ssize_t splice_with_holes(struct file *file_in, loff_t *pos_in,
 		size = end - *pos_in;
 splice:
 		ret = do_splice_direct(file_in, pos_in, file_out, pos_out,
-				size, 0);
+				size, flags);
 		if (ret < 0)
 			goto out;
 		total += ret;
@@ -1598,6 +1599,7 @@ static ssize_t splice_with_holes(struct file *file_in, loff_t *pos_in,
 out:
 	return total ? total : ret;
 }
+EXPORT_SYMBOL(splice_with_holes);
 
 /*
  * copy_file_range() differs from regular file read and write in that it
@@ -1663,7 +1665,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 
 do_splice:
 	ret = splice_with_holes(file_in, &pos_in, file_out, &pos_out,
-			len > MAX_RW_COUNT ? MAX_RW_COUNT : len);
+			len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
 
 done:
 	if (ret > 0) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..8f96400ab944 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1799,6 +1799,8 @@ extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
 extern ssize_t vfs_write(struct file *, const char __user *, size_t, loff_t *);
 extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *, rwf_t);
+extern ssize_t splice_with_holes(struct file *, loff_t *, struct file *,
+				   loff_t *, size_t, unsigned int);
 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,
-- 
2.16.3

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Amir Goldstein]

On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/overlayfs/copy_up.c |  2 +-
>  fs/read_write.c        | 10 ++++++----
>  include/linux/fs.h     |  2 ++
>  3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8bede0742619..6634a85255ae 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>                         break;
>                 }
>
> -               bytes = do_splice_direct(old_file, &old_pos,
> +               bytes = splice_with_holes(old_file, &old_pos,
>                                          new_file, &new_pos,
>                                          this_len, SPLICE_F_MOVE);


Add.. you can remove this comment above :)
        /* FIXME: copy up sparse files efficiently */

For the record, when I added vfs_clone_file_range() above,
Dave Chinner has suggested to replace the entire block with
vfs_copy_file_range(), which would do all the fallbacks.
Since then, vfs_copy_file_range() gained "try to clone first".

There are still differences between the loop in this function and
the loop in vfs_copy_file_range(). Perhaps the differences could
be smoothed away, I did not check recently.

I am not asking that you do this as part of your work, simply
pointing out an opportunity.

Thanks,
Amir.

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Dave Chinner]

On Thu, May 03, 2018 at 10:57:09PM +0300, Amir Goldstein wrote:
> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/overlayfs/copy_up.c |  2 +-
> >  fs/read_write.c        | 10 ++++++----
> >  include/linux/fs.h     |  2 ++
> >  3 files changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 8bede0742619..6634a85255ae 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> >                         break;
> >                 }
> >
> > -               bytes = do_splice_direct(old_file, &old_pos,
> > +               bytes = splice_with_holes(old_file, &old_pos,
> >                                          new_file, &new_pos,
> >                                          this_len, SPLICE_F_MOVE);
> 
> 
> Add.. you can remove this comment above :)
>         /* FIXME: copy up sparse files efficiently */
> 
> For the record, when I added vfs_clone_file_range() above,
> Dave Chinner has suggested to replace the entire block with
> vfs_copy_file_range(), which would do all the fallbacks.
> Since then, vfs_copy_file_range() gained "try to clone first".

Yup, we want the copy offload infrastructure to be used if at all
possible, so we get consistent behaviour for everyone trying to
optimise copy behaviour.  In this case, I think that it is relevant
that we heard at LSFMM that userspace utils don't want to use
copy_file_range() because it doesn't "optimise for sparse files".

>From that perspective, perhaps this needs "hole preserving copy"
behaviour needs to be moved inside copy-file_range() (and therefore
do_splice_direct()) and triggered by a new flag for
copy_file_range(). e.g. COPY_FILE_SPARSE. That way callers can tell
the kernel they want a sparse copy, and the kernel can attempt that
rather a copy that converts holes to zeros.

In most cases, filesystems that implement efficient offloads already
preserve sparseness, but the do_splice_direct() fallback does not.
If we fix that, then we're a big step closer to getting utilities
like cp and rsync to use copy_file_range() instead of bit shuffling
through userspace to copy data.....

> I am not asking that you do this as part of your work, simply
> pointing out an opportunity.

*nod*

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Goldwyn Rodrigues]

On 05/03/2018 02:57 PM, Amir Goldstein wrote:
> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>>  fs/overlayfs/copy_up.c |  2 +-
>>  fs/read_write.c        | 10 ++++++----
>>  include/linux/fs.h     |  2 ++
>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index 8bede0742619..6634a85255ae 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>>                         break;
>>                 }
>>
>> -               bytes = do_splice_direct(old_file, &old_pos,
>> +               bytes = splice_with_holes(old_file, &old_pos,
>>                                          new_file, &new_pos,
>>                                          this_len, SPLICE_F_MOVE);
> 
> 
> Add.. you can remove this comment above :)
>         /* FIXME: copy up sparse files efficiently */
> 
> For the record, when I added vfs_clone_file_range() above,
> Dave Chinner has suggested to replace the entire block with
> vfs_copy_file_range(), which would do all the fallbacks.
> Since then, vfs_copy_file_range() gained "try to clone first".
> 
> There are still differences between the loop in this function and
> the loop in vfs_copy_file_range(). Perhaps the differences could
> be smoothed away, I did not check recently.

There is a difference. copy_file_range(2) or do_splice_direct() can
return short writes. I think this loop is performed to cover short writes.

I suppose we could remove the clone_file_range() before the loop and
replace the do_splice_direct() with vfs_copy_file_range().

-- 
Goldwyn

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Goldwyn Rodrigues]

On 05/03/2018 05:11 PM, Dave Chinner wrote:
> On Thu, May 03, 2018 at 10:57:09PM +0300, Amir Goldstein wrote:
>> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>
>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>> ---
>>>  fs/overlayfs/copy_up.c |  2 +-
>>>  fs/read_write.c        | 10 ++++++----
>>>  include/linux/fs.h     |  2 ++
>>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index 8bede0742619..6634a85255ae 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>>>                         break;
>>>                 }
>>>
>>> -               bytes = do_splice_direct(old_file, &old_pos,
>>> +               bytes = splice_with_holes(old_file, &old_pos,
>>>                                          new_file, &new_pos,
>>>                                          this_len, SPLICE_F_MOVE);
>>
>>
>> Add.. you can remove this comment above :)
>>         /* FIXME: copy up sparse files efficiently */
>>
>> For the record, when I added vfs_clone_file_range() above,
>> Dave Chinner has suggested to replace the entire block with
>> vfs_copy_file_range(), which would do all the fallbacks.
>> Since then, vfs_copy_file_range() gained "try to clone first".
> 
> Yup, we want the copy offload infrastructure to be used if at all
> possible, so we get consistent behaviour for everyone trying to
> optimise copy behaviour.  In this case, I think that it is relevant
> that we heard at LSFMM that userspace utils don't want to use
> copy_file_range() because it doesn't "optimise for sparse files".

Sorry, I did not cc you, but did you check the [2/3] of this patchset [2]?

> 
> From that perspective, perhaps this needs "hole preserving copy"
> behaviour needs to be moved inside copy-file_range() (and therefore
> do_splice_direct()) and triggered by a new flag for
> copy_file_range(). e.g. COPY_FILE_SPARSE. That way callers can tell
> the kernel they want a sparse copy, and the kernel can attempt that
> rather a copy that converts holes to zeros.

In the patchset description [0], I ask if this should be the default
feature (I think it should be) and holes should be converted to zeros as
a special flag.. The only argument against it could be that applications
are already using this interface, so this would be a change in behavior.

> 
> In most cases, filesystems that implement efficient offloads already
> preserve sparseness, but the do_splice_direct() fallback does not.
> If we fix that, then we're a big step closer to getting utilities
> like cp and rsync to use copy_file_range() instead of bit shuffling
> through userspace to copy data.....

Yes, I agree and hence the patchset. Besides, I think it should work
across filesystems too [1].

> 
>> I am not asking that you do this as part of your work, simply
>> pointing out an opportunity.
> 
> *nod*
> 
> Cheers,
> 
> Dave.
> 
[0] https://marc.info/?l=linux-fsdevel&m=152536120311694&w=2
[1] https://marc.info/?l=linux-fsdevel&m=152536121111700&w=2
[2] https://marc.info/?l=linux-fsdevel&m=152536121111700&w=2

-- 
Goldwyn

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Goldwyn Rodrigues]

On 05/03/2018 02:57 PM, Amir Goldstein wrote:
> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>>  fs/overlayfs/copy_up.c |  2 +-
>>  fs/read_write.c        | 10 ++++++----
>>  include/linux/fs.h     |  2 ++
>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index 8bede0742619..6634a85255ae 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>>                         break;
>>                 }
>>
>> -               bytes = do_splice_direct(old_file, &old_pos,
>> +               bytes = splice_with_holes(old_file, &old_pos,
>>                                          new_file, &new_pos,
>>                                          this_len, SPLICE_F_MOVE);
> 
> 
> Add.. you can remove this comment above :)
>         /* FIXME: copy up sparse files efficiently */
> 
> For the record, when I added vfs_clone_file_range() above,
> Dave Chinner has suggested to replace the entire block with
> vfs_copy_file_range(), which would do all the fallbacks.
> Since then, vfs_copy_file_range() gained "try to clone first".
> 
> There are still differences between the loop in this function and
> the loop in vfs_copy_file_range(). Perhaps the differences could
> be smoothed away, I did not check recently.
> 

There is a difference. copy_file_range(2) or do_splice_direct() can
return short writes. I think this loop is performed to cover short writes.

I suppose we could remove the clone_file_range() before the loop and
replace the do_splice_direct() with vfs_copy_file_range().

-- 
Goldwyn

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Amir Goldstein]

On Fri, May 4, 2018 at 4:31 AM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
>
> On 05/03/2018 02:57 PM, Amir Goldstein wrote:
>> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>>
>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>> ---
>>>  fs/overlayfs/copy_up.c |  2 +-
>>>  fs/read_write.c        | 10 ++++++----
>>>  include/linux/fs.h     |  2 ++
>>>  3 files changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index 8bede0742619..6634a85255ae 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>>>                         break;
>>>                 }
>>>
>>> -               bytes = do_splice_direct(old_file, &old_pos,
>>> +               bytes = splice_with_holes(old_file, &old_pos,
>>>                                          new_file, &new_pos,
>>>                                          this_len, SPLICE_F_MOVE);
>>
>>
>> Add.. you can remove this comment above :)
>>         /* FIXME: copy up sparse files efficiently */
>>
>> For the record, when I added vfs_clone_file_range() above,
>> Dave Chinner has suggested to replace the entire block with
>> vfs_copy_file_range(), which would do all the fallbacks.
>> Since then, vfs_copy_file_range() gained "try to clone first".
>>
>> There are still differences between the loop in this function and
>> the loop in vfs_copy_file_range(). Perhaps the differences could
>> be smoothed away, I did not check recently.
>>
>
> There is a difference. copy_file_range(2) or do_splice_direct() can
> return short writes. I think this loop is performed to cover short writes.
>
> I suppose we could remove the clone_file_range() before the loop and
> replace the do_splice_direct() with vfs_copy_file_range().
>

There are other differences, like file_start_write().
To resolve them would probably require factoring out do_copy_file_range()
with an argument to determine whether or not the function will try to
copy as much as possible by looping.

BTW, It's a bit odd that at the moment SPLICE_F_MOVE is only tested by
fuse and fuse cannot be an overlayfs upper fs. Maybe I am missing something.

Thanks,
Amir.

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Dave Chinner]

On Thu, May 03, 2018 at 08:29:36PM -0500, Goldwyn Rodrigues wrote:
> 
> 
> On 05/03/2018 05:11 PM, Dave Chinner wrote:
> > On Thu, May 03, 2018 at 10:57:09PM +0300, Amir Goldstein wrote:
> >> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>>
> >>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>> ---
> >>>  fs/overlayfs/copy_up.c |  2 +-
> >>>  fs/read_write.c        | 10 ++++++----
> >>>  include/linux/fs.h     |  2 ++
> >>>  3 files changed, 9 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> >>> index 8bede0742619..6634a85255ae 100644
> >>> --- a/fs/overlayfs/copy_up.c
> >>> +++ b/fs/overlayfs/copy_up.c
> >>> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> >>>                         break;
> >>>                 }
> >>>
> >>> -               bytes = do_splice_direct(old_file, &old_pos,
> >>> +               bytes = splice_with_holes(old_file, &old_pos,
> >>>                                          new_file, &new_pos,
> >>>                                          this_len, SPLICE_F_MOVE);
> >>
> >>
> >> Add.. you can remove this comment above :)
> >>         /* FIXME: copy up sparse files efficiently */
> >>
> >> For the record, when I added vfs_clone_file_range() above,
> >> Dave Chinner has suggested to replace the entire block with
> >> vfs_copy_file_range(), which would do all the fallbacks.
> >> Since then, vfs_copy_file_range() gained "try to clone first".
> > 
> > Yup, we want the copy offload infrastructure to be used if at all
> > possible, so we get consistent behaviour for everyone trying to
> > optimise copy behaviour.  In this case, I think that it is relevant
> > that we heard at LSFMM that userspace utils don't want to use
> > copy_file_range() because it doesn't "optimise for sparse files".
> 
> Sorry, I did not cc you, but did you check the [2/3] of this patchset [2]?

No, I didn't look at it. Amir mentioned my name and something we'd
talked about in the past, and I simply responded to the context in
that email....

> > From that perspective, perhaps this needs "hole preserving copy"
> > behaviour needs to be moved inside copy-file_range() (and therefore
> > do_splice_direct()) and triggered by a new flag for
> > copy_file_range(). e.g. COPY_FILE_SPARSE. That way callers can tell
> > the kernel they want a sparse copy, and the kernel can attempt that
> > rather a copy that converts holes to zeros.
> 
> In the patchset description [0], I ask if this should be the default
> feature (I think it should be) and holes should be converted to zeros as
> a special flag.. The only argument against it could be that applications
> are already using this interface, so this would be a change in behavior.

In general, we don't change the behaviour of syscalls after the
fact. We have flags to control behaviour, so applications that want
to preserve holes will just have to be changed to use that flag.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Christoph Hellwig]

On Sun, May 06, 2018 at 09:16:41AM +1000, Dave Chinner wrote:
> > In the patchset description [0], I ask if this should be the default
> > feature (I think it should be) and holes should be converted to zeros as
> > a special flag.. The only argument against it could be that applications
> > are already using this interface, so this would be a change in behavior.
> 
> In general, we don't change the behaviour of syscalls after the
> fact. We have flags to control behaviour, so applications that want
> to preserve holes will just have to be changed to use that flag.

copy_file_range isn't documented to either preserve or not preserve
holes.  And in fact it will preserve holes (or even create new ones)
for any filesystem that implements it as a clone like btrfs or xfs..

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

On May 5, 2018, at 5:16 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, May 03, 2018 at 08:29:36PM -0500, Goldwyn Rodrigues wrote:
>> 
>> On 05/03/2018 05:11 PM, Dave Chinner wrote:
>>> 
>>> Yup, we want the copy offload infrastructure to be used if at all
>>> possible, so we get consistent behaviour for everyone trying to
>>> optimise copy behaviour.  In this case, I think that it is relevant
>>> that we heard at LSFMM that userspace utils don't want to use
>>> copy_file_range() because it doesn't "optimise for sparse files".
>>> 
>>> From that perspective, perhaps this needs "hole preserving copy"
>>> behaviour needs to be moved inside copy-file_range() (and therefore
>>> do_splice_direct()) and triggered by a new flag for
>>> copy_file_range(). e.g. COPY_FILE_SPARSE. That way callers can tell
>>> the kernel they want a sparse copy, and the kernel can attempt that
>>> rather a copy that converts holes to zeros.
>> 
>> In the patchset description [0], I ask if this should be the default
>> feature (I think it should be) and holes should be converted to zeros as
>> a special flag.. The only argument against it could be that applications
>> are already using this interface, so this would be a change in behavior.
> 
> In general, we don't change the behaviour of syscalls after the
> fact. We have flags to control behaviour, so applications that want
> to preserve holes will just have to be changed to use that flag.

It's hard to think of a normal use case where someone created a file with
holes, but wants the holes to be filled in when the file is copied...

The "cp" utility defaults to "--sparse=auto", but allows "--sparse=never"
to force hole filling, so it makes sense to handle the in-kernel copy
the same way.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Dave Chinner]

On Mon, May 07, 2018 at 02:16:38PM +0200, Christoph Hellwig wrote:
> On Sun, May 06, 2018 at 09:16:41AM +1000, Dave Chinner wrote:
> > > In the patchset description [0], I ask if this should be the default
> > > feature (I think it should be) and holes should be converted to zeros as
> > > a special flag.. The only argument against it could be that applications
> > > are already using this interface, so this would be a change in behavior.
> > 
> > In general, we don't change the behaviour of syscalls after the
> > fact. We have flags to control behaviour, so applications that want
> > to preserve holes will just have to be changed to use that flag.
> 
> copy_file_range isn't documented to either preserve or not preserve
> holes.  And in fact it will preserve holes (or even create new ones)
> for any filesystem that implements it as a clone like btrfs or xfs..

Right, but that's the entire problem. And you can't even say it will
preserve holes on XFS, because that relies on a filesystem format
feature that most users currently don't have enabled. Hence it will
be sparse preserving on some XFS filesystems but not on most.

This sort of whacky undefined behaviour w.r.t. sparseness was the
reason we were given at LSFMM for cp and rsync not implementing
copy_file_range() - they could not control it according to the
user's direction. Hence my suggestion that we need flags to
specifically direct the behaviour of the syscall so that userspace
will actually use it....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Christoph Hellwig]

On Tue, May 08, 2018 at 09:16:46AM +1000, Dave Chinner wrote:
> This sort of whacky undefined behaviour w.r.t. sparseness was the
> reason we were given at LSFMM for cp and rsync not implementing
> copy_file_range() - they could not control it according to the
> user's direction. Hence my suggestion that we need flags to
> specifically direct the behaviour of the syscall so that userspace
> will actually use it....

They can just use SEEK_HOLE/DATA and just copy the chunk they care
about.  Especially as they already have the SEEK_HOLE/DATA logic
for the plain old copy anyway - that is the only thing they have
to create holes in the destination file to start with.  Nevermind
that a file system with inline dedup will happily create holes for
them underneath.

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Dave Chinner]

On Tue, May 08, 2018 at 06:02:42AM +0200, Christoph Hellwig wrote:
> On Tue, May 08, 2018 at 09:16:46AM +1000, Dave Chinner wrote:
> > This sort of whacky undefined behaviour w.r.t. sparseness was the
> > reason we were given at LSFMM for cp and rsync not implementing
> > copy_file_range() - they could not control it according to the
> > user's direction. Hence my suggestion that we need flags to
> > specifically direct the behaviour of the syscall so that userspace
> > will actually use it....
> 
> They can just use SEEK_HOLE/DATA and just copy the chunk they care
> about.  Especially as they already have the SEEK_HOLE/DATA logic
> for the plain old copy anyway

Well, you think they would given what we've told them in the past
about using fiemap for finding holes and the potential for data
corruption it comes along with. But - as I found out recently - cp
is still using fiemap to find holes, not SEEK_HOLE/DATA. See:

https://github.com/coreutils/coreutils/blob/master/src/extent-scan.c

> - that is the only thing they have
> to create holes in the destination file to start with.  Nevermind
> that a file system with inline dedup will happily create holes for
> them underneath.

Yup, I know. However, it's not me that I'm suggesting we do this
for....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Goldwyn Rodrigues]

On 05/08/2018 05:06 AM, Dave Chinner wrote:
> On Tue, May 08, 2018 at 06:02:42AM +0200, Christoph Hellwig wrote:
>> On Tue, May 08, 2018 at 09:16:46AM +1000, Dave Chinner wrote:
>>> This sort of whacky undefined behaviour w.r.t. sparseness was the
>>> reason we were given at LSFMM for cp and rsync not implementing
>>> copy_file_range() - they could not control it according to the
>>> user's direction. Hence my suggestion that we need flags to
>>> specifically direct the behaviour of the syscall so that userspace
>>> will actually use it....
>>
>> They can just use SEEK_HOLE/DATA and just copy the chunk they care
>> about.  Especially as they already have the SEEK_HOLE/DATA logic
>> for the plain old copy anyway
> 
> Well, you think they would given what we've told them in the past
> about using fiemap for finding holes and the potential for data
> corruption it comes along with. But - as I found out recently - cp
> is still using fiemap to find holes, not SEEK_HOLE/DATA. See:
> 
> https://github.com/coreutils/coreutils/blob/master/src/extent-scan.c
> 
>> - that is the only thing they have
>> to create holes in the destination file to start with.  Nevermind
>> that a file system with inline dedup will happily create holes for
>> them underneath.
> 
> Yup, I know. However, it's not me that I'm suggesting we do this
> for....
> 

What should the default behavior (without flags) be? Should it create
holes or not? If the filesystem supports reflink then we would end up
with destination having holes and if it does not we will have
destination not having holes even though the filesystem supports sparse
files. It is not consistent though there is no documentation to specify
it will be one way or the other. Are users okay with inconsistent
behavior? Depending on the answer, we can add either one of the two
options: CFR_FILL_HOLES or CFR_KEEP_HOLES. Alternatively, we can
document the state of holes in the destination is not determinant and
coreutils/cp can perform the lseek(SEEK_HOLE/DATA)

If we do handle the hole behavior, should the individual filesystems
handle this or should we handle in in VFS. VFS seems simple because
NFS42: nfs42_copy_args does not seem to have a field which could
represent flags.

If cp calls copy_file_range(), it will clone the portion if the
filesystem supports it, which may not work with "cp --reflink=never"
option. In that case, should we have CFR_NO_REFLINK option for
copy_file_range()?


-- 
Goldwyn

Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Christoph Hellwig]

On Tue, May 08, 2018 at 11:11:33AM -0500, Goldwyn Rodrigues wrote:
> What should the default behavior (without flags) be?

Create holes if and how the file systems thinks is best.  That
might even include creating holes if the original file didnt have
any.

> options: CFR_FILL_HOLES or CFR_KEEP_HOLES. Alternatively, we can
> document the state of holes in the destination is not determinant and
> coreutils/cp can perform the lseek(SEEK_HOLE/DATA)

Create holes by default, and no flag that just create confusion as
holes are a file system implementation detail subject to change
at any time.

> If cp calls copy_file_range(), it will clone the portion if the
> filesystem supports it, which may not work with "cp --reflink=never"
> option. In that case, should we have CFR_NO_REFLINK option for
> copy_file_range()?

cp --reflink=never is pretty idiotic.  Think of inline dedup for example
which will just create reflink behinds its back in the file system,
or just dedup invisibly to it in the storage device.

> 
> 
> -- 
> Goldwyn
---end quoted text---