[TransferEngine] Introduce generic NVMeoF transport implementation (#780)

[nickyc975]

This pull request adds support for file segments to Transfer Engine and Mooncake Store, then introduces a generic NVMeoF transport implementation to Transfer Engine.

Please read the RFC (#780) and the documents in this PR for more information.

[SgtPepperr]

Hello, I'm very happy to see the code support for NVMe over Fabrics as a transport. However, I have a small question to ask. Will adding NVMe segments here conflict with the currently implemented tiered caching mechanism #578 , Mooncake+3FS, or are the two compatible?

[nickyc975]

Hello, I'm very happy to see the code support for NVMe over Fabrics as a transport. However, I have a small question to ask. Will adding NVMe segments here conflict with the currently implemented tiered caching mechanism #578 , Mooncake+3FS, or are the two compatible?

Hello, the two should be compatible by design. Although not recommended, NVMe segments can be used with 3FS, just like memory segments.

[stmatengss]

Could we add more parameters in setup or use environment variables to complete the file setup?

[nickyc975]

Yes, we can.
The only difference between setup_with_files and setup is that setup_with_files takes a files argument instead of the global_segment_size argument. We can add the files argument as the last argument of setup for compatibility. Do you think this is better than adding setup_with_files?

[stmatengss]

Currently working on releasing 0.3.5 and will review this PR ASAP. @nickyc975

[tianlang-wq]

1.请问一下通过NVMeoF实现传输，是client持久化的逻辑吗？
2.这块可以替换客户端持久化的逻辑吗，除了DFS好像没有什么办法统一所有客户端的持久层？

[nickyc975]

1.请问一下通过NVMeoF实现传输，是client持久化的逻辑吗？ 2.这块可以替换客户端持久化的逻辑吗，除了DFS好像没有什么办法统一所有客户端的持久层？

NVMeoF传输与当前的client持久化逻辑是相互独立的，暂时还不能替换现有的持久化逻辑。

[stmatengss]

To avoid add a new parameter. Could we combine FileBufferID and buffer_ptr as a union or an optional?

[nickyc975]

Actually the file_id parameter can be removed according to #883 . I am working on it.

[stmatengss]

Should be #ifdef USE_NVMEOF_GENERIC here?

[nickyc975]

Logically we can have other transports than support file buffers. Therefore, file buffer components are intended to be separated from NVMeoF components.

[stmatengss]

Could you resolve the conflicts? thx. @nickyc975

[nickyc975]

Could you resolve the conflicts? thx. @nickyc975

Done.

[stmatengss]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a generic NVMeoF transport implementation, which is a significant and well-structured feature addition. The changes are extensive, touching documentation, CMake build files, C++ source code for both the store and transfer engine, and Python bindings. The overall implementation looks solid. I've identified a few areas for improvement, primarily related to maintainability and documentation clarity. Specifically, I've pointed out a typo in the documentation, suggested refactoring a couple of functions that use goto to improve readability, and recommended adding more detailed comments to explain a workaround for CacheLib's memory allocation.

[gemini-code-assist]

There appears to be a typo in the IP address for the metadata server. It's written as 127.0.0.0.0, but it should likely be 127.0.0.1 to match the other examples and standard loopback usage.

Suggested change

	--metadata_server=http://127.0.0.0.0:8080/metadata \
	--metadata_server=http://127.0.0.1:8080/metadata \

[gemini-code-assist]

The comment "Add a padding to base to support zero-based buffers" is a bit brief. To improve maintainability, could you add a more detailed explanation for why this padding is necessary for CacheLib? For example, explaining if CacheLib has issues with zero addresses or if this is to avoid some other specific behavior would be very helpful for future developers who might need to work with this code.

[gemini-code-assist]

The getFileSize function uses goto statements for flow control. While functional, this can make the code harder to read and maintain. Consider refactoring to avoid goto, for example by using early returns. This will improve code clarity and align with modern C++ best practices.

static size_t getFileSize(const std::string& file) {
    size_t size = 0;
    int fd = open(file.c_str(), O_RDONLY);
    if (fd < 0) {
        LOG(ERROR) << "Failed to open file " << file << ", errno=" << errno;
        return 0;
    }

    struct stat st;
    if (fstat(fd, &st) < 0) {
        LOG(ERROR) << "Failed fstat on file " << file << ", errno=" << errno;
        close(fd);
        return 0;
    }

    if (S_ISLNK(st.st_mode)) {
        LOG(ERROR) << "File " << file << " is a symbolic link";
        close(fd);
        return 0;
    }

    if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
        if (ioctl(fd, BLKGETSIZE64, &size) < 0) {
            LOG(ERROR) << "Failed ioctl on file " << file
                       << ", errno=" << errno;
            size = 0;
        }
    } else if (S_ISREG(st.st_mode)) {
        size = st.st_size;
    }

    close(fd);
    return size;
}

[gemini-code-assist]

This getFileSize function uses goto for error handling. To improve code readability and maintainability, consider refactoring it to use early returns instead. This makes the control flow more straightforward and is generally considered a better practice in modern C++.

static size_t getFileSize(const std::string &file) {
    size_t size = 0;
    struct stat st;

    int fd = open(file.c_str(), O_RDONLY);
    if (fd < 0) {
        return 0;
    }

    if (fstat(fd, &st) != 0) {
        close(fd);
        return 0;
    }

    if (S_ISLNK(st.st_mode)) {
        close(fd);
        return 0;
    }

    if (S_ISBLK(st.st_mode) || S_ISCHR(st.st_mode)) {
        ioctl(fd, BLKGETSIZE64, &size);
    } else if (S_ISREG(st.st_mode)) {
        size = st.st_size;
    }

    close(fd);
    return size;
}