diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fb21bebc5ebb15f7358ed56d0331f2e27216d6b..aa8775d9f0702182e7da2bdad8c990c94f281d92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] ### New +- Remove optimization, removing one RPC per operation ([!195](https://storage.bsc.es/gitlab/hpc/gekkofs/-/merge_request/195)). - Added the GekkoFS proxy as an optional gateway between client and daemon. The proxy is started on each compute node that houses clients ([!191](https://storage.bsc.es/gitlab/hpc/gekkofs/-/merge_request/191)). - Additional options for the GekkoFS daemon were added to integrate the GekkoFS proxy. diff --git a/include/client/rpc/forward_metadata.hpp b/include/client/rpc/forward_metadata.hpp index ac3cd4512b4af918395c3ed799f02eb3272dc960..88ed1118884241042a2589be5f769a80e188483c 100644 --- a/include/client/rpc/forward_metadata.hpp +++ b/include/client/rpc/forward_metadata.hpp @@ -62,7 +62,7 @@ forward_rename(const std::string& oldpath, const std::string& newpath, #endif // HAS_RENAME int -forward_remove(const std::string& path, const int8_t num_copies); +forward_remove(const std::string& path, bool rm_dir, const int8_t num_copies); int forward_decr_size(const std::string& path, size_t length, const int copy); diff --git a/include/client/rpc/forward_metadata_proxy.hpp b/include/client/rpc/forward_metadata_proxy.hpp index 2e87106faee307b0662811649cbfef17ec29c247..8e5ff1defa04f789d173ddae3172eca50f11e36c 100644 --- a/include/client/rpc/forward_metadata_proxy.hpp +++ b/include/client/rpc/forward_metadata_proxy.hpp @@ -23,7 +23,7 @@ int forward_stat_proxy(const std::string& path, std::string& attr); int -forward_remove_proxy(const std::string& path); +forward_remove_proxy(const std::string& path, bool rm_dir); int forward_decr_size_proxy(const std::string& path, size_t length); diff --git a/include/client/rpc/rpc_types.hpp b/include/client/rpc/rpc_types.hpp index a259e46d140df13f61e8410847e798a5e561afc4..485619e60ea432d64baa4e7d2b79e126c3c34b73 100644 --- a/include/client/rpc/rpc_types.hpp +++ b/include/client/rpc/rpc_types.hpp @@ -520,7 +520,8 @@ struct remove_metadata { hermes::detail::post_to_mercury(ExecutionContext*); public: - input(const std::string& path) : m_path(path) {} + input(const std::string& path, bool rm_dir) + : m_path(path), m_rm_dir(rm_dir) {} input(input&& rhs) = default; @@ -537,14 +538,21 @@ struct remove_metadata { return m_path; } - explicit input(const rpc_rm_node_in_t& other) : m_path(other.path) {} + bool + rm_dir() const { + return m_rm_dir; + } + + explicit input(const rpc_rm_node_in_t& other) + : m_path(other.path), m_rm_dir(other.rm_dir) {} explicit operator rpc_rm_node_in_t() { - return {m_path.c_str()}; + return {m_path.c_str(), m_rm_dir}; } private: std::string m_path; + bool m_rm_dir; }; class output { @@ -3113,7 +3121,8 @@ struct remove_proxy { hermes::detail::post_to_mercury(ExecutionContext*); public: - input(const std::string& path) : m_path(path) {} + input(const std::string& path, bool rm_dir) + : m_path(path), m_rm_dir(rm_dir) {} input(input&& rhs) = default; @@ -3130,14 +3139,21 @@ struct remove_proxy { return m_path; } - explicit input(const rpc_rm_node_in_t& other) : m_path(other.path) {} + bool + rm_dir() const { + return m_rm_dir; + } + + explicit input(const rpc_rm_node_in_t& other) + : m_path(other.path), m_rm_dir(other.rm_dir) {} explicit operator rpc_rm_node_in_t() { - return {m_path.c_str()}; + return {m_path.c_str(), m_rm_dir}; } private: std::string m_path; + bool m_rm_dir; }; class output { diff --git a/include/common/common_defs.hpp b/include/common/common_defs.hpp index f9768d8090b67c49715cc91b0b4cf8cbc4ca53ca..206ab7de192c4cf2741d0da16a63184d4bd0f1bb 100644 --- a/include/common/common_defs.hpp +++ b/include/common/common_defs.hpp @@ -90,9 +90,17 @@ constexpr auto ucx_all = "ucx+all"; constexpr auto ucx_tcp = "ucx+tcp"; constexpr auto ucx_rc = "ucx+rc"; constexpr auto ucx_ud = "ucx+ud"; +/* + * Disable warning for unused variable + * This is a bug in gcc that was fixed in version 13. + * XXX Can be removed in the future + */ +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wunused-variable" constexpr auto all_remote_protocols = {ofi_sockets, ofi_tcp, ofi_verbs, ofi_psm2, ucx_all, ucx_tcp, ucx_rc, ucx_ud}; +#pragma GCC diagnostic pop } // namespace protocol } // namespace gkfs::rpc diff --git a/include/common/rpc/rpc_types.hpp b/include/common/rpc/rpc_types.hpp index e247d14ac718fc301b0e96b64cef55e41970b2a6..40fc200057d679ee837c72debf6ad053333050fb 100644 --- a/include/common/rpc/rpc_types.hpp +++ b/include/common/rpc/rpc_types.hpp @@ -48,7 +48,8 @@ MERCURY_GEN_PROC(rpc_path_only_in_t, ((hg_const_string_t) (path))) MERCURY_GEN_PROC(rpc_stat_out_t, ((hg_int32_t) (err))((hg_const_string_t) (db_val))) -MERCURY_GEN_PROC(rpc_rm_node_in_t, ((hg_const_string_t) (path))) +MERCURY_GEN_PROC(rpc_rm_node_in_t, + ((hg_const_string_t) (path))((hg_bool_t) (rm_dir))) MERCURY_GEN_PROC( rpc_rm_metadata_out_t, diff --git a/include/proxy/rpc/forward_metadata.hpp b/include/proxy/rpc/forward_metadata.hpp index c5ff43098377995439b49924548f3db50b4bff23..a16e5f5cf66da76b73c48ea95961ecc079e947e2 100644 --- a/include/proxy/rpc/forward_metadata.hpp +++ b/include/proxy/rpc/forward_metadata.hpp @@ -25,7 +25,7 @@ std::pair forward_stat(const std::string& path); int -forward_remove(const std::string& path); +forward_remove(const std::string& path, bool rm_dir); int forward_decr_size(const std::string& path, size_t length); diff --git a/src/client/gkfs_functions.cpp b/src/client/gkfs_functions.cpp index 90a1ca0ae8d7cc092a1e2cabeb1a4ccd33183ac3..ec04145f1fc169239e80e0f961bf78d8ffcd5d01 100644 --- a/src/client/gkfs_functions.cpp +++ b/src/client/gkfs_functions.cpp @@ -339,6 +339,8 @@ gkfs_create(const std::string& path, mode_t mode) { */ int gkfs_remove(const std::string& path) { +#ifdef HAS_SYMLINKS +#ifdef HAS_RENAME auto md = gkfs::utils::get_metadata(path); if(!md) { return -1; @@ -349,8 +351,6 @@ gkfs_remove(const std::string& path) { errno = EISDIR; return -1; } -#ifdef HAS_SYMLINKS -#ifdef HAS_RENAME if(md.value().blocks() == -1) { errno = ENOENT; return -1; @@ -365,7 +365,8 @@ gkfs_remove(const std::string& path) { return -1; } } - auto err = gkfs::rpc::forward_remove(new_path, CTX->get_replicas()); + auto err = gkfs::rpc::forward_remove(new_path, false, + CTX->get_replicas()); if(err) { errno = err; return -1; @@ -376,9 +377,9 @@ gkfs_remove(const std::string& path) { #endif // HAS_SYMLINKS int err = 0; if(gkfs::config::proxy::fwd_remove && CTX->use_proxy()) { - err = gkfs::rpc::forward_remove_proxy(path); + err = gkfs::rpc::forward_remove_proxy(path, false); } else { - err = gkfs::rpc::forward_remove(path, CTX->get_replicas()); + err = gkfs::rpc::forward_remove(path, false, CTX->get_replicas()); } if(err) { errno = err; @@ -478,7 +479,11 @@ gkfs_rename(const string& old_path, const string& new_path) { return -1; } // Delete old file - err = gkfs::rpc::forward_remove(old_path, CTX->get_replicas()); + auto is_dir = false; + if(S_ISDIR(md_old->mode())) + is_dir = true; + err = gkfs::rpc::forward_remove(old_path, is_dir, + CTX->get_replicas()); if(err) { errno = err; return -1; @@ -1340,19 +1345,17 @@ gkfs_opendir(const std::string& path) { */ int gkfs_rmdir(const std::string& path) { + int err; + // check that directory is empty if a strict dir hierarchy is enforced + // TODO rename #define +#if GKFS_CREATE_CHECK_PARENTS auto md = gkfs::utils::get_metadata(path); if(!md) { LOG(DEBUG, "Error: Path '{}' err code '{}' ", path, strerror(errno)); return -1; } - if(!S_ISDIR(md->mode())) { - LOG(DEBUG, "Path '{}' is not a directory", path); - errno = ENOTDIR; - return -1; - } - auto ret = gkfs::rpc::forward_get_dirents(path); - auto err = ret.first; + err = ret.first; if(err) { errno = err; return -1; @@ -1363,10 +1366,11 @@ gkfs_rmdir(const std::string& path) { errno = ENOTEMPTY; return -1; } +#endif if(gkfs::config::proxy::fwd_remove && CTX->use_proxy()) { - err = gkfs::rpc::forward_remove_proxy(path); + err = gkfs::rpc::forward_remove_proxy(path, true); } else { - err = gkfs::rpc::forward_remove(path, CTX->get_replicas()); + err = gkfs::rpc::forward_remove(path, true, CTX->get_replicas()); } if(err) { errno = err; diff --git a/src/client/rpc/forward_metadata.cpp b/src/client/rpc/forward_metadata.cpp index e569d873f7f0281b620556940c97d2d414d93575..c5c30957352c4f517ff0d6234492f3cf304b5eed 100644 --- a/src/client/rpc/forward_metadata.cpp +++ b/src/client/rpc/forward_metadata.cpp @@ -135,7 +135,7 @@ forward_stat(const std::string& path, string& attr, const int copy) { * @return error code */ int -forward_remove(const std::string& path, const int8_t num_copies) { +forward_remove(const std::string& path, bool rm_dir, const int8_t num_copies) { if(gkfs::config::proxy::fwd_remove && CTX->use_proxy()) { LOG(WARNING, "{} was called even though proxy should be used!", __func__); @@ -159,7 +159,8 @@ forward_remove(const std::string& path, const int8_t num_copies) { // 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) + ->post(endp, path, + rm_dir) .get() .at(0); @@ -174,9 +175,9 @@ forward_remove(const std::string& path, const int8_t num_copies) { return EBUSY; } } - // if file is not a regular file and it's size is 0, data does not need to + // if file is not a regular file or it's size is 0, data does not need to // be removed, thus, we exit - if(!(S_ISREG(mode) && (size != 0))) + if(!S_ISREG(mode) || size == 0) return 0; diff --git a/src/client/rpc/forward_metadata_proxy.cpp b/src/client/rpc/forward_metadata_proxy.cpp index 0e43b352d7480986d94793e47c337530f2c0458a..b2892ec69cc8848baa58badee0bed28e2eeb908d 100644 --- a/src/client/rpc/forward_metadata_proxy.cpp +++ b/src/client/rpc/forward_metadata_proxy.cpp @@ -78,7 +78,7 @@ forward_stat_proxy(const std::string& path, string& attr) { } int -forward_remove_proxy(const std::string& path) { +forward_remove_proxy(const std::string& path, bool rm_dir) { auto endp = CTX->proxy_host(); try { @@ -88,7 +88,8 @@ forward_remove_proxy(const std::string& path) { // 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_proxy_service->post(endp, path) + auto out = ld_proxy_service + ->post(endp, path, rm_dir) .get() .at(0); LOG(DEBUG, "Got response success: {}", out.err()); diff --git a/src/daemon/handler/srv_metadata.cpp b/src/daemon/handler/srv_metadata.cpp index 7351b2fb9428f661afa23c788ea7f03f609ba960..d34247403e2c27f2f26e93b1bc53d1d1c59ecce8 100644 --- a/src/daemon/handler/srv_metadata.cpp +++ b/src/daemon/handler/srv_metadata.cpp @@ -241,23 +241,30 @@ rpc_srv_remove_metadata(hg_handle_t handle) { GKFS_DATA->spdlogger()->debug("{}() Got remove metadata RPC with path '{}'", __func__, in.path); - // 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(); - if constexpr(gkfs::config::metadata::implicit_data_removal) { - if(S_ISREG(md.mode()) && (md.size() != 0)) - GKFS_DATA->storage()->destroy_chunk_space(in.path); + if(S_ISDIR(md.mode()) && !in.rm_dir) { + // return is directory errorcode if request was not rmdir + out.err = EISDIR; + } else if(!S_ISDIR(md.mode()) && in.rm_dir) { + // return is not directory errorcode if request was rmdir + out.err = ENOTDIR; + } else { + // remove metadata (and implicitly data if enabled + gkfs::metadata::remove(in.path); + out.err = 0; + out.mode = md.mode(); + out.size = S_ISDIR(md.mode()) ? 0 : md.size(); + // if file, remove metadata and also return mode and 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::NotFoundException& e) { - GKFS_DATA->spdlogger()->warn( - "{}(): path '{}' message '{}'. Continuing, setting out.err 0.", - __func__, in.path, e.what()); - out.err = 0; + // This exception is only thrown from get() if the entry does not exist + // remove() does not throw this exception + out.err = ENOENT; } catch(const gkfs::metadata::DBException& e) { GKFS_DATA->spdlogger()->error("{}(): path '{}' message '{}'", __func__, in.path, e.what()); diff --git a/src/proxy/rpc/forward_metadata.cpp b/src/proxy/rpc/forward_metadata.cpp index 830b7db517244ebbde05b30482e1792dce74095d..36f83fb361805774caf6035e0a36f41db3fe7343 100644 --- a/src/proxy/rpc/forward_metadata.cpp +++ b/src/proxy/rpc/forward_metadata.cpp @@ -22,7 +22,7 @@ using namespace std; namespace { std::tuple -remove_metadata(const std::string& path) { +remove_metadata(const std::string& path, bool rm_dir) { hg_handle_t rpc_handle = nullptr; rpc_rm_node_in_t daemon_in{}; rpc_rm_metadata_out_t daemon_out{}; @@ -31,6 +31,7 @@ remove_metadata(const std::string& path) { uint32_t mode = 0; // fill in daemon_in.path = path.c_str(); + daemon_in.rm_dir = rm_dir; // Create handle PROXY_DATA->log()->debug("{}() Creating Margo handle ...", __func__); auto endp = PROXY_DATA->rpc_endpoints().at( @@ -51,9 +52,11 @@ remove_metadata(const std::string& path) { if(ret == HG_SUCCESS) { PROXY_DATA->log()->debug("{}() Got response success: {}", __func__, daemon_out.err); - mode = daemon_out.mode; - size = daemon_out.size; err = daemon_out.err; + if(!err) { + mode = daemon_out.mode; + size = daemon_out.size; + } margo_free_output(rpc_handle, &daemon_out); } else { // something is wrong @@ -233,16 +236,15 @@ forward_stat(const std::string& path) { } int -forward_remove(const std::string& path) { - auto [err, mode, size] = remove_metadata(path); +forward_remove(const std::string& path, bool rm_dir) { + auto [err, mode, size] = remove_metadata(path, rm_dir); if(err != 0) { return err; } - // if file is not a regular file and it's size is 0, data does not need to + // if file is not a regular file or it's size is 0, data does not need to // be removed, thus, we exit - if(!(S_ISREG(mode) && (size != 0))) { + if(!S_ISREG(mode) || size == 0) return 0; - } return remove_data(path); } diff --git a/src/proxy/rpc/srv_metadata.cpp b/src/proxy/rpc/srv_metadata.cpp index df07724d1cfeacf37e9ed07fa5fd6bbd8f6248d6..4b973c03a48bec0f184f573d12c92365131b4050 100644 --- a/src/proxy/rpc/srv_metadata.cpp +++ b/src/proxy/rpc/srv_metadata.cpp @@ -79,7 +79,8 @@ proxy_rpc_srv_remove(hg_handle_t handle) { } PROXY_DATA->log()->debug("{}() Got RPC with path '{}'", __func__, client_in.path); - client_out.err = gkfs::rpc::forward_remove(client_in.path); + client_out.err = + gkfs::rpc::forward_remove(client_in.path, client_in.rm_dir); PROXY_DATA->log()->debug("{}() Sending output err '{}'", __func__, client_out.err);