use fstatfs to detect whether current file system supports shared locks for files opened for writing

[adamsitnik]

I've spent entire week of my life thinking about file locking, so please excuse me if the following description is too grumpy.

Windows background

As we all know, .NET originally supported only Windows. Because of that, many of our APIs have a very strong Windows influence.
All available Windows API that allow for opening files (CreateFileW, NtCreateFile and more), require the caller to provide shareMode which is translated to a mandatory file lock:

• 0 (in .NET represented with FilShare.None): the file can't be opened for read, write or delete until the handle is closed.
• 1 (FileShare.Read): the file can be opened for reading.
• 2 (FileShare.Write): the file can be opened for writing.
• 4 (FileShare.Delete): the file can be opened for deleting.

Unix

Unix systems have a different approach to file locking. It's only advisory and requires an additional syscall (flock). flock offers two kind of locks:

• LOCK_EX - exclusive lock, only one process can hold it for a given file at a given time.
• LOCK_SH - shared lock, which should be translated to "nobody can request an exclusive lock". There can be multiple processes holding a shared lock on the same file.

Our mappig for FileShare is following: FileShare.None is translated to LOCK_EX, while everything else is mapped to LOCK_SH.

Depending on the File System and Kernel version the lock can be:

• local for NFS for kernels up to 2.6.11
• not propagated over SMB for kernels up to 5.4
• mandatory for CIFS since Linux 5.5

The problem

When we open a file for writing and use flock to acquire a shared lock, some file systems (NFS, CIFS and SMB) map it to a lock that... prevents ourselves from writing to the file.

