From 5a39d5c4f8a86a9c94449061c293eaaa0ce90bb8 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Tue, 24 Jan 2017 23:45:54 +0100 Subject: [PATCH 1/2] make LoggedIO.close_segment reentrant if anything blows up in the middle of a (first) invocation of close_segment() and an exception gets raised, it could happen that close_segment() gets called again (e.g. in Repository.__del__ or elsewhere). As the self._write_fd was set to None rather late, it would re-enter the if-block then. The new code gets the value of self._write_fd and also sets it to None in one tuple assignment, so re-entrance does not happen. Also, it uses try/finally to make sure the important parts (fd.close()) gets executed, even if there are exceptions in the other parts. --- borg/repository.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/borg/repository.py b/borg/repository.py index ddaf01439..4c7df2174 100644 --- a/borg/repository.py +++ b/borg/repository.py @@ -821,19 +821,25 @@ class LoggedIO: self.close_segment() # after-commit fsync() def close_segment(self): - if self._write_fd: - self.segment += 1 - self.offset = 0 - self._write_fd.flush() - os.fsync(self._write_fd.fileno()) - if hasattr(os, 'posix_fadvise'): # only on UNIX - # tell the OS that it does not need to cache what we just wrote, - # avoids spoiling the cache for the OS and other processes. - os.posix_fadvise(self._write_fd.fileno(), 0, 0, os.POSIX_FADV_DONTNEED) - dirname = os.path.dirname(self._write_fd.name) - self._write_fd.close() - sync_dir(dirname) - self._write_fd = None + # set self._write_fd to None early to guard against reentry from error handling code pathes: + fd, self._write_fd = self._write_fd, None + if fd is not None: + dirname = None + try: + self.segment += 1 + self.offset = 0 + dirname = os.path.dirname(fd.name) + fd.flush() + fileno = fd.fileno() + os.fsync(fileno) + if hasattr(os, 'posix_fadvise'): # only on UNIX + # tell the OS that it does not need to cache what we just wrote, + # avoids spoiling the cache for the OS and other processes. + os.posix_fadvise(fileno, 0, 0, os.POSIX_FADV_DONTNEED) + finally: + fd.close() + if dirname: + sync_dir(dirname) MAX_DATA_SIZE = MAX_OBJECT_SIZE - LoggedIO.put_header_fmt.size From add38e8cdeb4242a64e5939c764ff6a314d85b5b Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Fri, 27 Jan 2017 21:09:55 +0100 Subject: [PATCH 2/2] ignore posix_fadvise errors in repository.py, work around #2095 note: we also ignore the call's return value in _chunker.c. both is harmless as the call not working does not cause incorrect function, just worse performance due to constant flooding of the cache (as if we would not issue the call). --- borg/repository.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/borg/repository.py b/borg/repository.py index 4c7df2174..e85c33d6f 100644 --- a/borg/repository.py +++ b/borg/repository.py @@ -833,9 +833,19 @@ class LoggedIO: fileno = fd.fileno() os.fsync(fileno) if hasattr(os, 'posix_fadvise'): # only on UNIX - # tell the OS that it does not need to cache what we just wrote, - # avoids spoiling the cache for the OS and other processes. - os.posix_fadvise(fileno, 0, 0, os.POSIX_FADV_DONTNEED) + try: + # tell the OS that it does not need to cache what we just wrote, + # avoids spoiling the cache for the OS and other processes. + os.posix_fadvise(fileno, 0, 0, os.POSIX_FADV_DONTNEED) + except OSError: + # usually, posix_fadvise can't fail for us, but there seem to + # be failures when running borg under docker on ARM, likely due + # to a bug outside of borg. + # also, there is a python wrapper bug, always giving errno = 0. + # https://github.com/borgbackup/borg/issues/2095 + # as this call is not critical for correct function (just to + # optimize cache usage), we ignore these errors. + pass finally: fd.close() if dirname: