From 15d8d4305a95155cb3c567219178297b9d98c466 Mon Sep 17 00:00:00 2001 From: Vincent Gross Date: Thu, 18 Oct 2018 17:36:59 +0200 Subject: [PATCH 1/3] Add tests for exclusive locks over shared locks. --- src/borg/locking.py | 2 +- src/borg/testsuite/locking.py | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/borg/locking.py b/src/borg/locking.py index e1b0c7fe09..931a963719 100644 --- a/src/borg/locking.py +++ b/src/borg/locking.py @@ -320,7 +320,7 @@ class Lock: This makes sure the lock is released again if the block is left, no matter how (e.g. if an exception occurred). """ - def __init__(self, path, exclusive=False, sleep=None, timeout=None, id=None, kill_stale_locks=False): + def __init__(self, path, exclusive=False, sleep=None, timeout=None, id=None, kill_stale_locks=False, pgid=None): self.path = path self.is_exclusive = exclusive self.sleep = sleep diff --git a/src/borg/testsuite/locking.py b/src/borg/testsuite/locking.py index 64a7928116..6908f46ac9 100644 --- a/src/borg/testsuite/locking.py +++ b/src/borg/testsuite/locking.py @@ -108,8 +108,22 @@ def test_exclusive(self, lockpath): assert len(lock._roster.get(EXCLUSIVE)) == 1 assert not lock._roster.empty(SHARED, EXCLUSIVE) + def test_exclusive_pgid_shared(self, lockpath): + lock1 = Lock(lockpath, exclusive=False, id=ID1).acquire() + with Lock(lockpath, exclusive=True, id=ID2, timeout=1, pgid=ID1): + assert len(lock1._roster.get(SHARED)) == 1 + assert len(lock1._roster.get(EXCLUSIVE)) == 1 + lock1.release() + + def test_exclusive_pgid_distinct(self, lockpath): + lock1 = Lock(lockpath, exclusive=False, id=ID1).acquire() + with pytest.raises(LockTimeout): + with Lock(lockpath, exclusive=True, id=ID2, timeout=1): + pass + lock1.release() + def test_upgrade(self, lockpath): - with Lock(lockpath, exclusive=False) as lock: + with Lock(lockpath, exclusive=False, pgid=ID1) as lock: lock.upgrade() lock.upgrade() # NOP assert len(lock._roster.get(SHARED)) == 0 @@ -117,7 +131,7 @@ def test_upgrade(self, lockpath): assert not lock._roster.empty(SHARED, EXCLUSIVE) def test_downgrade(self, lockpath): - with Lock(lockpath, exclusive=True) as lock: + with Lock(lockpath, exclusive=True, pgid=ID1) as lock: lock.downgrade() lock.downgrade() # NOP assert len(lock._roster.get(SHARED)) == 1 From 1fac3966b530c250e05ade833996e39ac6329343 Mon Sep 17 00:00:00 2001 From: Vincent Gross Date: Thu, 18 Oct 2018 18:16:23 +0200 Subject: [PATCH 2/3] Add support for exclusive lock on top of shared lock This only works if the process seeking the exclusive lock has its pgid equal to the shared lock holder's pid, and there is only one shared lock holder. With this it is possible to run borg commands from within borg with-lock. --- src/borg/archiver.py | 36 ++++++++++++++++++++++++----------- src/borg/locking.py | 9 +++++++-- src/borg/platform/__init__.py | 4 ++-- src/borg/platform/base.py | 7 +++++++ src/borg/platform/posix.pyx | 12 ++++++++++++ 5 files changed, 53 insertions(+), 15 deletions(-) diff --git a/src/borg/archiver.py b/src/borg/archiver.py index 2a775fe5a8..4dc315ab97 100644 --- a/src/borg/archiver.py +++ b/src/borg/archiver.py @@ -1530,18 +1530,24 @@ def do_with_lock(self, args, repository): # we can only do this for local repositories (with .io), though: if hasattr(repository, 'io'): repository.io.close_segment() + # we need to commit the "no change" operation we did to the manifest + # because it created a new segment file in the repository. if we would + # roll back, the same file would be later used otherwise (for other content). + # that would be bad if somebody uses rsync with ignore-existing (or + # any other mechanism relying on existing segment data not changing). + # see issue #1867. + repository.commit(compact=False) + if args.shared_lock: + try: + repository.lock.downgrade() + except AttributeError: + # this is only for local repositories + self.print_error('Running borg with-lock using shared locks is supported only for local repositories.') + return self.exit_code env = prepare_subprocess_env(system=True) - try: - # we exit with the return code we get from the subprocess - return subprocess.call([args.command] + args.args, env=env) - finally: - # we need to commit the "no change" operation we did to the manifest - # because it created a new segment file in the repository. if we would - # roll back, the same file would be later used otherwise (for other content). - # that would be bad if somebody uses rsync with ignore-existing (or - # any other mechanism relying on existing segment data not changing). - # see issue #1867. - repository.commit(compact=False) + os.setpgid(0,0) + # we exit with the return code we get from the subprocess + return subprocess.call([args.command] + args.args, env=env) @with_repository(manifest=False, exclusive=True) def do_compact(self, args, repository): @@ -3788,6 +3794,12 @@ def define_archive_filters_group(subparser, *, sort_by=True, first_last=True): for its termination, release the lock and return the user command's return code as borg's return code. + It is possible to relax the lock with --shared-lock, so that you can run borg commands + within borg with-lock. This is only supported for local repositories, as the locking + protocol cannot account for concurrent access between two or more hosts. + This is useful if you want to check a repository state before running some maintenance + operations without race conditions or Time Of Check/Time Of Use problems. + .. note:: If you copy a repository with the lock held, the lock will be present in @@ -3801,6 +3813,8 @@ def define_archive_filters_group(subparser, *, sort_by=True, first_last=True): formatter_class=argparse.RawDescriptionHelpFormatter, help='run user command with lock held') subparser.set_defaults(func=self.do_with_lock) + subparser.add_argument('--shared-lock', dest='shared_lock', action='store_true', + help='use a shared lock rather than an exclusive one') subparser.add_argument('location', metavar='REPOSITORY', type=location_validator(archive=False), help='repository to lock') diff --git a/src/borg/locking.py b/src/borg/locking.py index 931a963719..7317abb956 100644 --- a/src/borg/locking.py +++ b/src/borg/locking.py @@ -326,6 +326,7 @@ def __init__(self, path, exclusive=False, sleep=None, timeout=None, id=None, kil self.sleep = sleep self.timeout = timeout self.id = id or platform.get_process_id() + self.pgid = pgid or id or platform.get_process_group() # globally keeping track of shared and exclusive lockers: self._roster = LockRoster(path + '.roster', id=id, kill_stale_locks=kill_stale_locks) # an exclusive lock, used for: @@ -364,8 +365,12 @@ def _wait_for_readers_finishing(self, remove, sleep): try: if remove is not None: self._roster.modify(remove, REMOVE) - if len(self._roster.get(SHARED)) == 0: - return # we are the only one and we keep the lock! + # We keep the lock if we are the only one, or if it is held only by a reader + # whose pid equals our pgid. + # A process can only set its own and its children pgids, and only to the target's pid + # or a process' pgid from the same session (see setpgid(2)) + if self._roster.get(SHARED).issubset((self.pgid,)): + return # we are the only one, or we are in the shared group, and we keep the lock! # restore the roster state as before (undo the roster change): if remove is not None: self._roster.modify(remove, ADD) diff --git a/src/borg/platform/__init__.py b/src/borg/platform/__init__.py index afe1bc135e..8a037c42be 100644 --- a/src/borg/platform/__init__.py +++ b/src/borg/platform/__init__.py @@ -11,12 +11,12 @@ from .base import set_flags, get_flags from .base import SaveFile, SyncFile, sync_dir, fdatasync, safe_fadvise from .base import swidth, API_VERSION -from .base import process_alive, get_process_id, local_pid_alive, fqdn, hostname, hostid +from .base import process_alive, get_process_id, get_process_group, local_pid_alive, fqdn, hostname, hostid OS_API_VERSION = API_VERSION if not sys.platform.startswith(('win32', )): - from .posix import process_alive, local_pid_alive + from .posix import process_alive, local_pid_alive, get_process_group # posix swidth implementation works for: linux, freebsd, darwin, openindiana, cygwin from .posix import swidth diff --git a/src/borg/platform/base.py b/src/borg/platform/base.py index c4e11f2707..67fcd5b387 100644 --- a/src/borg/platform/base.py +++ b/src/borg/platform/base.py @@ -270,6 +270,13 @@ def get_process_id(): return hostid, pid, thread_id +def get_process_group(): + """ + Return group tuple (hostname, pgid, thread_id) for 'us'. + """ + raise NotImplementedError + + def process_alive(host, pid, thread): """ Check if the (host, pid, thread_id) combination corresponds to a potentially alive process. diff --git a/src/borg/platform/posix.pyx b/src/borg/platform/posix.pyx index 8d84a95c5b..80fce713b0 100644 --- a/src/borg/platform/posix.pyx +++ b/src/borg/platform/posix.pyx @@ -61,3 +61,15 @@ def local_pid_alive(pid): return False # Any other error (eg. permissions) means that the process ID refers to a live process. return True + + +def get_process_group(): + """ + Return group id (hostname, pgid, thread_id) from identification tuple for 'us'. + This always returns the current pgid, which might be different from before, e.g. if os.setpgid() was used. + """ + from . import hostid + + thread_id = 0 + pgid = os.getpgid(0) + return hostid, pgid, thread_id From d50eb3c197dac15b1a41feb71318d412fc464006 Mon Sep 17 00:00:00 2001 From: Vincent Gross Date: Tue, 23 Oct 2018 15:51:03 +0200 Subject: [PATCH 3/3] Fixup for flake8 --- src/borg/archiver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/borg/archiver.py b/src/borg/archiver.py index 4dc315ab97..5e40f44280 100644 --- a/src/borg/archiver.py +++ b/src/borg/archiver.py @@ -1545,7 +1545,7 @@ def do_with_lock(self, args, repository): self.print_error('Running borg with-lock using shared locks is supported only for local repositories.') return self.exit_code env = prepare_subprocess_env(system=True) - os.setpgid(0,0) + os.setpgid(0, 0) # we exit with the return code we get from the subprocess return subprocess.call([args.command] + args.args, env=env)