The biggest issue is that in contrary to CIFS and SMB, NFS reports following writes as successful despite the fact that they actually fail to write anything to disk (#44546 (comment)):

openat(AT_FDCWD, "/home/hcsuser/mnt/14/second.txt", O_WRONLY|O_CREAT|O_CLOEXEC, 0666) = 20
fstat(20, {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
flock(20, LOCK_SH|LOCK_NB)              = 0
fadvise64(20, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
ftruncate(20, 0)                        = 0
lseek(20, 0, SEEK_CUR)                  = 0
write(20, "example text to write\n", 22) = 22 // write  returned 22, not -1 which would mean an error
flock(20, LOCK_UN)                      = 0
close(20)   

Disclaimer: this is why I don't perceive this as a .NET bug, but more as a workaround for few File System imperfections...

Possible solutions

@stephentoub has listed an excellent list of possible solutions:

1. Make no changes in FileStream, and work with the powers that be around Windows NFS to "improve" behaviors here when accessing Windows NFS from Linux with POSIX file locks, where "improve" means to better match the semantics experienced with a typical Linux file system.

I am afraid that this is not an option, as simply there are too many File Systems that exhibit similar issues (NFS, CIFS and SMB).

2. Rip out all of this FileShare logic entirely, making it a nop on Linux. This is a breaking change, in that code relying on FileShare.None vs anything else for protection against concurrent usage with other FileStreams will now be silently broken. On the positive side, we'd shave off a few syscalls per FileStream :)

My main concern is that removing the locking entirely could result in silent errors that would be hard to diagnose for scenarios where our users use it for process synchronization.

FileStream locked;
try
{
    locked = File.Open($pathToAfileUsedByOtherProcessForLocking, FileMode.Open, FileAccess.Write, FileShare.None);
}
catch (IOException)
{
    Console.WriteLine("the file is locked");
    return;
}

Console.WriteLine("the file is not locked");
DoSomethingThatAssumesItsSafeToDoSo();

locked.Dispose();

I have given it a try and I got simply scared of how many tests were failing. As an owner of System.IO area I am not willing to introduce such a breaking change, especially so late in the release.

3. Enable opting-out (or as a variant of (2) opting in) to this behavior via an environment variable or other form of switch, e.g. if a particular environment variable is set, FileStream doesn't do any locking. An opt-out would provide a no-code-change workaround if someone knows they're hitting the issue. An opt-in would mean someone being silently broken due to unexpected concurrent usage would have a workaround when discovered.

This is a great idea, I've added System.IO.DisableFileLocking app context switch and DOTNET_SYSTEM_IO_DISABLEFILELOCKING env var to control it.
However, it's not a complete solution because for NFS we are not observing any exceptions, so users might not even notice the problem.

4. Find some way of detecting in FileStream on Linux that it's accessing a Windows NFS server and don't do the advisory locking in such cases. For example, maybe we fstatfs on the open file descriptor, and skip the advisory locking for some known set of file systems (e.g. NFS_SUPER_MAGIC), or the opposite, only doing it for some allowlist. I don't know what the extra syscall would do to perf, or whether we can be sufficiently accurate in the detection here.

It just works without introducing any breaking changes and thanks to the fact that we have recently removed one sys call for opening files for writing (#55206) .NET 6 would still be faster than .NET 5:

BenchmarkDotNet=v0.13.0.1559-nightly, OS=ubuntu 18.04
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores

Method	fileSize	userBufferSize	options	.NET 5	.NET 6 #55206	.NET 6 #55191	this PR
Write	1024	1024	None	36.533 us	34.367 us	34.553 us	33.768 us
Write	1024	1024	Asynchronous	35.108 us	34.854 us	34.576 us	34.645 us
OpenClose	1024	?	None	5.180 us	3.385 us	3.626 us	4.378 us
OpenClose	1024	?	Asynchronous	5.498 us	3.626 us	3.861 us	4.501 us
Write	1048576	512	None	1,220.053 us	1,193.259 us	1,211.106 us	1,232.766 us
Write	1048576	512	Asynchronous	2,182.725 us	2,179.588 us	1,208.690 us	1,204.874 us
Write	1048576	4096	None	1,154.081 us	1,146.565 us	1,159.902 us	1,168.063 us
Write	1048576	4096	Asynchronous	1,868.579 us	1,718.107 us	1,170.033 us	1,154.542 us
Write	104857600	4096	None	116,409.618 us	123,970.718 us	117,088.489 us	119,706.173 us
Write	104857600	4096	Asynchronous	160,874.999 us	160,940.932 us	126,086.723 us	114,330.524 us

For all the benchmarks that don't just open the file but actually perform at least a single write there is no visible difference. Moreover, we perform the extra sys call only when the file is opened for writing and file share is different than FileShare.None. So even if it becomes a problem for someone, they can use FileShare.None or use the flags mentioned above.

This solution has a weak point: we might miss adding some File System(s) to the list. In such a case, users can quickly workaround it by using the config switch mentioned above. Assuming, that call to write is going to fail for this File System.

5. Some limited variant of (2) where on Linux we use FileShare.None in more cases to limit the impact of this with Windows NFS Servers, while also incurring some of the breaks mentioned.

It would work only for the code that we control and would require our users to modify their source code, which is not always possible (using closed source libraries).

6. Find some other heretofore unknown way of implementing FileShare in FileStream on Linux that doesn't rely on file locks.

I did not find such a way, however I've came up with one more idea: release the lock when a write fails with EACCES. It worked fine for SMB (#53182 (comment)), however it would not work for NFS because again the failed writes are reported as successful.

On NFS it would work if we had opened the file with O_SYNC (#44546 (comment)). I've tried it and all tests were passing but the performance regressed by from x100 to x800. That is definitely not acceptable.

fixes #42790
fixes #44546
fixes #53182
fixes #54966

Edit: I am soon going to add description and benchmark numbers, until then please ignore the PR

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I am soon going to add description and benchmark numbers, until then please ignore the PR

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO
Milestone:	6.0.0

[stephentoub]

(I know you said ignore the PR, but I commented on a few things that could impact your benchmarking. I'll reserve judgment further until I see the perf numbers, but even though I suggested this as one possible approach, it seems like it'll be adding overhead to try to salvage a fundamentally flawed attempt at mimicking .NET behavior on Windows... I'm not sure it's the right tradeoff. )

[adamsitnik]

@stephentoub I've added the description which hopefully answers all the questions. PTAL

[stephentoub]

thanks to the fact that we have recently removed one sys call for opening files for writing (#55206) .NET 6 would still be faster than .NET 5

That will only apply if you never read or write the file. In the case of regular files, the data we got back from stat allowed us to avoid a subsequent lseek to determine whether the file was seekable. By not calling fstat, we're no longer able to avoid that:
https://github.com/dotnet/runtime/pull/55206/files#diff-f8685e8faaeb5154cda72b03d5482f4053ff073d54c1f4e924f57a1550a8346bR89
and then the moment we actually read or write the file, that will access SafeFileHandle.CanSeek which will then do the lseek.

This also presumes that all syscalls have the same performance impact. How do the overheads of fstat and fstatfs compare?

[jeffhandley]

@adamsitnik I understand we won't be able to test all configurations before the Preview 7 snap. We should move this PR forward to get it in for Preview 7 regardless. We can seek community/partner help in testing against Preview 7 / RC1 bits and address issues if we find any. We would want to capture the list of configs tested and update it along the way though.

[adamsitnik]

I understand we won't be able to test all configurations before the Preview 7 snap.

I've just finished testing the fix using client and server machine provided to me by @rtestardi and it works fine for NFS.

Thanks to help of @sba923 I was able to confirm that it's going to work for Samba as well (#53182 (comment))

[adamsitnik]

the data we got back from stat allowed us to avoid a subsequent lseek

I remember, I was the person who added this optimization quite recently

the moment we actually read or write the file, that will access SafeFileHandle.CanSeek which will then do the lseek.

That is a very good point. I've rerun all the benchmarks to see how all of that affected the performance. Here are the results:

Method	fileSize	userBufferSize	options	.NET 5	.NET 6 #55206	.NET 6 #55191	this PR
Write	1024	1024	None	36.533 us	34.367 us	34.553 us	33.768 us
Write	1024	1024	Asynchronous	35.108 us	34.854 us	34.576 us	34.645 us
OpenClose	1024	?	None	5.180 us	3.385 us	3.626 us	4.378 us
OpenClose	1024	?	Asynchronous	5.498 us	3.626 us	3.861 us	4.501 us
Write	1048576	512	None	1,220.053 us	1,193.259 us	1,211.106 us	1,232.766 us
Write	1048576	512	Asynchronous	2,182.725 us	2,179.588 us	1,208.690 us	1,204.874 us
Write	1048576	4096	None	1,154.081 us	1,146.565 us	1,159.902 us	1,168.063 us
Write	1048576	4096	Asynchronous	1,868.579 us	1,718.107 us	1,170.033 us	1,154.542 us
Write	104857600	4096	None	116,409.618 us	123,970.718 us	117,088.489 us	119,706.173 us
Write	104857600	4096	Asynchronous	160,874.999 us	160,940.932 us	126,086.723 us	114,330.524 us

Based on the first benchmark, where we just open the file and perform a single small write (1024 bytes) we can see that adding this additional syscall does not regress the perfomance (even small write takes much more time than opening the file),

[jozkee]

This might be more clear.

Suggested change

	if (CanLockTheFile(lockOperation, access) && !(_isLocked = Interop.Sys.FLock(this, lockOperation | Interop.Sys.LockOperations.LOCK_NB) >= 0))
	{
	if (CanLockTheFile(lockOperation, access) && Interop.Sys.FLock(this, lockOperation | Interop.Sys.LockOperations.LOCK_NB) < 0))
	{
	_isLocked = true;

[jozkee]

LGTM; thanks, Adam.

[LMSSonos]

Thanks to all for the great work, will this be backported to 3.1 or will it stay in 6.x only?

[rtestardi]

yes, thank you adam and all!!!

[stephentoub]

I've rerun all the benchmarks to see how all of that affected the performance.

OK, thanks for checking.

[adamsitnik]

will this be backported to 3.1 or will it stay in 6.x only

@stephentoub @jeffhandley @danmoseley what is your opinion on that? based on the number of reactions for workaround there seems to be multiple customers affected

[LMSSonos]

I'm not sure if this is related, but we added the nobrl workaround and that helped when writing files (netcore 3.1, alpine linux to CIFS running in kubernetes). But nobrl did not help when using File.Copy which creates the file copy as expected but then fails with

Exception:System.UnauthorizedAccessException: Access to the path is denied. ---> 
System.IO.IOException: Operation not permitted 
-- End of inner exception stack trace 
--- 
at System.IO.FileSystem.CopyFile(String sourceFullPath, String destFullPath, Boolean overwrite)
at System.IO.File.Copy(String sourceFileName, String destFileName, Boolean overwrite) 
at MyImport.Import.MyImport(String filePath, Boolean full) in /src/MyImport/Import.cs:line 211

[sba923]

will this be backported to 3.1 or will it stay in 6.x only

if it's only 6.x, it will take a looooooooong time before this ripples into PowerShell (where I first detected the issue) 😭

[danmoseley]

Re 3.1 backport - my initial reaction would be concern that we would change something so fundamental in a release that folks are explicitly using for stability.

If the need is sufficiently high, would it suffice to backport an appcontext flag, without all the fstatfs work? This would be a different appcontext flag, which opted into the no-locking behavior, so almost everyone would see no change. The appcontext flag would only exist in 3.1.

[danmoseley]

BTW kudos for your thorough writeup @adamsitnik .

[danmoseley]

if it's only 6.x, it will take a looooooooong time before this ripples into PowerShell (where I first detected the issue)

@sba923 .NET 6.0 ships next month and I expect PowerShell 7.2 will ship approximately then. It should already contain this fix: https://github.com/PowerShell/PowerShell/releases