fs: bring back macos-specific copyfile(3) #2578

Artoria2e5 · 2019-12-16T12:26:36Z

Go check out #3654 instead. @mischnic is awesome.

copyfile(3) does not always do its own open according to permission rules. We can solve that by guarding it with one of our own opens. This way we can keep the cool clonefile thing for huge node_modules and still have sensible results.

MacOS does not have faccess(2) in case you are wondering why I did this. There is only access(2) with the "real UID" shenanigans that will bite us in the future.

This PR partially reverts #2233. Tests in the said PR are kept, and they should pass.

This reverts commit 5bc4d77.

Artoria2e5 · 2019-12-16T12:33:35Z

We need to do an open first per nodejs/node#26936. Amending.

For the record, this one is possibly due to this part in copyfile: if(chmod(s->dst, (s->sb.st_mode | S_IWUSR) & ~S_IFMT) == 0). Absolute unit.

For clonefileat it should fail correctly, since COPYFILE_UNLINK is not default and the syscall always fails when dest exists

We can do our own open!

bnoordhuis

@cjihrig You probably want to take a look at this?

src/unix/fs.c

cjihrig

MacOS does not have faccess(2) in case you are wondering why I did this. There is only access(2) with the "real UID" shenanigans that will bite us in the future.

Is using fstat() an option? At least we wouldn't be creating and unlinking the file.

cjihrig · 2019-12-16T14:57:43Z

src/unix/fs.c

  int result;
  int err;
  size_t bytes_to_send;
  int64_t in_offset;
  ssize_t bytes_written;
-


Can you add this line back.

src/unix/fs.c

Artoria2e5 · 2019-12-17T02:25:47Z

fstat is not the best idea because ACL is a thing on macOS. The correct way to do so is via the access(2) family of APIs, but macOS does not have a faccess(2) / faccessat(2) that does EUID instead of the UID with AT_EACCESS. A quick Google search I did yesterday finds CarbonStdCLib.o, apparently a C library polyfill, doing something similar for faccess.

Unlinking is what the OS X cp(1) command does for -c anyways, so it kind of is idiomatic in this use.

Gonna fix the branch thing later today (UTC+8).

Artoria2e5 · 2019-12-17T02:31:31Z

Hmm, looks like there is faccessat(2) on macOS >= 10.10. What's the OS support policy for libuv and nodejs? (Looks like I can do that for node, which only supports macOS >= 10.11.)

cjihrig · 2019-12-17T14:54:00Z

libuv supports macOS >= 10.7. See the list of supported platforms here.

Artoria2e5 · 2019-12-17T15:46:13Z

A good-ish polyfill for our desired functionality is found in gnulib euidaccess(3) (GPL!!). It gives two implementations: the correct but non-reentrant one using a setre{u,g}id-access-setre{u,g}id pattern and the traditional stat thing which is incorrect with ACL. So which one is more evil here, changing a global state or being incorrect in a case not yet tested?

Since faccessat(2) is a syscall, I can possibly get away with doing a version check like the clone part and syscall(2) the whole thing away on 10.10+. I still need to pick a way to polyfill this feature for [10.7, 10.10), so please do give your suggestion on that. It should look like:

#if defined(__APPLE__) && !TARGET_OS_IPHONE
/** @returns 1: writeable. 2: does not exist. */
static int fs__mac_canwrite(const char* path, int darwin_kern_maj) {
  int res = 0;
  // 10.7+
  if (darwin_kern_maj >= 11) { 
    // TODO: rewrite to use syscall(2) and sys/syscall.h!
    res = faccessat(AT_FDCWD, path, W_OK | R_OK, AT_EACCESS) == 0;
    goto test_exist;
  }
  /* polyfill here */
test_exist: // should be reused by stat or access polyfills
  if (res == 0 && errorno == ENOENT)
    return 2;
  return res;
}

static bool fs__mac_can_copy(const char* path, int darwin_kern_maj, int excl) {
  int res;
  res = fs__mac_canwrite(path, darwin_kern_maj);
  if (res != 2)
    return ! excl;
  // Apple manpage says their dirname does not modify the input. Trust it for now.
  return !! fs__mac_canwrite(dirname(path), darwin_kern_maj);
}
#endif

richardlau · 2019-12-17T16:17:43Z

I presume the gnulib implementation will be GPL and therefore cannot be used in libuv.

Artoria2e5 · 2019-12-17T16:19:22Z

