-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
[DO NOT MERGE][LevelDB] Do no crash if filesystem can't fsync #10000
Conversation
To be clear: this happens only happens while syncing a directory handle in
I'd say we should warn once when this happens, that data syncing can not be guaranteed over network filesystems. Even if it succeeds all that fsync can (generally) guarantee with network filesystems is that the data has been written to the server, not that the server has written it to disk However if there is no access to a logger from there it becomes difficult... Edit: Likely we want to run this through https://github.com/bitcoin-core/leveldb (but it's fine to have the PR open here for visibility, it just shouldn't be merged directly). |
Shouldn't we at least log a warning in this case? Maybe even fail if pruning is enabled? (Presumably it means a crash is likely to corrupt the index?) |
It should not crash. There is reasons to use shared folder, one is in my posted issue. I agree for a warning. We can attempt fsync into bitcoin code on a dummy folder to see if it is enabled, and if not, provide a warning. |
Pruning nodes don't have data to reindex from. I know if I downloaded 100 GB, and then had to re-do the download due to this, I'd be pretty annoyed... Better to find out sooner so I can use a different filesystem and avoid the re-download. |
No, it should never crash/fail on this. If people run a datadirectory on a network FS they take some extra risk. There's nothing that we can do in that case (apart from warn). |
Yeah - maybe just try fsync() on the data directory at startup? No need even to make a dummy dir. |
for using Another way is to test in |
To do that, best is probably to check separately
Yes. Unfortunately they don't deal in log levels. Making an exception for a single message is indeed ugly. You could do it linux-kernel |
The easiest would be prefixed log. The fsync on a folder is only done if Sync is called on a MANIFEST file. Which seems to be rather rare. Will check my assertion soon. |
The Linux manpage says
So this seems only important after adding new files (or deleting them), which is probably why it's only done on MANIFEST changes. |
So I think we do not really to show a log as this is rare occurence. (I want to still check that with dbcache to 0 though) I also want to note also that on windows, running bitcoin core from a shared SMB folder works fine. |
I think in any case we should merge it. Windows version works fine on SMB file system, only the linux version crash. |
@NicolasDorier I agree |
@NicolasDorier What exactly are you doing to get the Windows version to work on a SMB share? In bug #10018 I had the exact opposite experience using a SMB share from a Server 2016 File Server Failover Cluster with a Server 2016 Remote Desktop Services server publishing Bitcoin Core as a RemoteApp. Did you map the share as a drive letter as opposed to using a UNC path? |
Yes I did. Indeed // path did not work, I think this is why I did it. |
Where to ACK that, here or in bitcoin-core/leveldb-subtree#16 ? |
merged on bitcoin-core/leveldb-subtree#16 |
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier.
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier.
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier.
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier, ported to leveldb 1.22 by Wladimir J. ban der Laan.
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier, ported to leveldb 1.22 by Wladimir J. van der Laan.
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier, ported to leveldb 1.22 by Wladimir J. van der Laan.
This code moved, re-apply the fix elsewhere. See - bitcoin/bitcoin#10000 - bitcoin-core/leveldb-old#16 Original change by Nicolas Dorier, ported to leveldb 1.22 by Wladimir J. van der Laan.
Solve #9996.
There should be some kind of warning if fsync is not supported. It is unclear how to implement that easily though since we don't have access to any logger here.
I encountered the problem as I could not run bitcoin on a network share on cifs. (I could previously, which is strange)
It seems that https://www.kernel.org/doc/Documentation/filesystems/cifs/CHANGES Version 1.57 seems to indicate that cifs should not fail, but just not guarantee that the server properly synched on the drive. This is not what I am experiencing.
Ping @laanwj