From 2973a43ecc0d044425385a91edea8bb0e245f0b6 Mon Sep 17 00:00:00 2001 From: Mykyta Bozhenko <21245729+cheeeee@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:50:21 -0400 Subject: [PATCH] Limit unsupported fsync tolerance to directory descriptors Resolve supported capability errno values for the host platform and keep unknown values and ordinary descriptor failures strict. Route directory sync callers explicitly and preserve primary sync failures when descriptor cleanup also fails. CASSANDRA-14380 Generated-by: Claude (Anthropic) --- CHANGES.txt | 1 + .../cassandra/db/lifecycle/LogReplica.java | 2 +- .../apache/cassandra/hints/HintsCatalog.java | 7 +- .../service/accord/journal/ReplayMarkers.java | 8 +- .../apache/cassandra/utils/NativeLibrary.java | 126 ++++++++++++- .../org/apache/cassandra/utils/SyncUtil.java | 10 +- .../cassandra/utils/NativeLibraryTest.java | 173 ++++++++++++++++++ 7 files changed, 303 insertions(+), 24 deletions(-) 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() {