Skip to content

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

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.)

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

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
ContributorAuthor

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

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
ContributorAuthor

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().

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
ContributorAuthor

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

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

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

@rvagg
Copy link
Member

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
The APIs are implemented but currently not documented.
@ronkorving
Copy link
ContributorAuthor

Nit addressed.

@rvagg
Copy link
Member

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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
@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 <...>.

@ronkorvingronkorving deleted the doc-fs-datasync branch February 25, 2016 11:54
@ronkorving
Copy link
ContributorAuthor

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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
@Fishrock123Fishrock123 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

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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
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 <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@ronkorving@rvagg@Fishrock123@saghul@bnoordhuis@MylesBorins@mscdex