MB-61458: Remove sendfile implementation for GetFileFragment

It does not give us a performance boost *AND*:

1. It isn't supported on all platforms
2. We can't use it on top of TLS

We shouldn't keep the extra complexity around due to the above

Change-Id: I8027ca52c7cb380ce0e33f7cf3c044a4cbdf44e7
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/236622
Reviewed-by: Jim Walker <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: trondn

daemon/connection.h

@@ -426,23 +426,6 @@ class Connection : public ConnectionIface, public DcpMessageProducersIface {
     virtual void chainDataToOutputStream(
             std::unique_ptr<SendBuffer> buffer) = 0;
 
-    /// Do the underlying implementation used by this backed support using
-    /// sendfile(2) to transfer data between two file descriptors without
-    /// copying the data via userspace. Given that it is only our bufferevent
-    /// backend supporting this (and for !TLS) we save ourself some typing
-    /// by not making it a pure virtual
-    virtual bool isSendfileSupported() const {
-        return false;
-    }
-
-    /// Send length bytes starting from the provided offset of the file
-    /// descriptor fd to the other end. Given that it is only our bufferevent
-    /// backend supporting this (and for !TLS) we save ourself some typing
-    /// by not making it a pure virtual
-    virtual cb::engine_errc sendFile(int fd, off_t offset, off_t length) {
-        return cb::engine_errc::not_supported;
-    }
-
     /**
      * Enable the datatype which corresponds to the feature
      *

daemon/connection_libevent.cc

@@ -410,16 +410,6 @@ void LibeventConnection::chainDataToOutputStream(
     updateSendBytes(data.size());
 }
 
-cb::engine_errc LibeventConnection::sendFile(int fd,
-                                             off_t offset,
-                                             off_t length) {
-    if (evbuffer_add_file(
-                bufferevent_get_output(bev.get()), fd, offset, length) == 0) {
-        return cb::engine_errc::success;
-    }
-    throw std::bad_alloc();
-}
-
 constexpr const char* invalidPacketHeaderMessage =
         "Invalid packet header detected";
 

daemon/connection_libevent.h

@@ -26,12 +26,6 @@ class LibeventConnection : public Connection {
     void copyToOutputStream(std::string_view data) override;
     void copyToOutputStream(gsl::span<std::string_view> data) override;
     void chainDataToOutputStream(std::unique_ptr<SendBuffer> buffer) override;
-#ifndef WIN32
-    bool isSendfileSupported() const override {
-        return !isTlsEnabled();
-    }
-#endif
-    cb::engine_errc sendFile(int fd, off_t offset, off_t length) override;
     bool isPacketAvailable() const override;
     const cb::mcbp::Header& getPacket() const override;
     void nextPacket() override;

daemon/protocol/mcbp/get_file_fragment_context.cc

@@ -29,7 +29,6 @@
 GetFileFragmentContext::GetFileFragmentContext(Cookie& cookie)
     : SteppableCommandContext(cookie),
       uuid(cookie.getRequest().getKeyString()),
-      useSendfile(cookie.getConnection().isSendfileSupported()),
       max_read_size(Settings::instance().getFileFragmentMaxReadSize()),
       state(State::Initialize),
       chunk_size(Settings::instance().getFileFragmentMaxChunkSize()) {
@@ -41,9 +40,7 @@ GetFileFragmentContext::GetFileFragmentContext(Cookie& cookie)
     offset = stoll(json.value("offset", "0"));
     length = stoll(json["length"].get<std::string>());
     length = std::min(length, max_read_size);
-    if (!useSendfile) {
-        filestream.exceptions(std::ifstream::failbit | std::ifstream::badbit);
-    }
+    filestream.exceptions(std::ifstream::failbit | std::ifstream::badbit);
 }
 
 GetFileFragmentContext::~GetFileFragmentContext() {
@@ -80,9 +77,6 @@ cb::engine_errc GetFileFragmentContext::step() {
         case State::ChainFileChunk:
             ret = chain_file_chunk();
             break;
-        case State::TransferWithSendFile:
-            ret = transfer_with_sendfile();
-            break;
         case State::Done:
             cookie.clearEwouldblock();
             return cb::engine_errc::success;
@@ -123,32 +117,8 @@ cb::engine_errc GetFileFragmentContext::initialize() {
                         return;
                     }
 
-                    if (useSendfile) {
-                        fd = ::open(filename.c_str(), O_RDONLY);
-                        if (fd == -1) {
-                            LOG_WARNING_CTX(
-                                    "Failed to open file",
-                                    {"conn_id", cookie.getConnectionId()},
-                                    {"error", cb_strerror()},
-                                    {"file", filename});
-                            if (errno == ENOENT) {
-                                cookie.notifyIoComplete(
-                                        cb::engine_errc::no_such_key);
-                                return;
-                            }
-                            if (errno == EACCES) {
-                                cookie.notifyIoComplete(
-                                        cb::engine_errc::no_access);
-                                return;
-                            }
-                            cookie.notifyIoComplete(cb::engine_errc::failed);
-                            return;
-                        }
-                    } else {
-                        filestream.open(filename,
-                                        std::ios::binary | std::ios::in);
-                        filestream.seekg(offset);
-                    }
+                    filestream.open(filename, std::ios::binary | std::ios::in);
+                    filestream.seekg(offset);
 
                     length = std::min(length, max_read_size);
                     state = State::SendResponseHeader;
@@ -172,11 +142,7 @@ cb::engine_errc GetFileFragmentContext::send_response_header() {
                                    {},
                                    length,
                                    PROTOCOL_BINARY_RAW_BYTES);
-    if (connection.isSendfileSupported()) {
-        state = State::TransferWithSendFile;
-    } else {
-        state = State::ReadFileChunk;
-    }
+    state = State::ReadFileChunk;
     return cb::engine_errc::success;
 }
 
@@ -216,16 +182,6 @@ cb::engine_errc GetFileFragmentContext::read_file_chunk() {
     return cb::engine_errc::success;
 }
 
-cb::engine_errc GetFileFragmentContext::transfer_with_sendfile() {
-    const auto ret = connection.sendFile(fd, offset, length);
-    if (ret == cb::engine_errc::success) {
-        // evbuffer took the ownership..
-        fd = -1;
-    }
-    state = State::Done;
-    return ret;
-}
-
 cb::engine_errc GetFileFragmentContext::chain_file_chunk() {
     std::unique_ptr<folly::IOBuf> iob;
     chunk.swap(iob);

daemon/protocol/mcbp/get_file_fragment_context.h

@@ -43,7 +43,6 @@ class GetFileFragmentContext : public SteppableCommandContext {
         SendResponseHeader,
         ReadFileChunk,
         ChainFileChunk,
-        TransferWithSendFile,
         Done
     };
 
@@ -58,9 +57,7 @@ class GetFileFragmentContext : public SteppableCommandContext {
     cb::engine_errc send_response_header();
     cb::engine_errc read_file_chunk();
     cb::engine_errc chain_file_chunk();
-    cb::engine_errc transfer_with_sendfile();
     const std::string uuid;
-    const bool useSendfile;
     std::string filename;
     std::size_t id{0};
     std::size_t offset{0};