Resolve "Refactor path resolution"

Marc Vefrequested to merge
jathenst/281-refactor-path-resolution into master

Refactors path resolve mechanism and adds two new CMake options:

  • GKFS_USE_LEGACY_PATH_RESOLVE - Use the legacy implementation of the resolve function, deprecated (default: OFF)
  • GKFS_FOLLOW_EXTERNAL_SYMLINKS - Enable support for following external links for resolving the path (default: OFF)
    • This is automatically enabled in the deprecated version and causes an lstat() system call on each individual path component. This has been an issue in the past where performance was considerably impacted by the mountpath being placed within the parallel file system.
    • It is now disabled by default to improve performance. In case, it causes issues, we can re-enable it.

Closes #281 (closed)

Edited by Marc Vef

Merge request reports

CMake/gkfs-options.cmake

+16 −0
Changes for CMake/gkfs-options.cmake: 16 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -234,6 +234,14 @@ gkfs_define_option( 
    DEFAULT_VALUE OFF

)





# use old resolve function

gkfs_define_option(

  GKFS_USE_LEGACY_PATH_RESOLVE

  HELP_TEXT "Use the old implementation of the resolve function"

  DEFAULT_VALUE OFF

)



cmake_dependent_option(GKFS_INSTALL_TESTS "Install GekkoFS self tests" OFF "GKFS_BUILD_TESTS" OFF)
@@ -265,6 +273,14 @@ gkfs_define_option( 
  DESCRIPTION "Compile with support for rename ops (experimental)"

)



## external link support

gkfs_define_option(

  GKFS_FOLLOW_EXTERNAL_SYMLINKS

  HELP_TEXT "Enable support for following external links for resolving the path"

  DEFAULT_VALUE OFF

  DESCRIPTION "Compile with lstat usage in path resolve"

)





################################################################################

# Options and variables that control how GekkoFS behaves internally

include/client/path.hpp

+10 −1
Changes for include/client/path.hpp: 10 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -36,7 +36,16 @@ unsigned int 
match_components(const std::string& path, unsigned int& path_components,

                 const std::vector<std::string>& components);



bool

/// @resolve_last_link is used only for the old implementation:

/// GKFS_USE_LEGACY_PATH_RESOLVE

std::pair<bool, std::string>

resolve(const std::string& path, bool resolve_last_link = true);



std::pair<bool, std::string>

resolve_new(const std::string& path);



[[deprecated(

        "Use GKFS_USE_LEGACY_PATH_RESOLVE to use old implementation")]] bool

resolve(const std::string& path, std::string& resolved,

        bool resolve_last_link = true);

src/client/path.cpp

+101 −1
Changes for src/client/path.cpp: 101 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -34,10 +34,11 @@ 


#include <common/path_util.hpp>



#include <stack>

#include <utility>

#include <vector>

#include <string>

#include <cassert>

#include <climits>



#ifndef BYPASS_SYSCALL

#include <libsyscall_intercept_hook_point.h>
@@ -100,6 +101,105 @@ match_components(const string& path, unsigned int& path_components, 
    return matched;

}



string

follow_symlinks(const string& path) {

    struct stat st {};

    if(lstat(path.c_str(), &st) < 0) {

        LOG(DEBUG, "path \"{}\" does not exist", path);

        return path;

    }

    if(S_ISLNK(st.st_mode)) {

        auto link_resolved = ::unique_ptr<char[]>(new char[PATH_MAX]);

        if(realpath(path.c_str(), link_resolved.get()) == nullptr) {



            LOG(ERROR,

                "Failed to get realpath for link \"{}\". "

                "Error: {}",

                path, ::strerror(errno));

            return path;

        }

        // substituute resolved with new link path

        return link_resolved.get();

    }

    return path;

}



pair<bool, string>

resolve(const string& path, bool resolve_last_link) {

#ifdef GKFS_USE_LEGACY_PATH_RESOLVE

    string resolved;

    bool is_in_path = resolve(path, resolved, resolve_last_link);

    return make_pair(is_in_path, resolved);

#else

    return resolve_new(path);

#endif

}



pair<bool, string>

