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

Set of offset-based APIs for thread-safe file IO #53669

Merged
merged 74 commits into from
Jun 15, 2021

Conversation

adamsitnik
Copy link
Member

I've moved all the logic responsible for opening and initializing SafeFileHandle to SafeFileHandle. A lot of duplicated code (mostly for initializing ThreadPoolBinding) got removed. On Unix, it's also now much more clear how many syscalls we perform to just open a file.

Other changes:

  • Instead of using NtCreateFile for cases when preallocationSize was specified and CreateFileW when not, I've followed @JeremyKuhne suggestion and switched to using NtCreateFile only. This has simplified the code and provided some minor perf gain. I've discovered a bug in the previous implementation (related to DesiredAccess.DELETE) fixed it, and added a test
  • On Unix, I've realized that once we call fstat() to ensure that a given file is not a directory we can also determine whether a given file is seekable or not. This has removed one syscall for the initialization of CanSeek.

Fixes #24847

@carlossanlop @Jozkee @stephentoub PTAL. Once this PR is merged, I would like to finally publish the .NET Blog post.

cc @alexbudmsft

adamsitnik and others added 30 commits May 27, 2021 13:00
Co-authored-by: Stephen Toub <stoub@microsoft.com>
* with NtCreateFile there is no need for Validation (NtCreateFile returns error and we never create a safe file handle)
* unify status codes
* complete error mapping
adamsitnik and others added 2 commits June 11, 2021 20:39
Co-authored-by: Stephen Toub <stoub@microsoft.com>
{
public byte* Base;
public UIntPtr Count;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: nuint


int64_t count = 0;
int fileDescriptor = ToFileDescriptor(fd);
#if HAVE_PREADV && !defined(TARGET_WASM) // preadv is buggy on WASM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's open an issue in runtime for now and include the link here.

@@ -41,6 +41,14 @@ typedef struct
// add more fields when needed.
} ProcessStatus;

// NOTE: the layout of this type is intended to exactly match the layout of a `struct iovec`. There are
// assertions in pal_networking.c that validate this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move them to here?

// We require that IOVector have the same layout as iovec.
c_static_assert(sizeof(IOVector) == sizeof(iovec));
c_static_assert(sizeof_member(IOVector, Base) == sizeof_member(iovec, iov_base));
c_static_assert(offsetof(IOVector, Base) == offsetof(iovec, iov_base));
c_static_assert(sizeof_member(IOVector, Count) == sizeof_member(iovec, iov_len));
c_static_assert(offsetof(IOVector, Count) == offsetof(iovec, iov_len));

System.IO - FileStream automation moved this from In progress to Reviewer approved Jun 15, 2021
adamsitnik and others added 2 commits June 15, 2021 16:55
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@adamsitnik adamsitnik merged commit 9d771a2 into dotnet:main Jun 15, 2021
System.IO - FileStream automation moved this from Reviewer approved to Done Jun 15, 2021
@janvorli
Copy link
Member

@adamsitnik this change has broken my local build on Apple Silicon. It fails with:

/Users/janvorli/git/runtime2/src/libraries/Native/Unix/System.Native/pal_io.c:1491:21: error: implicit declaration of function 'preadv' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
      while ((count = preadv(fileDescriptor, (struct iovec*)vectors, (int)vectorCount, (off_t)fileOffset)) < 0 && errno == EINTR);
                      ^
  /Users/janvorli/git/runtime2/src/libraries/Native/Unix/System.Native/pal_io.c:1531:21: error: implicit declaration of function 'pwritev' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
      while ((count = pwritev(fileDescriptor, (struct iovec*)vectors, (int)vectorCount, (off_t)fileOffset)) < 0 && errno == EINTR);
                      ^
  2 errors generated.

However the checks for HAVE_PREADV and HAVE_PWRITEV in the configuration have succeeded. I suspect that the functions have moved to a different header file in recent SDK and the pal_io.c doesn't include it. But I am not sure yet.

@janvorli
Copy link
Member

Btw, this was a clean build after git clean -xdf.

@janvorli
Copy link
Member

I can see that the preadv / pwritew in my SDK are available only if:
#if (!defined(_POSIX_C_SOURCE) && !defined(_XOPEN_SOURCE)) || defined(_DARWIN_C_SOURCE)
Looking at the compiler options for the pal_io.c, we set the _XOPEN_SOURCE, so that blocks the preadv / pwritev.

@filipnavara
Copy link
Member

filipnavara commented Jun 15, 2021

@janvorli I suspect it may be the macOS version passed to the compiler as minimum version. The APIs were added in macOS 11. If the cake checks don't pass the target minimum macOS version it will likely use the latest and find the APIs. The actual compilation likely targets older macOS version and either compiles against older SDK or the SDK hides the symbols.

Nevermind, I see you already checked the headers meanwhile. Also I realized that the Apple Silicon version may target macOS 11 as minimum (unlike the x64 one).

@janvorli
Copy link
Member

Adding _DARWIN_C_SOURCE definition for the System.Native (for OSX target) fixed the build for me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

Async File IO APIs mimicking Win32 OVERLAPPED
10 participants