From dc2a6ac1d7584b9a223698f13d2ca63dcab6735d Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Fri, 7 Aug 2026 12:22:22 +0200 Subject: [PATCH] multifilesystem: Improve performance of PROPFIND The PROPFIND does not need to check the filesystem for the file existence, because the list comes from the directory listing, similar as the other parts already do it. That was highly visible with large collections (10k+ items), causing massive delays. Also avoid repeated stat() calls on the same path. --- CHANGELOG.md | 2 ++ radicale/storage/multifilesystem/discover.py | 4 ++- radicale/storage/multifilesystem/get.py | 10 ++++-- radicale/storage/multifilesystem/upload.py | 6 ++-- radicale/tests/test_storage.py | 35 ++++++++++++++++++++ 5 files changed, 52 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1cce6f..c8ddb42a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog ## 3.8.0.dev +* Fix: storage/multifilesystem: depth:1 PROPFIND no longer re-runs the filesystem collision check (path_to_filesystem) for every item in a collection; this made listing large collections O(n^2) on file systems not detected as collision-free +* Improve: storage/multifilesystem: avoid redundant stat() calls per item in get/upload when use_mtime_and_size_for_item_cache is enabled ## 3.7.8 * Fix: time-range filter on a VTODO having DTSTART/DUE and also CREATED/COMPLETED used the CREATED->COMPLETED duration instead of the DTSTART->DUE one, so completed tasks were missing from (or wrongly returned by) calendar-query REPORT results diff --git a/radicale/storage/multifilesystem/discover.py b/radicale/storage/multifilesystem/discover.py index ff564b79..ea0e57b0 100644 --- a/radicale/storage/multifilesystem/discover.py +++ b/radicale/storage/multifilesystem/discover.py @@ -88,7 +88,9 @@ class StoragePartDiscover(StorageBase): for href in collection._list(): with child_context_manager(sane_path, href): - item = collection._get(href) + # We don't need to check for collisions, because the file + # names are from _list() (os.scandir). + item = collection._get(href, verify_href=False) if item is not None: yield item diff --git a/radicale/storage/multifilesystem/get.py b/radicale/storage/multifilesystem/get.py index 7d5101fe..679157c2 100644 --- a/radicale/storage/multifilesystem/get.py +++ b/radicale/storage/multifilesystem/get.py @@ -68,6 +68,7 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, return None else: path = os.path.join(self._filesystem_path, href) + item_stat: Optional[os.stat_result] = None try: if self._storage._use_mtime_and_size_for_item_cache is True: # try to avoid "open" @@ -78,6 +79,9 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, raise IsADirectoryError(path) if not os.access(path, os.R_OK): raise PermissionError(path) + # Single stat() reused below for both the cache hash and + # last_modified, instead of stat-ing the same path 3 times. + item_stat = os.stat(path) else: with open(path, "rb") as f: # early read of the content @@ -95,7 +99,8 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, # The hash of the component in the file system. This is used to check, # if the entry in the cache is still valid. if self._storage._use_mtime_and_size_for_item_cache is True: - cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns) + assert item_stat is not None + cache_hash = self._item_cache_mtime_and_size(item_stat.st_size, item_stat.st_mtime_ns) if self._storage._debug_cache_actions is True: logger.debug("Item cache check for: %r with mtime and size %r", path, cache_hash) else: @@ -149,7 +154,8 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, logger.debug("Item cache hit for: %r", path) last_modified = time.strftime( "%a, %d %b %Y %H:%M:%S GMT", - time.gmtime(os.path.getmtime(path))) + time.gmtime(item_stat.st_mtime if item_stat is not None + else os.path.getmtime(path))) # Don't keep reference to ``vobject_item``, because it requires a lot # of memory. return radicale_item.Item( diff --git a/radicale/storage/multifilesystem/upload.py b/radicale/storage/multifilesystem/upload.py index 358b7004..48d626d5 100644 --- a/radicale/storage/multifilesystem/upload.py +++ b/radicale/storage/multifilesystem/upload.py @@ -50,7 +50,8 @@ class CollectionPartUpload(CollectionPartGet, CollectionPartCache, (href, self.path, e)) from e # store cache file if self._storage._use_mtime_and_size_for_item_cache is True: - cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns) + path_stat = os.stat(path) + cache_hash = self._item_cache_mtime_and_size(path_stat.st_size, path_stat.st_mtime_ns) if self._storage._debug_cache_actions is True: logger.debug("Item cache store for: %r with mtime and size %r", path, cache_hash) else: @@ -121,7 +122,8 @@ class CollectionPartUpload(CollectionPartGet, CollectionPartCache, # store cache file if self._storage._use_mtime_and_size_for_item_cache is True: - cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns) + path_stat = os.stat(path) + cache_hash = self._item_cache_mtime_and_size(path_stat.st_size, path_stat.st_mtime_ns) if self._storage._debug_cache_actions is True: logger.debug("Item cache store for: %r with mtime and size %r", path, cache_hash) else: diff --git a/radicale/tests/test_storage.py b/radicale/tests/test_storage.py index d2eb0f80..57014fc7 100644 --- a/radicale/tests/test_storage.py +++ b/radicale/tests/test_storage.py @@ -344,6 +344,41 @@ class TestMultiFileSystem(BaseTest): logs = caplog.messages assert len([log for log in logs if "File name collision" in log]) == 2 + def test_propfind_depth1_does_not_check_collisions_per_item(self) -> None: + """Listing a collection (PROPFIND depth:1) must not re-run the + filesystem collision check (path_to_filesystem) once per item. + """ + self.mkcalendar("/calendar.ics/") + item_count = 20 + for index in range(item_count): + uid = "event%d" % index + event = ("BEGIN:VCALENDAR\r\nVERSION:2.0\r\n" + "BEGIN:VEVENT\r\nUID:%s\r\nSUMMARY:Event %d\r\n" + "DTSTART:20130901T180000Z\r\nEND:VEVENT\r\n" + "END:VCALENDAR\r\n" % (uid, index)) + self.put("/calendar.ics/%s.ics" % uid, event) + + call_count = 0 + original_path_to_filesystem = pathutils.path_to_filesystem + + def counting_path_to_filesystem(*args, **kwargs): + nonlocal call_count + call_count += 1 + return original_path_to_filesystem(*args, **kwargs) + + with pytest.MonkeyPatch.context() as monkeypatch: + monkeypatch.setattr(pathutils, "path_to_filesystem", + counting_path_to_filesystem) + _, responses = self.propfind("/calendar.ics/", """\ + + + + +""", HTTP_DEPTH="1") + + assert len(responses) == item_count + 1 + assert call_count < item_count + @pytest.mark.skipif(not shutil.which("flock"), reason="flock command not found") @pytest.mark.skipif(radicale.log.logger.getEffectiveLevel() == logging.INFO, reason="requires loglevel DEBUG") def test_hook_placeholders_PUT(self, caplog) -> None: