From a762222396d21990b2c0772300660312e7a58b6c Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 5 Dec 2016 15:39:05 -0800 Subject: [PATCH] volume: Use a map of maps for VolumeStore.refs The current implementation of getRefs is a bit fragile. It returns a slice to callers without copying its contents, and assumes the contents will not be modified elsewhere. Also, the current implementation of Dereference requires copying the slice of references, excluding the one we wish to remove. To improve both of these things, change refs to be a map of maps. Deleting an item becomes trivial, and returning a slice of references necessitates copying from the map. Signed-off-by: Aaron Lehmann --- volume/store/store.go | 68 ++++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/volume/store/store.go b/volume/store/store.go index 06c0260308..9b896703f6 100644 --- a/volume/store/store.go +++ b/volume/store/store.go @@ -67,7 +67,7 @@ func New(rootPath string) (*VolumeStore, error) { vs := &VolumeStore{ locks: &locker.Locker{}, names: make(map[string]volume.Volume), - refs: make(map[string][]string), + refs: make(map[string]map[string]struct{}), labels: make(map[string]map[string]string), options: make(map[string]map[string]string), } @@ -110,20 +110,39 @@ func (s *VolumeStore) getNamed(name string) (volume.Volume, bool) { } func (s *VolumeStore) setNamed(v volume.Volume, ref string) { + name := v.Name() + s.globalLock.Lock() - s.names[v.Name()] = v + s.names[name] = v if len(ref) > 0 { - s.refs[v.Name()] = append(s.refs[v.Name()], ref) + if s.refs[name] == nil { + s.refs[name] = make(map[string]struct{}) + } + s.refs[name][ref] = struct{}{} } s.globalLock.Unlock() } +// hasRef returns true if the given name has at least one ref. +// Callers of this function are expected to hold the name lock. +func (s *VolumeStore) hasRef(name string) bool { + s.globalLock.RLock() + l := len(s.refs[name]) + s.globalLock.RUnlock() + return l > 0 +} + // getRefs gets the list of refs for a given name // Callers of this function are expected to hold the name lock. func (s *VolumeStore) getRefs(name string) []string { s.globalLock.RLock() - refs := s.refs[name] - s.globalLock.RUnlock() + defer s.globalLock.RUnlock() + + refs := make([]string, 0, len(s.refs[name])) + for r := range s.refs[name] { + refs = append(refs, r) + } + return refs } @@ -149,7 +168,7 @@ type VolumeStore struct { // This is used for making lookups faster so we don't have to probe all drivers names map[string]volume.Volume // refs stores the volume name and the list of things referencing it - refs map[string][]string + refs map[string]map[string]struct{} // labels stores volume labels for each volume labels map[string]map[string]string // options stores volume options for each volume @@ -308,7 +327,7 @@ func (s *VolumeStore) checkConflict(name, driverName string) (volume.Volume, err return v, nil } - if len(s.getRefs(v.Name())) > 0 { + if s.hasRef(v.Name()) { // Containers are referencing this volume but it doesn't seem to exist anywhere. // Return a conflict error here, the user can fix this with `docker volume rm -f` return nil, errors.Wrapf(errNameConflict, "found references to volume '%s' in driver '%s' but the volume was not found in the driver -- you may need to remove containers referencing this volume or force remove the volume to re-create it", name, vDriverName) @@ -393,6 +412,7 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st s.globalLock.Lock() s.labels[name] = labels s.options[name] = opts + s.refs[name] = make(map[string]struct{}) s.globalLock.Unlock() if s.db != nil { @@ -529,9 +549,8 @@ func (s *VolumeStore) Remove(v volume.Volume) error { s.locks.Lock(name) defer s.locks.Unlock(name) - refs := s.getRefs(name) - if len(refs) > 0 { - return &OpErr{Err: errVolumeInUse, Name: v.Name(), Op: "remove", Refs: refs} + if s.hasRef(name) { + return &OpErr{Err: errVolumeInUse, Name: v.Name(), Op: "remove", Refs: s.getRefs(name)} } vd, err := volumedrivers.RemoveDriver(v.DriverName()) @@ -551,30 +570,27 @@ func (s *VolumeStore) Remove(v volume.Volume) error { // Dereference removes the specified reference to the volume func (s *VolumeStore) Dereference(v volume.Volume, ref string) { - s.locks.Lock(v.Name()) - defer s.locks.Unlock(v.Name()) + name := v.Name() + + s.locks.Lock(name) + defer s.locks.Unlock(name) s.globalLock.Lock() defer s.globalLock.Unlock() - var refs []string - for _, r := range s.refs[v.Name()] { - if r != ref { - refs = append(refs, r) - } + if s.refs[name] != nil { + delete(s.refs[name], ref) } - s.refs[v.Name()] = refs } // Refs gets the current list of refs for the given volume func (s *VolumeStore) Refs(v volume.Volume) []string { - s.locks.Lock(v.Name()) - defer s.locks.Unlock(v.Name()) + name := v.Name() - refs := s.getRefs(v.Name()) - refsOut := make([]string, len(refs)) - copy(refsOut, refs) - return refsOut + s.locks.Lock(name) + defer s.locks.Unlock(name) + + return s.getRefs(name) } // FilterByDriver returns the available volumes filtered by driver name @@ -605,9 +621,9 @@ func (s *VolumeStore) FilterByDriver(name string) ([]volume.Volume, error) { func (s *VolumeStore) FilterByUsed(vols []volume.Volume, used bool) []volume.Volume { return s.filter(vols, func(v volume.Volume) bool { s.locks.Lock(v.Name()) - l := len(s.getRefs(v.Name())) + hasRef := s.hasRef(v.Name()) s.locks.Unlock(v.Name()) - if (used && l > 0) || (!used && l == 0) { + if used == hasRef { return true } return false