Resolve "Remove superfluous RPC during remove"

Marc Vefrequested to merge
marc/293-remove-superfluous-rpc-during-remove into master

This MR improves remove() performance by avoiding one RPC.

Before the change, the metadata of a path was fetched to check whether it is a directory or file. This is important because rmdir() and unlink() should not delete the wrong object. However, this meant that two RPCs were done per remove operation.

This update changes this behavior by checking the directory/file on the server during any remove operation. For this, an additional RPC field was added, which communicates whether the intent is to remove a directory. Overall, the semantics stay the same. The special case for rename() still requires the metadata check beforehand and is unchanged.

IO500 has shown that this optimization doubles throughput for latency-sensitive operations, i.e., zero-byte files or small files.

Depends on !191 (merged)

Closes #293 (closed)

Edited by Marc Vef

Merge request reports

include/client/rpc/forward_metadata.hpp

+1 −1
Changes for include/client/rpc/forward_metadata.hpp: 1 added line, 1 removed line.
Original line number Diff line number Diff line
@@ -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);

include/client/rpc/forward_metadata_proxy.hpp

+1 −1
Changes for include/client/rpc/forward_metadata_proxy.hpp: 1 added line, 1 removed line.
Original line number Diff line number Diff line
@@ -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);

include/client/rpc/rpc_types.hpp

+22 −6
Changes for include/client/rpc/rpc_types.hpp: 22 added lines, 6 removed lines.
Original line number Diff line number Diff line
@@ -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 {

include/common/rpc/rpc_types.hpp

+2 −1
Changes for include/common/rpc/rpc_types.hpp: 2 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -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,

include/common/common_defs.hpp

+8 −0
Changes for include/common/common_defs.hpp: 8 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -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