Skip to content
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

doc: document fs.fdatasync(Sync) #5402

Closed
wants to merge 1 commit into from

Conversation

ronkorving
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

fs

Description of change

The fs.fdatasync and fs.fdatasyncSync APIs are implemented,
but are currently not documented. This adds the documentation.

@rvagg
Copy link
Member

rvagg commented Feb 24, 2016

Perhaps there's a reason they are not documented? @bnoordhuis @indutny @saghul any ideas? Is there any platform variability in this API that may be worth documenting?

@Fishrock123
Copy link
Contributor

What exactly is this? My Mac doesn't appear to have it. (well, no man page.)

@mscdex mscdex added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Feb 24, 2016
@mscdex mscdex changed the title doc: document fs.datasync(Sync) doc: document fs.fdatasync(Sync) Feb 24, 2016
@rvagg
Copy link
Member

rvagg commented Feb 24, 2016

implemented using fcntl on OSX using F_FULLFSYNC:

     F_FULLFSYNC        Does the same thing as fsync(2) then asks the drive to flush all buffered data to the permanent storage device (arg is ignored).  This is currently
                        implemented on HFS, MS-DOS (FAT), and Universal Disk Format (UDF) file systems.  The operation may take quite a while to complete.  Certain FireWire
                        drives have also been known to ignore the request to flush their buffered data.

fdatasync(2) is:

       fsync() transfers ("flushes") all modified in-core data of (i.e., modified buffer cache pages for) the file referred to by the file descriptor fd to the disk device (or other perma‐
       nent storage device) so that all changed information can be retrieved even after the system crashed or was rebooted.  This includes writing through  or  flushing  a  disk  cache  if
       present.  The call blocks until the device reports that the transfer has completed.  It also flushes metadata information associated with the file (see stat(2)).

See http://linux.die.net/man/2/fdatasync

Implemented using FlushFileBuffers() on Windows https://msdn.microsoft.com/en-us/library/windows/desktop/aa364439%28v=vs.85%29.aspx

Flushes the buffers of a specified file and causes all buffered data to be written to a file.

They all seem equivalent to me but perhaps there's some variability that would impact on publicising it.

@ronkorving
Copy link
Contributor Author

There's a possibility I actually want to use this on a project, so I hope we can make these public.

@saghul
Copy link
Member

saghul commented Feb 24, 2016

IIRC they are all equivalent except for the OSX implementation, which is "stronger". Btw, should fdatasync(2) be a link to some online man page?

@ronkorving
Copy link
Contributor Author

I pretty much copied the fs.fsync(Sync) documentation. So if we want to link, we should probably do that across the board in a separate PR.

As far as I understand it, fdatasync turns into fsync if filesize or file meta data has changed. Or perhaps these days it really is the same. But as long as the syscalls are there, and it's supported by libuv, why not expose it? It's in the OS, and I'm guessing it's not likely to go anywhere.

In any case, are you considering removing it from libuv? If not, I think we can document it for Node.

@bnoordhuis
Copy link
Member

Perhaps there's a reason they are not documented?

Don't think so, probably just an oversight. It's been around since at least node.js v0.4.

In any case, are you considering removing it from libuv?

Nope, it's supported and documented.

As far as I understand it, fdatasync turns into fsync if filesize or file meta data has changed.

fdatasync() just flushes data (what you've written), not metadata (access time, modify time, etc.) Changed metadata that has an effect on data (e.g. file size) is flushed, however.

Apropos F_FULLFSYNC, we use that because OS X 10.5 didn't have a fdatasync() system call. We don't support 10.5 anymore so I think we can just switch it over to the real fdatasync().

@@ -436,6 +436,15 @@ to the completion callback.

Synchronous fchown(2). Returns `undefined`.

## fs.fdatasync(fd, callback)

Asynchronous fdatasync(2). No arguments other than a possible exception are given
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit: I think this line is just over 80 columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

81, you're right :)
Will fix. There are more lines in this file beyond 80 chars btw, but that's for another PR.

@bnoordhuis
Copy link
Member

LGTM with nit.

@saghul
Copy link
Member

saghul commented Feb 24, 2016

Apropos F_FULLFSYNC, we use that because OS X 10.5 didn't have a fdatasync() system call. We don't support 10.5 anymore so I think we can just switch it over to the real fdatasync().

AFAIS there is still no fdatasync in OSX:

$  grep -R fdatasync /usr/include/
/usr/include//python2.6/pyconfig.h:/* Define if you have the 'fdatasync' function. */
/usr/include//python2.6/pyport.h:extern int fdatasync(int);
/usr/include//python2.7/pyconfig.h:/* Define if you have the 'fdatasync' function. */
/usr/include//python2.7/pyport.h:extern int fdatasync(int);
/usr/include//sys/syscall.h:#define SYS_fdatasync      187

@bnoordhuis
Copy link
Member

There's no libc wrapper, only the system call (i.e. syscall(SYS_fdatasync, fd)).

@saghul
Copy link
Member

saghul commented Feb 24, 2016

Yep, I saw that; I guess we can just use the syscall straight.

@rvagg
Copy link
Member

rvagg commented Feb 24, 2016

well in that case, this lgtm, the details of how it's called on osx are a matter for libuv

saghul added a commit to saghul/libuv that referenced this pull request Feb 24, 2016
saghul added a commit to saghul/libuv that referenced this pull request Feb 24, 2016
Refs: nodejs/node#5402
PR-URL: libuv#732
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The APIs are implemented but currently not documented.
@ronkorving
Copy link
Contributor Author

Nit addressed.

@rvagg
Copy link
Member

rvagg commented Feb 25, 2016

lgtm

bnoordhuis pushed a commit that referenced this pull request Feb 25, 2016
The APIs are implemented but currently not documented.

PR-URL: #5402
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@bnoordhuis
Copy link
Member

Thanks Ron, landed in 4578902. I did a git commit --amend --author=... on the commit, you were showing up as Author: ronkorving <...>.

@bnoordhuis bnoordhuis closed this Feb 25, 2016
@ronkorving ronkorving deleted the doc-fs-datasync branch February 25, 2016 11:54
@ronkorving
Copy link
Contributor Author

Thanks for merging and thanks for the notice. I've changed my git name to "Ron Korving" so this shouldn't happen again.

rvagg pushed a commit that referenced this pull request Feb 27, 2016
The APIs are implemented but currently not documented.

PR-URL: #5402
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
The APIs are implemented but currently not documented.

PR-URL: #5402
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
@MylesBorins
Copy link
Contributor

Is it safe to assume this should be backported?

@rvagg
Copy link
Member

rvagg commented Mar 10, 2016

yep, please do @thealphanerd

MylesBorins pushed a commit that referenced this pull request Mar 10, 2016
The APIs are implemented but currently not documented.

PR-URL: #5402
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
The APIs are implemented but currently not documented.

PR-URL: #5402
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
The APIs are implemented but currently not documented.

PR-URL: #5402
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants