Set of offset-based APIs for thread-safe file IO by adamsitnik · Pull Request #53669 · dotnet/runtime

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnik · 2021-06-03T10:38:07Z

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

…Helpers to SafeFileHandle itself

…le.OpenHandle to FileStreamHelpers

…omFile.ReadAtOffset are going to need it)

…s it's a Windows-specific concept

…Seek for regular files

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

… file

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub · 2021-06-15T12:00:52Z

+ {
+ public byte* Base;
+ public UIntPtr Count;
+ }


stephentoub · 2021-06-15T12:07:53Z

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


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

stephentoub · 2021-06-15T13:09:40Z

 } 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.


Should we move them to here?
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Lines 163 to 168 in c87354c
// 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));

Co-authored-by: Stephen Toub <stoub@microsoft.com>

janvorli · 2021-06-15T20:29:15Z

@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 · 2021-06-15T20:31:44Z

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

janvorli · 2021-06-15T20:39:34Z

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 · 2021-06-15T20:40:52Z

@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 · 2021-06-15T20:46:11Z

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

adamsitnikand others added 30 commits May 27, 2021 13:00

move logic related to opening and initializing handle from FileStream…
d37d1ee
…Helpers to SafeFileHandle itself

move the arguments validation logic common for FileStream ctor and Fi…
539393d
…le.OpenHandle to FileStreamHelpers

introduce File.OpenHandle
b60e876

fix the new tests
c61d639

FileStream created from a handle should allow for bufferSize == 0
ad252de

File.OpenHandle should always use full path
1540dba

fix a bug in NtCreateFile mapping logic
d8a3113

always use NtCreateFile for SafeFileHandle.Open
02cd564

move the ThreadPoolBinding initialization logic to one place
1430dd6

IsAsync: use syscall when not provided and validate it early
ebaa92a

read all FileOptions, not just FileOptions.Asynchronous
fbbcd4d

move IsPipe CanSeek to SafeFileHandle as well (the new APIs like Rand…
7f94f85
…omFile.ReadAtOffset are going to need it)

disable the Append test as it's currently not supported by design
33ecb8d

expose SafeFileHandle.IsAsync and write a test for it
f1af8f6

fix compilation errors on Linux
a46da91

don't try to test DeleteOnClose for FileStreams created from handle a…
9ef23eb
…s it's a Windows-specific concept

take advantage of knowing the file type and avoid one syscall for Can…
485eb93
…Seek for regular files

revert some ordering changes to make the code easier to review
c8818aa

Apply suggestions from code review
dc13de0
Co-authored-by: Stephen Toub <stoub@microsoft.com>

add missing ;
6c58252

simplify Windows implementation:
fb24f5c
* 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

unify CanSeek and IsPipe for the .NET 5 Windows strategy as well
8baf0bd

move the !MS_IO_REDIST methods from File.cs to File.netcoreapp.cs…
50aba1f
… file

RandomAccess: GetLength, Read and Write single buffer
143d31c

fix Unix build
1d41d65

refactor the tests
2d8ac90

ReadAsync and WriteAsync
bbbd267

move AsyncWindowsFileStreamStrategy.ValueTaskSource to SafeFileHandle
46d9b6f

ReadScatter and WriteGather
a946c5d

some polishing
4f807ef

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.IO.FileSystem/tests/RandomAccess/SectorAlignedMemory.Windows.cs Outdated

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs Outdated

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/File.netcoreapp.cs Outdated

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs Outdated

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs Outdated

stephentoub reviewed Jun 11, 2021
View reviewed changes

Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs

adamsitnikand others added 2 commits June 11, 2021 20:39

Apply suggestions from code review
05e65eb
Co-authored-by: Stephen Toub <stoub@microsoft.com>

address code review feedback
c87354c

filipnavara mentioned this pull request Jun 14, 2021
IOException when building from mapping pack files into memory dotnet/Nerdbank.GitVersioning#584
Closed

stephentoub approved these changes Jun 15, 2021
View reviewed changes

adamsitnikand others added 2 commits June 15, 2021 16:55

Apply suggestions from code review
f80bbb7
Co-authored-by: Stephen Toub <stoub@microsoft.com>

address code review feedback
07051fa

adamsitnik merged commit 9d771a2 into dotnet:mainJun 15, 2021

adamsitnik mentioned this pull request Jun 21, 2021
A System.NullReferenceException was throwing when get a **gif** image using Image.FromFile() method #54475
Closed

sandreenko mentioned this pull request Jun 21, 2021
Arm64 Apple libs build fails, a recent regression #54505
Closed

DrewScoggins mentioned this pull request Jun 22, 2021
[Perf] Regressions in System.IO.Tests.Perf_FileStream #54570
Closed

danmoseley mentioned this pull request Jun 23, 2021
Sporadic failure in file enumeration tests on Unix #54589
Closed

adamsitnik deleted the randomAcess branch June 24, 2021 09:10

adamsitnik mentioned this pull request Jun 24, 2021
FileStream.ReadAsync() with isAsync: true is throwing IOException #54143
Closed

adamsitnik mentioned this pull request Jul 1, 2021
All methods that accept bufferSize and use FileStream internally should allow for passing zero to disable buffering #53497
Closed

ghost locked as resolved and limited conversation to collaborators Jul 24, 2021