Fix locking on WSL using flock instead of fcntl

meshcollider commented at 0:35 am on April 19, 2020: contributor

A bug in WSL means that fcntl does not exclusively lock files, allowing multiple instances of bitcoin to use the same datadir. If we instead use flock, it works correctly. Passes Travis, but testing on some OS variety would be sensible.

From what I can tell, flock and fcntl don’t work with each other on linux, so it would still be possible to run a node with this code change and a node before it with the same datadir (this isn’t true for Mac/FreeBSD). flock also doesn’t support NFS on MacOS and linux<2.6.12 while fcntl did. See here for example: https://gavv.github.io/articles/file-locks/

If changing to flock for all systems is inadvisable, it would also be possible to just detect WSL and use flock when on that platform to avoid the bug.

fanquake added the label Windows on Apr 19, 2020

meshcollider added this to the milestone 0.20.0 on Apr 19, 2020

fanquake added the label Utils/log/libs on Apr 19, 2020

MarcoFalke added the label Needs gitian build on Apr 19, 2020

DrahtBot commented at 4:43 am on April 20, 2020: member

Gitian builds

File	commit 6ae99aab5d97b06d46ff940111b290f1eeb90045(master)	commit 1a36387be3c7417f879aaeebcf2c5e95065b1b3f(master and this pull)
bitcoin-0.20.99-aarch64-linux-gnu-debug.tar.gz	`9e74baea5087f75d...`	`dd9caf6fa1126a3f...`
bitcoin-0.20.99-aarch64-linux-gnu.tar.gz	`b97f8fed4ef242bc...`	`5a3144105bfe97ca...`
bitcoin-0.20.99-arm-linux-gnueabihf-debug.tar.gz	`c3cafca0d2c385b7...`	`16c59265a3f49440...`
bitcoin-0.20.99-arm-linux-gnueabihf.tar.gz	`9cf7842f6c7e72f9...`	`d3117c583c9ae443...`
bitcoin-0.20.99-osx-unsigned.dmg	`b8882f2b1e257bee...`	`279210aeb73a73ea...`
bitcoin-0.20.99-osx64.tar.gz	`9100c2ace54ffeb2...`	`fc5834c15be4b962...`
bitcoin-0.20.99-riscv64-linux-gnu-debug.tar.gz	`e3794553bdeb3406...`	`2f6bad18dccf3b6f...`
bitcoin-0.20.99-riscv64-linux-gnu.tar.gz	`3d60c7b6ad3a3b7a...`	`05d470ffe6c8efac...`
bitcoin-0.20.99-win64-debug.zip	`923a2a8d708b1196...`	`33b8e7fbaabee227...`
bitcoin-0.20.99-win64-setup-unsigned.exe	`4fd60fb4ee6d252a...`	`e32c5565b0039492...`
bitcoin-0.20.99-win64.zip	`014a0e245da57de1...`	`f74643de10c60c9c...`
bitcoin-0.20.99-x86_64-linux-gnu-debug.tar.gz	`6b72060bfdf9cd8a...`	`21ddfffa1ee0f8e9...`
bitcoin-0.20.99-x86_64-linux-gnu.tar.gz	`d2e0d4726ecd84f6...`	`9eec3e7ef9c754bc...`
bitcoin-0.20.99.tar.gz	`5f8eaab85a59877b...`	`fe3dd2ab68a6220f...`
bitcoin-core-linux-0.21-res.yml	`b9c0b8516f1f7476...`	`799ccd04f4502d7c...`
bitcoin-core-osx-0.21-res.yml	`aa56d3073ba6853c...`	`fa86f0cc5bfb2217...`
bitcoin-core-win-0.21-res.yml	`127d7553b4c8553c...`	`938bc5b198d8f409...`
linux-build.log	`826d7ba2297e1251...`	`5d1e9e9b6410d255...`
osx-build.log	`a889d7e96df8cc6a...`	`4023f21916323bf7...`
win-build.log	`6311c7e3c77d0b67...`	`37762874f39f9ee8...`
bitcoin-core-linux-0.21-res.yml.diff		`eeb2e55b504104ca...`
bitcoin-core-osx-0.21-res.yml.diff		`c465a6caf5f4f7d5...`
bitcoin-core-win-0.21-res.yml.diff		`6e7e44435c9ef0a9...`
linux-build.log.diff		`b6ed5e37226cd11c...`
osx-build.log.diff		`67b8c8e6f6898f29...`
win-build.log.diff		`c39e17d5668588b9...`

