drivers/quota.Control: lock a mutex when called

[nalind]

The "quotas" map and "next project ID" field don't take well to being created and updated concurrently in multiple threads. Throw a lock around the whole object when any of its exported methods are called. Fixes #1394.

[rhatdan]

LGTM

[mtrmac]

At the local object level LGTM.

(Internal users don’t need this because the internal user is the overlay driver, called under the layer store lock — but CRI-O is calling driver.ReadWriteDiskUsage.)

---

Oh, actually external callers have direct access to the graph driver via store.GraphDriver??! What does that mean for #1214 ? Do we need to re-add that, and add other synchronization on all the public methods of all drivers?

Does the ReadWriteDiskUsage need to care about concurrent layer updates / removals? I.e. should the actual locking happen at the ReadWriteDiskUsage API level, to synchronize with all other layer updates by the graph driver?

Or is the right decision to pretend that the public access to the graph driver mostly doesn’t exist, it’s the caller’s responsibility not to do that, and we treat ReadWriteDiskUsage as a special case highly-optimized for concurrency? If that’s the intent, I think it would be useful to have widespread comments pointing that special case out.

[mtrmac]

(I appreciate that this fixes #1394 as is, and it’s better to have this PR than to have nothing. It’s just… a whole new can of worms I was not aware of.

Even if the individual disk usage computation was fine vs. concurrent updates/removals, and it would be fine to return such mostly-invalid data, I’m at least worried about things like Driver.dir not being aware of the need to correctly work outside of the usual locks.)

[nalind]

It's not exposed through the Store object. I think this is about library consumers calling into the quota package directly, which is something podman has apparently been doing for a while.

[nalind]

Rebased.

[mtrmac]

The CRI-O backtrace in the original report points at https://github.com/cri-o/cri-o/blob/80228a5d6a4b26e9ed2363ca6a936f5fb604c607/internal/lib/stats/stats_server.go#L189-L197 , which uses a published method of Store:

storage/store.go

Line 154 in 9e425af

GraphDriver() (drivers.Driver, error)

.

(The Podman user I can find is https://github.com/containers/podman/blob/b15510694b81929dbc107d354fdb0a83d0bfaa2a/libpod/runtime_volume_common.go#L181-L197 , but that’s a private single-use quota.Control, so I can’t see that that one requires locking.)

[nalind]

Yikes, I thought we'd dropped that somewhere along the way.

[mtrmac]

WDYT about marking Store.GraphDriver deprecated?

That would require giving CRI-O a new Store API to get the container size, and that could lock via readAllLayerStores. And something to expose Driver.Metadata for Podman’s inspect functions.