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

fs.unlink should not remove nodes with open file descriptors #314

Open
humphd opened this issue Oct 17, 2014 · 7 comments
Open

fs.unlink should not remove nodes with open file descriptors #314

humphd opened this issue Oct 17, 2014 · 7 comments
Labels
Milestone

Comments

@humphd
Copy link
Contributor

humphd commented Oct 17, 2014

Follow-up from #311, unlink should not remove the file if there are open descriptors. Currently we only check nlinks < 1, but we should also test for relevant OFDs. From unlink(2):

If that name was the last link to a file and no processes have the file open the file is deleted and the space it was using is made available for reuse.

Doing this properly is tricky, since we need to somehow share ofd info across instances/tabs, and deal with the case that a fs instance doesn't close an open fd (e.g., crashes, tab closes early).

When we fix this, the test in tests/spec/fs.open.spec.js will need to get rewritten, since it currently assumes that unlink can remove a node while there are ofds.

@modeswitch
Copy link
Member

SessionStorage might be useful here as a shared OFD cache for multiple FS instances. Similar to the way we use it for watches.

@humphd
Copy link
Contributor Author

humphd commented Oct 20, 2014

So I thought about this, too. But I'm stuck on how we rely on the numbers when an instance dies unexpectedly. Can we count on beforeunload to always reduce the count by the correct amount? I'm unclear. If we can, then this is solved.

@modeswitch
Copy link
Member

We can't rely on beforeunload if power goes out.

Definitely don't want to delete a file that might have open descriptors, so better to move the node to a graveyard until we're sure the file can go. This means we need to improve startup checks to clean out deleted files and empty the OFD cache. If FS instances register themselves in localstorage, they can know if other instances are around and skip the cleanup.

It's worth noting that these problems go away when we can start using ServiceWorkers, as that will allow us to have a single FS instance that can serve multiple applications. Much more like a traditional file system.

@humphd
Copy link
Contributor Author

humphd commented Oct 20, 2014

Let's imagine two fs instances, A and B. A starts and opens a file /file. B starts and also opens /file, so we now have 2 open descriptors to /file. A crashes, which means we actually have a single descriptor, but B doesn't know that. How do we deal with this case? On unlink should B send a message to all other instances to ask how many open descriptors there are for a given path? How long do we wait to decide that A has died before proceeding (i.e., assuming 0)?

There are still some hard problems here.

@modeswitch
Copy link
Member

So when B unlinks the file, it sees the other OFD for A and makes a weak link to the file and removes the remaining link. Since A crashed, the OFD will remain until the application is restarted after all instances have been closed. Only then can we be sure that the file has no active references. This would only happen if the application fails to shutdown correctly, and I wouldn't optimize for that case.

By weak link, I mean a reference that keeps the file around while there are OFDs and normal links, but will allow the file to be removed when all OFDs and normal links go away. This could be done by linking the file to a special directory node that's only accessible from the supernode. The downside here is that metadata must be read when a file closes to see if there are links left. Maybe we should be doing that anyway.

@humphd
Copy link
Contributor Author

humphd commented Oct 20, 2014

Currently fs.close(fd) is sync, so changing that to be async would be a good start.

@humphd
Copy link
Contributor Author

humphd commented Oct 20, 2014

Actually, I'm wrong, it's already sync, see http://nodejs.org/api/fs.html#fs_fs_close_fd_callback

@modeswitch modeswitch added this to the 0.2 milestone Nov 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants