Re: [PATCH v2 10/9] copy_file_range.2: New page documenting copy_file_range()

On 09/14/2015 02:32 PM, Darrick J. Wong wrote:

On Sun, Sep 13, 2015 at 09:50:18AM +0200, Michael Kerrisk (man-pages) wrote:

quoted

Hi Anna,

On 09/11/2015 10:30 PM, Anna Schumaker wrote:

quoted

copy_file_range() is a new system call for copying ranges of data
completely in the kernel.  This gives filesystems an opportunity to
implement some kind of "copy acceleration", such as reflinks or
server-side-copy (in the case of NFS).

Signed-off-by: Anna Schumaker <redacted>

Thanks for writing such a nice first draft man page! I have a few
comments below. Would you be willing to revise and resubmit?

quoted

---
 man2/copy_file_range.2 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 man2/copy_file_range.2

diff --git a/man2/copy_file_range.2 b/man2/copy_file_range.2
new file mode 100644
index 0000000..84912b5
--- /dev/null
+++ b/man2/copy_file_range.2

@@ -0,0 +1,188 @@
+.\"This manpage is Copyright (C) 2015 Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>

We need a license for this page. Please see
https://www.kernel.org/doc/man-pages/licenses.html

quoted

+.TH COPY 2 2015-8-31 "Linux" "Linux Programmer's Manual"

Make the month 2 digits (leading 0).

quoted

+.SH NAME
+copy_file_range \- Copy a range of data from one file to another
+.SH SYNOPSIS
+.nf
+.B #include <linux/copy.h>
+.B #include <sys/syscall.h>

117 more quoted lines

Is there a better behavior for flags=0?  I was thinking this would be what people want when they don't care how the copy happens in the kernel.

Anna

quoted

What are the semantics with respect to signals, especially if data 
copying a very large file range? This info should be included in the 
man page, probably under NOTES.

quoted

+.SH RETURN VALUE
+Upon successful completion,
+.BR copy_file_range ()
+will return the number of bytes copied between files.
+This could be less than the length originally requested.
+
+On error,
+.BR copy_file_range ()
+returns \-1 and
+.I errno
+is set to indicate the error.
+.SH ERRORS
+.TP
+.B EBADF
+One or more file descriptors are not valid,
+or do not have proper read-write mode;

I think that last line can go. I mean, isn't this point (again)
covered in the next few lines?

Or change the ';' to a ':'?

quoted

quoted

+.I fd_in
+is not open for reading; or
+.I fd_out
+is not open for writing.
+.TP
+.B EINVAL
+Requested range extends beyond the end of the file; or the

s/file/source file/  (right?)

quoted

+.I flags
+argument is set to an invalid value.
+.TP

139 more quoted lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 22/09/15 21:10, Anna Schumaker wrote:

On 09/14/2015 02:32 PM, Darrick J. Wong wrote:

quoted

On Sun, Sep 13, 2015 at 09:50:18AM +0200, Michael Kerrisk (man-pages) wrote:

quoted

Hi Anna,

quoted

quoted

Furthermore, I even wonder if explicitly specifying flags as
COPY_FR_COPY | COPY_FR_REFLINK should just generate an EINVAL
error. 0 already gives us the behavior described above,
and allowing the combination COPY_FR_COPY | COPY_FR_REFLINK
perhaps just contributes to misleading the user that these
flags are orthogonal, when in reality they are not. What do
you think?

Personally, I think it's a little weird that one turns on reflink with a flag;
turns on regular copy with a different flag; and turns on both by not
specifying either flag. :)

Is there a better behavior for flags=0?  I was thinking this would be what people want when they don't care how the copy happens in the kernel.

As a user, I'm fine with this default and the interface in general.

thanks,
Pádraig.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Sep 22, 2015 at 04:10:47PM -0400, Anna Schumaker wrote:

On 09/14/2015 02:32 PM, Darrick J. Wong wrote:

quoted

On Sun, Sep 13, 2015 at 09:50:18AM +0200, Michael Kerrisk (man-pages) wrote:

quoted

Hi Anna,

On 09/11/2015 10:30 PM, Anna Schumaker wrote:

quoted

copy_file_range() is a new system call for copying ranges of data
completely in the kernel.  This gives filesystems an opportunity to
implement some kind of "copy acceleration", such as reflinks or
server-side-copy (in the case of NFS).

Signed-off-by: Anna Schumaker <redacted>

Thanks for writing such a nice first draft man page! I have a few
comments below. Would you be willing to revise and resubmit?

quoted

---
 man2/copy_file_range.2 | 188 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 man2/copy_file_range.2

diff --git a/man2/copy_file_range.2 b/man2/copy_file_range.2
new file mode 100644
index 0000000..84912b5
--- /dev/null
+++ b/man2/copy_file_range.2

@@ -0,0 +1,188 @@
+.\"This manpage is Copyright (C) 2015 Anna Schumaker <Anna.Schumaker@Netapp.com>

We need a license for this page. Please see
https://www.kernel.org/doc/man-pages/licenses.html

quoted

+.TH COPY 2 2015-8-31 "Linux" "Linux Programmer's Manual"

Make the month 2 digits (leading 0).

quoted

+.SH NAME
+copy_file_range \- Copy a range of data from one file to another
+.SH SYNOPSIS
+.nf
+.B #include <linux/copy.h>

121 more quoted lines

[sorry, was out on vacation for a week]

Probably not.  I guess you could "#define COPY_FR_CHEFS_SURPRISE 0" so at least
it'd be obvious in userland source code that the last argument is flags
and not just some number that happens to be zero.  (I don't like when I have
to look up the manpage to remember which arguments are numbers and which are
flags.)

(OTOH I concede that we do have a precedent of flags == 0 meaning 'use sane
defaults'.)

--D

Anna

quoted

quoted

What are the semantics with respect to signals, especially if data 
copying a very large file range? This info should be included in the 
man page, probably under NOTES.

quoted

+.SH RETURN VALUE
+Upon successful completion,
+.BR copy_file_range ()
+will return the number of bytes copied between files.
+This could be less than the length originally requested.
+
+On error,
+.BR copy_file_range ()
+returns \-1 and
+.I errno
+is set to indicate the error.
+.SH ERRORS
+.TP
+.B EBADF
+One or more file descriptors are not valid,
+or do not have proper read-write mode;

I think that last line can go. I mean, isn't this point (again)
covered in the next few lines?

Or change the ';' to a ':'?

quoted

quoted

+.I fd_in
+is not open for reading; or
+.I fd_out
+is not open for writing.
+.TP
+.B EINVAL
+Requested range extends beyond the end of the file; or the

s/file/source file/  (right?)

147 more quoted lines