From ceecde6d42edf9ca17fd043dec4da3ca88ef81f2 Mon Sep 17 00:00:00 2001 From: Marc Vef Date: Tue, 17 Nov 2020 18:13:39 +0100 Subject: [PATCH 1/4] File create: exist check now done on daemon. Configuration added --- include/config.hpp | 5 ++ include/daemon/backend/exceptions.hpp | 5 ++ include/daemon/backend/metadata/db.hpp | 5 +- src/client/gkfs_functions.cpp | 100 ++++++++++++------------- src/daemon/backend/metadata/db.cpp | 71 +++++++++++++++++- src/daemon/handler/srv_metadata.cpp | 5 +- src/daemon/ops/metadentry.cpp | 7 +- 7 files changed, 144 insertions(+), 54 deletions(-) diff --git a/include/config.hpp b/include/config.hpp index 33fbe3f8c..c75cb0ef5 100644 --- a/include/config.hpp +++ b/include/config.hpp @@ -48,6 +48,11 @@ constexpr auto use_ctime = false; constexpr auto use_mtime = false; constexpr auto use_link_cnt = false; constexpr auto use_blocks = false; + +// metadata logic +// Check for existence of file metadata before create. This done on RocksDB +// level +constexpr auto create_exist_check = true; } // namespace metadata namespace rpc { diff --git a/include/daemon/backend/exceptions.hpp b/include/daemon/backend/exceptions.hpp index bcf1f0c9b..1bb033ff6 100644 --- a/include/daemon/backend/exceptions.hpp +++ b/include/daemon/backend/exceptions.hpp @@ -30,6 +30,11 @@ public: explicit NotFoundException(const std::string& s) : DBException(s){}; }; +class ExistsException : public DBException { +public: + explicit ExistsException(const std::string& s) : DBException(s){}; +}; + } // namespace metadata } // namespace gkfs diff --git a/include/daemon/backend/metadata/db.hpp b/include/daemon/backend/metadata/db.hpp index c09b33b90..4b1ace3a0 100644 --- a/include/daemon/backend/metadata/db.hpp +++ b/include/daemon/backend/metadata/db.hpp @@ -37,7 +37,7 @@ public: static inline void throw_rdb_status_excpt(const rdb::Status& s); - MetadataDB(const std::string& path); + explicit MetadataDB(const std::string& path); std::string get(const std::string& key) const; @@ -45,6 +45,9 @@ public: void put(const std::string& key, const std::string& val); + void + put_no_exist(const std::string& key, const std::string& val); + void remove(const std::string& key); diff --git a/src/client/gkfs_functions.cpp b/src/client/gkfs_functions.cpp index 1884817e3..e2408106e 100644 --- a/src/client/gkfs_functions.cpp +++ b/src/client/gkfs_functions.cpp @@ -128,74 +128,74 @@ gkfs_open(const std::string& path, mode_t mode, int flags) { errno = ENOTSUP; return -1; } - - bool exists = true; - auto md = gkfs::utils::get_metadata(path); - if(!md) { - if(errno == ENOENT) { - exists = false; - } else { - LOG(ERROR, "Error while retriving stat to file"); - return -1; - } - } - - if(!exists) { - if(!(flags & O_CREAT)) { - // file doesn't exists and O_CREAT was not set - errno = ENOENT; - return -1; - } - - /*** CREATION ***/ - assert(flags & O_CREAT); - + // metadata pointer assigned during create or stat + std::shared_ptr md = nullptr; + if(flags & O_CREAT) { if(flags & O_DIRECTORY) { LOG(ERROR, "O_DIRECTORY use with O_CREAT. NOT SUPPORTED"); errno = ENOTSUP; return -1; } - // no access check required here. If one is using our FS they have the // permissions. - if(gkfs_create(path, mode | S_IFREG)) { - LOG(ERROR, "Error creating non-existent file: '{}'", - strerror(errno)); - return -1; + int err = gkfs_create(path, mode | S_IFREG); + if(err) { + if(errno == EEXIST) { + // file exists, O_CREAT was set + if(flags & O_EXCL) { + // File exists and O_EXCL & O_CREAT was set + errno = EEXIST; + return -1; + } + // file exists, O_CREAT was set O_EXCL wasnt, so function does + // not fail this case is actually undefined as per `man 2 open` + md = gkfs::util::get_metadata(path); + } else { + LOG(ERROR, "Error creating file: '{}'", strerror(errno)); + return -1; + } + } else { + // file was successfully created. Add to filemap + return CTX->file_map()->add( + std::make_shared(path, flags)); } } else { - /* File already exists */ - - if(flags & O_EXCL) { - // File exists and O_EXCL was set - errno = EEXIST; - return -1; + md = gkfs::util::get_metadata(path); + if(!md) { + if(errno == ENOENT) { + // file doesn't exists and O_CREAT was not set + return -1; + } else { + LOG(ERROR, "Error stating existing file"); + return -1; + } } + } + assert(md); #ifdef HAS_SYMLINKS - if(md->is_link()) { - if(flags & O_NOFOLLOW) { - LOG(WARNING, "Symlink found and O_NOFOLLOW flag was specified"); - errno = ELOOP; - return -1; - } - return gkfs_open(md->target_path(), mode, flags); + if(md->is_link()) { + if(flags & O_NOFOLLOW) { + LOG(WARNING, "Symlink found and O_NOFOLLOW flag was specified"); + errno = ELOOP; + return -1; } + return gkfs_open(md->target_path(), mode, flags); + } #endif - if(S_ISDIR(md->mode())) { - return gkfs_opendir(path); - } + if(S_ISDIR(md->mode())) { + return gkfs_opendir(path); + } - /*** Regular file exists ***/ - assert(S_ISREG(md->mode())); + /*** Regular file exists ***/ + assert(S_ISREG(md->mode())); - if((flags & O_TRUNC) && ((flags & O_RDWR) || (flags & O_WRONLY))) { - if(gkfs_truncate(path, md->size(), 0)) { - LOG(ERROR, "Error truncating file"); - return -1; - } + if((flags & O_TRUNC) && ((flags & O_RDWR) || (flags & O_WRONLY))) { + if(gkfs_truncate(path, md->size(), 0)) { + LOG(ERROR, "Error truncating file"); + return -1; } } diff --git a/src/daemon/backend/metadata/db.cpp b/src/daemon/backend/metadata/db.cpp index 9879d7ae6..c7fbc44a6 100644 --- a/src/daemon/backend/metadata/db.cpp +++ b/src/daemon/backend/metadata/db.cpp @@ -24,6 +24,11 @@ extern "C" { namespace gkfs::metadata { + +/** + * Called when the daemon is started: Connects to the KV store + * @param path where KV store data is stored + */ MetadataDB::MetadataDB(const std::string& path) : path(path) { // Optimize RocksDB. This is the easiest way to get RocksDB to perform well options.IncreaseParallelism(); @@ -41,6 +46,12 @@ MetadataDB::MetadataDB(const std::string& path) : path(path) { this->db.reset(rdb_ptr); } +/** + * Exception wrapper on Status object. Throws NotFoundException if + * s.IsNotFound(), general DBException otherwise + * @param RocksDB status + * @throws DBException + */ void MetadataDB::throw_rdb_status_excpt(const rdb::Status& s) { assert(!s.ok()); @@ -52,6 +63,12 @@ MetadataDB::throw_rdb_status_excpt(const rdb::Status& s) { } } +/** + * Gets a KV store value for a key + * @param key + * @return value + * @throws DBException on failure, NotFoundException if entry doesn't exist + */ std::string MetadataDB::get(const std::string& key) const { std::string val; @@ -62,6 +79,12 @@ MetadataDB::get(const std::string& key) const { return val; } +/** + * Puts an entry into the KV store + * @param key + * @param val + * @throws DBException on failure + */ void MetadataDB::put(const std::string& key, const std::string& val) { assert(gkfs::path::is_absolute(key)); @@ -74,6 +97,25 @@ MetadataDB::put(const std::string& key, const std::string& val) { } } +/** + * Puts an entry into the KV store if it doesn't exist. This function does not + * use a mutex. + * @param key + * @param val + * @throws DBException on failure, ExistException if entry already exists + */ +void +MetadataDB::put_no_exist(const std::string& key, const std::string& val) { + if(exists(key)) + throw ExistsException(key); + put(key, val); +} + +/** + * Removes an entry from the KV store + * @param key + * @throws DBException on failure, NotFoundException if entry doesn't exist + */ void MetadataDB::remove(const std::string& key) { auto s = db->Delete(write_opts, key); @@ -82,6 +124,12 @@ MetadataDB::remove(const std::string& key) { } } +/** + * checks for existence of an entry + * @param key + * @return true if exists + * @throws DBException on failure + */ bool MetadataDB::exists(const std::string& key) { std::string val; @@ -101,7 +149,7 @@ MetadataDB::exists(const std::string& key) { * @param old_key * @param new_key * @param val - * @return + * @throws DBException on failure, NotFoundException if entry doesn't exist */ void MetadataDB::update(const std::string& old_key, const std::string& new_key, @@ -116,6 +164,14 @@ MetadataDB::update(const std::string& old_key, const std::string& new_key, } } +/** + * Increases only the size part of the metadentry via a RocksDB Operand + * Operation. E.g., called before a write() call + * @param key + * @param size + * @param append + * @throws DBException on failure + */ void MetadataDB::increase_size(const std::string& key, size_t size, bool append) { auto uop = IncreaseSizeOperand(size, append); @@ -125,6 +181,13 @@ MetadataDB::increase_size(const std::string& key, size_t size, bool append) { } } +/** + * Decreases only the size part of the metadentry via a RocksDB Operand + * Operation E.g., called before a truncate() call + * @param key + * @param size + * @throws DBException on failure + */ void MetadataDB::decrease_size(const std::string& key, size_t size) { auto uop = DecreaseSizeOperand(size); @@ -254,6 +317,12 @@ MetadataDB::iterate_all() { } } +/** + * Called when RocksDB connection is established. + * Used for setting KV store settings + * see here: https://github.com/facebook/rocksdb/wiki/RocksDB-Tuning-Guide + * @param options + */ void MetadataDB::optimize_rocksdb_options(rdb::Options& options) { options.max_successive_merges = 128; diff --git a/src/daemon/handler/srv_metadata.cpp b/src/daemon/handler/srv_metadata.cpp index 8e9f8ddf8..1c3e8f4b8 100644 --- a/src/daemon/handler/srv_metadata.cpp +++ b/src/daemon/handler/srv_metadata.cpp @@ -15,10 +15,10 @@ #include #include #include +#include #include #include -#include using namespace std; @@ -46,6 +46,8 @@ rpc_srv_create(hg_handle_t handle) { // create metadentry gkfs::metadata::create(in.path, md); out.err = 0; + } catch(const gkfs::metadata::ExistsException& e) { + out.err = EEXIST; } catch(const std::exception& e) { GKFS_DATA->spdlogger()->error("{}() Failed to create metadentry: '{}'", __func__, e.what()); @@ -193,6 +195,7 @@ rpc_srv_remove(hg_handle_t handle) { hg_return_t rpc_srv_update_metadentry(hg_handle_t handle) { + // Note: Currently this handler is not called by the client. rpc_update_metadentry_in_t in{}; rpc_err_out_t out{}; diff --git a/src/daemon/ops/metadentry.cpp b/src/daemon/ops/metadentry.cpp index 9bca39131..7005d29b2 100644 --- a/src/daemon/ops/metadentry.cpp +++ b/src/daemon/ops/metadentry.cpp @@ -78,6 +78,7 @@ get_dirents_extended(const std::string& dir) { * Creates metadata (if required) and dentry at the same time * @param path * @param mode + * @throws DBException */ void create(const std::string& path, Metadata& md) { @@ -95,7 +96,11 @@ create(const std::string& path, Metadata& md) { if(GKFS_DATA->ctime_state()) md.ctime(time); } - GKFS_DATA->mdb()->put(path, md.serialize()); + if(gkfs::config::metadata::create_exist_check) { + GKFS_DATA->mdb()->put_no_exist(path, md.serialize()); + } else { + GKFS_DATA->mdb()->put(path, md.serialize()); + } } /** -- GitLab From 21a87f7aa78ae9587dea238df94088ab3df1c2a4 Mon Sep 17 00:00:00 2001 From: Marc Vef Date: Thu, 17 Dec 2020 11:47:56 +0100 Subject: [PATCH 2/4] Client: Removing shared metadata pointers from gkfs functions --- include/client/preload_util.hpp | 2 +- src/client/gkfs_functions.cpp | 62 ++++++++++++++++++--------------- src/client/hooks.cpp | 7 ++-- src/client/preload_util.cpp | 14 ++++---- 4 files changed, 47 insertions(+), 38 deletions(-) diff --git a/include/client/preload_util.hpp b/include/client/preload_util.hpp index c8dc9fcfd..1e272c8f5 100644 --- a/include/client/preload_util.hpp +++ b/include/client/preload_util.hpp @@ -55,7 +55,7 @@ to_underlying(E e) { return static_cast::type>(e); } -std::shared_ptr +std::optional get_metadata(const std::string& path, bool follow_links = false); int diff --git a/src/client/gkfs_functions.cpp b/src/client/gkfs_functions.cpp index e2408106e..285f96715 100644 --- a/src/client/gkfs_functions.cpp +++ b/src/client/gkfs_functions.cpp @@ -128,8 +128,8 @@ gkfs_open(const std::string& path, mode_t mode, int flags) { errno = ENOTSUP; return -1; } - // metadata pointer assigned during create or stat - std::shared_ptr md = nullptr; + // metadata object filled during create or stat + gkfs::metadata::Metadata md{}; if(flags & O_CREAT) { if(flags & O_DIRECTORY) { LOG(ERROR, "O_DIRECTORY use with O_CREAT. NOT SUPPORTED"); @@ -138,18 +138,24 @@ gkfs_open(const std::string& path, mode_t mode, int flags) { } // no access check required here. If one is using our FS they have the // permissions. - int err = gkfs_create(path, mode | S_IFREG); + auto err = gkfs_create(path, mode | S_IFREG); if(err) { if(errno == EEXIST) { // file exists, O_CREAT was set if(flags & O_EXCL) { // File exists and O_EXCL & O_CREAT was set - errno = EEXIST; return -1; } // file exists, O_CREAT was set O_EXCL wasnt, so function does // not fail this case is actually undefined as per `man 2 open` - md = gkfs::util::get_metadata(path); + auto md_ = gkfs::utils::get_metadata(path); + if(!md_) { + LOG(ERROR, + "Could not get metadata after creating file '{}': '{}'", + path, strerror(errno)); + return -1; + } + md = *md_; } else { LOG(ERROR, "Error creating file: '{}'", strerror(errno)); return -1; @@ -160,40 +166,38 @@ gkfs_open(const std::string& path, mode_t mode, int flags) { std::make_shared(path, flags)); } } else { - md = gkfs::util::get_metadata(path); - if(!md) { - if(errno == ENOENT) { - // file doesn't exists and O_CREAT was not set - return -1; - } else { - LOG(ERROR, "Error stating existing file"); - return -1; + auto md_ = gkfs::utils::get_metadata(path); + if(!md_) { + if(errno != ENOENT) { + LOG(ERROR, "Error stating existing file '{}'", path); } + // file doesn't exists and O_CREAT was not set + return -1; } + md = *md_; } - assert(md); #ifdef HAS_SYMLINKS - if(md->is_link()) { + if(md.is_link()) { if(flags & O_NOFOLLOW) { LOG(WARNING, "Symlink found and O_NOFOLLOW flag was specified"); errno = ELOOP; return -1; } - return gkfs_open(md->target_path(), mode, flags); + return gkfs_open(md.target_path(), mode, flags); } #endif - if(S_ISDIR(md->mode())) { + if(S_ISDIR(md.mode())) { return gkfs_opendir(path); } /*** Regular file exists ***/ - assert(S_ISREG(md->mode())); + assert(S_ISREG(md.mode())); if((flags & O_TRUNC) && ((flags & O_RDWR) || (flags & O_WRONLY))) { - if(gkfs_truncate(path, md->size(), 0)) { + if(gkfs_truncate(path, md.size(), 0)) { LOG(ERROR, "Error truncating file"); return -1; } @@ -257,8 +261,10 @@ gkfs_remove(const std::string& path) { if(!md) { return -1; } - bool has_data = S_ISREG(md->mode()) && (md->size() != 0); - auto err = gkfs::rpc::forward_remove(path, !has_data, md->size()); + gkfs::metadata::Metadata md{attr.value()}; + + bool has_data = S_ISREG(md.mode()) && (md.size() != 0); + auto err = gkfs::rpc::forward_remove(path, !has_data, md.size()); if(err) { errno = err; return -1; @@ -278,7 +284,6 @@ int gkfs_access(const std::string& path, const int mask, bool follow_links) { auto md = gkfs::utils::get_metadata(path, follow_links); if(!md) { - errno = ENOENT; return -1; } return 0; @@ -549,6 +554,7 @@ gkfs_truncate(const std::string& path, off_t length) { if(!md) { return -1; } + auto size = md->size(); if(static_cast(length) > size) { LOG(DEBUG, "Length is greater then file size: {} > {}", length, size); @@ -862,11 +868,11 @@ gkfs_pread_ws(int fd, void* buf, size_t count, off64_t offset) { */ int gkfs_opendir(const std::string& path) { - auto md = gkfs::utils::get_metadata(path); if(!md) { return -1; } + if(!S_ISDIR(md->mode())) { LOG(DEBUG, "Path is not a directory"); errno = ENOTDIR; @@ -893,8 +899,7 @@ int gkfs_rmdir(const std::string& path) { auto md = gkfs::utils::get_metadata(path); if(!md) { - LOG(DEBUG, "Path '{}' does not exist: ", path); - errno = ENOENT; + LOG(DEBUG, "Error: Path '{}' err code '{}' ", path, strerror(errno)); return -1; } if(!S_ISDIR(md->mode())) { @@ -1089,7 +1094,7 @@ gkfs_mk_symlink(const std::string& path, const std::string& target_path) { * So that application know we don't support link to directory. */ auto target_md = gkfs::utils::get_metadata(target_path, false); - if(target_md != nullptr) { + if(target_md) { auto trg_mode = target_md->mode(); if(!(S_ISREG(trg_mode) || S_ISLNK(trg_mode))) { assert(S_ISDIR(trg_mode)); @@ -1104,11 +1109,12 @@ gkfs_mk_symlink(const std::string& path, const std::string& target_path) { } auto link_md = gkfs::utils::get_metadata(path, false); - if(link_md != nullptr) { + if(link_md) { LOG(DEBUG, "Link exists: '{}'", path); errno = EEXIST; return -1; } + auto err = gkfs::rpc::forward_mk_symlink(path, target_path); if(err) { errno = err; @@ -1131,7 +1137,7 @@ gkfs_mk_symlink(const std::string& path, const std::string& target_path) { int gkfs_readlink(const std::string& path, char* buf, int bufsize) { auto md = gkfs::utils::get_metadata(path, false); - if(md == nullptr) { + if(!md) { LOG(DEBUG, "Named link doesn't exist"); return -1; } diff --git a/src/client/hooks.cpp b/src/client/hooks.cpp index 59d98d79b..dbe997a4d 100644 --- a/src/client/hooks.cpp +++ b/src/client/hooks.cpp @@ -615,10 +615,11 @@ hook_chdir(const char* path) { if(internal) { // path falls in our namespace auto md = gkfs::utils::get_metadata(rel_path); - if(md == nullptr) { - LOG(ERROR, "{}() path does not exists", __func__); - return -ENOENT; + if(!md) { + LOG(ERROR, "{}() path {} errno {}", __func__, path, errno); + return -errno; } + if(!S_ISDIR(md->mode())) { LOG(ERROR, "{}() path is not a directory", __func__); return -ENOTDIR; diff --git a/src/client/preload_util.cpp b/src/client/preload_util.cpp index d4cc99488..a18b902ef 100644 --- a/src/client/preload_util.cpp +++ b/src/client/preload_util.cpp @@ -165,20 +165,21 @@ load_hostfile(const std::string& path) { namespace gkfs::utils { + /** - * Retrieve metadata from daemon + * Retrieve metadata from daemon and return Metadata object * errno may be set * @param path * @param follow_links - * @return shared_ptr for metadata, nullptr else + * @return Metadata */ -std::shared_ptr +optional get_metadata(const string& path, bool follow_links) { std::string attr; auto err = gkfs::rpc::forward_stat(path, attr); if(err) { errno = err; - return nullptr; + return {}; } #ifdef HAS_SYMLINKS if(follow_links) { @@ -187,15 +188,16 @@ get_metadata(const string& path, bool follow_links) { err = gkfs::rpc::forward_stat(md.target_path(), attr); if(err) { errno = err; - return nullptr; + return {}; } md = gkfs::metadata::Metadata{attr}; } } #endif - return make_shared(attr); + return gkfs::metadata::Metadata{attr}; } + /** * Converts the Metadata object into a stat struct, which is needed by Linux * @param path -- GitLab From e8e189c68e2e497928dc687fb84499815a122b86 Mon Sep 17 00:00:00 2001 From: Marc Vef Date: Wed, 18 Nov 2020 14:42:31 +0100 Subject: [PATCH 3/4] Remove: Separating metadata and data removal logic Removes 1 stat RPC per remove operation --- include/client/rpc/forward_metadata.hpp | 3 +- include/client/rpc/rpc_types.hpp | 144 ++++++++++++++++++++++-- include/daemon/handler/rpc_defs.hpp | 4 +- include/global/global_defs.hpp | 3 +- include/global/rpc/rpc_types.hpp | 3 + src/client/gkfs_functions.cpp | 7 +- src/client/rpc/forward_metadata.cpp | 80 +++++++------ src/client/rpc/rpc_types.cpp | 4 +- src/daemon/daemon.cpp | 6 +- src/daemon/handler/srv_metadata.cpp | 51 ++++++++- src/daemon/ops/metadentry.cpp | 6 +- 11 files changed, 243 insertions(+), 68 deletions(-) diff --git a/include/client/rpc/forward_metadata.hpp b/include/client/rpc/forward_metadata.hpp index 71a0adee8..a5ae1976e 100644 --- a/include/client/rpc/forward_metadata.hpp +++ b/include/client/rpc/forward_metadata.hpp @@ -41,8 +41,7 @@ int forward_stat(const std::string& path, std::string& attr); int -forward_remove(const std::string& path, bool remove_metadentry_only, - ssize_t size); +forward_remove(const std::string& path); int forward_decr_size(const std::string& path, size_t length); diff --git a/include/client/rpc/rpc_types.hpp b/include/client/rpc/rpc_types.hpp index 22127d918..021d3fc0a 100644 --- a/include/client/rpc/rpc_types.hpp +++ b/include/client/rpc/rpc_types.hpp @@ -462,8 +462,8 @@ struct stat { }; //============================================================================== -// definitions for remove -struct remove { +// definitions for remove metadata +struct remove_metadata { // forward declarations of public input/output types for this RPC class input; @@ -471,23 +471,23 @@ struct remove { class output; // traits used so that the engine knows what to do with the RPC - using self_type = remove; + using self_type = remove_metadata; using handle_type = hermes::rpc_handle; using input_type = input; using output_type = output; using mercury_input_type = rpc_rm_node_in_t; - using mercury_output_type = rpc_err_out_t; + using mercury_output_type = rpc_rm_metadata_out_t; // RPC public identifier // (N.B: we reuse the same IDs assigned by Margo so that the daemon // understands Hermes RPCs) - constexpr static const uint64_t public_id = 2549415936; + constexpr static const uint64_t public_id = 2087845888; // RPC internal Mercury identifier constexpr static const hg_id_t mercury_id = public_id; // RPC name - constexpr static const auto name = gkfs::rpc::tag::remove; + constexpr static const auto name = gkfs::rpc::tag::remove_metadata; // requires response? constexpr static const auto requires_response = true; @@ -498,7 +498,7 @@ struct remove { // Mercury callback to serialize output arguments constexpr static const auto mercury_out_proc_cb = - HG_GEN_PROC_NAME(rpc_err_out_t); + HG_GEN_PROC_NAME(rpc_rm_metadata_out_t); class input { @@ -541,9 +541,10 @@ struct remove { hermes::detail::post_to_mercury(ExecutionContext*); public: - output() : m_err() {} + output() : m_err(), m_size(), m_mode() {} - output(int32_t err) : m_err(err) {} + output(int32_t err, int64_t size, uint32_t mode) + : m_err(err), m_size(size), m_mode(mode) {} output(output&& rhs) = default; @@ -555,8 +556,10 @@ struct remove { output& operator=(const output& other) = default; - explicit output(const rpc_err_out_t& out) { + explicit output(const rpc_rm_metadata_out_t& out) { m_err = out.err; + m_size = out.size; + m_mode = out.mode; } int32_t @@ -564,8 +567,21 @@ struct remove { return m_err; } + int64_t + size() const { + return m_size; + } + + uint32_t + mode() const { + return m_mode; + }; + + private: int32_t m_err; + int64_t m_size; + uint32_t m_mode; }; }; @@ -1285,6 +1301,114 @@ struct mk_symlink { #endif // HAS_SYMLINKS +//============================================================================== +// definitions for remove data +struct remove_data { + + // forward declarations of public input/output types for this RPC + class input; + + class output; + + // traits used so that the engine knows what to do with the RPC + using self_type = remove_data; + using handle_type = hermes::rpc_handle; + using input_type = input; + using output_type = output; + using mercury_input_type = rpc_rm_node_in_t; + using mercury_output_type = rpc_err_out_t; + + // RPC public identifier + // (N.B: we reuse the same IDs assigned by Margo so that the daemon + // understands Hermes RPCs) + constexpr static const uint64_t public_id = 2649292800; + + // RPC internal Mercury identifier + constexpr static const hg_id_t mercury_id = public_id; + + // RPC name + constexpr static const auto name = gkfs::rpc::tag::remove_data; + + // requires response? + constexpr static const auto requires_response = true; + + // Mercury callback to serialize input arguments + constexpr static const auto mercury_in_proc_cb = + HG_GEN_PROC_NAME(rpc_rm_node_in_t); + + // Mercury callback to serialize output arguments + constexpr static const auto mercury_out_proc_cb = + HG_GEN_PROC_NAME(rpc_err_out_t); + + class input { + + template + friend hg_return_t + hermes::detail::post_to_mercury(ExecutionContext*); + + public: + input(const std::string& path) : m_path(path) {} + + input(input&& rhs) = default; + + input(const input& other) = default; + + input& + operator=(input&& rhs) = default; + + input& + operator=(const input& other) = default; + + std::string + path() const { + return m_path; + } + + explicit input(const rpc_rm_node_in_t& other) : m_path(other.path) {} + + explicit operator rpc_rm_node_in_t() { + return {m_path.c_str()}; + } + + private: + std::string m_path; + }; + + class output { + + template + friend hg_return_t + hermes::detail::post_to_mercury(ExecutionContext*); + + public: + output() : m_err() {} + + output(int32_t err) : m_err(err) {} + + output(output&& rhs) = default; + + output(const output& other) = default; + + output& + operator=(output&& rhs) = default; + + output& + operator=(const output& other) = default; + + explicit output(const rpc_err_out_t& out) { + m_err = out.err; + } + + int32_t + err() const { + return m_err; + } + + private: + int32_t m_err; + }; +}; + //============================================================================== // definitions for write_data struct write_data { diff --git a/include/daemon/handler/rpc_defs.hpp b/include/daemon/handler/rpc_defs.hpp index a721ccebb..8321b9c85 100644 --- a/include/daemon/handler/rpc_defs.hpp +++ b/include/daemon/handler/rpc_defs.hpp @@ -29,7 +29,7 @@ DECLARE_MARGO_RPC_HANDLER(rpc_srv_stat) DECLARE_MARGO_RPC_HANDLER(rpc_srv_decr_size) -DECLARE_MARGO_RPC_HANDLER(rpc_srv_remove) +DECLARE_MARGO_RPC_HANDLER(rpc_srv_remove_metadata) DECLARE_MARGO_RPC_HANDLER(rpc_srv_update_metadentry) @@ -48,6 +48,8 @@ DECLARE_MARGO_RPC_HANDLER(rpc_srv_mk_symlink) // data +DECLARE_MARGO_RPC_HANDLER(rpc_srv_remove_data) + DECLARE_MARGO_RPC_HANDLER(rpc_srv_read) DECLARE_MARGO_RPC_HANDLER(rpc_srv_write) diff --git a/include/global/global_defs.hpp b/include/global/global_defs.hpp index 51a1ee565..aec77c113 100644 --- a/include/global/global_defs.hpp +++ b/include/global/global_defs.hpp @@ -25,7 +25,8 @@ namespace tag { constexpr auto fs_config = "rpc_srv_fs_config"; constexpr auto create = "rpc_srv_mk_node"; constexpr auto stat = "rpc_srv_stat"; -constexpr auto remove = "rpc_srv_rm_node"; +constexpr auto remove_metadata = "rpc_srv_rm_metadata"; +constexpr auto remove_data = "rpc_srv_rm_data"; constexpr auto decr_size = "rpc_srv_decr_size"; constexpr auto update_metadentry = "rpc_srv_update_metadentry"; constexpr auto get_metadentry_size = "rpc_srv_get_metadentry_size"; diff --git a/include/global/rpc/rpc_types.hpp b/include/global/rpc/rpc_types.hpp index 3cdc72bde..5f1f5f745 100644 --- a/include/global/rpc/rpc_types.hpp +++ b/include/global/rpc/rpc_types.hpp @@ -36,6 +36,9 @@ MERCURY_GEN_PROC(rpc_stat_out_t, MERCURY_GEN_PROC(rpc_rm_node_in_t, ((hg_const_string_t)(path))) +MERCURY_GEN_PROC(rpc_rm_metadata_out_t, + ((hg_int32_t)(err))((hg_int64_t)(size))((hg_uint32_t)(mode))) + MERCURY_GEN_PROC(rpc_trunc_in_t, ((hg_const_string_t)(path))((hg_uint64_t)(length))) diff --git a/src/client/gkfs_functions.cpp b/src/client/gkfs_functions.cpp index 285f96715..927cc39c9 100644 --- a/src/client/gkfs_functions.cpp +++ b/src/client/gkfs_functions.cpp @@ -261,10 +261,7 @@ gkfs_remove(const std::string& path) { if(!md) { return -1; } - gkfs::metadata::Metadata md{attr.value()}; - - bool has_data = S_ISREG(md.mode()) && (md.size() != 0); - auto err = gkfs::rpc::forward_remove(path, !has_data, md.size()); + auto err = gkfs::rpc::forward_remove(path); if(err) { errno = err; return -1; @@ -920,7 +917,7 @@ gkfs_rmdir(const std::string& path) { errno = ENOTEMPTY; return -1; } - err = gkfs::rpc::forward_remove(path, true, 0); + err = gkfs::rpc::forward_remove(path); if(err) { errno = err; return -1; diff --git a/src/client/rpc/forward_metadata.cpp b/src/client/rpc/forward_metadata.cpp index e8bd05dad..a5daa98d9 100644 --- a/src/client/rpc/forward_metadata.cpp +++ b/src/client/rpc/forward_metadata.cpp @@ -102,44 +102,52 @@ forward_stat(const std::string& path, string& attr) { * small files (file_size / chunk_size) < number_of_daemons where no broadcast * to all daemons is used to remove all chunks. Otherwise, a broadcast to all * daemons is used. + * + * This function only attempts data removal if data exists (determined when + * metadata is removed) * @param path - * @param remove_metadentry_only - * @param size * @return error code */ int -forward_remove(const std::string& path, const bool remove_metadentry_only, - const ssize_t size) { - - // if only the metadentry should be removed, send one rpc to the - // metadentry's responsible node to remove the metadata - // else, send an rpc to all hosts and thus broadcast chunk_removal. - if(remove_metadentry_only) { - auto endp = - CTX->hosts().at(CTX->distributor()->locate_file_metadata(path)); +forward_remove(const std::string& path) { - try { + auto endp = CTX->hosts().at(CTX->distributor()->locate_file_metadata(path)); + int64_t size = 0; + uint32_t mode = 0; - LOG(DEBUG, "Sending RPC ..."); - // TODO(amiranda): add a post() with RPC_TIMEOUT to hermes so that - // we can retry for RPC_TRIES (see old commits with margo) - // TODO(amiranda): hermes will eventually provide a post(endpoint) - // returning one result and a broadcast(endpoint_set) returning a - // result_set. When that happens we can remove the .at(0) :/ - auto out = ld_network_service->post(endp, path) - .get() - .at(0); + /* + * Send one RPC to metadata destination and remove metadata while retrieving + * size and mode to determine if data needs to removed too + */ + try { + LOG(DEBUG, "Sending RPC ..."); + // TODO(amiranda): add a post() with RPC_TIMEOUT to hermes so that we + // can retry for RPC_TRIES (see old commits with margo) + // TODO(amiranda): hermes will eventually provide a post(endpoint) + // returning one result and a broadcast(endpoint_set) returning a + // result_set. When that happens we can remove the .at(0) :/ + auto out = + ld_network_service->post(endp, path) + .get() + .at(0); - LOG(DEBUG, "Got response success: {}", out.err()); + LOG(DEBUG, "Got response success: {}", out.err()); - return out.err() ? out.err() : 0; - } catch(const std::exception& ex) { - LOG(ERROR, "while getting rpc output"); - return EBUSY; - } + if(out.err()) + return out.err(); + size = out.size(); + mode = out.mode(); + } catch(const std::exception& ex) { + LOG(ERROR, "while getting rpc output"); + return EBUSY; } + // if file is not a regular file and it's size is 0, data does not need to + // be removed, thus, we exit + if(!(S_ISREG(mode) && (size != 0))) + return 0; + - std::vector> handles; + std::vector> handles; // Small files if(static_cast(size / gkfs::config::rpc::chunksize) < @@ -150,9 +158,10 @@ forward_remove(const std::string& path, const bool remove_metadentry_only, try { LOG(DEBUG, "Sending RPC to host: {}", endp_metadata.to_string()); - gkfs::rpc::remove::input in(path); - handles.emplace_back(ld_network_service->post( - endp_metadata, in)); + gkfs::rpc::remove_data::input in(path); + handles.emplace_back( + ld_network_service->post( + endp_metadata, in)); uint64_t chnk_start = 0; uint64_t chnk_end = size / gkfs::config::rpc::chunksize; @@ -171,8 +180,8 @@ forward_remove(const std::string& path, const bool remove_metadentry_only, LOG(DEBUG, "Sending RPC to host: {}", endp_chnk.to_string()); handles.emplace_back( - ld_network_service->post(endp_chnk, - in)); + ld_network_service->post( + endp_chnk, in)); } } catch(const std::exception& ex) { LOG(ERROR, @@ -184,7 +193,7 @@ forward_remove(const std::string& path, const bool remove_metadentry_only, try { LOG(DEBUG, "Sending RPC to host: {}", endp.to_string()); - gkfs::rpc::remove::input in(path); + gkfs::rpc::remove_data::input in(path); // TODO(amiranda): add a post() with RPC_TIMEOUT to hermes so // that we can retry for RPC_TRIES (see old commits with margo) @@ -194,7 +203,8 @@ forward_remove(const std::string& path, const bool remove_metadentry_only, // happens we can remove the .at(0) :/ handles.emplace_back( - ld_network_service->post(endp, in)); + ld_network_service->post(endp, + in)); } catch(const std::exception& ex) { // TODO(amiranda): we should cancel all previously posted diff --git a/src/client/rpc/rpc_types.cpp b/src/client/rpc/rpc_types.cpp index 048a7d0e0..85b0de899 100644 --- a/src/client/rpc/rpc_types.cpp +++ b/src/client/rpc/rpc_types.cpp @@ -22,7 +22,7 @@ hermes::detail::register_user_request_types() { (void) registered_requests().add(); (void) registered_requests().add(); (void) registered_requests().add(); - (void) registered_requests().add(); + (void) registered_requests().add(); (void) registered_requests().add(); (void) registered_requests().add(); (void) registered_requests().add(); @@ -31,7 +31,7 @@ hermes::detail::register_user_request_types() { #ifdef HAS_SYMLINKS (void) registered_requests().add(); #endif // HAS_SYMLINKS - + (void) registered_requests().add(); (void) registered_requests().add(); (void) registered_requests().add(); (void) registered_requests().add(); diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index f8b7f9928..cb4f23a9d 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -93,8 +93,10 @@ register_server_rpcs(margo_instance_id mid) { rpc_stat_out_t, rpc_srv_stat); MARGO_REGISTER(mid, gkfs::rpc::tag::decr_size, rpc_trunc_in_t, rpc_err_out_t, rpc_srv_decr_size); - MARGO_REGISTER(mid, gkfs::rpc::tag::remove, rpc_rm_node_in_t, rpc_err_out_t, - rpc_srv_remove); + MARGO_REGISTER(mid, gkfs::rpc::tag::remove_metadata, rpc_rm_node_in_t, + rpc_rm_metadata_out_t, rpc_srv_remove_metadata); + MARGO_REGISTER(mid, gkfs::rpc::tag::remove_data, rpc_rm_node_in_t, + rpc_err_out_t, rpc_srv_remove_data); MARGO_REGISTER(mid, gkfs::rpc::tag::update_metadentry, rpc_update_metadentry_in_t, rpc_err_out_t, rpc_srv_update_metadentry); diff --git a/src/daemon/handler/srv_metadata.cpp b/src/daemon/handler/srv_metadata.cpp index 1c3e8f4b8..c61b1ce30 100644 --- a/src/daemon/handler/srv_metadata.cpp +++ b/src/daemon/handler/srv_metadata.cpp @@ -148,27 +148,64 @@ rpc_srv_decr_size(hg_handle_t handle) { hg_return_t -rpc_srv_remove(hg_handle_t handle) { +rpc_srv_remove_metadata(hg_handle_t handle) { rpc_rm_node_in_t in{}; - rpc_err_out_t out{}; + rpc_rm_metadata_out_t out{}; auto ret = margo_get_input(handle, &in); if(ret != HG_SUCCESS) GKFS_DATA->spdlogger()->error( "{}() Failed to retrieve input from handle", __func__); assert(ret == HG_SUCCESS); - GKFS_DATA->spdlogger()->debug("{}() Got remove node RPC with path '{}'", + GKFS_DATA->spdlogger()->debug("{}() Got remove metadata RPC with path '{}'", __func__, in.path); - // Remove metadentry if exists on the node and remove all chunks for that - // file + // Remove metadentry if exists on the node try { + auto md = gkfs::metadata::get(in.path); gkfs::metadata::remove(in.path); out.err = 0; + out.mode = md.mode(); + out.size = md.size(); } catch(const gkfs::metadata::DBException& e) { GKFS_DATA->spdlogger()->error("{}(): path '{}' message '{}'", __func__, in.path, e.what()); out.err = EIO; + } catch(const std::exception& e) { + GKFS_DATA->spdlogger()->error("{}() path '{}' message '{}'", __func__, + in.path, e.what()); + out.err = EBUSY; + } + + GKFS_DATA->spdlogger()->debug("{}() Sending output '{}'", __func__, + out.err); + auto hret = margo_respond(handle, &out); + if(hret != HG_SUCCESS) { + GKFS_DATA->spdlogger()->error("{}() Failed to respond", __func__); + } + // Destroy handle when finished + margo_free_input(handle, &in); + margo_destroy(handle); + return HG_SUCCESS; +} + +hg_return_t +rpc_srv_remove_data(hg_handle_t handle) { + rpc_rm_node_in_t in{}; + rpc_err_out_t out{}; + + auto ret = margo_get_input(handle, &in); + if(ret != HG_SUCCESS) + GKFS_DATA->spdlogger()->error( + "{}() Failed to retrieve input from handle", __func__); + assert(ret == HG_SUCCESS); + GKFS_DATA->spdlogger()->debug("{}() Got remove data RPC with path '{}'", + __func__, in.path); + + // Remove all chunks for that file + try { + GKFS_DATA->storage()->destroy_chunk_space(in.path); + out.err = 0; } catch(const gkfs::data::ChunkStorageException& e) { GKFS_DATA->spdlogger()->error( "{}(): path '{}' errcode '{}' message '{}'", __func__, in.path, @@ -669,7 +706,9 @@ DEFINE_MARGO_RPC_HANDLER(rpc_srv_stat) DEFINE_MARGO_RPC_HANDLER(rpc_srv_decr_size) -DEFINE_MARGO_RPC_HANDLER(rpc_srv_remove) +DEFINE_MARGO_RPC_HANDLER(rpc_srv_remove_metadata) + +DEFINE_MARGO_RPC_HANDLER(rpc_srv_remove_data) DEFINE_MARGO_RPC_HANDLER(rpc_srv_update_metadentry) diff --git a/src/daemon/ops/metadentry.cpp b/src/daemon/ops/metadentry.cpp index 7005d29b2..2f6457ffa 100644 --- a/src/daemon/ops/metadentry.cpp +++ b/src/daemon/ops/metadentry.cpp @@ -126,10 +126,10 @@ update_size(const string& path, size_t io_size, off64_t offset, bool append) { } /** - * Remove metadentry if exists and try to remove all chunks for path + * Remove metadentry if exists * @param path * @return - * @throws gkfs::metadata::DBException, gkfs::data::ChunkStorageException + * @throws gkfs::metadata::DBException */ void remove(const string& path) { @@ -142,8 +142,6 @@ remove(const string& path) { GKFS_DATA->mdb()->remove(path); // remove metadata from KV store } catch(const NotFoundException& e) { } - GKFS_DATA->storage()->destroy_chunk_space( - path); // destroys all chunks for the path on this node } } // namespace gkfs::metadata -- GitLab From ff0c5dc16c52f9d650623faac6ef86462572aeef Mon Sep 17 00:00:00 2001 From: Marc Vef Date: Fri, 20 Nov 2020 10:27:22 +0100 Subject: [PATCH 4/4] Adding implicit_data_removal config for optimized remove operations --- include/config.hpp | 7 +++++++ src/client/rpc/forward_metadata.cpp | 15 +++++++++------ src/daemon/backend/data/chunk_storage.cpp | 4 ++-- src/daemon/handler/srv_metadata.cpp | 9 +++++++++ 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/include/config.hpp b/include/config.hpp index c75cb0ef5..2fea22dfe 100644 --- a/include/config.hpp +++ b/include/config.hpp @@ -48,6 +48,13 @@ constexpr auto use_ctime = false; constexpr auto use_mtime = false; constexpr auto use_link_cnt = false; constexpr auto use_blocks = false; +/* + * If true, all chunks on the same host are removed during a metadata remove + * rpc. This is a technical optimization that reduces the number of RPCs for + * remove operations. This setting could be useful for future asynchronous + * remove implementations where the data should not be removed immediately. + */ +constexpr auto implicit_data_removal = true; // metadata logic // Check for existence of file metadata before create. This done on RocksDB diff --git a/src/client/rpc/forward_metadata.cpp b/src/client/rpc/forward_metadata.cpp index a5daa98d9..fa494e492 100644 --- a/src/client/rpc/forward_metadata.cpp +++ b/src/client/rpc/forward_metadata.cpp @@ -169,12 +169,15 @@ forward_remove(const std::string& path) { for(uint64_t chnk_id = chnk_start; chnk_id <= chnk_end; chnk_id++) { const auto chnk_host_id = CTX->distributor()->locate_data(path, chnk_id); - /* - * If the chnk host matches the metadata host the remove request - * as already been sent as part of the metadata remove request. - */ - if(chnk_host_id == metadata_host_id) - continue; + if constexpr(gkfs::config::metadata::implicit_data_removal) { + /* + * If the chnk host matches the metadata host the remove + * request as already been sent as part of the metadata + * remove request. + */ + if(chnk_host_id == metadata_host_id) + continue; + } const auto endp_chnk = CTX->hosts().at(chnk_host_id); LOG(DEBUG, "Sending RPC to host: {}", endp_chnk.to_string()); diff --git a/src/daemon/backend/data/chunk_storage.cpp b/src/daemon/backend/data/chunk_storage.cpp index a343a132a..ecc56b942 100644 --- a/src/daemon/backend/data/chunk_storage.cpp +++ b/src/daemon/backend/data/chunk_storage.cpp @@ -108,8 +108,8 @@ ChunkStorage::destroy_chunk_space(const string& file_path) const { try { // Note: remove_all does not throw an error when path doesn't exist. auto n = fs::remove_all(chunk_dir); - log_->debug("{}() Removed '{}' files from '{}'", __func__, n, - chunk_dir); + log_->debug("{}() Removed '{}' files and directories from '{}'", + __func__, n, chunk_dir); } catch(const fs::filesystem_error& e) { auto err_str = fmt::format( "{}() Failed to remove chunk directory. Path: '{}', Error: '{}'", diff --git a/src/daemon/handler/srv_metadata.cpp b/src/daemon/handler/srv_metadata.cpp index c61b1ce30..03f604324 100644 --- a/src/daemon/handler/srv_metadata.cpp +++ b/src/daemon/handler/srv_metadata.cpp @@ -167,10 +167,19 @@ rpc_srv_remove_metadata(hg_handle_t handle) { out.err = 0; out.mode = md.mode(); out.size = md.size(); + if constexpr(gkfs::config::metadata::implicit_data_removal) { + if(S_ISREG(md.mode()) && (md.size() != 0)) + GKFS_DATA->storage()->destroy_chunk_space(in.path); + } } catch(const gkfs::metadata::DBException& e) { GKFS_DATA->spdlogger()->error("{}(): path '{}' message '{}'", __func__, in.path, e.what()); out.err = EIO; + } catch(const gkfs::data::ChunkStorageException& e) { + GKFS_DATA->spdlogger()->error( + "{}(): path '{}' errcode '{}' message '{}'", __func__, in.path, + e.code().value(), e.what()); + out.err = e.code().value(); } catch(const std::exception& e) { GKFS_DATA->spdlogger()->error("{}() path '{}' message '{}'", __func__, in.path, e.what()); -- GitLab