Yeah, someone will need to wipe their brain with only the phrase "implement an access check based on stat() or setuid and access(2)" left if they decided to implement it here. Should possibly add a click warning on the gnulib link.

* remove a stat dependency retained from copying the non-Darwin open() codepath. We don't need to make it super right here since it's deleted soon afterwards, so just a 0700 will do. * retain whatever copyfile tries to say in the errno, instead of the generic error -1.

stale · 2020-01-13T18:01:21Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-07-07T15:49:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

cjihrig · 2020-08-09T21:33:58Z

With this change, the macOS implementation has more overlap with the Unix implementation than it previously did. The overlap is more prone to getting out of sync - for example, the Unix implementation no longer opens the destination with O_TRUNC like this PR does.

Maybe we should look into using fcopyfile() only for clone operations and use the common implementation for everything else. It would essentially be a macOS specific version of this code block.

zkochan · 2022-01-09T01:23:15Z

ping

Jarred-Sumner · 2022-01-13T00:31:03Z

To give you a sense of the impact here — bun’s implementation of fs.copyFileSync which uses copyfile() on macOS performs 2x faster than node’s fs.copyFileSync when copying a 1 GB file.

bnoordhuis · 2022-01-13T05:24:10Z

I opened #3406 earlier this week to raise the baseline to macOS 10.15. When accepted and merged, I think that would unblock this PR.

uvesten · 2022-01-14T13:08:59Z

To give you a sense of the impact here — bun’s implementation of fs.copyFileSync which uses copyfile() on macOS performs 2x faster than node’s fs.copyFileSync when copying a 1 GB file.

That's good, but I thought the main issue here was that APFS Copy on Write is not supported. When it is supported, the speedup should be closer to infinity than 2x.

Jarred-Sumner · 2022-01-14T13:14:59Z

This wasn’t with the COPYFILE_FICLONE flag, so whether or not it used copy on write is up to the implementation of copyfile()

…

On Fri, Jan 14, 2022 at 5:09 AM Petter Uvesten ***@***.***> wrote: To give you a sense of the impact here — bun’s implementation of fs.copyFileSync which uses copyfile() on macOS performs 2x faster than node’s fs.copyFileSync when copying a 1 GB file. That's good, but I thought the main issue here was that APFS Copy on Write is not supported. When it is supported, the speedup should be closer to infinity than 2x. — Reply to this email directly, view it on GitHub <#2578 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFNGS7IQTIBOXES67KGINLUWAN7NANCNFSM4J3JET2A> . You are receiving this because you commented.Message ID: ***@***.***>

bnoordhuis · 2022-02-08T13:21:45Z

The baseline is now macos 10.15. There's one remaining comment from @cjihrig so I defer to him on whether it's ready for merging. Let me know if I should also review.

cjihrig

LGTM if the tests pass.

bnoordhuis · 2022-02-10T09:57:38Z

src/unix/fs.c

