diff --git a/CHANGES.txt b/CHANGES.txt index aa356a2da163..772e4d6a03da 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 7.0 + * Tolerate unsupported fsync only for kernel-verified directory descriptors, using host-resolved errno values; on AIX, whose jnr Errno table has no ENOTSUP constant, only EINVAL/EOPNOTSUPP are tolerated (CASSANDRA-14380) * Allow CQLSSTableWriter to specify SSTable id generator to use (CASSANDRA-21012) * Reject LIKE patterns with a wildcard (%) anywhere other than the start or end (CASSANDRA-21068) * Support pluggable default role initialization (CASSANDRA-21546) diff --git a/src/java/org/apache/cassandra/db/lifecycle/LogReplica.java b/src/java/org/apache/cassandra/db/lifecycle/LogReplica.java index 9ee713916d30..d085b9f40b22 100644 --- a/src/java/org/apache/cassandra/db/lifecycle/LogReplica.java +++ b/src/java/org/apache/cassandra/db/lifecycle/LogReplica.java @@ -141,7 +141,7 @@ void syncDirectory() try { if (directoryDescriptor >= 0) - NativeLibrary.trySync(directoryDescriptor); + NativeLibrary.trySyncDirectory(directoryDescriptor, getDirectory()); } catch (FSError e) { diff --git a/src/java/org/apache/cassandra/hints/HintsCatalog.java b/src/java/org/apache/cassandra/hints/HintsCatalog.java index 000054ea610f..cd676c3624f6 100644 --- a/src/java/org/apache/cassandra/hints/HintsCatalog.java +++ b/src/java/org/apache/cassandra/hints/HintsCatalog.java @@ -44,6 +44,7 @@ import org.apache.cassandra.io.util.FileUtils; import org.apache.cassandra.utils.NativeLibrary; import org.apache.cassandra.utils.SyncUtil; +import org.apache.cassandra.utils.Throwables; import static java.util.stream.Collectors.groupingBy; @@ -170,8 +171,10 @@ void fsyncDirectory() { try { - SyncUtil.trySync(fd); - NativeLibrary.tryCloseFD(fd); + Throwables.maybeFail(() -> { + if (!SyncUtil.SKIP_SYNC) + NativeLibrary.trySyncDirectory(fd, hintsDirectory.absolutePath()); + }, () -> NativeLibrary.tryCloseFD(fd)); } catch (FSError e) // trySync failed { diff --git a/src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java b/src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java index 14845c31ae7f..c7549e8557ac 100644 --- a/src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java +++ b/src/java/org/apache/cassandra/service/accord/journal/ReplayMarkers.java @@ -84,13 +84,7 @@ public static long readMarker(File file) private static void trySyncJournalDirectory() { - trySyncDirectory(getAccordJournalDirectory()); - } - - private static void trySyncDirectory(String path) - { - int fd = NativeLibrary.tryOpenDirectory(path); - NativeLibrary.trySync(fd); + NativeLibrary.trySyncDirectory(getAccordJournalDirectory()); } public static File saveDirectory() diff --git a/src/java/org/apache/cassandra/utils/NativeLibrary.java b/src/java/org/apache/cassandra/utils/NativeLibrary.java index ec00162e195f..fe829a5032ab 100644 --- a/src/java/org/apache/cassandra/utils/NativeLibrary.java +++ b/src/java/org/apache/cassandra/utils/NativeLibrary.java @@ -23,6 +23,7 @@ import java.nio.channels.FileChannel; import java.util.concurrent.TimeUnit; +import com.google.common.annotations.VisibleForTesting; import com.sun.jna.LastErrorException; import org.slf4j.Logger; @@ -31,8 +32,13 @@ import org.apache.cassandra.io.FSWriteError; import org.apache.cassandra.io.util.File; import org.apache.cassandra.io.util.FileInputStreamPlus; +import org.apache.cassandra.io.util.FileUtils; + +import jnr.constants.Constant; +import jnr.constants.ConstantSet; import static org.apache.cassandra.config.CassandraRelevantProperties.IGNORE_MISSING_NATIVE_FILE_HINTS; +import static org.apache.cassandra.config.CassandraRelevantProperties.JAVA_IO_TMPDIR; import static org.apache.cassandra.config.CassandraRelevantProperties.OS_ARCH; import static org.apache.cassandra.config.CassandraRelevantProperties.OS_NAME; import static org.apache.cassandra.utils.LocalizeString.toLowerCaseLocalized; @@ -65,6 +71,8 @@ public enum OSType private static final int F_NOCACHE = 48; /* Mac OS X specific flag, turns cache on/off */ private static final int O_DIRECT = 040000; /* fcntl.h */ private static final int O_RDONLY = 00000000; /* fcntl.h */ + @VisibleForTesting + static final int O_DIRECTORY; /* fcntl.h; jnr-resolved value is confirmed against the kernel at class init, see verifyODirectory() */ private static final int POSIX_FADV_NORMAL = 0; /* fadvise.h */ private static final int POSIX_FADV_RANDOM = 1; /* fadvise.h */ @@ -103,6 +111,10 @@ public enum OSType default: wrappedLibrary = new NativeLibraryLinux(); } + ConstantSet openFlags = ConstantSet.getConstantSet("OpenFlags"); + Constant oDirectory = openFlags == null ? null : openFlags.getConstant("O_DIRECTORY"); + O_DIRECTORY = verifyODirectory(isHostConstant(oDirectory) ? oDirectory.intValue() : 0); + if (toLowerCaseLocalized(OS_ARCH.getString()).contains("ppc")) { if (osType == LINUX) @@ -128,6 +140,39 @@ else if (osType == AIX) } } + /** + * jnr-constants only maps a handful of architectures (aarch64, s390x, mips64el, loongarch64) to their + * per-arch OpenFlags tables; on others (e.g. ppc64le, arm32) a resolved value can silently be the wrong + * flag for this host. Do not trust the table: confirm it opens java.io.tmpdir and rejects a regular file + * with ENOTDIR before relying on it to gate directory-fsync tolerance. + */ + private static int verifyODirectory(int candidate) + { + if (candidate == 0) + return 0; + File probe = FileUtils.createDeletableTempFile("odirectory-probe", "tmp"); + boolean verified; + try + { + wrappedLibrary.callClose(wrappedLibrary.callOpen(JAVA_IO_TMPDIR.getString(), O_RDONLY | candidate)); + wrappedLibrary.callClose(wrappedLibrary.callOpen(probe.path(), O_RDONLY | candidate)); + verified = false; // must reject a regular file with ENOTDIR; it did not + } + catch (RuntimeException | UnsatisfiedLinkError e) + { + ConstantSet errnos = ConstantSet.getConstantSet("Errno"); + Constant enotdir = errnos == null ? null : errnos.getConstant("ENOTDIR"); + verified = e instanceof LastErrorException && matchesErrno(enotdir, errno((LastErrorException) e)); + } + finally + { + probe.tryDelete(); + } + if (!verified) + logger.info("O_DIRECTORY capability probe failed; disabling directory fsync tolerance"); + return verified ? candidate : 0; + } + private NativeLibrary() {} /** @@ -299,12 +344,17 @@ public static int tryFcntl(int fd, int command, int flags) } public static int tryOpenDirectory(String path) + { + return tryOpenDirectory(path, wrappedLibrary); + } + + private static int tryOpenDirectory(String path, NativeLibraryWrapper library) { int fd = -1; try { - return wrappedLibrary.callOpen(path, O_RDONLY); + return library.callOpen(path, O_RDONLY | O_DIRECTORY); } catch (UnsatisfiedLinkError e) { @@ -316,20 +366,45 @@ public static int tryOpenDirectory(String path) throw e; if (REQUIRE) - logger.warn("open({}, O_RDONLY) failed, errno ({}).", path, errno(e)); + logger.warn("openDirectory({}) failed, errno ({}).", path, errno(e)); } return fd; } public static void trySync(int fd) + { + trySync(fd, null, wrappedLibrary); + } + + /** + * Sync a descriptor opened for a directory, tolerating filesystems without directory fsync support. + * The caller retains ownership of the descriptor. + */ + public static void trySyncDirectory(int fd, String path) + { + trySync(fd, path, wrappedLibrary); + } + + public static void trySyncDirectory(String path) + { + trySyncDirectory(path, wrappedLibrary); + } + + static void trySyncDirectory(String path, NativeLibraryWrapper library) + { + int fd = tryOpenDirectory(path, library); + Throwables.maybeFail(() -> trySync(fd, path, library), () -> tryCloseFD(fd, library)); + } + + static void trySync(int fd, String directory, NativeLibraryWrapper library) { if (fd == -1) return; try { - wrappedLibrary.callFsync(fd); + library.callFsync(fd); } catch (UnsatisfiedLinkError e) { @@ -340,23 +415,64 @@ public static void trySync(int fd) if (!(e instanceof LastErrorException)) throw e; + int err = errno(e); + // Capability errors are safe to ignore only for a kernel-verified directory descriptor. + if (directory != null && isUnsupportedDirectorySync(err)) + { + // Key the throttle on the directory so one unsupported mount cannot silence the others. + NoSpamLogger.log(logger, NoSpamLogger.Level.WARN, directory, 10, TimeUnit.MINUTES, + "Directory fsync on {} not supported by underlying filesystem, ignoring: errno ({})", directory, err); + return; + } + if (REQUIRE) { - String errMsg = String.format("fsync(%s) failed, errno (%s) %s", fd, errno(e), e.getMessage()); + String errMsg = String.format("fsync(%s) failed, errno (%s) %s", fd, err, e.getMessage()); logger.warn(errMsg); throw new FSWriteError(e, errMsg); } } } + private static boolean isUnsupportedDirectorySync(int error) + { + // Without a kernel-verified O_DIRECTORY the descriptor cannot be confirmed to be a directory, + // so capability errors are not tolerated. + if (O_DIRECTORY == 0) + return false; + + ConstantSet errors = ConstantSet.getConstantSet("Errno"); + // Missing host constants remain failures rather than using another platform's values. + return errors != null && (matchesErrno(errors.getConstant("EINVAL"), error) + || matchesErrno(errors.getConstant("ENOTSUP"), error) + || matchesErrno(errors.getConstant("EOPNOTSUPP"), error)); + } + + private static boolean matchesErrno(Constant expected, int actual) + { + return expected != null && expected.defined() && expected.intValue() == actual; + } + + private static boolean isHostConstant(Constant constant) + { + // jnr's unknown-platform fallback reports defined() == true for synthetic, non-native OpenFlags + // values; used only to sanity-check the resolved O_DIRECTORY candidate before it is probed above. + return constant != null && constant.defined() && !(constant instanceof jnr.constants.platform.fake.OpenFlags); + } + public static void tryCloseFD(int fd) + { + tryCloseFD(fd, wrappedLibrary); + } + + private static void tryCloseFD(int fd, NativeLibraryWrapper library) { if (fd == -1) return; try { - wrappedLibrary.callClose(fd); + library.callClose(fd); } catch (UnsatisfiedLinkError e) { diff --git a/src/java/org/apache/cassandra/utils/SyncUtil.java b/src/java/org/apache/cassandra/utils/SyncUtil.java index 121275d57969..67a6f15df601 100644 --- a/src/java/org/apache/cassandra/utils/SyncUtil.java +++ b/src/java/org/apache/cassandra/utils/SyncUtil.java @@ -119,14 +119,6 @@ public static void trySyncDir(File dir) if (SKIP_SYNC) return; - int directoryFD = NativeLibrary.tryOpenDirectory(dir.path()); - try - { - trySync(directoryFD); - } - finally - { - NativeLibrary.tryCloseFD(directoryFD); - } + NativeLibrary.trySyncDirectory(dir.path()); } } diff --git a/test/unit/org/apache/cassandra/utils/NativeLibraryTest.java b/test/unit/org/apache/cassandra/utils/NativeLibraryTest.java index 1856af1a645b..2b2fc5cf06d9 100644 --- a/test/unit/org/apache/cassandra/utils/NativeLibraryTest.java +++ b/test/unit/org/apache/cassandra/utils/NativeLibraryTest.java @@ -18,15 +18,40 @@ */ package org.apache.cassandra.utils; +import java.io.IOException; +import java.nio.file.Files; + +import com.sun.jna.LastErrorException; import org.junit.Assert; import org.junit.Test; +import org.slf4j.LoggerFactory; +import org.apache.cassandra.io.FSWriteError; import org.apache.cassandra.io.util.File; import org.apache.cassandra.io.util.FileUtils; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import jnr.constants.Constant; +import jnr.constants.ConstantSet; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.catchThrowable; +import static org.junit.Assume.assumeTrue; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + public class NativeLibraryTest { + private static final int FD = 42; + private static final String DIRECTORY = "directory"; + @Test public void testSkipCache() { @@ -35,6 +60,154 @@ public void testSkipCache() NativeLibrary.trySkipCache(file.path(), 0, 0); } + @Test + public void testCapabilityErrorsAreToleratedForDirectoriesOnly() + { + for (String name : new String[]{ "EINVAL", "ENOTSUP", "EOPNOTSUPP" }) + assertDirectoryOnlyCapability(name); + } + + @Test + public void testSyncPreservesStorageAndDescriptorErrors() + { + assumeTrue(NativeLibrary.isEnabled()); + for (String name : new String[]{ "EIO", "EBADF" }) + assertSyncFailure(new LastErrorException(hostErrno(name))); + } + + @Test + public void testNativeDirectorySync() throws IOException + { + File directory = new File(Files.createTempDirectory("native-directory-sync")); + try + { + int fd = NativeLibrary.tryOpenDirectory(directory.path()); + assumeTrue(fd != -1); + try + { + NativeLibrary.trySyncDirectory(fd, directory.path()); + } + finally + { + NativeLibrary.tryCloseFD(fd); + } + Assert.assertTrue(new File(directory, "after-sync").createFileIfNotExists()); + } + finally + { + directory.deleteRecursive(); + } + } + + @Test + public void testOpenDirectoryRejectsNonDirectory() + { + assumeTrue(NativeLibrary.isEnabled() && NativeLibrary.O_DIRECTORY != 0); + File file = FileUtils.createDeletableTempFile("testOpenDirectory", "1"); + Assert.assertEquals(-1, NativeLibrary.tryOpenDirectory(file.path())); + } + + @Test + public void testUnsupportedDirectorySyncNamesEveryDirectory() + { + assumeTrue(NativeLibrary.isEnabled() && NativeLibrary.O_DIRECTORY != 0); + LastErrorException failure = new LastErrorException(hostErrno("EINVAL")); + Logger logger = (Logger) LoggerFactory.getLogger(NativeLibrary.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try + { + // Distinct directories must each be reported: the throttle is keyed per directory. + for (String directory : new String[]{ "/unsupported-mount-a", "/unsupported-mount-b" }) + { + NativeLibraryWrapper library = mock(NativeLibraryWrapper.class); + when(library.callOpen(eq(directory), anyInt())).thenReturn(FD); + when(library.callFsync(FD)).thenThrow(failure); + NativeLibrary.trySyncDirectory(directory, library); + } + assertThat(appender.list).anyMatch(event -> event.getFormattedMessage().contains("/unsupported-mount-a")) + .anyMatch(event -> event.getFormattedMessage().contains("/unsupported-mount-b")); + } + finally + { + logger.detachAppender(appender); + } + } + + @Test + public void testDirectorySyncClosesDescriptorOnUnexpectedFailure() + { + NativeLibraryWrapper library = directoryLibrary(); + IllegalStateException failure = new IllegalStateException("fsync failed"); + when(library.callFsync(FD)).thenThrow(failure); + + assertThatThrownBy(() -> NativeLibrary.trySyncDirectory(DIRECTORY, library)).isSameAs(failure); + verify(library).callClose(FD); + } + + @Test + public void testDirectorySyncPreservesFailureWhenCloseFails() + { + assumeTrue(NativeLibrary.isEnabled()); + LastErrorException syncFailure = new LastErrorException(hostErrno("EIO")); + LastErrorException closeFailure = new LastErrorException(hostErrno("EBADF")); + NativeLibraryWrapper library = directoryLibrary(); + when(library.callFsync(FD)).thenThrow(syncFailure); + when(library.callClose(FD)).thenThrow(closeFailure); + + Throwable failure = catchThrowable(() -> NativeLibrary.trySyncDirectory(DIRECTORY, library)); + assertThat(failure).isInstanceOf(FSWriteError.class).hasCause(syncFailure); + Assert.assertEquals(1, failure.getSuppressed().length); + assertThat(failure.getSuppressed()[0]).isInstanceOf(FSWriteError.class).hasCause(closeFailure); + } + + private static void assertDirectoryOnlyCapability(String name) + { + assumeTrue(NativeLibrary.isEnabled() && NativeLibrary.O_DIRECTORY != 0); + LastErrorException failure = new LastErrorException(hostErrno(name)); + NativeLibraryWrapper library = directoryLibrary(); + when(library.callFsync(FD)).thenThrow(failure); + + NativeLibrary.trySyncDirectory(DIRECTORY, library); + verify(library).callFsync(FD); + verify(library).callClose(FD); + + assertThatThrownBy(() -> NativeLibrary.trySync(FD, null, library)) + .isInstanceOf(FSWriteError.class) + .hasCause(failure); + } + + private static void assertSyncFailure(LastErrorException failure) + { + NativeLibraryWrapper library = directoryLibrary(); + when(library.callFsync(FD)).thenThrow(failure); + + assertThatThrownBy(() -> NativeLibrary.trySync(FD, null, library)) + .isInstanceOf(FSWriteError.class) + .hasCause(failure); + assertThatThrownBy(() -> NativeLibrary.trySyncDirectory(DIRECTORY, library)) + .isInstanceOf(FSWriteError.class) + .hasCause(failure); + verify(library).callClose(FD); + } + + private static NativeLibraryWrapper directoryLibrary() + { + NativeLibraryWrapper library = mock(NativeLibraryWrapper.class); + when(library.callOpen(eq(DIRECTORY), anyInt())).thenReturn(FD); + return library; + } + + private static int hostErrno(String name) + { + ConstantSet constants = ConstantSet.getConstantSet("Errno"); + assumeTrue(constants != null); + Constant error = constants.getConstant(name); + assumeTrue(error != null && error.defined() && !(error instanceof jnr.constants.platform.fake.Errno)); + return error.intValue(); + } + @Test public void getPid() {