nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.98k stars 29.79k forks source link

fs: Change the default value of `mode` argument of `fs.copyFile()` to `fs.constants.COPYFILE_FICLONE` #47861

Open tetsuharuohzeki opened 1 year ago

tetsuharuohzeki commented 1 year ago

What is the problem this feature will solve?

Today's operating systems and some file systems supports copy-on-write operation for copying a file (e.g. btrfs on Linux, APFS on Apple's Darwins).

fs.copyFile() has already supported them as opt-in behavior if the underlying platform support such operations and libuv support them.

However, it's still opt-in behavior.

The default value of mode argument. of fs.copyFile() is 0. This mean that fs.copyFile() does not use copy-on-write and use traditional copying mechanism operation even if the underlying platform and libuv supports such operation.

An user can use copy-on-write operation by passingfs.constants.COPYFILE_FICLONE to mode argument.

But if we forget to pass it, fs.copyFile() does not use an enhancement underlying mechanism. We (all programmers) tend to forget to supply a such optional flag to API and to lose a chance to improve a performance. I think this is a bug for API egonomics and having a value we should fix.

What is the feature you are proposing to solve the problem?

My proposal is changing the default value of mode argument of fs.copyFile() to fs.constants.COPYFILE_FICLONE from 0.

By this change, all exist programs using fs.copyFile() in all over the world can get a chance to improve their throughput without any code change.

If the underlying platform does not support copy-on-write behavior, then fs.constants.COPYFILE_FICLONE flag fallback into the traditional approach. I think this change is painless approach.

fs.constants.COPYFILE_FICLONE: The copy operation will attempt to create a copy-on-write reflink. If the platform does not support copy-on-write, then a fallback copy mechanism is used.

https://nodejs.org/api/fs.html#fspromisescopyfilesrc-dest-mode

What alternatives have you considered?

  1. This change might be a "breaking change" if we think it strictly. If there is a user program expect fs.copyFile() copy a file without copy-on-write, this change would be problematic.
    • I think it's not be a problem to change a file copying mechanism for user program. It's an abstracted problem by a platform (Node.js, operation system, and file system). An user program should be agnostic about it.
    • But I'm not sure about the Node.js' committee's breaking change policy, I note about this point as a consideration.
  2. This also affects the behavior of other APIs that uses fs.copyFile() internally (e.g. fs.cp()).
benjamingr commented 1 year ago

@nodejs/fs

LiviaMedeiros commented 1 year ago

This looks like a breaking change that is welcome to have in the very next major release.

Might be worth mentioning that coreutils defaults to --reflink=auto since 9.0 release in 2021-09-21 (along with other related changes: copy_file_range, SEEK_HOLE), so this would be consistent with modern cp behaviour.

bnoordhuis commented 1 year ago

I'm undecided on whether we should be making that the default. The problem with COW is that it moves ENOSPC errors from when the file is copied to when it's mutated, something that violates the Principle of Least Surprise.

tetsuharuohzeki commented 1 year ago

I totally follow this issue is a breaking change (Thank you for your guide!).

The problem with COW is that it moves ENOSPC errors from when the file is copied to when it's mutated, something that violates the Principle of Least Surprise.

I feel this point would be able to happen even if file mutation with fs.writeFile() or fs.appendFile() increases a file size on filesystem which does not support CoW operation. I think it cannot be a problem by marking this as a breaking change.

bnoordhuis commented 1 year ago

I hid the previous comment. Lots of words, little content, strong ChatGPT vibe.

edit: comment was deleted

github-actions[bot] commented 1 year ago

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

github-actions[bot] commented 11 months ago

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

sindresorhus commented 11 months ago

I also think that fs.constants.COPYFILE_FICLONE should be the default.

Can someone reopen so that we can have an official decision made on this?

puskin94 commented 2 months ago

Any more opinion on the topic?