Mark removable Drive caches for cryptohome.

components/drive/chromeos/file_cache.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/drive/chromeos/file_cache.h"
 
+#include <fcntl.h>
+#include <linux/fs.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/xattr.h>
 #include <unistd.h>
 
 #include <queue>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_info.h"
 #include "build/build_config.h"
 #include "components/drive/drive.pb.h"
 #include "components/drive/drive_api_util.h"
 #include "components/drive/file_system_core_util.h"
 #include "components/drive/resource_metadata_storage.h"
 #include "google_apis/drive/task_util.h"
 #include "net/base/filename_util.h"
 #include "net/base/mime_sniffer.h"
 #include "net/base/mime_util.h"
 
 namespace drive {
 namespace internal {
 namespace {
 
 // Returns ID extracted from the path.
 std::string GetIdFromPath(const base::FilePath& path) {
   return util::UnescapeCacheFileName(path.BaseName().AsUTF8Unsafe());
 }
 
 base::FilePath GetPathForId(const base::FilePath& cache_directory,
                             const std::string& id) {
   return cache_directory.Append(
       base::FilePath::FromUTF8Unsafe(util::EscapeCacheFileName(id)));
 }
 
+// Set extended file attribute as |name| |value| pair.

[hashimoto, 2016/05/02 08:29:20]

nit: "Set" -> "Sets"
The same goes for others.

[oka, 2016/05/09 14:27:39]

Done.

+bool Setxattr(const std::string& path, const std::string& name,

[hashimoto, 2016/05/02 08:29:20]

How about making the name more descriptive (e.g. SetExtendedFileAttribute)?

Our style guide says we should "eschew abbreviation".
(https://google.github.io/styleguide/cppguide.html#General_Naming_Rules)
The same goes for others.

Also, please use base::FilePath for file paths.

[oka, 2016/05/09 14:27:39]

Done.

+    const std::string& value) {
+  return setxattr(path.c_str(), name.c_str(), value.c_str(), value.size() + 1,
+      0) == 0;
+}
+
+// Get extended file attribute associated with |name| and set it to |value|.
+// If nothing is associated with |name|, set "" to |value|.
+bool Getxattr(const std::string& path, const std::string& name,
+    std::string* value, size_t size) {

[hashimoto, 2016/05/02 08:29:20]

Do we actually care about the contents of the attribute?
Can't we just call getxattr(path, name, nullptr, 0) and check that it doesn't
fail with ENOATTR to know if it's marked or not?

[oka, 2016/05/09 14:27:40]

That's a good idea. Though I removed the func, I'll do it in cryptohome code.
Thanks.

+  value->resize(size - 1);
+  char* v = string_as_array(value);
+  ssize_t ret_size = getxattr(path.c_str(), name.c_str(), v, size);
+  if (ret_size < 0) {
+    if (errno == ENODATA) {
+      value->assign("");
+      return true;
+    } else {
+      PLOG(ERROR) << "getxattr: " << name;
+      return false;
+    }
+  }
+  value->resize(ret_size - 1);
+  return true;
+}
+
+// Change attributes of the file with |flags|,
+// e.g. FS_UNRM_FL | FS_IMMUTABLE_FL. See linux/fs.h for available flags.

[hashimoto, 2016/05/02 08:29:20]

nit: FS_UNRM_FL and FS_IMMUTABLE_FL are totally unrelated to this code.
How about using FS_NODUMP_FL as an example here, with a description about what
it means, and how cryptohome handles it?

[oka, 2016/05/09 14:27:40]

Done.

+// Returns whether operation succeeded.
+bool Chattr(const std::string& path, int64_t flags) {

[hashimoto, 2016/05/02 08:29:20]

Could you give this function a more readable name (e.g. SetFileAttributes)?
In comment you can write that this function does the same thing as chattr(1).
The same goes for Lsattr().

[oka, 2016/05/09 14:27:40]

Done.

+  int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY));

[hashimoto, 2016/05/02 08:29:20]

Why don't you use base::File with which you don't need to call close()
everywhere?

[oka, 2016/05/09 14:27:40]

Done.

+  if (fd < 0) {
+    PLOG(ERROR) << "open: " << path;
+    return false;
+  }
+  if (ioctl(fd, FS_IOC_SETFLAGS, &flags) < 0) {
+    PLOG(ERROR) << "ioctl" << path;
+    IGNORE_EINTR(close(fd));
+    return false;
+  }
+  IGNORE_EINTR(close(fd));
+  return true;
+}
+
+int64_t Lsattr(const std::string& path) {

[hashimoto, 2016/05/02 08:29:20]

Please add a comment.

[oka, 2016/05/09 14:27:40]

Done.

+  int fd = HANDLE_EINTR(open(path.c_str(), O_RDONLY));
+  if (fd < 0) {
+    PLOG(ERROR) << "open: " << path;
+    return -1;
+  }
+  int64_t flags = 0;
+  if (ioctl(fd, FS_IOC_GETFLAGS, &flags) < 0) {
+    PLOG(ERROR) << "ioctl";
+    IGNORE_EINTR(close(fd));
+    return -1;
+  }
+  IGNORE_EINTR(close(fd));
+  return flags;
+}
+
+// Mark the cache file to be removable by cryptohome.
+bool SetRemovable(const base::FilePath& path) {
+  int64_t flags = Lsattr(path.value());
+  if (flags < 0) return false;
+  if ((flags & FS_NODUMP_FL) == FS_NODUMP_FL) return true;
+
+  return Chattr(path.value(), flags | FS_NODUMP_FL);
+}
+
+// Mark the cache file to be unremovable by cryptohome.
+bool UnsetRemovable(const base::FilePath& path) {
+  // File is to be created.
+  if (!base::PathExists(path)) return true;

[hashimoto, 2016/05/02 08:29:20]

This behavior is surprising, and not symmetric with SetRemovable().
How about checking cache_entry.is_present() in the caller code when appropriate?

[oka, 2016/05/09 14:27:40]

Done. Added existence check to Pin.

+
+  int64_t flags = Lsattr(path.value());
+  if (flags < 0) return false;
+  if ((flags & FS_NODUMP_FL) == 0) return true;
+
+  return Chattr(path.value(), flags & ~(static_cast<int64_t>(FS_NODUMP_FL)));
+}
+
+// Mark |path| as drive cache dir.
+// Returns if the operation succeeded.
+bool MarkAsDriveCacheDir(const base::FilePath& path) {
+  return SetRemovable(path)
+      && Setxattr(path.value(), "user.GCacheFiles", "true");

[hashimoto, 2016/05/02 08:29:20]

What is the point of using "true" instead of an empty string?

Also, please make "user.GCacheFiles" a constant with a name.

[oka, 2016/05/09 14:27:39]

Done.

+}
+
 typedef std::pair<base::File::Info, ResourceEntry> CacheInfo;
 
 class CacheInfoLatestCompare {
  public:
   bool operator()(const CacheInfo& info_a, const CacheInfo& info_b) {
     return info_a.first.last_accessed < info_b.first.last_accessed;
   }
 };
 
 const size_t kMaxNumOfEvictedCacheFiles = 30000;
@@ skipping 217 matching lines @@
     return FILE_ERROR_FAILED;
   }
 
   // Now that file operations have completed, update metadata.
   FileCacheEntry* cache_state =
       entry.mutable_file_specific_info()->mutable_cache_state();
   cache_state->set_md5(md5);
   cache_state->set_is_present(true);
   if (md5.empty())
     cache_state->set_is_dirty(true);
+
+  if (!cache_state->is_pinned() && !cache_state->is_dirty()) {
+    if (!SetRemovable(dest_path))
+      return FILE_ERROR_FAILED;
+  } else {
+    if (!UnsetRemovable(dest_path))
+      return FILE_ERROR_FAILED;
+  }
+
   return storage_->PutEntry(entry);
 }
 
 FileError FileCache::Pin(const std::string& id) {
   AssertOnSequencedWorkerPool();
 
   ResourceEntry entry;
   FileError error = storage_->GetEntry(id, &entry);
   if (error != FILE_ERROR_OK)
     return error;
+
   entry.mutable_file_specific_info()->mutable_cache_state()->set_is_pinned(
       true);
+  if (!UnsetRemovable(GetCacheFilePath(entry.local_id())))
+    return FILE_ERROR_FAILED;
+
   return storage_->PutEntry(entry);
 }
 
 FileError FileCache::Unpin(const std::string& id) {
   AssertOnSequencedWorkerPool();
 
   // Unpinning a file means its entry must exist in cache.
   ResourceEntry entry;
   FileError error = storage_->GetEntry(id, &entry);
   if (error != FILE_ERROR_OK)
     return error;
 
   // Now that file operations have completed, update metadata.
   if (entry.file_specific_info().cache_state().is_present()) {
     entry.mutable_file_specific_info()->mutable_cache_state()->set_is_pinned(
         false);
+    if (!entry.file_specific_info().cache_state().is_dirty()) {
+      if (!SetRemovable(GetCacheFilePath(entry.local_id())))
+        return FILE_ERROR_FAILED;
+    }
   } else {
     // Remove the existing entry if we are unpinning a non-present file.
     entry.mutable_file_specific_info()->clear_cache_state();
   }
   error = storage_->PutEntry(entry);
   if (error != FILE_ERROR_OK)
     return error;
 
   // Now it's a chance to free up space if needed.
   FreeDiskSpaceIfNeededFor(0);
@@ skipping 46 matching lines @@
   ResourceEntry entry;
   FileError error = storage_->GetEntry(id, &entry);
   if (error != FILE_ERROR_OK)
     return error;
   if (!entry.file_specific_info().cache_state().is_present()) {
     LOG(WARNING) << "Can't mark dirty a file that wasn't cached: " << id;
     return FILE_ERROR_NOT_FOUND;
   }
 
   entry.mutable_file_specific_info()->mutable_cache_state()->set_is_dirty(true);
+  if (!UnsetRemovable(GetCacheFilePath(entry.local_id())))
+    return FILE_ERROR_FAILED;
+
   entry.mutable_file_specific_info()->mutable_cache_state()->clear_md5();
   error = storage_->PutEntry(entry);
   if (error != FILE_ERROR_OK)
     return error;
 
   write_opened_files_[id]++;
   file_closer->reset(new base::ScopedClosureRunner(
       base::Bind(&google_apis::RunTaskWithTaskRunner,
                  blocking_task_runner_,
                  base::Bind(&FileCache::CloseForWrite,
@@ skipping 51 matching lines @@
 
   // If a file is not dirty (it should have been marked dirty via OpenForWrite),
   // clearing its dirty state is an invalid operation.
   if (!entry.file_specific_info().cache_state().is_dirty()) {
     LOG(WARNING) << "Can't clear dirty state of a non-dirty file: " << id;
     return FILE_ERROR_INVALID_OPERATION;
   }
 
   entry.mutable_file_specific_info()->mutable_cache_state()->set_is_dirty(
       false);
+  if (!entry.file_specific_info().cache_state().is_pinned()) {
+    if (!SetRemovable(GetCacheFilePath(entry.local_id())))
+      return FILE_ERROR_FAILED;
+  }
+
   return storage_->PutEntry(entry);
 }
 
 FileError FileCache::Remove(const std::string& id) {
   AssertOnSequencedWorkerPool();
 
   ResourceEntry entry;
 
   // If entry doesn't exist, nothing to do.
   FileError error = storage_->GetEntry(id, &entry);
@@ skipping 46 matching lines @@
           clear_md5();
       if (storage_->PutEntry(new_entry) != FILE_ERROR_OK)
         return false;
     }
   }
   if (it->HasError())
     return false;
 
   if (!RenameCacheFilesToNewFormat())
     return false;
+
+  if (!ResolveMetadataInconsistency()) {
+    return false;
+  }
+
+  if (ShouldStartDriveCacheMigration()) {
+    if (!MigrateCacheFiles()) {
+      return false;
+    }
+  }
+
   return true;
 }
 
+bool FileCache::ShouldStartDriveCacheMigration() {

[hashimoto, 2016/05/02 08:29:19]

IIUC, because of IO scheduling, a system crash or a system power loss can result
in a situation where /files has attributes, but files under that directory don't
on the next boot.

Can't we just run the attribute setting code on every run?
How much does it affect the performance?

[oka, 2016/05/09 14:27:39]

I experimentally confirmed setting attributes is not that slow (0.5s for 1000
files).
Let's run the attribute setting code on every run.

+  std::string value = "";
+  if (!Getxattr(cache_file_directory_.value(), "user.GCacheFiles", &value, 5)) {
+    PLOG(WARNING) << "Getxattr: " << cache_file_directory_.value();
+  }
+  int64_t flags = Lsattr(cache_file_directory_.value());
+  if (flags < 0) return false;
+
+  return value != "true" || ((flags & FS_NODUMP_FL) == 0);
+}
+
 void FileCache::Destroy() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   in_shutdown_.Set();
 
   // Destroy myself on the blocking pool.
   // Note that base::DeletePointer<> cannot be used as the destructor of this
   // class is private.
   blocking_task_runner_->PostTask(
       FROM_HERE,
@@ skipping 148 matching lines @@
       continue;
     }
     const std::string& id = GetIdFromPath(new_path);
     new_path = GetCacheFilePath(util::CanonicalizeResourceId(id));
     if (new_path != current && !base::Move(current, new_path))
       return false;
   }
   return true;
 }
 
