Merge pull request ceph#45214 from cbodley/wip-rgw-ancient-curl-workaround

cmake/rgw: remove workaround for ancient libcurl

Reviewed-by: Yehuda Sadeh <[email protected]>

Author: cbodley

CMakeLists.txt

@@ -363,10 +363,8 @@ else()
   set(EXE_LINKER_USE_PIE ${ENABLE_SHARED})
 endif()
 
-find_package(CURL REQUIRED)
-set(CMAKE_REQUIRED_INCLUDES ${CURL_INCLUDE_DIRS})
-set(CMAKE_REQUIRED_LIBRARIES ${CURL_LIBRARIES})
-CHECK_SYMBOL_EXISTS(curl_multi_wait curl/curl.h HAVE_CURL_MULTI_WAIT)
+# require libcurl with good curl_multi_wait(), see https://tracker.ceph.com/issues/15915
+find_package(CURL 7.32 REQUIRED)
 
 find_package(OpenSSL REQUIRED)
 set(CRYPTO_LIBS OpenSSL::Crypto)

src/include/config-h.in.cmake

@@ -121,9 +121,6 @@
 #cmakedefine HAVE_LIBTCMALLOC
 #cmakedefine LIBTCMALLOC_MISSING_ALIGNED_ALLOC
 
-/* Define if have curl_multi_wait() */
-#cmakedefine HAVE_CURL_MULTI_WAIT 1
-
 /* AsyncMessenger RDMA conditional compilation */
 #cmakedefine HAVE_RDMA
 

src/rgw/rgw_http_client.cc

@@ -721,8 +721,7 @@ int RGWHTTPTransceiver::send_data(void* ptr, size_t len, bool* pause)
 static int clear_signal(int fd)
 {
   // since we're in non-blocking mode, we can try to read a lot more than
-  // one signal from signal_thread() to avoid later wakeups. non-blocking reads
-  // are also required to support the curl_multi_wait bug workaround
+  // one signal from signal_thread() to avoid later wakeups
   std::array<char, 256> buf{};
   int ret = ::read(fd, (void *)buf.data(), buf.size());
   if (ret < 0) {
@@ -732,68 +731,6 @@ static int clear_signal(int fd)
   return 0;
 }
 
-#if HAVE_CURL_MULTI_WAIT
-
-static std::once_flag detect_flag;
-static bool curl_multi_wait_bug_present = false;
-
-static int detect_curl_multi_wait_bug(CephContext *cct, CURLM *handle,
-                                      int write_fd, int read_fd)
-{
-  int ret = 0;
-
-  // write to write_fd so that read_fd becomes readable
-  uint32_t buf = 0;
-  ret = ::write(write_fd, &buf, sizeof(buf));
-  if (ret < 0) {
-    ret = -errno;
-    ldout(cct, 0) << "ERROR: " << __func__ << "(): write() returned " << ret << dendl;
-    return ret;
-  }
-
-  // pass read_fd in extra_fds for curl_multi_wait()
-  int num_fds;
-  struct curl_waitfd wait_fd;
-
-  wait_fd.fd = read_fd;
-  wait_fd.events = CURL_WAIT_POLLIN;
-  wait_fd.revents = 0;
-
-  ret = curl_multi_wait(handle, &wait_fd, 1, 0, &num_fds);
-  if (ret != CURLM_OK) {
-    ldout(cct, 0) << "ERROR: curl_multi_wait() returned " << ret << dendl;
-    return -EIO;
-  }
-
-  // curl_multi_wait should flag revents when extra_fd is readable. if it
-  // doesn't, the bug is present and we can't rely on revents
-  if (wait_fd.revents == 0) {
-    curl_multi_wait_bug_present = true;
-    ldout(cct, 0) << "WARNING: detected a version of libcurl which contains a "
-        "bug in curl_multi_wait(). enabling a workaround that may degrade "
-        "performance slightly." << dendl;
-  }
-
-  return clear_signal(read_fd);
-}
-
-static bool is_signaled(const curl_waitfd& wait_fd)
-{
-  if (wait_fd.fd < 0) {
-    // no fd to signal
-    return false;
-  }
-
-  if (curl_multi_wait_bug_present) {
-    // we can't rely on revents, so we always return true if a wait_fd is given.
-    // this means we'll be trying a non-blocking read on this fd every time that
-    // curl_multi_wait() wakes up
-    return true;
-  }
-
-  return wait_fd.revents > 0;
-}
-
 static int do_curl_wait(CephContext *cct, CURLM *handle, int signal_fd)
 {
   int num_fds;
@@ -809,72 +746,16 @@ static int do_curl_wait(CephContext *cct, CURLM *handle, int signal_fd)
     return -EIO;
   }
 
-  if (is_signaled(wait_fd)) {
-    ret = clear_signal(signal_fd);
-    if (ret < 0) {
-      ldout(cct, 0) << "ERROR: " << __func__ << "(): read() returned " << ret << dendl;
-      return ret;
-    }
-  }
-  return 0;
-}
-
-#else
-
-static int do_curl_wait(CephContext *cct, CURLM *handle, int signal_fd)
-{
-  fd_set fdread;
-  fd_set fdwrite;
-  fd_set fdexcep;
-  int maxfd = -1;
- 
-  FD_ZERO(&fdread);
-  FD_ZERO(&fdwrite);
-  FD_ZERO(&fdexcep);
-
-  /* get file descriptors from the transfers */ 
-  int ret = curl_multi_fdset(handle, &fdread, &fdwrite, &fdexcep, &maxfd);
-  if (ret) {
-    ldout(cct, 0) << "ERROR: curl_multi_fdset returned " << ret << dendl;
-    return -EIO;
-  }
-
-  if (signal_fd > 0) {
-    FD_SET(signal_fd, &fdread);
-    if (signal_fd >= maxfd) {
-      maxfd = signal_fd + 1;
-    }
-  }
-
-  /* forcing a strict timeout, as the returned fdsets might not reference all fds we wait on */
-  uint64_t to = cct->_conf->rgw_curl_wait_timeout_ms;
-#define RGW_CURL_TIMEOUT 1000
-  if (!to)
-    to = RGW_CURL_TIMEOUT;
-  struct timeval timeout;
-  timeout.tv_sec = to / 1000;
-  timeout.tv_usec = to % 1000;
-
-  ret = select(maxfd+1, &fdread, &fdwrite, &fdexcep, &timeout);
-  if (ret < 0) {
-    ret = -errno;
-    ldout(cct, 0) << "ERROR: select returned " << ret << dendl;
-    return ret;
-  }
-
-  if (signal_fd > 0 && FD_ISSET(signal_fd, &fdread)) {
+  if (wait_fd.revents > 0) {
     ret = clear_signal(signal_fd);
     if (ret < 0) {
       ldout(cct, 0) << "ERROR: " << __func__ << "(): read() returned " << ret << dendl;
       return ret;
     }
   }
-
   return 0;
 }
 
-#endif
-
 void *RGWHTTPManager::ReqsThread::entry()
 {
   manager->reqs_thread_entry();
@@ -1174,14 +1055,6 @@ int RGWHTTPManager::start()
     return -e;
   }
 
-#ifdef HAVE_CURL_MULTI_WAIT
-  // on first initialization, use this pipe to detect whether we're using a
-  // buggy version of libcurl
-  std::call_once(detect_flag, detect_curl_multi_wait_bug, cct,
-                 static_cast<CURLM*>(multi_handle),
-                 thread_pipe[1], thread_pipe[0]);
-#endif
-
   is_started = true;
   reqs_thread = new ReqsThread(this);
   reqs_thread->create("http_manager");

src/rgw/rgw_http_client_curl.cc

@@ -66,14 +66,6 @@ void init_ssl(){
 namespace rgw {
 namespace curl {
 
-static void check_curl()
-{
-#ifndef HAVE_CURL_MULTI_WAIT
-  derr << "WARNING: libcurl doesn't support curl_multi_wait()" << dendl;
-  derr << "WARNING: cross zone / region transfer performance may be affected" << dendl;
-#endif
-}
-
 #if defined(WITH_CURL_OPENSSL) && OPENSSL_API_COMPAT < 0x10100000L
 void init_ssl() {
   ::openssl::init_ssl();
@@ -100,8 +92,6 @@ bool fe_inits_ssl(boost::optional <const fe_map_t&> m, long& curl_global_flags){
 std::once_flag curl_init_flag;
 
 void setup_curl(boost::optional<const fe_map_t&> m) {
-  check_curl();
-
   long curl_global_flags = CURL_GLOBAL_ALL;
 
   #if defined(WITH_CURL_OPENSSL) && OPENSSL_API_COMPAT < 0x10100000L