Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split RW and RO Layer locks per method #473

Closed
wants to merge 1 commit into from
Closed

Split RW and RO Layer locks per method #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/containers-storage/layers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
var listLayersTree = false

func layers(flags *mflag.FlagSet, action string, m storage.Store, args []string) int {
layers, err := m.Layers()
layers, err := m.RWLayers()
if err != nil {
fmt.Fprintf(os.Stderr, "%+v\n", err)
return 1
Expand Down
38 changes: 12 additions & 26 deletions layers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,10 @@ func (r *layerStore) saveMounts() error {
return r.loadMounts()
}

func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Driver) (LayerStore, error) {
// newLayerStore creates a new layer storage based on the provided directories
// and selected driver. The `withMountsLock` bool indicates if we want to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really withMountsLock or readWrite? The latter seems semantically more useful, and might e.g. allow skipping the MkdirAll calls below.

// enable mount locking, which is necessary to have write access to the storage.
func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Driver, withMountsLock bool) (LayerStore, error) {
saschagrunert marked this conversation as resolved.
Show resolved Hide resolved
if err := os.MkdirAll(rundir, 0700); err != nil {
return nil, err
}
Expand All @@ -502,9 +505,12 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri
if err != nil {
return nil, err
}
mountsLockfile, err := GetLockfile(filepath.Join(rundir, "mountpoints.lock"))
if err != nil {
return nil, err
var mountsLockfile Locker
if withMountsLock {
mountsLockfile, err = GetLockfile(filepath.Join(rundir, "mountpoints.lock"))
if err != nil {
return nil, err
}
}
rlstore := layerStore{
lockfile: lockfile,
Expand All @@ -524,27 +530,6 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri
return &rlstore, nil
}

func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (ROLayerStore, error) {
lockfile, err := GetROLockfile(filepath.Join(layerdir, "layers.lock"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

newLayerStore as a replacement for this will always create a RW lockfile. Is that desirable? It changes semantics of r.lockfile.RecursiveLock == r.RecursiveLock, which seems worrisome.

OTOH

storage/pkg/lockfile/lockfile.go

Lines 92 to 100 in 9f88983

if locker, ok := lockfiles[cleanPath]; ok {
if ro && locker.IsReadWrite() {
return nil, errors.Errorf("lock %q is not a read-only lock", cleanPath)
}
if !ro && !locker.IsReadWrite() {
return nil, errors.Errorf("lock %q is not a read-write lock", cleanPath)
}
return locker, nil
}
does not support having both kinds of lock for a single path.

if err != nil {
return nil, err
}
rlstore := layerStore{
lockfile: lockfile,
mountsLockfile: nil,
driver: driver,
rundir: rundir,
layerdir: layerdir,
byid: make(map[string]*Layer),
bymount: make(map[string]*Layer),
byname: make(map[string]*Layer),
}
if err := rlstore.Load(); err != nil {
return nil, err
}
return &rlstore, nil
}

func (r *layerStore) lookup(id string) (*Layer, bool) {
if layer, ok := r.byid[id]; ok {
return layer, ok
Expand Down Expand Up @@ -1470,7 +1455,8 @@ func (r *layerStore) Modified() (bool, error) {
}

func (r *layerStore) IsReadWrite() bool {
return r.lockfile.IsReadWrite()
// the mountsLockfile is only set for read-write enabled stores.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(This is preexisting = non-blocking:) This looks like a important invariant; it should be documented with the structure field.

return r.lockfile.IsReadWrite() && r.mountsLockfile != nil
saschagrunert marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

r.lockfile.IsReadWrite is always true now, AFAICS.

}

func (r *layerStore) TouchedSince(when time.Time) bool {
Expand Down
Loading