From b9e666be4c0949d34eed4fbc7b062700e47a6700 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Sun, 31 Dec 2023 17:25:11 -0800 Subject: [PATCH 1/5] Improve crash consistency --- .../java/hudson/util/FileChannelWriter.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/core/src/main/java/hudson/util/FileChannelWriter.java b/core/src/main/java/hudson/util/FileChannelWriter.java index 77e6d807c206..80132b86732c 100644 --- a/core/src/main/java/hudson/util/FileChannelWriter.java +++ b/core/src/main/java/hudson/util/FileChannelWriter.java @@ -1,5 +1,6 @@ package hudson.util; +import hudson.Functions; import java.io.BufferedWriter; import java.io.IOException; import java.io.Writer; @@ -11,6 +12,7 @@ import java.nio.file.OpenOption; import java.nio.file.Path; import java.util.logging.Logger; +import jenkins.util.SystemProperties; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -29,11 +31,16 @@ @Restricted(NoExternalUse.class) public class FileChannelWriter extends Writer implements Channel { + private static final boolean REQUIRES_DIR_FSYNC = + SystemProperties.getBoolean(FileChannelWriter.class.getName() + ".requiresDirFsync", true); + private static final Logger LOGGER = Logger.getLogger(FileChannelWriter.class.getName()); private final Charset charset; private final FileChannel channel; + private final Path parent; + /** * {@link FileChannel#force(boolean)} is a very costly operation. This flag has been introduced mostly to * accommodate Jenkins' previous behaviour, when using a simple {@link java.io.BufferedWriter}. @@ -64,6 +71,7 @@ public class FileChannelWriter extends Writer implements Channel { this.forceOnFlush = forceOnFlush; this.forceOnClose = forceOnClose; channel = FileChannel.open(filePath, options); + parent = filePath.getParent(); } @Override @@ -78,6 +86,11 @@ public void flush() throws IOException { if (forceOnFlush) { LOGGER.finest("Flush is forced"); channel.force(true); + if (REQUIRES_DIR_FSYNC && !Functions.isWindows()) { + try (FileChannel parentChannel = FileChannel.open(parent)) { + parentChannel.force(true); + } + } } else { LOGGER.finest("Force disabled on flush(), no-op"); } @@ -93,6 +106,11 @@ public void close() throws IOException { if (channel.isOpen()) { if (forceOnClose) { channel.force(true); + if (REQUIRES_DIR_FSYNC && !Functions.isWindows()) { + try (FileChannel parentChannel = FileChannel.open(parent)) { + parentChannel.force(true); + } + } } channel.close(); } From 5591c01c3df7f8de4aaa3a59f4667dbcfd7635cb Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Mon, 1 Jan 2024 13:50:35 -0800 Subject: [PATCH 2/5] `fsync` the correct parent directory --- .../java/hudson/util/AtomicFileWriter.java | 11 +++++++++++ .../java/hudson/util/FileChannelWriter.java | 18 ------------------ 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java index 4e871b3262f0..4f5077baaf6f 100644 --- a/core/src/main/java/hudson/util/AtomicFileWriter.java +++ b/core/src/main/java/hudson/util/AtomicFileWriter.java @@ -26,11 +26,13 @@ import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; +import hudson.Functions; import java.io.File; import java.io.FileWriter; import java.io.IOException; import java.io.Writer; import java.lang.ref.Cleaner; +import java.nio.channels.FileChannel; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.nio.file.AtomicMoveNotSupportedException; @@ -62,6 +64,9 @@ public class AtomicFileWriter extends Writer { private static /* final */ boolean DISABLE_FORCED_FLUSH = SystemProperties.getBoolean( AtomicFileWriter.class.getName() + ".DISABLE_FORCED_FLUSH"); + private static /* final */ boolean REQUIRES_DIR_FSYNC = SystemProperties.getBoolean( + AtomicFileWriter.class.getName() + ".REQUIRES_DIR_FSYNC", true); + static { if (DISABLE_FORCED_FLUSH) { LOGGER.log(Level.WARNING, "DISABLE_FORCED_FLUSH flag used, this could result in dataloss if failures happen in your storage subsystem."); @@ -234,6 +239,12 @@ public void commit() throws IOException { throw replaceFailed; } } + + if (!DISABLE_FORCED_FLUSH && REQUIRES_DIR_FSYNC && !Functions.isWindows()) { + try (FileChannel parentChannel = FileChannel.open(destPath.getParent())) { + parentChannel.force(true); + } + } } private static final class CleanupChecker implements Runnable { diff --git a/core/src/main/java/hudson/util/FileChannelWriter.java b/core/src/main/java/hudson/util/FileChannelWriter.java index 80132b86732c..77e6d807c206 100644 --- a/core/src/main/java/hudson/util/FileChannelWriter.java +++ b/core/src/main/java/hudson/util/FileChannelWriter.java @@ -1,6 +1,5 @@ package hudson.util; -import hudson.Functions; import java.io.BufferedWriter; import java.io.IOException; import java.io.Writer; @@ -12,7 +11,6 @@ import java.nio.file.OpenOption; import java.nio.file.Path; import java.util.logging.Logger; -import jenkins.util.SystemProperties; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -31,16 +29,11 @@ @Restricted(NoExternalUse.class) public class FileChannelWriter extends Writer implements Channel { - private static final boolean REQUIRES_DIR_FSYNC = - SystemProperties.getBoolean(FileChannelWriter.class.getName() + ".requiresDirFsync", true); - private static final Logger LOGGER = Logger.getLogger(FileChannelWriter.class.getName()); private final Charset charset; private final FileChannel channel; - private final Path parent; - /** * {@link FileChannel#force(boolean)} is a very costly operation. This flag has been introduced mostly to * accommodate Jenkins' previous behaviour, when using a simple {@link java.io.BufferedWriter}. @@ -71,7 +64,6 @@ public class FileChannelWriter extends Writer implements Channel { this.forceOnFlush = forceOnFlush; this.forceOnClose = forceOnClose; channel = FileChannel.open(filePath, options); - parent = filePath.getParent(); } @Override @@ -86,11 +78,6 @@ public void flush() throws IOException { if (forceOnFlush) { LOGGER.finest("Flush is forced"); channel.force(true); - if (REQUIRES_DIR_FSYNC && !Functions.isWindows()) { - try (FileChannel parentChannel = FileChannel.open(parent)) { - parentChannel.force(true); - } - } } else { LOGGER.finest("Force disabled on flush(), no-op"); } @@ -106,11 +93,6 @@ public void close() throws IOException { if (channel.isOpen()) { if (forceOnClose) { channel.force(true); - if (REQUIRES_DIR_FSYNC && !Functions.isWindows()) { - try (FileChannel parentChannel = FileChannel.open(parent)) { - parentChannel.force(true); - } - } } channel.close(); } From f1289fff36b4a49de34c40794182107e5ca8f55f Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Mon, 1 Jan 2024 13:55:56 -0800 Subject: [PATCH 3/5] More flexibility --- core/src/main/java/hudson/util/AtomicFileWriter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java index 4f5077baaf6f..d5f79e0a380c 100644 --- a/core/src/main/java/hudson/util/AtomicFileWriter.java +++ b/core/src/main/java/hudson/util/AtomicFileWriter.java @@ -65,7 +65,7 @@ public class AtomicFileWriter extends Writer { AtomicFileWriter.class.getName() + ".DISABLE_FORCED_FLUSH"); private static /* final */ boolean REQUIRES_DIR_FSYNC = SystemProperties.getBoolean( - AtomicFileWriter.class.getName() + ".REQUIRES_DIR_FSYNC", true); + AtomicFileWriter.class.getName() + ".REQUIRES_DIR_FSYNC", !Functions.isWindows()); static { if (DISABLE_FORCED_FLUSH) { @@ -240,7 +240,7 @@ public void commit() throws IOException { } } - if (!DISABLE_FORCED_FLUSH && REQUIRES_DIR_FSYNC && !Functions.isWindows()) { + if (!DISABLE_FORCED_FLUSH && REQUIRES_DIR_FSYNC) { try (FileChannel parentChannel = FileChannel.open(destPath.getParent())) { parentChannel.force(true); } From 72f2fe0241eabc90e5e1a65f8211d47764780f53 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Mon, 1 Jan 2024 13:58:02 -0800 Subject: [PATCH 4/5] Add reference to man page --- core/src/main/java/hudson/util/AtomicFileWriter.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java index d5f79e0a380c..d0832c72676c 100644 --- a/core/src/main/java/hudson/util/AtomicFileWriter.java +++ b/core/src/main/java/hudson/util/AtomicFileWriter.java @@ -240,6 +240,13 @@ public void commit() throws IOException { } } + + /* + * From fsync(2) on Linux: + * + * Calling fsync() does not necessarily ensure that the entry in the directory containing the file has also + * reached disk. For that an explicit fsync() on a file descriptor for the directory is also needed. + */ if (!DISABLE_FORCED_FLUSH && REQUIRES_DIR_FSYNC) { try (FileChannel parentChannel = FileChannel.open(destPath.getParent())) { parentChannel.force(true); From 4ffb75e60a1da4be4c31b61df7403246aadeec10 Mon Sep 17 00:00:00 2001 From: Basil Crow Date: Tue, 2 Jan 2024 10:47:17 -0800 Subject: [PATCH 5/5] fix formatting --- core/src/main/java/hudson/util/AtomicFileWriter.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/hudson/util/AtomicFileWriter.java b/core/src/main/java/hudson/util/AtomicFileWriter.java index d0832c72676c..fabe15c5a6b6 100644 --- a/core/src/main/java/hudson/util/AtomicFileWriter.java +++ b/core/src/main/java/hudson/util/AtomicFileWriter.java @@ -240,7 +240,6 @@ public void commit() throws IOException { } } - /* * From fsync(2) on Linux: *