New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for copyfile on Darwin #886

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@mre
Copy link

mre commented Jan 2, 2018

This PR adds support for the copyfile syscall. It is similar to splice on Linux and offers fast copying of pages in kernel-space.

Right now, the tests fail, because copyfile_state_t is not a struct, but a pointer.
I thought, using state: *mut copyfile_state_t in the function definition might be the way to go, but that doesn't work. Any help would be appreciated here.

Also, once this is fixed I also want to add fcopyfile, which should work on FreeBSD, too.
There are some other helper functions defined in the manpage (e.g. copyfile_state_alloc, copyfile_state_get,...) and we could add them, too if we want.

mre added some commits Dec 31, 2017

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 2, 2018

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 2, 2018

📌 Commit 9ddd523 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 2, 2018

⌛️ Testing commit 9ddd523 with merge 137b757...

bors added a commit that referenced this pull request Jan 2, 2018

Auto merge of #886 - mre:copyfile, r=alexcrichton
Add support for copyfile on Darwin

This PR adds support for the [`copyfile`](https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/copyfile.3.html) syscall. It is similar to [splice](http://man7.org/linux/man-pages/man2/splice.2.html) on Linux and offers fast copying of pages in kernel-space.

Right now, the tests fail, because `copyfile_state_t` is not a struct, but a pointer.
I thought, using `state: *mut copyfile_state_t` in the function definition might be the way to go, but that doesn't work. Any help would be appreciated here.

Also, once this is fixed I also want to add `fcopyfile`, which should [work on FreeBSD](https://stackoverflow.com/a/2180157/270334), too.
There are some other helper functions defined in the [manpage](https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man3/copyfile.3.html) (e.g. `copyfile_state_alloc`, `copyfile_state_get`,...) and we could add them, too if we want.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 2, 2018

💔 Test failed - status-travis

@mre

This comment has been minimized.

Copy link

mre commented Jan 3, 2018

Humm... don't really know what to try next. Any ideas @alexcrichton?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 3, 2018

@mre it looks like the verification program, libc-test, failed? I think maybe it's a missing header file?

@mre

This comment has been minimized.

Copy link

mre commented Jan 4, 2018

I thought copyfile.h would be enough. It was already added to the build script here and according to the docs, that should be all.

For a test, I also tried adding this:

cfg.header("sys/cdefs.h");
cfg.header("stdint.h");

These are the includes from the copyfile headerfile, but that didn't help. 🤔

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 4, 2018

Sorry I dunno myself, I'm not too familiar with this API

@mre

This comment has been minimized.

Copy link

mre commented Jan 6, 2018

Just in case anybody wants to give it a shot, here's the first error message I'm currently struggling with:

cargo:warning=/Users/travis/build/rust-lang/libc/target/i686-apple-darwin/debug/build/libc-test-f149ec9caa38d75d/out/main.c:10040:28: error: offsetof requires struct, union, or class type, 'copyfile_state_t' (aka 'struct _copyfile_state *') invalid
cargo:warning=                    return offsetof(copyfile_state_t, src);
cargo:warning=           ```

@mre mre referenced this pull request Jan 6, 2018

Closed

Add support for BSD's copyfile? #803

@asomers

This comment has been minimized.

Copy link
Contributor

asomers commented Jan 7, 2018

The author of that stackoverflow post is completely wrong when he says that fcopyfile is supported on FreeBSD.

@Susurrus

This comment has been minimized.

Copy link
Contributor

Susurrus commented Jan 7, 2018

The problem here is that you didn't implement the C API exactly as it is. In your Rust code copyfile_state_t refers to an entire struct while in the C code it refers to a _copyfile_state *. Does that make sense? You should change your code to also declare the _copyfile_state struct and also a typedef to use for copyfile_state_t.

@alexcrichton Is that the right way to go about this? I've not seen an API like this reproduced in libc, where it tries to hide the underlying type through a pointer typedef. The header says "this API is not stable" and I'm worried about if we implement this that the libc stability guarantees will bite us in the butt in the future.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 8, 2018

Hm yeah if possible I think it'd be best to avoid binding unstable APIs (leave them to downstream consumers), but @mre did you have a use case in mind when adding this?

@mre

This comment has been minimized.

Copy link

mre commented Jan 8, 2018

Thanks @asomers. Just tested on FreeBSD 10.3 and it doesn't work indeed. Added a comment to the StackOverflow thread.

Thanks for your suggestions @Susurrus. Highly appreciated! I tried to change the code according to your suggestions. It still doesn't build, but I'm pretty convinced it's entirely my fault. I've updated the PR. Currently, the first error message is:

cargo:warning=/Users/mendler/Code/upstream/libc/target/debug/build/libc-test-50dafc47d3c66791/out/main.c:9959:74: error: use of undeclared identifier '_copyfile_state_t'; did you mean 'copyfile_state_get'?
cargo:warning=            uint64_t __test_size__copyfile_state_t(void) { return sizeof(_copyfile_state_t); }
cargo:warning=                                                                         ^~~~~~~~~~~~~~~~~
cargo:warning=                                                                         copyfile_state_get

Maybe that has something to do with the underscore in '_copyfile_state_t?

@alexcrichton, @Susurrus: the API is marked as unstable, but it has been unchanged since ten years. I doubt that it will change in the future. If anything, it might be removed eventually. 😉 But so far, I don't know of any removal plans or alternatives.
Basically, it's the only way to copy files performantly, without going through userspace, similar to splice and sendfile for Linux. This stuff is nice to have for building webservers. NGINX supports sendfile, for example (but not copyfile).
My use-case is rather simple: implement a faster cat.

I'm totally fine if we don't want to merge that into libc. We could host that in a separate crate. For that, I would basically take libc, strip out everything that is not needed, and push it to crates.io. Your call.
Would still be happy for any input on how to get it working, though. 😁

@asomers

This comment has been minimized.

Copy link
Contributor

asomers commented Jan 8, 2018

@mre are you sure copyfile(3) is zero-copy? The man page doesn't say anything about zero-copy, splice, or page flipping. Indeed, the fact that the man page is in section 3 and there's no corresponding entry in section 2 suggests that there are no special system calls going on. I don't think it's zero-copy at all. I think it's basically just cp(1) turned into a library routine. It's main benefit seems to be that it knows how to copy a file's extended attributes.

@mre

This comment has been minimized.

Copy link

mre commented Jan 8, 2018

@asomers, you might be right. My initial assumption was based on this answer on StackOverflow:

The following code snippet should work on the most OS X (as of 10.5), (Free)BSD, and Linux (as of 2.6.33). The implementation is "zero-copy" for all platforms, meaning all of it is done in kernelspace and there is no copying of buffers or data in and out of userspace. Pretty much the best performance you can get.

I did some research now, and I also can't find any evidence of zero-copy. Therefore I stated the question on the SO thread. Lesson learned.
Thanks again for the input y'all. I guess we can close this. Seems like there's no way to do file-to-file zero-copy on Mac right now.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 10, 2018

Ah ok in that case I'll close for now, but we can always reopen later!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment