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

Call syncfs() on IDBFS when the C fsync() is called #2492

Closed
kripken opened this issue Jul 7, 2014 · 10 comments · Fixed by #3398
Closed

Call syncfs() on IDBFS when the C fsync() is called #2492

kripken opened this issue Jul 7, 2014 · 10 comments · Fixed by #3398

Comments

@kripken
Copy link
Member

kripken commented Jul 7, 2014

This seems reasonable to do, as IDBFS does have the a concept of writing out to storage. However, it is asynchronous which may be confusing.

@kmaccara2
Copy link

When using IDBFS to persist data, calling fsync() from a C/C++ program compiled through emscripten does nothing. Data that is persisted using IndexedDB (IDBFS) is written to a memory mount of the FS and is only synced to disk when syncfs() is called.

There are several issues with these semantics:

  1. The program calling fsync() expects that after this synchronous call, the data will be safely persisted to disk. Which is not actually the case.
  2. syncfs() is an asynchronous call
  3. syncfs() syncs the entire IDBFS to disk, which can be costly.

C/C++ programs tied to fsync() semantics need a synchronous solution to actually flush changes to disk without needing to sync the entire FS which may, or may not, have other changes which are not ready to be persisted.

@inolen
Copy link
Collaborator

inolen commented Jul 17, 2014

This was always unfortunate, which is why I never made it part of fsync.

I went the route of refactoring my code base to work with the callback of FS.syncfs for emscripten builds.

@kripken
Copy link
Member Author

kripken commented Jul 17, 2014

Yes, there doesn't seem to be much more we can do here, given how IndexedDB works. We do our best to provide something that looks like a "normal" filesystem, but it can't be perfect...

@juj juj added the filesystem label Jul 28, 2014
@kripken
Copy link
Member Author

kripken commented Aug 12, 2014

Going back to the list of 3 items in the 2nd comment, 1 and 2 refer to the asynchronous aspect of this operation. We can't do much about that - syncing to IDB is asynchronous. syncfs provides a callback which can be used (in a later browser main loop iteration) to be asynchronously notified of when the contents are flushed.

Item 3 refers to it syncing the entire filesystem. If this is costly, we could look into ways to create an API that allows syncing a folder or a file under an IDBFS mount. Do we have data showing that this is needed? (I would hope that browser IDB implementations run in the background and only process data that has actually changed, but I could be wrong.)

@timdream
Copy link
Contributor

timdream commented Feb 5, 2015

I would like to take on this issue, as part of approach to fix dreamlayers/em-dosbox#4. Not able to block fsync call is worse than ignoring it, so I think we should hook it up even if we can't return synchronously. My proposed fix will hook the call.

There is one problem though on item 3: whether or not to sync the entire file system. I am open to either option but I would like to know what's acceptable before I work on a patch. @kripken what's your opinion here?

(Without reading the patch my previous approach was to implement a AUTO_SYNCFS feature, if that's acceptable I will submit a pull request.)

@kripken
Copy link
Member Author

kripken commented Feb 8, 2015

I think syncfs works on a single mount? Sounds ok to sync a whole mount at a time, people can split them up if they want to sync separately.

Note btw that em-dosbox has experimented with using emterpreter+sync, which allows synchronous fsync. I don't think fsync is hooked up yet, but it would be easy, see recent work on emscripten_idb_* sync versions.

@timdream
Copy link
Contributor

timdream commented Feb 8, 2015

@kripken I assume your opinion is that using emterpreter+sync and call syncfs() on the mount is the acceptable impl? I will try to submit a patch to do that.

@timdream
Copy link
Contributor

timdream commented Feb 8, 2015

Hum, after reading EmterpreterAsync.handle() I can't figured out how fsync call could return an real error code asynchronously. I will finish up the tests and submit my original patch first.

@timdream
Copy link
Contributor

timdream commented May 7, 2015

Fixed by #3398? Should this issue be closed?

@kripken
Copy link
Member Author

kripken commented May 7, 2015

Yes, thanks.

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

Successfully merging a pull request may close this issue.

5 participants