From e85b3d63c5025a07d0e67971294681d76daf81b7 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Mon, 25 Sep 2023 12:44:43 +0200 Subject: [PATCH 1/7] only block unmount, if at least one open file exists, which was written to. --- .../org/cryptomator/frontend/fuse/OpenFile.java | 15 ++++++++++++++- .../frontend/fuse/OpenFileFactory.java | 10 ++++++++-- .../frontend/fuse/ReadOnlyAdapter.java | 10 ++++------ 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java index 5fc4dd9..a25ee55 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java @@ -19,11 +19,14 @@ public class OpenFile implements Closeable { private final Path path; private final FileChannel channel; + private volatile boolean writeCalled; + private OpenFile(Path path, FileChannel channel) { this.path = path; this.channel = channel; + this.writeCalled = false; } - + static OpenFile create(Path path, Set options, FileAttribute... attrs) throws IOException { FileChannel ch = FileChannel.open(path, options, attrs); return new OpenFile(path, ch); @@ -69,6 +72,7 @@ public int read(ByteBuffer buf, long num, long offset) throws IOException { * @throws IOException If an exception occurs during write. */ public int write(ByteBuffer buf, long num, long offset) throws IOException { + writeCalled = true; if (num > Integer.MAX_VALUE) { throw new IOException("Requested too many bytes"); } @@ -80,6 +84,15 @@ public int write(ByteBuffer buf, long num, long offset) throws IOException { return written; } + /** + * Tests, if the write method of this open file was called at least once. + * + * @return {@code true} if {@link OpenFile#write(ByteBuffer, long, long)} was called on this object, otherwise {@code false} + */ + public boolean isWrittenTo() { + return writeCalled; + } + @Override public void close() throws IOException { channel.close(); diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java index f1471cb..f7b6cf1 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java @@ -67,8 +67,14 @@ public void close(long fileHandle) throws ClosedChannelException, IOException { } } - public int getOpenFileCount(){ - return openFiles.size(); + /** + * Tests, if there exists {@link OpenFile}s, which has been written to. + * The check performed in this method is not atomic and therefore requires external synchronization. + * + * @return {@code true} if and only if at least one {@link OpenFile} exists, where {@link OpenFile#isWrittenTo()} returns {@code true}. Otherwise {@code false}. + */ + public boolean hasWrittenToOpenFiles(){ + return openFiles.entrySet().stream().anyMatch(entry -> entry.getValue().isWrittenTo()); } /** diff --git a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java index a4c8064..ff9fcbd 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java +++ b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java @@ -48,7 +48,6 @@ public sealed class ReadOnlyAdapter implements FuseNioAdapter permits ReadWriteA private final ReadOnlyDirectoryHandler dirHandler; private final ReadOnlyFileHandler fileHandler; private final ReadOnlyLinkHandler linkHandler; - private final BooleanSupplier hasOpenFiles; protected ReadOnlyAdapter(Errno errno, Path root, int maxFileNameLength, FileNameTranscoder fileNameTranscoder, FileStore fileStore, OpenFileFactory openFiles, ReadOnlyDirectoryHandler dirHandler, ReadOnlyFileHandler fileHandler) { this.errno = errno; @@ -61,7 +60,6 @@ protected ReadOnlyAdapter(Errno errno, Path root, int maxFileNameLength, FileNam this.dirHandler = dirHandler; this.fileHandler = fileHandler; this.linkHandler = new ReadOnlyLinkHandler(fileNameTranscoder); - this.hasOpenFiles = () -> openFiles.getOpenFileCount() != 0; } public static ReadOnlyAdapter create(Errno errno, Path root, int maxFileNameLength, FileNameTranscoder fileNameTranscoder) { @@ -305,7 +303,7 @@ public void destroy() { @Override public boolean isInUse() { try (PathLock pLock = lockManager.tryLockForWriting("/")) { - return hasOpenFiles.getAsBoolean(); + return openFiles.hasWrittenToOpenFiles(); } catch (AlreadyLockedException e) { return true; } @@ -320,7 +318,7 @@ public void close() throws IOException { * Attempts to get a specific error code that best describes the given exception. * As a side effect this logs the error. * - * @param e An exception + * @param e An exception * @param opDesc A human-friendly string describing what operation was attempted (for logging purposes) * @return A specific error code or -EIO. */ @@ -331,8 +329,8 @@ protected int getErrorCodeForGenericFileSystemException(FileSystemException e, S // LOG.warn("{} {} failed, name too long.", opDesc); // return -ErrorCodes.ENAMETOOLONG(); // } else { - LOG.error(opDesc + " failed.", e); - return -errno.eio(); + LOG.error(opDesc + " failed.", e); + return -errno.eio(); // } } } From a4988916828f43dd1de2c9594aee4c7636d6c227 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Mon, 25 Sep 2023 15:29:04 +0200 Subject: [PATCH 2/7] renamed variable and changed visibility --- .../java/org/cryptomator/frontend/fuse/OpenFile.java | 10 +++++----- .../cryptomator/frontend/fuse/OpenFileFactory.java | 11 ++++++----- .../cryptomator/frontend/fuse/ReadOnlyAdapter.java | 3 +-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java index a25ee55..44274e2 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java @@ -19,12 +19,12 @@ public class OpenFile implements Closeable { private final Path path; private final FileChannel channel; - private volatile boolean writeCalled; + private volatile boolean dirty; private OpenFile(Path path, FileChannel channel) { this.path = path; this.channel = channel; - this.writeCalled = false; + this.dirty = false; } static OpenFile create(Path path, Set options, FileAttribute... attrs) throws IOException { @@ -72,7 +72,7 @@ public int read(ByteBuffer buf, long num, long offset) throws IOException { * @throws IOException If an exception occurs during write. */ public int write(ByteBuffer buf, long num, long offset) throws IOException { - writeCalled = true; + dirty = true; if (num > Integer.MAX_VALUE) { throw new IOException("Requested too many bytes"); } @@ -89,8 +89,8 @@ public int write(ByteBuffer buf, long num, long offset) throws IOException { * * @return {@code true} if {@link OpenFile#write(ByteBuffer, long, long)} was called on this object, otherwise {@code false} */ - public boolean isWrittenTo() { - return writeCalled; + boolean isDirty() { + return dirty; } @Override diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java index f7b6cf1..99ffd07 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java @@ -68,13 +68,14 @@ public void close(long fileHandle) throws ClosedChannelException, IOException { } /** - * Tests, if there exists {@link OpenFile}s, which has been written to. - * The check performed in this method is not atomic and therefore requires external synchronization. + * Tests, if there exists dirty {@link OpenFile}s + * This method is neither atomic regarding the set of open files nor regarding each open file individually. Therefore, external synchronization is required. * - * @return {@code true} if and only if at least one {@link OpenFile} exists, where {@link OpenFile#isWrittenTo()} returns {@code true}. Otherwise {@code false}. + * @return {@code true} if and only if at least one dirty {@link OpenFile} exists. Otherwise {@code false}. + * @see OpenFile#isDirty() */ - public boolean hasWrittenToOpenFiles(){ - return openFiles.entrySet().stream().anyMatch(entry -> entry.getValue().isWrittenTo()); + boolean hastDirtyOpenFiles() { + return openFiles.entrySet().stream().anyMatch(entry -> entry.getValue().isDirty()); } /** diff --git a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java index ff9fcbd..0cd6627 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java +++ b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java @@ -32,7 +32,6 @@ import java.util.Collections; import java.util.EnumSet; import java.util.Set; -import java.util.function.BooleanSupplier; public sealed class ReadOnlyAdapter implements FuseNioAdapter permits ReadWriteAdapter { @@ -303,7 +302,7 @@ public void destroy() { @Override public boolean isInUse() { try (PathLock pLock = lockManager.tryLockForWriting("/")) { - return openFiles.hasWrittenToOpenFiles(); + return openFiles.hastDirtyOpenFiles(); } catch (AlreadyLockedException e) { return true; } From 6b58ab207151112fa86d6e937a30fede24bda607 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Mon, 25 Sep 2023 15:59:39 +0200 Subject: [PATCH 3/7] reset dirty flag on fsync --- src/main/java/org/cryptomator/frontend/fuse/OpenFile.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java index 44274e2..67f7f47 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java @@ -100,6 +100,7 @@ public void close() throws IOException { public void fsync(boolean metaData) throws IOException { channel.force(metaData); + dirty = false; } public void truncate(long size) throws IOException { From 55cf9822cbc40c9e2e58d1454737673357603e44 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Mon, 25 Sep 2023 15:59:52 +0200 Subject: [PATCH 4/7] add comment --- src/main/java/org/cryptomator/frontend/fuse/OpenFile.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java index 67f7f47..ec49fd2 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java @@ -19,6 +19,7 @@ public class OpenFile implements Closeable { private final Path path; private final FileChannel channel; + //"volatile" is fine, since methods of OpenFile ar externally synchronized private volatile boolean dirty; private OpenFile(Path path, FileChannel channel) { From 7680e5656cdc9e69b478465186e0d9abb1eafb86 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Mon, 25 Sep 2023 18:19:46 +0200 Subject: [PATCH 5/7] Update src/main/java/org/cryptomator/frontend/fuse/OpenFile.java Co-authored-by: Sebastian Stenzel --- src/main/java/org/cryptomator/frontend/fuse/OpenFile.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java index ec49fd2..85fb742 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java @@ -19,7 +19,11 @@ public class OpenFile implements Closeable { private final Path path; private final FileChannel channel; - //"volatile" is fine, since methods of OpenFile ar externally synchronized + /** + * Whether any data has been changed on this file. + * + * This value only changes while holding a write lock, see {@link org.cryptomator.frontend.fuse.locks.LockManager}. + */ private volatile boolean dirty; private OpenFile(Path path, FileChannel channel) { From 842f5076d475eef72082fca51bc6862286fa1819 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 29 Sep 2023 13:00:43 +0200 Subject: [PATCH 6/7] Imrpoved docs/naming --- src/main/java/org/cryptomator/frontend/fuse/OpenFile.java | 3 ++- .../java/org/cryptomator/frontend/fuse/OpenFileFactory.java | 6 +++--- .../java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java index 85fb742..3bba864 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFile.java @@ -90,7 +90,8 @@ public int write(ByteBuffer buf, long num, long offset) throws IOException { } /** - * Tests, if the write method of this open file was called at least once. + * Tests, if this OpenFile is dirty. + * An OpenFile is dirty, if its write method is called at least once. * * @return {@code true} if {@link OpenFile#write(ByteBuffer, long, long)} was called on this object, otherwise {@code false} */ diff --git a/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java b/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java index 99ffd07..04ab047 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java +++ b/src/main/java/org/cryptomator/frontend/fuse/OpenFileFactory.java @@ -68,13 +68,13 @@ public void close(long fileHandle) throws ClosedChannelException, IOException { } /** - * Tests, if there exists dirty {@link OpenFile}s - * This method is neither atomic regarding the set of open files nor regarding each open file individually. Therefore, external synchronization is required. + * Tests, if any {@link OpenFile} is considered dirty, e.g. might have pending changes. + * This method is neither atomic regarding the set of OpenFiles nor regarding each OpenFile individually. Therefore, external synchronization is required. * * @return {@code true} if and only if at least one dirty {@link OpenFile} exists. Otherwise {@code false}. * @see OpenFile#isDirty() */ - boolean hastDirtyOpenFiles() { + boolean hasDirtyFiles() { return openFiles.entrySet().stream().anyMatch(entry -> entry.getValue().isDirty()); } diff --git a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java index 0cd6627..fc9f5fa 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java +++ b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java @@ -302,7 +302,7 @@ public void destroy() { @Override public boolean isInUse() { try (PathLock pLock = lockManager.tryLockForWriting("/")) { - return openFiles.hastDirtyOpenFiles(); + return openFiles.hasDirtyFiles(); } catch (AlreadyLockedException e) { return true; } From a4e330bb0b8154847c9ee048f01b2788ad052a25 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Mon, 9 Oct 2023 12:34:37 +0200 Subject: [PATCH 7/7] revert unrelated changes --- .../java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java index fc9f5fa..ee99caa 100644 --- a/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java +++ b/src/main/java/org/cryptomator/frontend/fuse/ReadOnlyAdapter.java @@ -317,7 +317,7 @@ public void close() throws IOException { * Attempts to get a specific error code that best describes the given exception. * As a side effect this logs the error. * - * @param e An exception + * @param e An exception * @param opDesc A human-friendly string describing what operation was attempted (for logging purposes) * @return A specific error code or -EIO. */ @@ -328,8 +328,8 @@ protected int getErrorCodeForGenericFileSystemException(FileSystemException e, S // LOG.warn("{} {} failed, name too long.", opDesc); // return -ErrorCodes.ENAMETOOLONG(); // } else { - LOG.error(opDesc + " failed.", e); - return -errno.eio(); + LOG.error(opDesc + " failed.", e); + return -errno.eio(); // } } }