resolve_new(const string& path) {

    LOG(DEBUG, "path: \"{}\", mountdir: \"{}\"", path, CTX->mountdir());



    if(path.empty()) {

        return make_pair(false, "/");

    }



    string resolved = "";

    stack<size_t> last_component_pos;

    const string absolute_path = (path.at(0) == path::separator)

                                         ? path

                                         : CTX->cwd() + path::separator + path;



    for(size_t start = 0; start < absolute_path.size(); start++) {

        size_t end = absolute_path.find(path::separator, start);

        // catches the case without separator at the end

        if(end == string::npos) {

            end = absolute_path.size();

        }

        size_t comp_size = end - start;

        if(comp_size == 0 && absolute_path.at(start) == path::separator) {

            continue;

        }

        if(comp_size == 1 && absolute_path.at(start) == '.') {

            // component is '.', we skip it

            continue;

        }

        if(comp_size == 2 && absolute_path.at(start) == '.' &&

           absolute_path.at(start + 1) == '.') {

            // component is '..', we skip it

            LOG(DEBUG, "path: \"{}\", mountdir: \"{}\"", absolute_path,

                CTX->mountdir());

            if(last_component_pos.empty()) {

                resolved = "";

            } else {

                resolved.erase(last_component_pos.top());

                last_component_pos.pop();

            }

            continue;

        }

        // add `/<component>` to the reresolved path

        resolved.push_back(path::separator);

        last_component_pos.push(resolved.size() - 1);

        resolved.append(absolute_path, start, comp_size);

        start = end;



#ifdef GKFS_FOLLOW_EXTERNAL_SYMLINKS

        resolved = follow_symlinks(resolved);

#endif

    }



    if(resolved.substr(0, CTX->mountdir().size()) == CTX->mountdir()) {

        resolved.erase(1, CTX->mountdir().size());

        LOG(DEBUG, "internal: \"{}\"", resolved);

        return make_pair(true, resolved);

    }



    if(resolved.empty()) {

        resolved.push_back(path::separator);

    }

    LOG(DEBUG, "external: \"{}\"", resolved);

    return make_pair(false, resolved);

}



/** Resolve path to its canonical representation

 *

 * Populate `resolved` with the canonical representation of `path`.

src/client/preload_context.cpp

+9 −2
Changes for src/client/preload_context.cpp: 9 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -45,6 +45,7 @@ 


#include <cassert>

#include <filesystem>

#include <utility>



#ifndef BYPASS_SYSCALL

#include <libsyscall_intercept_hook_point.h>
@@ -317,7 +318,10 @@ PreloadContext::relativize_fd_path(int dirfd, const char* raw_path, 
        path = raw_path;

    }



    if(gkfs::path::resolve(path, relative_path, resolve_last_link)) {

    auto [is_in_path, resolved_path] =

            gkfs::path::resolve(path, resolve_last_link);

    relative_path = resolved_path;

    if(is_in_path) {

        return RelativizeStatus::internal;

    }

    return RelativizeStatus::external;
@@ -347,7 +351,10 @@ PreloadContext::relativize_path(const char* raw_path, 
        path = raw_path;

    }



    return gkfs::path::resolve(path, relative_path, resolve_last_link);

    auto [is_in_path, resolved_path] =

            gkfs::path::resolve(path, resolve_last_link);

    relative_path = resolved_path;

    return is_in_path;

}



const std::string&

tests/integration/directories/test_pathresolution.py

+1 −1
Changes for tests/integration/directories/test_pathresolution.py: 1 added line, 1 removed line.
Original line number Diff line number Diff line
@@ -86,7 +86,7 @@ def test_pathresolution(gkfs_daemon, gkfs_client): 
    ret = gkfs_client.chdir(extdir)

    assert ret.retval == 0



    ret = gkfs_client.getcwd_validate(str(intdir)+"../../../../../../../../../../../../../../../../../../.."+str(intdir))

    ret = gkfs_client.getcwd_validate(str(intdir)+"/../../../../../../../../../../../../../../../../../../.."+str(intdir))

    assert ret.path == str(intdir)

    assert ret.retval == 0