-// static
-bool FileCache::MigrateCacheFiles(const base::FilePath& from,
-                                  const base::FilePath& to_files,
-                                  const base::FilePath& to_links,
-                                  ResourceMetadataStorage* metadata_storage) {
+bool FileCache::MigrateCacheFiles() {
   std::unique_ptr<ResourceMetadataStorage::Iterator> it =
-      metadata_storage->GetIterator();
+      storage_->GetIterator();
+
   for (; !it->IsAtEnd(); it->Advance()) {
     const ResourceEntry& entry = it->GetValue();
     if (!entry.file_specific_info().cache_state().is_present()) {
       continue;
     }
 
-    // Ignore missing cache file case since it never succeeds.
-    // TODO(yawano): handle this case at metadata validation in FileCache.
-    const base::FilePath move_from = GetPathForId(from, entry.local_id());
-    if (!base::PathExists(move_from)) {
+    const base::FilePath filepath = GetPathForId(cache_file_directory_,
+        entry.local_id());
+
+    if (!base::PathExists(filepath)) {
+      PLOG(WARNING) << "Cache and metadata are inconsistent.";

[hashimoto, 2016/05/02 08:29:19]

It's strange to abort the initialization with a warning.
If we actually want to disable Drive here (this is what aborting the
initialization means), we should emit an ERROR.

[oka, 2016/05/09 14:27:39]

Merged two funcs, and removed this warning.

+      return false;
+    }
+
+    const FileCacheEntry& file_cache_entry =
+        entry.file_specific_info().cache_state();
+    if (file_cache_entry.is_pinned() || file_cache_entry.is_dirty()) {
+      if (!UnsetRemovable(filepath)) return false;
+    } else {
+      if (!SetRemovable(filepath)) return false;
+    }
+  }
+
+  return MarkAsDriveCacheDir(cache_file_directory_);
+}
+
+bool FileCache::ResolveMetadataInconsistency() {
+  std::unique_ptr<ResourceMetadataStorage::Iterator>
+    it = storage_->GetIterator();
+  for (; !it->IsAtEnd(); it->Advance()) {
+    ResourceEntry entry = it->GetValue();
+
+    if (!entry.file_specific_info().has_cache_state() ||
+        !entry.file_specific_info().cache_state().is_present()) {

[hashimoto, 2016/05/02 08:29:20]

Shouldn't we handle a case where !is_present but the file exists?
Anything can happen because of IO scheduling in case of a system crash (e.g.
kernel panic, power lost).

[oka, 2016/05/09 14:27:40]

Done.
I merged ResolveMetadataInconsistency and MigrateCacheFiles, and named it
MigrateCacheFilesResolvingInconsistency.

       continue;
     }
 
-    // Create hard link to cache file if it's pinned or dirty. cryptohome will
-    // not delete a cache file if there is a hard link to it.
-    const FileCacheEntry& file_cache_entry =
-        entry.file_specific_info().cache_state();
-    if (file_cache_entry.is_pinned() || file_cache_entry.is_dirty()) {
-      const base::FilePath link_path = GetPathForId(to_links, entry.local_id());
-      int link_result = link(move_from.AsUTF8Unsafe().c_str(),
-                             link_path.AsUTF8Unsafe().c_str());
-      if (link_result != 0 && errno != EEXIST) {
-        return false;
-      }
+    if (base::PathExists(GetCacheFilePath(entry.local_id()))) {
+      continue;
     }
 
-    // Move cache file.
-    const base::FilePath move_to = GetPathForId(to_files, entry.local_id());
-    if (!base::Move(move_from, move_to)) {
+    entry.mutable_file_specific_info()->mutable_cache_state()->set_is_present(
+        false);
+
+    if (storage_->PutEntry(entry) != FILE_ERROR_OK)
       return false;
-    }
   }
-
   return true;
 }
 
 void FileCache::CloseForWrite(const std::string& id) {
   AssertOnSequencedWorkerPool();
 
   std::map<std::string, int>::iterator it = write_opened_files_.find(id);
   if (it == write_opened_files_.end())
     return;
 
@@ skipping 21 matching lines @@
 
 bool FileCache::IsEvictable(const std::string& id, const ResourceEntry& entry) {
   return entry.file_specific_info().has_cache_state() &&
          !entry.file_specific_info().cache_state().is_pinned() &&
          !entry.file_specific_info().cache_state().is_dirty() &&
          !mounted_files_.count(id);
 }
 
 }  // namespace internal
 }  // namespace drive