fix deadlock #1371

ThomasWaldmann · 2016-07-24T14:36:53Z

see #1220 + commit comments.

The deadlock is fixed already.

But: needed a RPC API change to add the exclusive param. :-(

Also: Thinking about whether we maybe rather want a safe default for exclusive.
Currently it is usually False and you need to give exclusive=True - guess the opposite way would be safer, so one does not forget requesting an exclusive lock.

Nothing really bad would happen if one forgets though, the code has an automatic lock upgrade to an exclusive lock.

The lock upgrade/downgrade code was kept to support old clients.
Current code shall not use it, it is deprecated.

codecov-io · 2016-07-24T14:48:03Z

Current coverage is 82.20% (diff: 89.18%)

Merging #1371 into 1.0-maint will increase coverage by 0.14%

@@           1.0-maint      #1371   diff @@
===========================================
  Files             14         14          
  Lines           4433       4440     +7   
  Methods            0          0          
  Messages           0          0          
  Branches         776        777     +1   
===========================================
+ Hits            3638       3650    +12   
+ Misses           569        566     -3   
+ Partials         226        224     -2

Powered by Codecov. Last update 976925c...33e3348

ThomasWaldmann · 2016-07-28T13:21:10Z

From IRC discussion:

need to keep lock upgrade on txn start, for compat with older clients (== if there is no excl. lock already, do an upgrade)
thus: need to keep lock upgrade/downgrade code in borg 1.0, but deprecate it (emit warning about old client?)
keep all other changes as is, so new clients based on this code will not use lock upgrade

ThomasWaldmann · 2016-07-28T15:36:39Z

any comments about the exclusive default value? change it? in 1.0? in 1.1?

ThomasWaldmann · 2016-08-02T00:31:30Z

please test / review, this is the last essential issue to do for 1.0.7 milestone.

enkore · 2016-08-02T13:16:53Z

I'll review this offline

textshell · 2016-08-02T18:11:09Z

borg/remote.py

-                    self.id = self.call('open', self.location.path, create, lock_wait, lock, append_only)
-                except self.RPCError as err:
-                    if err.remote_type == 'TypeError':
-                        raise self.NoAppendOnlyOnServer() from err


Why remove this? Silently ignoring Repository settings on create doesn‘t sound nice.

it's now either both exclusive and append_only params (new api) or neither of them (old api).

what i meant was that the old api code path should do:

if append_only: raise self.NoAppendOnlyOnServer()

Thus keeping the safe guard from the original PR.

fixed, see new commit.

textshell · 2016-08-02T18:20:16Z

For #1374 i either need this in enforcing mode (i.e. situation with a client and server that both have the iv parts needs to never upgrade on put) or i need to add independent locking (or lock upgrading) to #1374

textshell · 2016-08-02T18:28:57Z

borg/testsuite/repository.py

-            with self.repository:
-                self.assert_raises(LockFailed, lambda: len(self.repository))
-                upgrade.assert_called_once_with()
-


Why remove this? I think this case is still relevant? i.e. we still need to upgrade, and we still need not to hit any asserts or other problems.

I couldn't make much sense of this test.
I first completely removed the lock upgrading code.
Later I re-added it for older clients.
But the current code does not do lock upgrades.

I think it still does.

Do a borg list with a remote repo that had it’s last writer crash and no other access since then

client eventually calls Repository.get

which calls Repository.get_transaction_id()

which checks for crash by index_transaction_id != segments_transaction_id

and calls Repository.replay_segments

which calls self.prepare_txn

which needs to upgrade

This is the one legitimate case i can come up where an always read only operation (that correctly uses an non exclusive lock) can get into prepare_txn.

This is now accounted for in my understanding.

textshell · 2016-08-02T18:59:03Z

Apart from the above this looks good.

Just noticed though that change-passphrase and migrate-to-repokey use Repository.save_key which is not covered by any lock. Not sure we care enough to make a separate ticket for that though.

ThomasWaldmann · 2016-08-02T22:35:03Z

@textshell please verify if the last changeset is what you meant.

textshell · 2016-08-03T06:04:55Z

Yes, that‘s what i meant. Can you please resurrect the test (or similar) for the lock upgrade on replay?
It seems the travis unhappiness is for unrelated reasons.

ThomasWaldmann · 2016-08-03T14:04:07Z

Test resurrected and differentiated into 2 tests for old and new clients.

enkore · 2016-08-05T10:46:33Z

thomas/fix-deadlock
with_repository arg changes: ok
prepare_txn: ok
locking code: not much of an idea...
test_replay_of_readonly_repository: hmm, look into test spec, why must be removed

I see the last bit was already addressed :)

ThomasWaldmann · 2016-08-05T12:10:24Z

@enkore guess the most delicate part is the compatibility.

ThomasWaldmann · 2016-08-05T12:31:04Z

about lock upgrade/downgrade: I wanted to deprecate that, due to the obvious deadlock issues, but it seems that even with the new code we need the lock upgrade for somehow unexpectedly needing a write lock, e.g. when borg list finds a crashed repo and needs to do segment replay.

So I think I am removing the deprecation notes and replacing them with a deadlock warning.

enkore · 2016-08-05T13:25:01Z

borg/remote.py

+                if err.remote_type != 'TypeError':
+                    raise
+                if append_only:
+                    raise self.NoAppendOnlyOnServer()


ThomasWaldmann · 2016-08-05T16:31:42Z

collapse / merge?

if we don't, we try to upgrade the lock. this is to support old clients talking to a new server and also to avoid bad consequences from coding mistakes for new clients.

enkore · 2016-08-05T16:56:22Z

yes

textshell · 2016-08-05T16:59:25Z

wrt collapse: I would at least keep the first two commits uncollapsed. Renames are alsways better in a separate commit. looks good now

lock upgrading is troublesome / may deadlock, do not advertise it.

…eplay old clients use self.exclusive = None and do a read->write lock upgrade when needed. new clients use self.exclusive = True/False and never upgrade. replay fakes an old client by setting self.exclusive = None to get a lock upgrade if needed.

textshell mentioned this pull request Jul 25, 2016

Fix AES-CTR nonce reusage #1374

Merged

9 tasks

ThomasWaldmann force-pushed the fix-deadlock branch 2 times, most recently from 2c0333e to 75a9378 Compare July 26, 2016 23:02

ThomasWaldmann force-pushed the fix-deadlock branch from 75a9378 to 972ffc2 Compare July 28, 2016 14:49

ThomasWaldmann changed the title ~~WIP: fix deadlock~~ fix deadlock Jul 28, 2016

ThomasWaldmann force-pushed the fix-deadlock branch from 972ffc2 to e37d497 Compare August 1, 2016 23:57

textshell reviewed Aug 2, 2016
View reviewed changes

ThomasWaldmann force-pushed the fix-deadlock branch from b09c12b to 9967cc4 Compare August 3, 2016 14:02

enkore reviewed Aug 5, 2016
View reviewed changes

borg/remote.py

if err.remote_type != 'TypeError':

raise

if append_only:

raise self.NoAppendOnlyOnServer()

Copy link

Contributor

enkore Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k

ThomasWaldmann added 2 commits August 5, 2016 18:52

add Lock.got_exclusive_lock

26007c0

make sure we have a excl. lock when starting a transaction

2a355e5

if we don't, we try to upgrade the lock. this is to support old clients talking to a new server and also to avoid bad consequences from coding mistakes for new clients.

ThomasWaldmann added 2 commits August 5, 2016 19:06

rename UpgradableLock to Lock

d3d51e1

lock upgrading is troublesome / may deadlock, do not advertise it.

fix local repo / upgrader tests

1e739fd

ThomasWaldmann force-pushed the fix-deadlock branch from 2e891de to abebedc Compare August 5, 2016 17:07

ThomasWaldmann added 2 commits August 5, 2016 19:11

change RPC API, fix remote repo tests

64dcbbf

ThomasWaldmann force-pushed the fix-deadlock branch from abebedc to 33e3348 Compare August 5, 2016 17:11

ThomasWaldmann merged commit 18654ea into borgbackup:1.0-maint Aug 5, 2016

ThomasWaldmann deleted the fix-deadlock branch August 5, 2016 17:48

ThomasWaldmann mentioned this pull request Aug 5, 2016

deadlocks with multiple borg create #1220

Closed

ThomasWaldmann mentioned this pull request Aug 15, 2016

Dealing with attic issues #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix deadlock #1371

fix deadlock #1371

ThomasWaldmann commented Jul 24, 2016 •

edited

Loading

codecov-io commented Jul 24, 2016 •

edited

Loading

ThomasWaldmann commented Jul 28, 2016

ThomasWaldmann commented Jul 28, 2016

ThomasWaldmann commented Aug 2, 2016

enkore commented Aug 2, 2016

textshell Aug 2, 2016

ThomasWaldmann Aug 2, 2016

textshell Aug 2, 2016

ThomasWaldmann Aug 2, 2016

textshell commented Aug 2, 2016 •

edited

Loading

textshell Aug 2, 2016

ThomasWaldmann Aug 2, 2016

textshell Aug 2, 2016

enkore Aug 5, 2016

textshell commented Aug 2, 2016

ThomasWaldmann commented Aug 2, 2016

textshell commented Aug 3, 2016

ThomasWaldmann commented Aug 3, 2016

enkore commented Aug 5, 2016

ThomasWaldmann commented Aug 5, 2016

ThomasWaldmann commented Aug 5, 2016

enkore Aug 5, 2016

ThomasWaldmann commented Aug 5, 2016

enkore commented Aug 5, 2016

textshell commented Aug 5, 2016

fix deadlock #1371

fix deadlock #1371

Conversation

ThomasWaldmann commented Jul 24, 2016 • edited Loading

codecov-io commented Jul 24, 2016 • edited Loading

Current coverage is 82.20% (diff: 89.18%)

ThomasWaldmann commented Jul 28, 2016

ThomasWaldmann commented Jul 28, 2016

ThomasWaldmann commented Aug 2, 2016

enkore commented Aug 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

textshell commented Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

textshell commented Aug 2, 2016

ThomasWaldmann commented Aug 2, 2016

textshell commented Aug 3, 2016

ThomasWaldmann commented Aug 3, 2016

enkore commented Aug 5, 2016

ThomasWaldmann commented Aug 5, 2016

ThomasWaldmann commented Aug 5, 2016

Choose a reason for hiding this comment

ThomasWaldmann commented Aug 5, 2016

enkore commented Aug 5, 2016

textshell commented Aug 5, 2016

ThomasWaldmann commented Jul 24, 2016 •

edited

Loading

codecov-io commented Jul 24, 2016 •

edited

Loading

textshell commented Aug 2, 2016 •

edited

Loading