From e777af90e878cb3618e583bc1b8ec796903ccbdb Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Wed, 18 Mar 2020 17:47:45 -0700 Subject: [PATCH 1/6] Separate filesystem registration from DSO loading. As we need to execute the same registration code when filesystems are registered statically, we move the DSO loading code to the `RegisterFilesystemPlugin` function and keep `RegisterFilesystemPluginImpl` available to be called regardless of the registration type (plugin or static dependency). --- .../filesystem/modular_filesystem.cc | 20 ++++++++- .../modular_filesystem_registration.cc | 41 +++++-------------- .../modular_filesystem_registration.h | 3 +- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/tensorflow/c/experimental/filesystem/modular_filesystem.cc b/tensorflow/c/experimental/filesystem/modular_filesystem.cc index 8645d3186c8..58541ea2b36 100644 --- a/tensorflow/c/experimental/filesystem/modular_filesystem.cc +++ b/tensorflow/c/experimental/filesystem/modular_filesystem.cc @@ -440,7 +440,25 @@ Status ModularWritableFile::Tell(int64* position) { } Status RegisterFilesystemPlugin(const std::string& dso_path) { - return filesystem_registration::RegisterFilesystemPluginImpl(dso_path); + // Step 1: Load plugin + Env* env = Env::Default(); + void* dso_handle; + TF_RETURN_IF_ERROR(env->LoadLibrary(dso_path.c_str(), &dso_handle)); + + // Step 2: Load symbol for `TF_InitPlugin` + void* dso_symbol; + TF_RETURN_IF_ERROR( + env->GetSymbolFromLibrary(dso_handle, "TF_InitPlugin", &dso_symbol)); + + // Step 3: Call `TF_InitPlugin` + TF_FilesystemPluginInfo info; + memset(&info, 0, sizeof(info)); + auto TF_InitPlugin = + reinterpret_cast(dso_symbol); + TF_InitPlugin(&info); + + // Step 4: Do the actual registration + return filesystem_registration::RegisterFilesystemPluginImpl(&info); } } // namespace tensorflow diff --git a/tensorflow/c/experimental/filesystem/modular_filesystem_registration.cc b/tensorflow/c/experimental/filesystem/modular_filesystem_registration.cc index 5f6c2048e56..174665f8927 100644 --- a/tensorflow/c/experimental/filesystem/modular_filesystem_registration.cc +++ b/tensorflow/c/experimental/filesystem/modular_filesystem_registration.cc @@ -14,7 +14,6 @@ limitations under the License. ==============================================================================*/ #include "tensorflow/c/experimental/filesystem/modular_filesystem_registration.h" -#include "tensorflow/c/experimental/filesystem/filesystem_interface.h" #include "tensorflow/c/experimental/filesystem/modular_filesystem.h" #include "tensorflow/c/tf_status_internal.h" #include "tensorflow/core/platform/env.h" @@ -304,40 +303,22 @@ static Status ValidatePluginMemoryRoutines( namespace filesystem_registration { -Status RegisterFilesystemPluginImpl(const std::string& dso_path) { - // Step 1: Load plugin - Env* env = Env::Default(); - void* dso_handle; - TF_RETURN_IF_ERROR(env->LoadLibrary(dso_path.c_str(), &dso_handle)); +Status RegisterFilesystemPluginImpl(const TF_FilesystemPluginInfo* info) { + TF_RETURN_IF_ERROR(ValidatePluginMemoryRoutines(info)); - // Step 2: Load symbol for `TF_InitPlugin` - void* dso_symbol; - TF_RETURN_IF_ERROR( - env->GetSymbolFromLibrary(dso_handle, "TF_InitPlugin", &dso_symbol)); - - // Step 3: Call `TF_InitPlugin` - TF_FilesystemPluginInfo info; - memset(&info, 0, sizeof(info)); - auto TF_InitPlugin = - reinterpret_cast(dso_symbol); - TF_InitPlugin(&info); - - // Step 4: Ensure plugin provides the memory management functions. - TF_RETURN_IF_ERROR(ValidatePluginMemoryRoutines(&info)); - - // Step 5: Validate and register all filesystems + // Validate and register all filesystems // Try to register as many filesystems as possible. // Free memory once we no longer need it Status status; - for (int i = 0; i < info.num_schemes; i++) { - status.Update(ValidateAndRegisterFilesystems(&info, i)); - info.plugin_memory_free(info.ops[i].scheme); - info.plugin_memory_free(info.ops[i].filesystem_ops); - info.plugin_memory_free(info.ops[i].random_access_file_ops); - info.plugin_memory_free(info.ops[i].writable_file_ops); - info.plugin_memory_free(info.ops[i].read_only_memory_region_ops); + for (int i = 0; i < info->num_schemes; i++) { + status.Update(ValidateAndRegisterFilesystems(info, i)); + info->plugin_memory_free(info->ops[i].scheme); + info->plugin_memory_free(info->ops[i].filesystem_ops); + info->plugin_memory_free(info->ops[i].random_access_file_ops); + info->plugin_memory_free(info->ops[i].writable_file_ops); + info->plugin_memory_free(info->ops[i].read_only_memory_region_ops); } - info.plugin_memory_free(info.ops); + info->plugin_memory_free(info->ops); return status; } diff --git a/tensorflow/c/experimental/filesystem/modular_filesystem_registration.h b/tensorflow/c/experimental/filesystem/modular_filesystem_registration.h index 4df063d560c..5b1a7d40556 100644 --- a/tensorflow/c/experimental/filesystem/modular_filesystem_registration.h +++ b/tensorflow/c/experimental/filesystem/modular_filesystem_registration.h @@ -15,12 +15,13 @@ limitations under the License. #ifndef TENSORFLOW_C_EXPERIMENTAL_FILESYSTEM_MODULAR_FILESYSTEM_REGISTRATION_H_ #define TENSORFLOW_C_EXPERIMENTAL_FILESYSTEM_MODULAR_FILESYSTEM_REGISTRATION_H_ +#include "tensorflow/c/experimental/filesystem/filesystem_interface.h" #include "tensorflow/core/platform/status.h" namespace tensorflow { namespace filesystem_registration { -Status RegisterFilesystemPluginImpl(const std::string& dso_path); +Status RegisterFilesystemPluginImpl(const TF_FilesystemPluginInfo* info); } // namespace filesystem_registration } // namespace tensorflow From ce298391eaa951a637d4155212e4927ea576e003 Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Wed, 18 Mar 2020 17:55:57 -0700 Subject: [PATCH 2/6] Add a header file for the POSIX filesystem. This header only exposes `TF_InitPlugin` as we need to call this function when registering the local filesystems statically. --- .../plugins/posix/posix_filesystem.h | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem.h diff --git a/tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem.h b/tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem.h new file mode 100644 index 00000000000..b2f169cdeab --- /dev/null +++ b/tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem.h @@ -0,0 +1,29 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +#ifndef TENSORFLOW_C_EXPERIMENTAL_FILESYSTEM_PLUGINS_POSIX_POSIX_FILESYSTEM_H_ +#define TENSORFLOW_C_EXPERIMENTAL_FILESYSTEM_PLUGINS_POSIX_POSIX_FILESYSTEM_H_ + +// Initialize the POSIX filesystem. +// +// In general, the `TF_InitPlugin` symbol doesn't need to be exposed in a header +// file, since the plugin registration will look for the symbol in the DSO file +// that provides the filesystem functionality. However, the POSIX filesystem +// needs to be statically registered in some tests and utilities for building +// the API files at the time of creating the pip package. Hence, we need to +// expose this function so that this filesystem can be statically registered +// when needed. +void TF_InitPlugin(TF_FilesystemPluginInfo* info); + +#endif // TENSORFLOW_C_EXPERIMENTAL_FILESYSTEM_MODULAR_FILESYSTEM_H_ From fe1d911498fac05155d93e72c11195591fb7d77b Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Wed, 18 Mar 2020 17:57:13 -0700 Subject: [PATCH 3/6] Add the `posix_filesystem.h` header to the `BUILD` target --- tensorflow/c/experimental/filesystem/plugins/posix/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/tensorflow/c/experimental/filesystem/plugins/posix/BUILD b/tensorflow/c/experimental/filesystem/plugins/posix/BUILD index 3707dafe518..395cbb75914 100644 --- a/tensorflow/c/experimental/filesystem/plugins/posix/BUILD +++ b/tensorflow/c/experimental/filesystem/plugins/posix/BUILD @@ -19,6 +19,7 @@ tf_cc_shared_object( cc_library( name = "posix_filesystem_impl", srcs = ["posix_filesystem.cc"], + hdrs = ["posix_filesystem.h"], deps = [ ":posix_filesystem_helper", "//tensorflow/c:tf_status", From d0259641873acc2dbe1e2d65c91d4a761bd1f42f Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Thu, 19 Mar 2020 10:33:20 -0700 Subject: [PATCH 4/6] Make visibility of modular_filesystem public We will need to depend on this to do the static registration of the filesystems. This is because a part of the registration is common between the plugin registration and the static registration. --- tensorflow/c/experimental/filesystem/BUILD | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tensorflow/c/experimental/filesystem/BUILD b/tensorflow/c/experimental/filesystem/BUILD index 602494aa087..46d094cecc4 100644 --- a/tensorflow/c/experimental/filesystem/BUILD +++ b/tensorflow/c/experimental/filesystem/BUILD @@ -27,6 +27,9 @@ cc_library( "modular_filesystem_registration.h", ], hdrs = ["modular_filesystem.h"], + # TODO(mihaimaruseac): Visibility should be more restrictive once we + # convert to modular filesystems everywhere + visibility = ["//visibility:public"], deps = [ ":filesystem_interface", "//tensorflow/c:tf_status_helper", From 9e1c010111a4ed05e2d6c34c7ed8bfe1482f25d4 Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Thu, 19 Mar 2020 10:35:47 -0700 Subject: [PATCH 5/6] Add `posix_filesystem_static` target and source. This implements the static registration of a filesystem, without needing to load the plugin DSO. Simply link this target in the binary which needs the filesystem to have the static filesystem imported. --- .../filesystem/plugins/posix/BUILD | 13 ++++++++ .../plugins/posix/posix_filesystem_static.cc | 33 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem_static.cc diff --git a/tensorflow/c/experimental/filesystem/plugins/posix/BUILD b/tensorflow/c/experimental/filesystem/plugins/posix/BUILD index 395cbb75914..49a412dfb6a 100644 --- a/tensorflow/c/experimental/filesystem/plugins/posix/BUILD +++ b/tensorflow/c/experimental/filesystem/plugins/posix/BUILD @@ -27,6 +27,19 @@ cc_library( ], ) +# Since building pip package and API tests require a filesystem, we provide a +# static registration target that they should link against. +cc_library( + name = "posix_filesystem_static", + srcs = ["posix_filesystem_static.cc"], + visibility = ["//visibility:public"], + deps = [ + ":posix_filesystem_impl", + "//tensorflow/c/experimental/filesystem:filesystem_interface", + "//tensorflow/c/experimental/filesystem:modular_filesystem", + ], +) + # Library implementing helper functionality, so that the above only contains # the API implementation for modular filesystems. cc_library( diff --git a/tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem_static.cc b/tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem_static.cc new file mode 100644 index 00000000000..355cfce213b --- /dev/null +++ b/tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem_static.cc @@ -0,0 +1,33 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +#include "tensorflow/c/experimental/filesystem/filesystem_interface.h" +#include "tensorflow/c/experimental/filesystem/modular_filesystem_registration.h" +#include "tensorflow/c/experimental/filesystem/plugins/posix/posix_filesystem.h" + +namespace tensorflow { + +// Register the POSIX filesystems statically. +// Return value will be unused +Status StaticallyRegisterLocalFilesystems() { + TF_FilesystemPluginInfo info; + TF_InitPlugin(&info); + return filesystem_registration::RegisterFilesystemPluginImpl(&info); +} + +// Perform the actual registration +static Status unused = StaticallyRegisterLocalFilesystems(); + +} // namespace tensorflow From 289d2827ce20b76a1b10b61380f6fbcae9577a16 Mon Sep 17 00:00:00 2001 From: Mihai Maruseac Date: Thu, 19 Mar 2020 21:50:57 -0700 Subject: [PATCH 6/6] Always link the static filesystem registration target. --- tensorflow/c/experimental/filesystem/plugins/posix/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/tensorflow/c/experimental/filesystem/plugins/posix/BUILD b/tensorflow/c/experimental/filesystem/plugins/posix/BUILD index 49a412dfb6a..3afe114b5a6 100644 --- a/tensorflow/c/experimental/filesystem/plugins/posix/BUILD +++ b/tensorflow/c/experimental/filesystem/plugins/posix/BUILD @@ -38,6 +38,7 @@ cc_library( "//tensorflow/c/experimental/filesystem:filesystem_interface", "//tensorflow/c/experimental/filesystem:modular_filesystem", ], + alwayslink = 1, ) # Library implementing helper functionality, so that the above only contains