lib/repo: Enable locking by default, make API non-experimental

[cgwalters]

The code has been sitting around for a while but since I disabled
it by default, I doubt anyone is really using it or relying on it.

This patch makes the existing APIs public, and turns on locking
by default. Conceptually these are two distinct things, and we
may actually want to split up the patches.

I don't think this will break anyone, but it's hard to say for sure.
It's also going to be hard to find out until we actually release
I suspect...

But anyone who is broken should be able to add locking=false into
their repo config.

On the APIs - I'm a bit concerned about the interactions over time
between libostree's use of the API and any apps that start using it.
For example, if an app specifies a SHARED lock in their code, then
later internally we decide to temporarily grab an EXCLUSIVE, but the
app had a second thread/process that was EXCLUSIVE already, and
that process was waiting on the first bit of code, then we could
deadlock. I can't think of a real world situation where this would happen
yet though.

[cgwalters]

Previous discussion: #813

[jlebon]

So, that seems sane to me. Though I'd like @dbnicholson to chime in if possible.

On the APIs - I'm a bit concerned about the interactions over time
between libostree's use of the API and any apps that start using it.

Hmm, I guess we could start annotating APIs that use the lock to try to help avoid this?

[cgwalters]

AFAICS flatpak isn't today using the repo locking APIs. @dbnicholson Are you aware of any applications which are today?

Perhaps the maximally conservative thing to do here is to keep the APIs private, and gradually extend the set of things that are safe to do concurrently with internal locking.

For example today we didn't add locking to checkouts; we could do so later and document it as now safe.

[dbnicholson]

AFAIK, it's all internal. Even our flatpak fork isn't using the locking directly, but I don't think we've received any more corrupted repo reports since we enabled the locking for transactions and prunes.

Sorry for not picking this up again. I'm definitely interested in completing this, but it's hard for me to find time to work on it.

I do believe that ultimately you'd want to have this be public so that clients can be in control. I envision a couple ways you'd want to control the locking externally:

• Taking an exclusive lock before doing a series of maintenance operations. Sort of like taking an exclusive lock at the beginning of a fsck operation, but where the app is doing more than one operation. For example, I would definitely take an exclusive lock in https://raw.githubusercontent.com/endlessm/eos-meta/master/eos-tech-support/eos-fix-ostree-repo if that was available.

• Taking a shared lock before doing a pull in anticipation of doing a subsequent checkout to ensure the ref/commit doesn't go away before doing the checkout.

[cgwalters]

I do believe that ultimately you'd want to have this be public so that clients can be in control. I envision a couple ways you'd want to control the locking externally:

I feel like concurrency cases divide into two: Those which mutate refs, and those that don't.

Taking a shared lock before doing a pull in anticipation of doing a subsequent checkout to ensure the ref/commit doesn't go away before doing the checkout.

But that just feels weird to me. Generally a set of refs is going to be "owned" by a particular piece of software, so it could do its own locking too. For flatpak for example I don't think we'd have some non-flatpak software going in and replacing flathub:runtime/org.gimp.GIMP.Locale/x86_64/stable or whatever right? Same for the host ref cases.

I'm not opposed to adding ostree locking for this, but would we really want flatpak/gnome-software or whatever simultaneously doing a checkout/deploy for a ref and updating it?

For example, I would definitely take an exclusive lock in https://raw.githubusercontent.com/endlessm/eos-meta/master/eos-tech-support/eos-fix-ostree-repo if that was available.

This one is interesting; we should drain some of this logic into libostree's fsck too.

[cgwalters]

Anyways for now I reworked this patch again to make the APIs unconditionally private for now. We can always change that later.

[cgwalters]

@rh-atomic-bot delegate=dbnicholson

[rh-atomic-bot]

✌️ @dbnicholson can now approve this pull request

[jlebon]

I'm going to merge this. I haven't personally played much with locking=true, but seeing the testsuite happy and knowing Endless has been running with it on for a while, I think this is pretty safe at this point. And there's always a locking=false escape hatch if needed.

@rh-atomic-bot r+ 77b0ef1

[rh-atomic-bot]

⌛ Testing commit 77b0ef1 with merge 4f93ea6...

[rh-atomic-bot]

💔 Test failed - status-atomicjenkins

[jlebon]

@rh-atomic-bot retry

[rh-atomic-bot]

⌛ Testing commit 77b0ef1 with merge de8cdfa...

[rh-atomic-bot]

💔 Test failed - status-atomicjenkins

[cgwalters]

@rh-atomic-bot retry

[rh-atomic-bot]

⌛ Testing commit 77b0ef1 with merge 8c15421...

[rh-atomic-bot]

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 8c15421 to master...