Why adding this behind flag? Why don't try the copy-on-write option first and if it's not supported use the fallback? IIUC this is how the OS X copyfile works (maybe I'm wrong though)

I've been using https://unix.stackexchange.com/questions/80351/why-is-cp-reflink-auto-not-the-default-behaviour as the motivation not to do it by default. I don't have really strong feelings one way or the other. However, if we do copy-on-write by default, then there should probably be a flag to opt out of it.

I'm fine either way. Just wanted to note that we probably want a consistent behavior between platforms. Is OS X doing a CoW on a APFS when calling copyfile()? If that's the case, can we opt out?

Is OS X doing a CoW on a APFS when calling copyfile()?

@santigimeno, I'm not 100% certain, but the copyfile_data() helper function in the copyfile() source appears to use a read-write loop.

Aside: we made a mistake in the previous PR: we don't check the flags, leaving the caller with no good way to check if libuv supports a particular flag.

Opened #1493

and even then it won't be in many mainstream distros' /usr/include for a while.

@bnoordhuis thoughts on moving this forward? Should I try to? I've seen #if HAVE_*_H used in other projects, but I don't believe libuv does that.

I'm not 100% certain, but the copyfile_data() helper function in the copyfile() source appears to use a read-write loop.

It looks you're right :)

@cjihrig You could try defining it yourself. Untested but if you #include <asm/ioctl.h>, I think this should work:

#ifndef FICLONE
#define FICLONE _IOW(0x94, 9, int)
#endif

Just hard-coding the value won't work because ioctls can have different values on different platforms.

@bnoordhuis updated to try your idea. I verified that the FICLONE path was being executed on the CI, but all such calls failed and had to go through the fallback path.

Would it be better to return an error if the ioctl is not supported, in case you really want copy-on-write semantics and not just a copy?

I originally wanted it to "just work" but after testing the thing, I'm thinking that might be a good idea.

Is OS X doing a CoW on a APFS when calling copyfile()?

@santigimeno, I'm not 100% certain, but the copyfile_data() helper function in the copyfile() source appears to use a read-write loop.

It seems like copyfile will only attempt to CoW-clone the file when the COPYFILE_CLONE flag is given.

ref: https://developer.apple.com/library/content/samplecode/APFSCloning/Listings/clonefile_main_c.html#//apple_ref/doc/uid/TP40017341-clonefile_main_c-DontLinkElementID_5

It would be nice if we could pass this flag when UV_FS_COPYFILE_FICLONE is passed.

@LinusU interesting. That flag isn't documented at https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/copyfile.3.html, which is the first Google search result for macos copyfile(3).

Yeah, I think that is an old manpage, this is from the latest beta of High Sierra:

 COPYFILE_CLONE_FORCE   Clone the file instead.  This is a force flag i.e. if cloning fails, an
                        error is returned.  This flag is equivalent to (COPYFILE_EXCL | COPYFILE_ACL
                        | COPYFILE_STAT | COPYFILE_XATTR | COPYFILE_DATA).  Note that if cloning is
                        successful, callbacks will not be invoked.  Note also that there is no sup-
                        port for cloning directories: if a directory is provided as the source, an
                        error will be returned.  (This is only applicable for the copyfile() func-
                        tion.)

 COPYFILE_CLONE         Try to clone the file instead.  This is a best try flag i.e. if cloning
                        fails, fallback to copying the file.  This flag is equivalent to (COPY-
                        FILE_EXCL | COPYFILE_ACL | COPYFILE_STAT | COPYFILE_XATTR | COPYFILE_DATA).
                        Note that if cloning is successful, callbacks will not be invoked.  Note
                        also that there is no support for cloning directories: if a directory is
                        provided as the source, this will instead copy the directory (recursively if
                        COPYFILE_RECURSIVE is also passed).  (This is only applicable for the
                        copyfile() function.)

Notice that the url starts with https://developer.apple.com/legacy/ 😄

Ha I did notice legacy in the URL, that's why I added the part about it being the first search result :-)

I'll definitely look into it.

Hehe :)

Awesome! Let me know if there is anything I can do to help 🙌

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/461/

Rebase is simple:

diff --cc test/test-fs-copyfile.c
index 460c1dc6,874a52af..00000000
--- a/test/test-fs-copyfile.c
+++ b/test/test-fs-copyfile.c
@@@ -120,12 -119,11 +120,18 @@@ TEST_IMPL(fs_copyfile) 
    ASSERT(r == 0);
    handle_result(&req);
  
 +  /* Copies a file of size zero. */
 +  unlink(dst);
 +  touch_file(src, 0);
 +  r = uv_fs_copyfile(NULL, &req, src, dst, 0, NULL);
 +  ASSERT(r == 0);
 +  handle_result(&req);
++  
+   /* Copies file using UV_FS_COPYFILE_FICLONE. */
+   unlink(dst);
+   r = uv_fs_copyfile(NULL, &req, fixture, dst, UV_FS_COPYFILE_FICLONE, NULL);
+   ASSERT(r == 0);
+   handle_result(&req);
  
    /* Copies file synchronously. Overwrites existing file. */
    r = uv_fs_copyfile(NULL, &req, fixture, dst, 0, NULL);
@@@ -155,9 -147,9 +161,9 @@@
    unlink(dst);
    r = uv_fs_copyfile(loop, &req, fixture, dst, 0, handle_result);
    ASSERT(r == 0);
-   ASSERT(result_check_count == 5);
-   uv_run(loop, UV_RUN_DEFAULT);
 -  ASSERT(result_check_count == 4);
 +  ASSERT(result_check_count == 6);
+   uv_run(loop, UV_RUN_DEFAULT);
 -  ASSERT(result_check_count == 5);
++  ASSERT(result_check_count == 7);
    unlink(dst); /* Cleanup */
  
    return 0;

Hi @cjihrig
What is the state of this? I really like this and will be very excited to see it in codebase.
Can I help in some way?

I've rebased this PR. Here is a passing CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/739/

Given these comments:

Would it be better to return an error if the ioctl is not supported, in case you really want copy-on-write semantics and not just a copy?

and

 COPYFILE_CLONE_FORCE   Clone the file instead.  This is a force flag i.e. if cloning fails, an
                        error is returned.  This flag is equivalent to (COPYFILE_EXCL | COPYFILE_ACL
                        | COPYFILE_STAT | COPYFILE_XATTR | COPYFILE_DATA).  Note that if cloning is
                        successful, callbacks will not be invoked.  Note also that there is no sup-
                        port for cloning directories: if a directory is provided as the source, an
                        error will be returned.  (This is only applicable for the copyfile() func-
                        tion.)

I think it makes sense to move forward with UV_FS_COPYFILE_FICLONE in this PR giving a best effort at cloning, and making a follow up PR adding UV_FS_COPYFILE_FICLONE_FORCE that returns an error if cloning is not supported.