Skip to content

Conversation

@wafuwafu13
Copy link

O_SYNC means "Write operations on the file will complete according to the requirements of synchronized I/O file integrity completion"(ref).
So, setting O_RDONLY and O_SYNC at the same time is inconsistent.
According to the following test, rs has the same result as r.

/* r */
flags = add_flags | UV_FS_O_RDONLY;
openFail(absent_file, UV_ENOENT);
writeFail(empty_file, UV_EBADF);
readExpect(empty_file, "", 0);
writeFail(dummy_file, UV_EBADF);
readExpect(dummy_file, "a", 1);
writeFail(empty_dir, UV_EBADF);
readFail(empty_dir, UV_EISDIR);
/* rs */
flags = add_flags | UV_FS_O_RDONLY | UV_FS_O_SYNC;
openFail(absent_file, UV_ENOENT);
writeFail(empty_file, UV_EBADF);
readExpect(empty_file, "", 0);
writeFail(dummy_file, UV_EBADF);
readExpect(dummy_file, "a", 1);
writeFail(empty_dir, UV_EBADF);
readFail(empty_dir, UV_EISDIR);

Also, rs is not in document https://nodejs.org/api/fs.html#file-system-flags .

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run. labels Sep 30, 2022
@mscdex
Copy link
Contributor

mscdex commented Sep 30, 2022

Changes to libuv need to be made upstream, not in this repo.

@mscdex
Copy link
Contributor

mscdex commented Sep 30, 2022

It may be better to instead treat 'rs'/'sr' as 'r' for better backwards compatibility, otherwise this could be a breaking change (I'm not sure how widespread 'rs'/'sr' is) since it will start throwing an error.

@wafuwafu13 wafuwafu13 changed the title fs: delete rs flag from internal code fs: define rs flag as O_RDONLY, not O_RDONLY | O_SYNC Sep 30, 2022
@wafuwafu13 wafuwafu13 changed the title fs: define rs flag as O_RDONLY, not O_RDONLY | O_SYNC fs: define rs flag as O_RDONLY, not O_RDONLY | O_SYNC Sep 30, 2022
@aduh95
Copy link
Contributor

aduh95 commented Oct 1, 2022

/cc @nodejs/fs

@bnoordhuis
Copy link
Member

I don't think you can say O_RDONLY and O_RDONLY|O_SYNC are equivalent. O_SYNC implies metadata is written to disk synchronously. That includes the access time.

@wafuwafu13
Copy link
Author

I thought it was O_DSYNC that is related to hardware, but now I understand that O_SYNC also has meaning other than reading and writing files.

https://man7.org/linux/man-pages/man2/open.2.html

O_SYNC provides synchronized I/O file integrity completion,
meaning write operations will flush data and all associated
metadata to the underlying hardware. O_DSYNC provides
synchronized I/O data integrity completion, meaning write
operations will flush data to the underlying hardware, but will
only flush metadata updates that are required to allow a
subsequent read operation to complete successfully

Before Linux 2.6.33, Linux implemented only the O_SYNC flag for
open(). However, when that flag was specified, most filesystems
actually provided the equivalent of synchronized I/O data
integrity completion (i.e., O_SYNC was actually implemented as
the equivalent of O_DSYNC).

@wafuwafu13 wafuwafu13 closed this Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants