MDEV-33379 innodb_log_file_buffering=OFF causes corruption on bcachefs

Apparently, invoking fcntl(fd, F_SETFL, O_DIRECT) will lead to
unexpected behaviour on Linux bcachefs and possibly other file systems,
depending on the operating system version. So, let us avoid doing that,
and instead just attempt to pass the O_DIRECT flag to open(). This should
make us compatible with NetBSD, IBM AIX, as well as Solaris and its
derivatives.

This fix does not change the fact that we had only implemented
innodb_log_file_buffering=OFF on systems where we can determine the
physical block size (typically 512 or 4096 bytes).
Currently, those operating systems are Linux and Microsoft Windows.

HAVE_FCNTL_DIRECT, os_file_set_nocache(): Remove.

OS_FILE_OVERWRITE, OS_FILE_CREATE_PATH: Remove (never used parameters).

os_file_log_buffered(), os_file_log_maybe_unbuffered(): Helper functions.

os_file_create_simple_func(): When applicable, initially attempt to
open files in O_DIRECT mode.

os_file_create_func(): When applicable, initially attempt to
open files in O_DIRECT mode.
For type==OS_LOG_FILE && create_mode != OS_FILE_CREATE
we will first invoke stat(2) on the file name to find out if the size
is compatible with O_DIRECT. If create_mode == OS_FILE_CREATE, we will
invoke fstat(2) on the created log file afterwards, and may close and
reopen the file in O_DIRECT mode if applicable.

create_temp_file(): Support O_DIRECT. This is only used if O_TMPFILE is
available and innodb_disable_sort_file_cache=ON (non-default value).
Notably, that setting never worked on Microsoft Windows.

row_merge_file_create_mode(): Split from row_merge_file_create_low().
Create a temporary file in the specified mode.

Reviewed by: Vladislav Vaintroub

cmake/os/AIX.cmake

@@ -34,8 +34,5 @@ ELSE()
   SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGE_FILES -maix64 -pthread -mcmodel=large")
 ENDIF()
 
-# fcntl(fd, F_SETFL, O_DIRECT) is not supported; O_DIRECT is an open(2) flag
-SET(HAVE_FCNTL_DIRECT 0 CACHE INTERNAL "")
-
 # make it WARN by default, not AUTO (that implies -Werror)
 SET(MYSQL_MAINTAINER_MODE "WARN" CACHE STRING "Enable MariaDB maintainer-specific warnings. One of: NO (warnings are disabled) WARN (warnings are enabled) ERR (warnings are errors) AUTO (warnings are errors in Debug only)")

cmake/os/SunOS.cmake

@@ -17,10 +17,6 @@ INCLUDE(CheckSymbolExists)
 INCLUDE(CheckCSourceRuns)
 INCLUDE(CheckCSourceCompiles) 
 
-# fcntl(fd, F_SETFL, O_DIRECT) is not supported,
-# and directio(3C) would only work on UFS or NFS, not ZFS.
-SET(HAVE_FCNTL_DIRECT 0 CACHE INTERNAL "")
-
 # Enable 64 bit file offsets
 SET(_FILE_OFFSET_BITS 64)
 

cmake/os/WindowsCache.cmake

@@ -44,7 +44,6 @@ SET(HAVE_EXECINFO_H CACHE  INTERNAL "")
 SET(HAVE_FCHMOD CACHE  INTERNAL "")
 SET(HAVE_FCNTL CACHE  INTERNAL "")
 SET(HAVE_FCNTL_H 1 CACHE  INTERNAL "")
-SET(HAVE_FCNTL_DIRECT 0 CACHE  INTERNAL "")
 SET(HAVE_FCNTL_NONBLOCK CACHE  INTERNAL "")
 SET(HAVE_FDATASYNC CACHE  INTERNAL "")
 SET(HAVE_DECL_FDATASYNC CACHE INTERNAL "")

config.h.cmake

@@ -30,7 +30,6 @@
 #cmakedefine HAVE_DLFCN_H 1
 #cmakedefine HAVE_EXECINFO_H 1
 #cmakedefine HAVE_FCNTL_H 1
-#cmakedefine HAVE_FCNTL_DIRECT 1
 #cmakedefine HAVE_FENV_H 1
 #cmakedefine HAVE_FLOAT_H 1
 #cmakedefine HAVE_FNMATCH_H 1

configure.cmake

@@ -706,7 +706,6 @@ CHECK_SYMBOL_EXISTS(O_NONBLOCK "unistd.h;fcntl.h" HAVE_FCNTL_NONBLOCK)
 IF(NOT HAVE_FCNTL_NONBLOCK)
  SET(NO_FCNTL_NONBLOCK 1)
 ENDIF()
-CHECK_SYMBOL_EXISTS(O_DIRECT "fcntl.h" HAVE_FCNTL_DIRECT)
 
 #
 # Test for how the C compiler does inline, if at all

extra/mariabackup/common.h

@@ -23,7 +23,6 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1335  USA
 
 #include <my_global.h>
 #include <mysql_version.h>
-#include <fcntl.h>
 #include <stdarg.h>
 #include <my_sys.h>
 

extra/mariabackup/fil_cur.cc

@@ -199,14 +199,6 @@ xb_fil_cur_open(
 		return(XB_FIL_CUR_SKIP);
 	}
 
-#ifdef HAVE_FCNTL_DIRECT
-	if (srv_file_flush_method == SRV_O_DIRECT
-	    || srv_file_flush_method == SRV_O_DIRECT_NO_FSYNC) {
-
-		os_file_set_nocache(cursor->file, node->name, "OPEN");
-	}
-#endif
-
 	posix_fadvise(cursor->file, 0, 0, POSIX_FADV_SEQUENTIAL);
 
 	cursor->page_size = node->space->physical_size();

extra/mariabackup/xtrabackup.cc

@@ -54,7 +54,6 @@ Street, Fifth Floor, Boston, MA 02110-1335 USA
 #include <scope.h>
 #include <sql_class.h>
 
-#include <fcntl.h>
 #include <string.h>
 
 #ifdef __linux__
@@ -2428,7 +2427,7 @@ static bool innodb_init()
   os_file_delete_if_exists_func(ib_logfile0.c_str(), nullptr);
   os_file_t file= os_file_create_func(ib_logfile0.c_str(),
                                       OS_FILE_CREATE, OS_FILE_NORMAL,
-#if defined _WIN32 || defined HAVE_FCNTL_DIRECT
+#if defined _WIN32 || defined O_DIRECT
                                       OS_DATA_FILE_NO_O_DIRECT,
 #else
                                       OS_DATA_FILE,

mysys/mf_tempfile.c

@@ -66,7 +66,11 @@ File create_temp_file(char *to, const char *dir, const char *prefix,
 
   DBUG_ENTER("create_temp_file");
   DBUG_PRINT("enter", ("dir: %s, prefix: %s", dir ? dir : "(null)", prefix));
+#if !defined _WIN32 && defined O_DIRECT
+  DBUG_ASSERT((mode & (O_EXCL | O_TRUNC | O_CREAT | O_RDWR | O_DIRECT)) == 0);
+#else
   DBUG_ASSERT((mode & (O_EXCL | O_TRUNC | O_CREAT | O_RDWR)) == 0);
+#endif
 
   mode|= O_TRUNC | O_CREAT | O_RDWR; /* not O_EXCL, see Windows code below */
 
@@ -118,16 +122,41 @@ File create_temp_file(char *to, const char *dir, const char *prefix,
 
     if ((MyFlags & MY_TEMPORARY) && O_TMPFILE_works)
     {
-      /* explictly don't use O_EXCL here has it has a different
-         meaning with O_TMPFILE
+      /*
+        explicitly don't use O_EXCL here has it has a different
+        meaning with O_TMPFILE
       */
-      if ((file= open(dir, (mode & ~O_CREAT) | O_TMPFILE | O_CLOEXEC,
-                      S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP)) >= 0)
+      const int flags= (mode & ~O_CREAT) | O_TMPFILE | O_CLOEXEC;
+      const mode_t open_mode= S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP;
+# ifdef O_DIRECT
+      static int O_TMPFILE_works_with_O_DIRECT= O_DIRECT;
+      const int try_O_DIRECT= mode & O_TMPFILE_works_with_O_DIRECT;
+      if (try_O_DIRECT)
+        file= open(dir, flags | O_DIRECT, open_mode);
+      else
+# endif
+      file= open(dir, flags, open_mode);
+
+      if (file >= 0)
       {
+# ifdef O_DIRECT
+      O_TMPFILE_works:
+# endif
         my_snprintf(to, FN_REFLEN, "%s/#sql/fd=%d", dir, file);
         file=my_register_filename(file, to, FILE_BY_O_TMPFILE,
                                   EE_CANTCREATEFILE, MyFlags);
       }
+# ifdef O_DIRECT
+      else if (errno == EINVAL && try_O_DIRECT)
+      {
+        file= open(dir, flags, open_mode);
+        if (file >= 0)
+        {
+          O_TMPFILE_works_with_O_DIRECT= 0;
+          goto O_TMPFILE_works;
+        }
+      }
+# endif
       else if (errno == EOPNOTSUPP || errno == EINVAL)
       {
         my_printf_error(EE_CANTCREATEFILE, "O_TMPFILE is not supported on %s "

storage/innobase/fil/fil0fil.cc

@@ -342,7 +342,7 @@ static bool fil_node_open_file_low(fil_node_t *node)
   ut_ad(node->space->is_closing());
   mysql_mutex_assert_owner(&fil_system.mutex);
   static_assert(((UNIV_ZIP_SIZE_MIN >> 1) << 3) == 4096, "compatibility");
-#if defined _WIN32 || defined HAVE_FCNTL_DIRECT
+#if defined _WIN32 || defined O_DIRECT
   ulint type;
   switch (FSP_FLAGS_GET_ZIP_SSIZE(node->space->flags)) {
   case 1:
@@ -1906,7 +1906,7 @@ fil_ibd_create(
 
 	static_assert(((UNIV_ZIP_SIZE_MIN >> 1) << 3) == 4096,
 		      "compatibility");
-#if defined _WIN32 || defined HAVE_FCNTL_DIRECT
+#if defined _WIN32 || defined O_DIRECT
 	ulint type;
 	switch (FSP_FLAGS_GET_ZIP_SSIZE(flags)) {
 	case 1:

storage/innobase/handler/ha_innodb.cc

@@ -4007,7 +4007,7 @@ static int innodb_init_params()
 
 	data_mysql_default_charset_coll = (ulint) default_charset_info->number;
 
-#ifdef HAVE_FCNTL_DIRECT
+#if !defined _WIN32 && defined O_DIRECT
 	if (srv_use_atomic_writes && my_may_have_atomic_write) {
 		/*
                   Force O_DIRECT on Unixes (on Windows writes are always

storage/innobase/include/fil0fil.h

@@ -63,7 +63,7 @@ enum srv_flush_t
   SRV_LITTLESYNC,
   /** do not flush after writing */
   SRV_NOSYNC,
-  /** invoke os_file_set_nocache() on data files. This implies using
+  /** Open or create files with O_DIRECT. This implies using
   unbuffered I/O but still fdatasync(), because some filesystems might
   not flush meta-data on write completion */
   SRV_O_DIRECT,

storage/innobase/include/os0file.h

@@ -113,11 +113,8 @@ enum os_file_create_t {
 					doesn't exist, error) */
 	OS_FILE_CREATE,			/*!< to create new file (if
 					exists, error) */
-	OS_FILE_OVERWRITE,		/*!< to create a new file, if exists
-					the overwrite old file */
 	OS_FILE_OPEN_RAW,		/*!< to open a raw device or disk
 					partition */
-	OS_FILE_CREATE_PATH,		/*!< to create the directories */
 	OS_FILE_OPEN_RETRY,		/*!< open with retry */
 
 	/** Flags that can be combined with the above values. Please ensure
@@ -144,7 +141,7 @@ static const ulint OS_FILE_NORMAL = 62;
 /** Types for file create @{ */
 static constexpr ulint OS_DATA_FILE = 100;
 static constexpr ulint OS_LOG_FILE = 101;
-#if defined _WIN32 || defined HAVE_FCNTL_DIRECT
+#if defined _WIN32 || defined O_DIRECT
 static constexpr ulint OS_DATA_FILE_NO_O_DIRECT = 103;
 #endif
 /* @} */
@@ -375,22 +372,6 @@ os_file_create_simple_no_error_handling_func(
 	bool*		success)
 	MY_ATTRIBUTE((warn_unused_result));
 
-#ifndef HAVE_FCNTL_DIRECT
-#define os_file_set_nocache(fd, file_name, operation_name) do{}while(0)
-#else
-/** Tries to disable OS caching on an opened file descriptor.
-@param[in]	fd		file descriptor to alter
-@param[in]	file_name	file name, used in the diagnostic message
-@param[in]	name		"open" or "create"; used in the diagnostic
-				message */
-void
-os_file_set_nocache(
-/*================*/
-	int	fd,		/*!< in: file descriptor to alter */
-	const char*	file_name,
-	const char*	operation_name);
-#endif
-
 #ifndef _WIN32 /* On Microsoft Windows, mandatory locking is used */
 /** Obtain an exclusive lock on a file.
 @param fd      file descriptor

storage/innobase/include/row0merge.h

@@ -165,14 +165,11 @@ row_merge_drop_indexes(
 prepare_inplace_alter_table_dict(). */
 void row_merge_drop_temp_indexes();
 
-/** Create temporary merge files in the given paramater path, and if
-UNIV_PFS_IO defined, register the file descriptor with Performance Schema.
-@param[in]	path	location for creating temporary merge files, or NULL
+/** Create a temporary file at the specified path.
+@param path location for creating temporary merge files, or nullptr
 @return File descriptor */
-pfs_os_file_t
-row_merge_file_create_low(
-	const char*	path)
-	MY_ATTRIBUTE((warn_unused_result));
+pfs_os_file_t row_merge_file_create_low(const char *path)
+  MY_ATTRIBUTE((warn_unused_result));
 /*********************************************************************//**
 Destroy a merge file. And de-register the file from Performance Schema
 if UNIV_PFS_IO is defined. */

storage/innobase/row/row0merge.cc

@@ -4349,9 +4349,7 @@ void row_merge_drop_temp_indexes()
 UNIV_PFS_IO defined, register the file descriptor with Performance Schema.
 @param[in]	path	location for creating temporary merge files, or NULL
 @return File descriptor */
-pfs_os_file_t
-row_merge_file_create_low(
-	const char*	path)
+static pfs_os_file_t row_merge_file_create_mode(const char *path, int mode)
 {
 	if (!path) {
 		path = mysql_tmpdir;
@@ -4392,6 +4390,13 @@ row_merge_file_create_low(
 	return(fd);
 }
 
+/** Create a temporary file at the specified path.
+@param path location for creating temporary merge files, or nullptr
+@return File descriptor */
+pfs_os_file_t row_merge_file_create_low(const char *path)
+{
+  return row_merge_file_create_mode(path, O_BINARY | O_SEQUENTIAL);
+}
 
 /** Create a merge file in the given location.
 @param[out]	merge_file	merge file structure
@@ -4402,17 +4407,16 @@ row_merge_file_create(
 	merge_file_t*	merge_file,
 	const char*	path)
 {
-	merge_file->fd = row_merge_file_create_low(path);
 	merge_file->offset = 0;
 	merge_file->n_rec = 0;
-#ifdef HAVE_FCNTL_DIRECT
-	if (merge_file->fd != OS_FILE_CLOSED) {
-		if (srv_disable_sort_file_cache) {
-			os_file_set_nocache(merge_file->fd,
-				"row0merge.cc", "sort");
-		}
-	}
+	merge_file->fd =
+		row_merge_file_create_mode(path,
+#if !defined _WIN32 && defined O_DIRECT
+					   srv_disable_sort_file_cache
+					   ? O_DIRECT | O_BINARY | O_SEQUENTIAL
+					   :
 #endif
+					   O_BINARY | O_SEQUENTIAL);
 	return(merge_file->fd);
 }