From 3c736cade8a509c92a0ab6c01347624bd7672b7c Mon Sep 17 00:00:00 2001 From: Unrud Date: Mon, 8 Aug 2016 04:07:01 +0200 Subject: [PATCH 1/4] Refactor: Move sync_directory into Collection class This is not used anywhere else. --- radicale/storage.py | 48 ++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/radicale/storage.py b/radicale/storage.py index f0de7cc..536a642 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -168,23 +168,6 @@ def path_to_filesystem(root, *paths): return safe_path -def sync_directory(path): - """Sync directory to disk. - - This only works on POSIX and does nothing on other systems. - - """ - if os.name == "posix": - fd = os.open(path, 0) - try: - if hasattr(fcntl, "F_FULLFSYNC"): - fcntl.fcntl(fd, fcntl.F_FULLFSYNC) - else: - os.fsync(fd) - finally: - os.close(fd) - - class UnsafePathError(ValueError): def __init__(self, path): message = "Can't translate name safely to filesystem: %s" % path @@ -427,6 +410,23 @@ class Collection(BaseCollection): return file_name raise FileExistsError(errno.EEXIST, "No usable file name found") + @staticmethod + def _sync_directory(path): + """Sync directory to disk. + + This only works on POSIX and does nothing on other systems. + + """ + if os.name == "posix": + fd = os.open(path, 0) + try: + if hasattr(fcntl, "F_FULLFSYNC"): + fcntl.fcntl(fd, fcntl.F_FULLFSYNC) + else: + os.fsync(fd) + finally: + os.close(fd) + @classmethod def _makedirs_synced(cls, filesystem_path): """Recursively create a directory and its parents in a sync'ed way. @@ -443,7 +443,7 @@ class Collection(BaseCollection): cls._makedirs_synced(parent_filesystem_path) # Possible race! os.makedirs(filesystem_path, exist_ok=True) - sync_directory(parent_filesystem_path) + cls._sync_directory(parent_filesystem_path) @classmethod def discover(cls, path, depth="0"): @@ -552,7 +552,7 @@ class Collection(BaseCollection): self.upload(self._find_available_file_name(), card) os.rename(tmp_filesystem_path, filesystem_path) - sync_directory(parent_dir) + cls._sync_directory(parent_dir) return cls(sane_path, principal=principal) @@ -561,9 +561,9 @@ class Collection(BaseCollection): os.rename( path_to_filesystem(item.collection._filesystem_path, item.href), path_to_filesystem(to_collection._filesystem_path, to_href)) - sync_directory(to_collection._filesystem_path) + cls._sync_directory(to_collection._filesystem_path) if item.collection._filesystem_path != to_collection._filesystem_path: - sync_directory(item.collection._filesystem_path) + cls._sync_directory(item.collection._filesystem_path) def list(self): try: @@ -639,9 +639,9 @@ class Collection(BaseCollection): dir=parent_dir) as tmp_dir: os.rename(self._filesystem_path, os.path.join( tmp_dir, os.path.basename(self._filesystem_path))) - sync_directory(parent_dir) + self._sync_directory(parent_dir) else: - sync_directory(parent_dir) + self._sync_directory(parent_dir) else: # Delete an item if not is_safe_filesystem_path_component(href): @@ -654,7 +654,7 @@ class Collection(BaseCollection): if etag and etag != get_etag(text): raise EtagMismatchError(etag, get_etag(text)) os.remove(path) - sync_directory(os.path.dirname(path)) + self._sync_directory(os.path.dirname(path)) def get_meta(self, key): if os.path.exists(self._props_path): From c336e0581e9c70bf08a054877d02d0469ff3b618 Mon Sep 17 00:00:00 2001 From: Unrud Date: Mon, 8 Aug 2016 05:02:36 +0200 Subject: [PATCH 2/4] Remove atomicwrites Unfortunately the library doesn't support disabling of disk syncing, fortunately we only need a small subset of it's functionality which is easy to implement. --- radicale/storage.py | 31 +++++++++++++++++-------------- setup.py | 2 +- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/radicale/storage.py b/radicale/storage.py index 536a642..06b2905 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -37,9 +37,8 @@ from hashlib import md5 from importlib import import_module from itertools import groupby from random import getrandbits -from tempfile import TemporaryDirectory +from tempfile import TemporaryDirectory, NamedTemporaryFile -from atomicwrites import AtomicWriter import vobject @@ -192,16 +191,6 @@ class EtagMismatchError(ValueError): super().__init__(message) -class _EncodedAtomicWriter(AtomicWriter): - def __init__(self, path, encoding, mode="w", overwrite=True): - self._encoding = encoding - return super().__init__(path, mode, overwrite=True) - - def get_fileobject(self, **kwargs): - return super().get_fileobject( - encoding=self._encoding, prefix=".Radicale.tmp-", **kwargs) - - class Item: def __init__(self, collection, item, href, last_modified=None): self.collection = collection @@ -399,8 +388,22 @@ class Collection(BaseCollection): @contextmanager def _atomic_write(self, path, mode="w"): - with _EncodedAtomicWriter(path, self.encoding, mode).open() as fd: - yield fd + dir = os.path.dirname(path) + tmp = NamedTemporaryFile(mode=mode, dir=dir, encoding=self.encoding, + delete=False, prefix=".Radicale.tmp-") + try: + yield tmp + if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"): + fcntl.fcntl(tmp.fileno(), fcntl.F_FULLFSYNC) + else: + os.fsync(tmp.fileno()) + tmp.close() + os.rename(tmp.name, path) + except: + tmp.close() + os.remove(tmp.name) + raise + self._sync_directory(dir) def _find_available_file_name(self): # Prevent infinite loop diff --git a/setup.py b/setup.py index 839f982..156afd7 100755 --- a/setup.py +++ b/setup.py @@ -66,7 +66,7 @@ setup( packages=["radicale"], provides=["radicale"], scripts=["bin/radicale"], - install_requires=["vobject", "atomicwrites"], + install_requires=["vobject"], setup_requires=pytest_runner, tests_require=[ "pytest-runner", "pytest-cov", "pytest-flake8", "pytest-isort"], From f5f52582a1a9935b0f5f50c2c5a1797103d377d0 Mon Sep 17 00:00:00 2001 From: Unrud Date: Mon, 8 Aug 2016 05:17:41 +0200 Subject: [PATCH 3/4] Add option to disable syncing to disk Disabling syncing increases the risk of data loss when the system crashes or power fails. On the positive it can increase the performance to a great extent. --- config | 5 +++++ radicale/config.py | 1 + radicale/storage.py | 15 +++++++++------ 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/config b/config index a0922b3..1cdfb75 100644 --- a/config +++ b/config @@ -103,6 +103,11 @@ # Folder for storing local collections, created if not present #filesystem_folder = ~/.config/radicale/collections +# Sync all changes to disk during requests. (This can impair performance.) +# Disabling it increases the risk of data loss, when the system crashes or +# power fails! +#fsync = True + # Command that is run after changes to storage #hook = # Example: git add -A && (git diff --cached --quiet || git commit -m "Changes by "%(user)s) diff --git a/radicale/config.py b/radicale/config.py index 980cf78..1e02656 100644 --- a/radicale/config.py +++ b/radicale/config.py @@ -58,6 +58,7 @@ INITIAL_CONFIG = { "type": "multifilesystem", "filesystem_folder": os.path.expanduser( "~/.config/radicale/collections"), + "fsync": "True", "hook": ""}, "logging": { "config": "/etc/radicale/logging", diff --git a/radicale/storage.py b/radicale/storage.py index 06b2905..9542ad6 100644 --- a/radicale/storage.py +++ b/radicale/storage.py @@ -393,10 +393,11 @@ class Collection(BaseCollection): delete=False, prefix=".Radicale.tmp-") try: yield tmp - if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"): - fcntl.fcntl(tmp.fileno(), fcntl.F_FULLFSYNC) - else: - os.fsync(tmp.fileno()) + if self.configuration.getboolean("storage", "fsync"): + if os.name == "posix" and hasattr(fcntl, "F_FULLFSYNC"): + fcntl.fcntl(tmp.fileno(), fcntl.F_FULLFSYNC) + else: + os.fsync(tmp.fileno()) tmp.close() os.rename(tmp.name, path) except: @@ -413,13 +414,15 @@ class Collection(BaseCollection): return file_name raise FileExistsError(errno.EEXIST, "No usable file name found") - @staticmethod - def _sync_directory(path): + @classmethod + def _sync_directory(cls, path): """Sync directory to disk. This only works on POSIX and does nothing on other systems. """ + if not cls.configuration.getboolean("storage", "fsync"): + return if os.name == "posix": fd = os.open(path, 0) try: From 6d85a731e5ffdea2ca5e161bfba82ff4e53fb64b Mon Sep 17 00:00:00 2001 From: Unrud Date: Mon, 8 Aug 2016 05:30:16 +0200 Subject: [PATCH 4/4] Disable syncing to disk for tests This reduces test time by almost 70%. --- radicale/tests/test_base.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/radicale/tests/test_base.py b/radicale/tests/test_base.py index 8d06dbc..db6d8a2 100644 --- a/radicale/tests/test_base.py +++ b/radicale/tests/test_base.py @@ -711,6 +711,8 @@ class TestMultiFileSystem(BaseRequests, BaseTest): super().setup() self.colpath = tempfile.mkdtemp() self.configuration.set("storage", "filesystem_folder", self.colpath) + # Disable syncing to disk for better performance + self.configuration.set("storage", "fsync", "False") self.application = Application(self.configuration, self.logger) def teardown(self): @@ -725,6 +727,8 @@ class TestCustomStorageSystem(BaseRequests, BaseTest): super().setup() self.colpath = tempfile.mkdtemp() self.configuration.set("storage", "filesystem_folder", self.colpath) + # Disable syncing to disk for better performance + self.configuration.set("storage", "fsync", "False") self.application = Application(self.configuration, self.logger) def teardown(self):