DrahtBot removed the label Needs gitian build on Apr 20, 2020

laanwj commented at 11:07 am on April 22, 2020: member

If this is a bug in WSL, why not wait for them to fix it instead? I don’t think it’s desirable to change our handling of locks on all UNIX platforms just for a bug in their implementation. If we need is, please add it for WSL only and add a comment that it can be removed again when the upstream bug is fixed (with a link to the upstream bug).

laanwj commented at 11:15 am on April 22, 2020: member

Can you please test that this fixes your issue @mtrycz.

meshcollider commented at 12:24 pm on April 22, 2020: contributor

@laanwj the bug was reported upstream in 2017 with no fix in sight, so it seems it is up to us

mtrycz commented at 1:27 pm on April 22, 2020: none

I’ve ran my test plan (from the related issue), and I can confirm that the test fails in the latest master 9e8e813d and the v0.19.1 tag, and that it passes in this Pull Request.

However, I’m running more extended tests and some are failing. Please allow some time to make an adequate description.

wumpus commented at 3:30 pm on April 22, 2020: none

Github @wumpus is not bitcoin wumpus.

in src/fs.cpp:61 in eb06694f56 outdated

64-        reason = GetErrorReason();
65-        return false;
66+    // Exclusive file locking is broken on WSL using fcntl (issue #18622)
67+    // This workaround can be removed once the bug on WSL is fixed
68+    struct utsname uname_data;
69+    if (uname(&uname_data) == 0 && std::string(uname_data.version).find("Microsoft") != std::string::npos) {

sipa commented at 2:59 am on April 23, 2020:

This condition should probably be cached in a static bool local variable.

EDIT: and only conditionally compiled for WIN32?

meshcollider commented at 4:45 am on April 23, 2020:

@sipa it isn’t “win32” because its the linux subsystem, so it runs in the non-windows branch

sipa commented at 7:27 am on April 23, 2020:

Ugh.

sipa commented at 8:40 am on April 23, 2020:

Still, please cache the result.

mtrycz commented at 7:30 am on April 23, 2020: none

I have ran test/functional/test_runner.py several times both on master and this PR.

The specific issue is solved for WSL on this PR’s code.

There are also several test failures (on both branches) that are reproducible, and some that are non-deterministic. If this sounds new, should I open a new issue with this?

in src/fs.cpp:62 in e86aaa641f outdated

65-        return false;
66+
67+    // Exclusive file locking is broken on WSL using fcntl (issue #18622)
68+    // This workaround can be removed once the bug on WSL is fixed
69+    // Cache the result to avoid multiple system calls
70+    static bool is_wsl_cache_exists = false;

sipa commented at 1:20 am on April 24, 2020:

It’s a crazy edge case, and unlikely on realistic platforms, but this is technically UB if two threads simultaneously reach the if (!is_wsl_cache_exists) check. The most C++ish solution is this I think: https://github.com/sipa/bitcoin/commit/9470c59e7290774972f94250d04caae161bc7678.

luke-jr commented at 9:13 pm on April 24, 2020: member

Is there any reason we can’t check/take both locks?

Fix WSL file locking by using flock instead of fcntl

Co-authored-by: sipa <pieter@wuille.net>

e8fa0a3d20

meshcollider commented at 0:17 am on April 26, 2020: contributor

Thanks @sipa, updated :)

laanwj commented at 12:13 pm on May 11, 2020: member

I’m removing this from the 0.20.0 milestone. It’s not a regression in 0.20, it’s an upstream bug in “Linux emulation”, ~~and I think this fix is not quite ready yet.~~

laanwj removed this from the milestone 0.20.0 on May 11, 2020

laanwj added this to the milestone 0.20.1 on May 11, 2020

laanwj commented at 3:18 pm on May 28, 2020: member

