Simplify and fix cache volume locking #4352
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtrmac
force-pushed
the
cache-locks
branch
2 times, most recently
from
c857841 to
e7d6600
Compare
Contributor
Author
|
This adds no new tests:
|
Collaborator
|
Could we get this in first please #4349 and then we can rebase the current PR ? |
Contributor
Author
|
Sure. The overlap is actually pretty small. |
Collaborator
|
@mtrmac Could you rebase please :) |
They exist in memory anyway, so this is more efficient: we avoid the need to manually touch the filesystem again, the associated costs - and the error paths go away. [NO NEW TESTS NEEDED] Signed-off-by: Miloslav Trmač <mitr@redhat.com>
By construction it's now quite clear that the locks should always be locked. Don't even bother with AssertLockedForWriting(), that's partially (checking for lock ownership, not for read-write ownership) implied by Unlock() already. [NO NEW TESTS NEEDED] Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Maintain a list of _all_ the locks, not just the last one. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It can return at most one lock, so don't return an array. Should not change behavior right now, but it will simplify cleanup. [NO NEW TESTS NEEDED] Signed-off-by: Miloslav Trmač <mitr@redhat.com>
rhatdan
reviewed
Oct 19, 2022
Member
|
Other then spelling mistake LGTM |
... and use a more traditional error handling model, where responsibility for the cleanup passes to the caller _only_ if the called function succeeds. To reinforce that, hard-code nil returns on error paths instead of returning the locks. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Member
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, mtrmac The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
What this PR does / why we need it:
sharing=locked(an undocumented feature AFAICS): Pass around lock objects instead of files, completely eliminating various error pathsLockfile.Locked, which is going away in API break: Remove Lockfile.Locked storage#1399 ).How to verify it
Existing tests? I didn’t even try running the code.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Cc: @flouthoc who added the feature in #3820.
Does this PR introduce a user-facing change?