+
+  /* Check OS version. Cloning is only supported on macOS >= 10.12. */
+  if (req->flags & UV_FS_COPYFILE_FICLONE_FORCE) {
+    if (can_clone == 0) {


Suggested change

if (can_clone == 0) {

if (0 == uv__load_relaxed(&can_clone)) {

bnoordhuis · 2022-02-10T09:57:43Z

src/unix/fs.c

+      if (1 != sscanf(buf, "%d", &major))
+        abort();
+
+      can_clone = -1 + 2 * (major >= 16);  /* macOS >= 10.12 */


Suggested change

can_clone = -1 + 2 * (major >= 16); /* macOS >= 10.12 */

/* macOS >= 10.12 */

uv__store_relaxed(&can_clone, -1 + 2 * (major >= 16));

bnoordhuis · 2022-02-10T09:58:13Z

src/unix/fs.c

+      can_clone = -1 + 2 * (major >= 16);  /* macOS >= 10.12 */
+    }
+
+    if (can_clone < 0)


Suggested change

if (can_clone < 0)

if (0 > uv__load_relaxed(&can_clone))

bnoordhuis · 2022-02-10T10:01:49Z

src/unix/fs.c

+  /* Copyfile(2) tries to chmod the file when a rw open fails. This causes
+   * nodejs/node#26936.
+   * Make it behave by doing our own open-test, because faccessat(2) is not
+   * present on OS X < 10.10. */
+  dstfd = uv_fs_open(NULL,
+                     &fs_req,
+                     req->new_path,
+                     dst_flags,
+                     S_IRWXU,
+                     NULL);
+  uv_fs_req_cleanup(&fs_req);


With 10.15 as the baseline, it's presumably okay to use faccessat() now?

bnoordhuis · 2022-02-10T10:04:09Z

src/unix/fs.c

+  if (dstfd < 0) {
+    return dstfd;
+  } else {
+    err = uv__close_nocheckstdio(dstfd);
+    if (err) {
+      return err;
+    }
+    /* We might have created it. We also don't want clone to fail without
+     * UV_FS_COPYFILE_EXCL. */
+    unlink(req->new_path);
+  }
+
+  if (copyfile(req->path, req->new_path, NULL, flags)) {
+    return UV__ERR(errno);
+  }
+  return 0;


Stylistic:

Suggested change

if (dstfd < 0) {

return dstfd;

} else {

err = uv__close_nocheckstdio(dstfd);

if (err) {

return err;

}

/* We might have created it. We also don't want clone to fail without

* UV_FS_COPYFILE_EXCL. */

unlink(req->new_path);

}

if (copyfile(req->path, req->new_path, NULL, flags)) {

return UV__ERR(errno);

}

return 0;

if (dstfd < 0)

return dstfd;

err = uv__close_nocheckstdio(dstfd);

if (err)

return err;

/* We might have created it. We also don't want clone to fail without

* UV_FS_COPYFILE_EXCL. */

unlink(req->new_path);

if (copyfile(req->path, req->new_path, NULL, flags))

return UV__ERR(errno);

return 0;

bnoordhuis · 2022-02-10T10:04:24Z

src/unix/fs.c

+    return UV__ERR(errno);
+  }
+  return 0;
+#else


Suggested change

#else

#else /* defined(__APPLE__) && !TARGET_OS_IPHONE */

bnoordhuis · 2022-02-10T10:04:52Z

src/unix/fs.c

@@ -1287,6 +1359,7 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) {

  errno = UV__ERR(result);
  return -1;
+#endif


Suggested change

#endif

#endif /* defined(__APPLE__) && !TARGET_OS_IPHONE */

Makes it easier to keep track of what else/endif belongs to what ifdef.

stale · 2022-04-16T03:17:05Z

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

zkochan · 2022-04-16T12:37:49Z

Never Gonna Give You Up

FezVrasta · 2022-04-16T16:30:52Z

Adding my comment so my anti stale bot will take care of this pr from now on

bnoordhuis · 2022-04-16T16:40:34Z

There's some unaddressed feedback I left in February. Most is stylistic but the faccessat() question needs an answer before this can go in.

Also, a gentle reminder that bots and their owners are likely to meet with swift and permanent bans.

stale · 2022-06-12T17:49:48Z

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

zkochan · 2022-06-12T18:32:34Z

Where is the anti stale bot?

cjihrig · 2022-06-12T18:41:28Z

I've added the not-stale label. Hopefully that will keep stale bot away.

bnoordhuis · 2022-06-12T20:02:30Z

@Artoria2e5 Do you plan on picking up this pull request again? If not, maybe someone else wants to step up and get this over the finish line?

mischnic · 2022-06-22T10:56:30Z

If not, maybe someone else wants to step up and get this over the finish line?

I've tried to incorporate your feedback: #3654

Artoria2e5 · 2022-09-07T07:28:11Z

In light of an updated PR being available I am closing this one. Just don't have time for code when I am all stuck at McD, I am afraid.

Revert "fs: remove macOS-specific copyfile(3)"

92cc506

This reverts commit 5bc4d77.

Artoria2e5 force-pushed the mac-copyfile-mode branch from 63f4cf5 to b3b3c43 Compare December 16, 2019 12:56

fs: make macos copyfile() respect permission

f2b9cfd

We can do our own open!

Artoria2e5 force-pushed the mac-copyfile-mode branch from b3b3c43 to f2b9cfd Compare December 16, 2019 12:59

bnoordhuis reviewed Dec 16, 2019

View reviewed changes

src/unix/fs.c Outdated Show resolved Hide resolved

cjihrig reviewed Dec 16, 2019

View reviewed changes

fixup! fs: make macos copyfile() respect permission

2d2f929

Artoria2e5 force-pushed the mac-copyfile-mode branch from 6e117ff to 2d2f929 Compare December 17, 2019 16:31

Artoria2e5 added 2 commits December 18, 2019 00:33

fixup! fs: make macos copyfile() respect permission

ffd8fb8

cristianl mentioned this pull request Jan 11, 2020

Use fs.copyFile with UV_FS_COPYFILE_FICLONE flag when ready pnpm/pnpm#1066

Closed

2 tasks

stale bot added the stale label Jan 13, 2020

netroy mentioned this pull request May 14, 2020

lambda.AssetCode() does not preserve file permissions aws/aws-cdk#7749

Closed

Artoria2e5 mentioned this pull request Jun 15, 2020

Consider hardlinks rather than separate copy of packages per app yarnpkg/yarn#499

Closed

stale bot removed the stale label Jun 16, 2020

Artoria2e5 requested review from bnoordhuis and cjihrig June 16, 2020 09:38

stale bot added the stale label Jul 7, 2020

vtjnash mentioned this pull request Jul 31, 2020

File cloning not working on APFS #2936

Open

richardlau mentioned this pull request Aug 4, 2020

fs.copyFile creates empty file if source and destination are the same nodejs/node#34624

Closed

stale bot removed the stale label Jan 9, 2022

cjihrig approved these changes Feb 10, 2022

View reviewed changes

bnoordhuis reviewed Feb 10, 2022

View reviewed changes

stale bot added the stale label Apr 16, 2022

stale bot removed the stale label Apr 16, 2022

stale bot added the stale label Jun 12, 2022

stale bot removed the stale label Jun 12, 2022

cjihrig added the not-stale Issues that should never be marked stale label Jun 12, 2022

mischnic mentioned this pull request Jun 22, 2022

fs: bring back macos-specific copyfile(3) #3654

Merged

zkochan mentioned this pull request Jul 9, 2022

Fix file cloning on macOS pnpm/pnpm#5001

Closed

Merge branch 'v1.x' into mac-copyfile-mode

3aa638e

Artoria2e5 closed this Sep 7, 2022

bnoordhuis mentioned this pull request Sep 23, 2022

macOS binaries: UV_FS_COPYFILE_FICLONE not supported 18.9.0 nodejs/node#44747

Closed

sramam mentioned this pull request Jul 30, 2023

Running out of disk space on macos pnpm/pnpm#6880

Open

	if (can_clone == 0) {
	if (0 == uv__load_relaxed(&can_clone)) {

	can_clone = -1 + 2 * (major >= 16); /* macOS >= 10.12 */
	/* macOS >= 10.12 */
	uv__store_relaxed(&can_clone, -1 + 2 * (major >= 16));

fs: bring back macos-specific copyfile(3) #2578

fs: bring back macos-specific copyfile(3) #2578

Conversation

Artoria2e5 commented Dec 16, 2019 • edited

Artoria2e5 commented Dec 16, 2019 • edited

bnoordhuis left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artoria2e5 commented Dec 17, 2019 • edited

Artoria2e5 commented Dec 17, 2019 • edited

cjihrig commented Dec 17, 2019

Artoria2e5 commented Dec 17, 2019 • edited

richardlau commented Dec 17, 2019

Artoria2e5 commented Dec 17, 2019 • edited

stale bot commented Jan 13, 2020

stale bot commented Jul 7, 2020

cjihrig commented Aug 9, 2020

zkochan commented Jan 9, 2022

Jarred-Sumner commented Jan 13, 2022

bnoordhuis commented Jan 13, 2022

uvesten commented Jan 14, 2022

Jarred-Sumner commented Jan 14, 2022 via email

bnoordhuis commented Feb 8, 2022

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 16, 2022

zkochan commented Apr 16, 2022

FezVrasta commented Apr 16, 2022

bnoordhuis commented Apr 16, 2022

stale bot commented Jun 12, 2022

zkochan commented Jun 12, 2022

cjihrig commented Jun 12, 2022

bnoordhuis commented Jun 12, 2022

mischnic commented Jun 22, 2022

Artoria2e5 commented Sep 7, 2022

Artoria2e5 commented Dec 16, 2019 •

edited

Artoria2e5 commented Dec 16, 2019 •

edited

Artoria2e5 commented Dec 17, 2019 •

edited

Artoria2e5 commented Dec 17, 2019 •

edited

Artoria2e5 commented Dec 17, 2019 •

edited

Artoria2e5 commented Dec 17, 2019 •

edited