Code review ACK e8fa0a3d2025509fcddc59fc618e91371542cf87

laanwj merged this on May 28, 2020

laanwj closed this on May 28, 2020

in src/fs.cpp:53 in e8fa0a3d20

49@@ -47,20 +50,38 @@ FileLock::~FileLock()
50     }
51 }
52 
53+static bool IsWSL()

laanwj commented at 3:57 pm on May 28, 2020:

I just realized after merging that we could shortcut this check to false on non-x86 platforms. But it doesn’t matter too much.

MarcoFalke added the label Needs backport (0.20) on May 28, 2020

MarcoFalke commented at 4:17 pm on May 28, 2020: member

Tagged with " Needs backport (0.20) "

sidhujag referenced this in commit 101dfbf298 on May 29, 2020

deadalnix referenced this in commit 5cf3c08e99 on May 29, 2020

fanquake referenced this in commit febebc4ea6 on Jun 9, 2020

fanquake removed the label Needs backport (0.20) on Jun 9, 2020

ComputerCraftr referenced this in commit 414a47cad7 on Jun 10, 2020

cculianu commented at 11:10 am on June 18, 2020: none

Note that this patch only answers the question IsWSL? for WSL ver 1. Ver 2 returns a totally different uts_name.version string as such:

WSL2 uts_name.version string: #1 SMP Wed Feb 19 06:37:35 UTC 2020

0$ uname -a
1Linux DESKTOP-TPEL8HN 4.19.104-microsoft-standard [#1](/bitcoin-bitcoin/1/) SMP Wed Feb 19 06:37:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

I am running on Win10 WSL2 with Ubuntu 20.04. See the attached C++ test program to convince yourself.

 0#include <iostream>
 1#include <string>
 2
 3#include <sys/file.h>
 4#include <sys/utsname.h>
 5
 6static bool IsWSL(std::string *out = nullptr) {
 7	struct utsname uname_data;
 8	auto res = uname(&uname_data);
 9	std::string ver{uname_data.version};
10	bool ret = res == 0 && ver.find("Microsoft") != std::string::npos;
11	if (out) *out = std::move(ver);
12	return ret;
13}
14
15int main()
16{
17	using std::cout;
18
19	std::string ver;
20	auto b = IsWSL(&ver);
21	cout << "Ver: " << ver << '\n';
22	cout << "IsWSL?: " << b << '\n';
23	return 0;
24}

I don’t know if the bug #18622 is still an issue on WSL2. I’m going to try and determine this and write back here when I do.

cculianu commented at 11:38 am on June 18, 2020: none

Follow-up: Can confirm that the upstream bug from microsoft on fcntl F_SETLK is fixed on WSL2. The linux/unix execution path that is taken on WSL2 (because IsWSL() returns false here) leads to correct behavior and the lock succeeds.

Suggest: Rename this function to IsWSL1 to make it clearer when it will return true and/or add a comment to that effect…

fanquake referenced this in commit 01c563708f on Jul 10, 2020

ftrader referenced this in commit e1e0d255e1 on Aug 17, 2020

backpacker69 referenced this in commit c98452eccf on Sep 8, 2020

Bushstar referenced this in commit cb55273e4e on Oct 21, 2020

Platinumwrist referenced this in commit 05e2740d92 on Oct 25, 2020

PastaPastaPasta referenced this in commit bc39351780 on Jun 27, 2021

PastaPastaPasta referenced this in commit eb4e0bb582 on Jun 28, 2021

PastaPastaPasta referenced this in commit db1964de24 on Jun 29, 2021

PastaPastaPasta referenced this in commit 72d1c13917 on Jul 1, 2021

PastaPastaPasta referenced this in commit 9307c10712 on Jul 1, 2021

PastaPastaPasta referenced this in commit a5fe159373 on Jul 14, 2021

PastaPastaPasta referenced this in commit afd217904a on Jul 15, 2021

random-zebra referenced this in commit c2b36fe7a2 on Aug 9, 2021

DrahtBot locked this on Feb 15, 2022

Fix locking on WSL using flock instead of fcntl #18700

Gitian builds