3 files changed, 51 insertions, 27 deletions

diff --git a/jni/FuseDaemon.cpp b/jni/FuseDaemon.cpp
index f2983290..67611bb8 100644
--- a/jni/FuseDaemon.cpp
+++ b/jni/FuseDaemon.cpp
@@ -296,7 +296,7 @@ struct fuse {
     FAdviser fadviser;
 
     std::atomic_bool* active;
-    bool passthrough;
+    std::atomic_bool passthrough;
 };
 
 static inline string get_name(node* n) {
@@ -971,24 +971,33 @@ static void pf_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t new_parent,
 static handle* create_handle_for_node(struct fuse* fuse, const string& path, int fd, node* node,
                                       const RedactionInfo* ri) {
     std::lock_guard<std::recursive_mutex> guard(fuse->lock);
-    // We don't want to use the FUSE VFS cache in two cases:
-    // 1. When redaction is needed because app A with EXIF access might access
-    // a region that should have been redacted for app B without EXIF access, but app B on
-    // a subsequent read, will be able to see the EXIF data because the read request for
-    // that region will be served from cache and not get to the FUSE daemon
-    // 2. When the file has a read or write lock on it. This means that the MediaProvider
-    // has given an fd to the lower file system to an app. There are two cases where using
-    // the cache in this case can be a problem:
-    // a. Writing to a FUSE fd with caching enabled will use the write-back cache and a
-    // subsequent read from the lower fs fd will not see the write.
-    // b. Reading from a FUSE fd with caching enabled may not see the latest writes using
-    // the lower fs fd because those writes did not go through the FUSE layer and reads from
-    // FUSE after that write may be served from cache
-    bool direct_io = ri->isRedactionNeeded() || is_file_locked(fd, path);
-
-    handle* h = new handle(fd, ri, !direct_io);
-    node->AddHandle(h);
-    return h;
+    handle* handle = nullptr;
+
+    if (fuse->passthrough) {
+        bool redaction_needed = ri->isRedactionNeeded();
+
+        handle = new struct handle(fd, ri, true /* cached */, !redaction_needed /* passthrough */);
+    } else {
+        // Without fuse->passthrough, we don't want to use the FUSE VFS cache in two cases:
+        // 1. When redaction is needed because app A with EXIF access might access
+        // a region that should have been redacted for app B without EXIF access, but app B on
+        // a subsequent read, will be able to see the EXIF data because the read request for
+        // that region will be served from cache and not get to the FUSE daemon
+        // 2. When the file has a read or write lock on it. This means that the MediaProvider
+        // has given an fd to the lower file system to an app. There are two cases where using
+        // the cache in this case can be a problem:
+        // a. Writing to a FUSE fd with caching enabled will use the write-back cache and a
+        // subsequent read from the lower fs fd will not see the write.
+        // b. Reading from a FUSE fd with caching enabled may not see the latest writes using
+        // the lower fs fd because those writes did not go through the FUSE layer and reads from
+        // FUSE after that write may be served from cache
+        bool direct_io = ri->isRedactionNeeded() || is_file_locked(fd, path);
+
+        handle = new struct handle(fd, ri, !direct_io /* cached */, false /* passthrough */);
+    }
+
+    node->AddHandle(handle);
+    return handle;
 }
 
 bool do_passthrough_enable(fuse_req_t req, struct fuse_file_info* fi, unsigned int fd) {
@@ -1070,9 +1079,12 @@ static void pf_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info* fi) {
     // TODO(b/173190192) ensuring that h->cached must be enabled in order to
     // user FUSE passthrough is a conservative rule and might be dropped as
     // soon as demonstrated its correctness.
-    if (fuse->passthrough && h->cached) {
+    if (h->passthrough) {
         if (!do_passthrough_enable(req, fi, fd)) {
-            LOG(WARNING) << "Passthrough OPEN failed for " << path;
+            // TODO: Should we crash here so we can find errors easily?
+            PLOG(ERROR) << "Passthrough OPEN failed for " << path;
+            fuse_reply_err(req, EFAULT);
+            return;
         }
     }
 
@@ -1642,9 +1654,11 @@ static void pf_create(fuse_req_t req,
     // TODO(b/173190192) ensuring that h->cached must be enabled in order to
     // user FUSE passthrough is a conservative rule and might be dropped as
     // soon as demonstrated its correctness.
-    if (fuse->passthrough && h->cached) {
+    if (h->passthrough) {
         if (!do_passthrough_enable(req, fi, fd)) {
-            LOG(WARNING) << "Passthrough CREATE failed for " << child_path;
+            PLOG(ERROR) << "Passthrough CREATE failed for " << child_path;
+            fuse_reply_err(req, EFAULT);
+            return;
         }
     }
 
@@ -1754,6 +1768,13 @@ static void fuse_logger(enum fuse_log_level level, const char* fmt, va_list ap)
 }
 
 bool FuseDaemon::ShouldOpenWithFuse(int fd, bool for_read, const std::string& path) {
+    if (fuse->passthrough) {
+        // Always open with FUSE if passthrough is enabled. This avoids the delicate file lock
+        // acquisition below to ensure VFS cache consistency and doesn't impact filesystem
+        // performance since read(2)/write(2) happen in the kernel
+        return true;
+    }
+
     bool use_fuse = false;
 
     if (active.load(std::memory_order_acquire)) {
diff --git a/jni/node-inl.h b/jni/node-inl.h
index 364a3279..c031c971 100644
--- a/jni/node-inl.h
+++ b/jni/node-inl.h
@@ -40,13 +40,15 @@ namespace mediaprovider {
 namespace fuse {
 
 struct handle {
-    explicit handle(int fd, const RedactionInfo* ri, bool cached) : fd(fd), ri(ri), cached(cached) {
+    explicit handle(int fd, const RedactionInfo* ri, bool cached, bool passthrough)
+        : fd(fd), ri(ri), cached(cached), passthrough(passthrough) {
         CHECK(ri != nullptr);
     }
 
     const int fd;
     const std::unique_ptr<const RedactionInfo> ri;
     const bool cached;
+    const bool passthrough;
 
     ~handle() { close(fd); }
 };
diff --git a/jni/node_test.cpp b/jni/node_test.cpp
index 357cea8e..0d7417f9 100644
--- a/jni/node_test.cpp
+++ b/jni/node_test.cpp
@@ -217,7 +217,8 @@ TEST_F(NodeTest, LookupAbsolutePath) {
 TEST_F(NodeTest, AddDestroyHandle) {
     unique_node_ptr node = CreateNode(nullptr, "/path");
 
-    handle* h = new handle(-1, new mediaprovider::fuse::RedactionInfo, true /* cached */);
+    handle* h = new handle(-1, new mediaprovider::fuse::RedactionInfo, true /* cached */,
+                           false /* passthrough */);
     node->AddHandle(h);
     ASSERT_TRUE(node->HasCachedHandle());
 
@@ -228,8 +229,8 @@ TEST_F(NodeTest, AddDestroyHandle) {
     // the node in question.
     EXPECT_DEATH(node->DestroyHandle(h), "");
     EXPECT_DEATH(node->DestroyHandle(nullptr), "");
-    std::unique_ptr<handle> h2(
-            new handle(-1, new mediaprovider::fuse::RedactionInfo, true /* cached */));
+    std::unique_ptr<handle> h2(new handle(-1, new mediaprovider::fuse::RedactionInfo,
+                                          true /* cached */, false /* passthrough */));
     EXPECT_DEATH(node->DestroyHandle(h2.get()), "");
 }