From aad345e1f4a643b11e262203d0963aec4f9df5cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 15 Sep 2022 03:24:33 +0200 Subject: [PATCH 01/43] Introduce store.writeToLayerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to avoid the repetition of getting, locking, and reloading, the store. Use it at least for the simplest writers. This could be more elegant with Go generics, returning (T, error), with T possibly being void = struct{}. Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 173 +++++++++++++++++++++++++------------------------------ 1 file changed, 77 insertions(+), 96 deletions(-) diff --git a/store.go b/store.go index a9bbbf2e15..8dd891c0dc 100644 --- a/store.go +++ b/store.go @@ -965,6 +965,23 @@ func (s *store) allLayerStores() ([]roLayerStore, error) { return append([]roLayerStore{primary}, additional...), nil } +// writeToLayerStore is a helper for working with store.getLayerStore(): +// It locks the store for writing, checks for updates, and calls fn() +// It returns the return value of fn, or its own error initializing the store. +func (s *store) writeToLayerStore(fn func(store rwLayerStore) error) error { + store, err := s.getLayerStore() + if err != nil { + return err + } + + store.Lock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return err + } + return fn(store) +} + // getImageStore obtains and returns a handle to the writable image store object // used by the Store. func (s *store) getImageStore() (rwImageStore, error) { @@ -1712,17 +1729,9 @@ func (s *store) LayerBigData(id, key string) (io.ReadCloser, error) { // SetLayerBigData stores a (possibly large) chunk of named data // associated with a layer. func (s *store) SetLayerBigData(id, key string, data io.Reader) error { - store, err := s.getLayerStore() - if err != nil { - return err - } - - store.Lock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return err - } - return store.SetBigData(id, key, data) + return s.writeToLayerStore(func(store rwLayerStore) error { + return store.SetBigData(id, key, data) + }) } func (s *store) SetImageBigData(id, key string, data []byte, digestManifest func([]byte) (digest.Digest, error)) error { @@ -2069,16 +2078,12 @@ func (s *store) RemoveNames(id string, names []string) error { func (s *store) updateNames(id string, names []string, op updateNameOperation) error { deduped := dedupeNames(names) - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - if rlstore.Exists(id) { + layerFound := false + if err := s.writeToLayerStore(func(rlstore rwLayerStore) error { + if !rlstore.Exists(id) { + return nil + } + layerFound = true switch op { case setNames: return rlstore.SetNames(id, deduped) @@ -2089,6 +2094,8 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e default: return errInvalidUpdateNameOperation } + }); err != nil || layerFound { + return err } ristore, err := s.getImageStore() @@ -2794,19 +2801,16 @@ func (s *store) Unmount(id string, force bool) (bool, error) { if layerID, err := s.ContainerLayerID(id); err == nil { id = layerID } - rlstore, err := s.getLayerStore() - if err != nil { - return false, err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return false, err - } - if rlstore.Exists(id) { - return rlstore.Unmount(id, force) - } - return false, ErrLayerUnknown + var res bool + err := s.writeToLayerStore(func(rlstore rwLayerStore) error { + if rlstore.Exists(id) { + var err error + res, err = rlstore.Unmount(id, force) + return err + } + return ErrLayerUnknown + }) + return res, err } func (s *store) Changes(from, to string) ([]archive.Change, error) { @@ -2901,80 +2905,57 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro } func (s *store) ApplyDiffFromStagingDirectory(to, stagingDirectory string, diffOutput *drivers.DriverWithDifferOutput, options *drivers.ApplyDiffOpts) error { - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - if !rlstore.Exists(to) { - return ErrLayerUnknown - } - return rlstore.ApplyDiffFromStagingDirectory(to, stagingDirectory, diffOutput, options) + return s.writeToLayerStore(func(rlstore rwLayerStore) error { + if !rlstore.Exists(to) { + return ErrLayerUnknown + } + return rlstore.ApplyDiffFromStagingDirectory(to, stagingDirectory, diffOutput, options) + }) } func (s *store) CleanupStagingDirectory(stagingDirectory string) error { - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - return rlstore.CleanupStagingDirectory(stagingDirectory) + return s.writeToLayerStore(func(rlstore rwLayerStore) error { + return rlstore.CleanupStagingDirectory(stagingDirectory) + }) } func (s *store) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffOpts, differ drivers.Differ) (*drivers.DriverWithDifferOutput, error) { - rlstore, err := s.getLayerStore() - if err != nil { - return nil, err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return nil, err - } - if to != "" && !rlstore.Exists(to) { - return nil, ErrLayerUnknown - } - return rlstore.ApplyDiffWithDiffer(to, options, differ) + var res *drivers.DriverWithDifferOutput + err := s.writeToLayerStore(func(rlstore rwLayerStore) error { + if to != "" && !rlstore.Exists(to) { + return ErrLayerUnknown + } + var err error + res, err = rlstore.ApplyDiffWithDiffer(to, options, differ) + return err + }) + return res, err } func (s *store) DifferTarget(id string) (string, error) { - rlstore, err := s.getLayerStore() - if err != nil { - return "", err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return "", err - } - if rlstore.Exists(id) { - return rlstore.DifferTarget(id) - } - return "", ErrLayerUnknown + var res string + err := s.writeToLayerStore(func(rlstore rwLayerStore) error { + if rlstore.Exists(id) { + var err error + res, err = rlstore.DifferTarget(id) + return err + } + return ErrLayerUnknown + }) + return res, err } func (s *store) ApplyDiff(to string, diff io.Reader) (int64, error) { - rlstore, err := s.getLayerStore() - if err != nil { - return -1, err - } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return -1, err - } - if rlstore.Exists(to) { - return rlstore.ApplyDiff(to, diff) - } - return -1, ErrLayerUnknown + var res int64 = -1 + err := s.writeToLayerStore(func(rlstore rwLayerStore) error { + if rlstore.Exists(to) { + var err error + res, err = rlstore.ApplyDiff(to, diff) + return err + } + return ErrLayerUnknown + }) + return res, err } func (s *store) layersByMappedDigest(m func(roLayerStore, digest.Digest) ([]Layer, error), d digest.Digest) ([]Layer, error) { From d31017c7b5910aad1c10c9ca3b8fc5341962885a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 10 Oct 2022 20:45:38 +0200 Subject: [PATCH 02/43] Introduce store.writeToContainerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to avoid the repetition of getting, locking, and reloading, the store. Use it at least for the simplest writers. This could be more elegant with Go generic returning (T, error), with T possibly being void = struct{}. Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 92 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 49 insertions(+), 43 deletions(-) diff --git a/store.go b/store.go index 8dd891c0dc..99be21be61 100644 --- a/store.go +++ b/store.go @@ -1024,6 +1024,23 @@ func (s *store) getContainerStore() (rwContainerStore, error) { return nil, ErrLoadError } +// writeToContainerStore is a convenience helper for working with store.getContainerStore(): +// It locks the store for writing, checks for updates, and calls fn() +// It returns the return value of fn, or its own error initializing the store. +func (s *store) writeToContainerStore(fn func(store rwContainerStore) error) error { + store, err := s.getContainerStore() + if err != nil { + return err + } + + store.Lock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return err + } + return fn(store) +} + func (s *store) canUseShifting(uidmap, gidmap []idtools.IDMap) bool { if s.graphDriver == nil || !s.graphDriver.SupportsShifting() { return false @@ -1462,31 +1479,28 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat return nil, err } layer = clayer.ID - rcstore, err := s.getContainerStore() - if err != nil { - return nil, err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return nil, err - } - options.IDMappingOptions = types.IDMappingOptions{ - HostUIDMapping: len(options.UIDMap) == 0, - HostGIDMapping: len(options.GIDMap) == 0, - UIDMap: copyIDMap(options.UIDMap), - GIDMap: copyIDMap(options.GIDMap), - } - container, err := rcstore.Create(id, names, imageID, layer, metadata, options) - if err != nil || container == nil { - if err2 := rlstore.Delete(layer); err2 != nil { - if err == nil { - err = fmt.Errorf("deleting layer %#v: %w", layer, err2) - } else { - logrus.Errorf("While recovering from a failure to create a container, error deleting layer %#v: %v", layer, err2) + + var container *Container + err = s.writeToContainerStore(func(rcstore rwContainerStore) error { + options.IDMappingOptions = types.IDMappingOptions{ + HostUIDMapping: len(options.UIDMap) == 0, + HostGIDMapping: len(options.GIDMap) == 0, + UIDMap: copyIDMap(options.UIDMap), + GIDMap: copyIDMap(options.GIDMap), + } + var err error + container, err = rcstore.Create(id, names, imageID, layer, metadata, options) + if err != nil || container == nil { + if err2 := rlstore.Delete(layer); err2 != nil { + if err == nil { + err = fmt.Errorf("deleting layer %#v: %w", layer, err2) + } else { + logrus.Errorf("While recovering from a failure to create a container, error deleting layer %#v: %v", layer, err2) + } } } - } + return err + }) return container, err } @@ -1989,16 +2003,9 @@ func (s *store) ContainerBigData(id, key string) ([]byte, error) { } func (s *store) SetContainerBigData(id, key string, data []byte) error { - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - return rcstore.SetBigData(id, key, data) + return s.writeToContainerStore(func(rcstore rwContainerStore) error { + return rcstore.SetBigData(id, key, data) + }) } func (s *store) Exists(id string) bool { @@ -2145,16 +2152,12 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e } } - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - if rcstore.Exists(id) { + containerFound := false + if err := s.writeToContainerStore(func(rcstore rwContainerStore) error { + if !rcstore.Exists(id) { + return nil + } + containerFound = true switch op { case setNames: return rcstore.SetNames(id, deduped) @@ -2165,7 +2168,10 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e default: return errInvalidUpdateNameOperation } + }); err != nil || containerFound { + return err } + return ErrLayerUnknown } From 8aec5e23849b14e0b7850816b0f0532154059fa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 10 Oct 2022 20:50:30 +0200 Subject: [PATCH 03/43] Introduce store.writeToImageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to avoid the repetition of getting, locking, and reloading, the store. Use it at least for the simplest writers. This could be more elegant with Go generics, returning (T, error), with T possibly being void = struct{}. Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 58 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/store.go b/store.go index 99be21be61..dd0129d5d2 100644 --- a/store.go +++ b/store.go @@ -1015,6 +1015,23 @@ func (s *store) allImageStores() ([]roImageStore, error) { return append([]roImageStore{primary}, additional...), nil } +// writeToImageStore is a convenience helper for working with store.getImageStore(): +// It locks the store for writing, checks for updates, and calls fn() +// It returns the return value of fn, or its own error initializing the store. +func (s *store) writeToImageStore(fn func(store rwImageStore) error) error { + store, err := s.getImageStore() + if err != nil { + return err + } + + store.Lock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return err + } + return fn(store) +} + // getContainerStore obtains and returns a handle to the container store object // used by the Store. func (s *store) getContainerStore() (rwContainerStore, error) { @@ -1189,22 +1206,18 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, o layer = ilayer.ID } - ristore, err := s.getImageStore() - if err != nil { - return nil, err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return nil, err - } - - creationDate := time.Now().UTC() - if options != nil && !options.CreationDate.IsZero() { - creationDate = options.CreationDate - } + var res *Image + err := s.writeToImageStore(func(ristore rwImageStore) error { + creationDate := time.Now().UTC() + if options != nil && !options.CreationDate.IsZero() { + creationDate = options.CreationDate + } - return ristore.Create(id, names, layer, metadata, creationDate, options.Digest) + var err error + res, err = ristore.Create(id, names, layer, metadata, creationDate, options.Digest) + return err + }) + return res, err } func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, createMappedLayer bool, rlstore rwLayerStore, lstores []roLayerStore, options types.IDMappingOptions) (*Layer, error) { @@ -1749,18 +1762,9 @@ func (s *store) SetLayerBigData(id, key string, data io.Reader) error { } func (s *store) SetImageBigData(id, key string, data []byte, digestManifest func([]byte) (digest.Digest, error)) error { - ristore, err := s.getImageStore() - if err != nil { - return err - } - - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return err - } - - return ristore.SetBigData(id, key, data, digestManifest) + return s.writeToImageStore(func(ristore rwImageStore) error { + return ristore.SetBigData(id, key, data, digestManifest) + }) } func (s *store) ImageSize(id string) (int64, error) { From 66f31da5e25b674531c1c8b00e731cbb4e2fb4c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 10 Oct 2022 21:05:14 +0200 Subject: [PATCH 04/43] Introduce store.writeToAllStores MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to avoid the repetition of getting, locking, and reloading, the stores. This also makes it less error-prone to use a consistent locking order (but note that other instances of manual locking still exist). Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 666 +++++++++++++++++++++++-------------------------------- 1 file changed, 272 insertions(+), 394 deletions(-) diff --git a/store.go b/store.go index dd0129d5d2..4700ebacd4 100644 --- a/store.go +++ b/store.go @@ -1058,6 +1058,42 @@ func (s *store) writeToContainerStore(fn func(store rwContainerStore) error) err return fn(store) } +// writeToAllStores is a convenience helper for writing to all three stores: +// It locks the stores for writing, checks for updates, and calls fn(). +// It returns the return value of fn, or its own error initializing the stores. +func (s *store) writeToAllStores(fn func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error) error { + rlstore, err := s.getLayerStore() + if err != nil { + return err + } + ristore, err := s.getImageStore() + if err != nil { + return err + } + rcstore, err := s.getContainerStore() + if err != nil { + return err + } + + rlstore.Lock() + defer rlstore.Unlock() + if err := rlstore.ReloadIfChanged(); err != nil { + return err + } + ristore.Lock() + defer ristore.Unlock() + if err := ristore.ReloadIfChanged(); err != nil { + return err + } + rcstore.Lock() + defer rcstore.Unlock() + if err := rcstore.ReloadIfChanged(); err != nil { + return err + } + + return fn(rlstore, ristore, rcstore) +} + func (s *store) canUseShifting(uidmap, gidmap []idtools.IDMap) bool { if s.graphDriver == nil || !s.graphDriver.SupportsShifting() { return false @@ -1518,45 +1554,18 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat } func (s *store) SetMetadata(id, metadata string) error { - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - ristore, err := s.getImageStore() - if err != nil { - return err - } - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - - if rlstore.Exists(id) { - return rlstore.SetMetadata(id, metadata) - } - if ristore.Exists(id) { - return ristore.SetMetadata(id, metadata) - } - if rcstore.Exists(id) { - return rcstore.SetMetadata(id, metadata) - } - return ErrNotAnID + return s.writeToAllStores(func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error { + if rlstore.Exists(id) { + return rlstore.SetMetadata(id, metadata) + } + if ristore.Exists(id) { + return ristore.SetMetadata(id, metadata) + } + if rcstore.Exists(id) { + return rcstore.SetMetadata(id, metadata) + } + return ErrNotAnID + }) } func (s *store) Metadata(id string) (string, error) { @@ -2277,417 +2286,286 @@ func (s *store) Lookup(name string) (string, error) { } func (s *store) DeleteLayer(id string) error { - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - ristore, err := s.getImageStore() - if err != nil { - return err - } - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - - if rlstore.Exists(id) { - if l, err := rlstore.Get(id); err != nil { - id = l.ID - } - layers, err := rlstore.Layers() - if err != nil { - return err - } - for _, layer := range layers { - if layer.Parent == id { - return fmt.Errorf("used by layer %v: %w", layer.ID, ErrLayerHasChildren) + return s.writeToAllStores(func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error { + if rlstore.Exists(id) { + if l, err := rlstore.Get(id); err != nil { + id = l.ID } - } - images, err := ristore.Images() - if err != nil { - return err - } - - for _, image := range images { - if image.TopLayer == id { - return fmt.Errorf("layer %v used by image %v: %w", id, image.ID, ErrLayerUsedByImage) + layers, err := rlstore.Layers() + if err != nil { + return err } - if stringutils.InSlice(image.MappedTopLayers, id) { - // No write access to the image store, fail before the layer is deleted - if _, ok := ristore.(*imageStore); !ok { - return fmt.Errorf("layer %v used by image %v: %w", id, image.ID, ErrLayerUsedByImage) + for _, layer := range layers { + if layer.Parent == id { + return fmt.Errorf("used by layer %v: %w", layer.ID, ErrLayerHasChildren) } } - } - containers, err := rcstore.Containers() - if err != nil { - return err - } - for _, container := range containers { - if container.LayerID == id { - return fmt.Errorf("layer %v used by container %v: %w", id, container.ID, ErrLayerUsedByContainer) + images, err := ristore.Images() + if err != nil { + return err } - } - if err := rlstore.Delete(id); err != nil { - return fmt.Errorf("delete layer %v: %w", id, err) - } - // The check here is used to avoid iterating the images if we don't need to. - // There is already a check above for the imageStore to be writeable when the layer is part of MappedTopLayers. - if istore, ok := ristore.(*imageStore); ok { for _, image := range images { + if image.TopLayer == id { + return fmt.Errorf("layer %v used by image %v: %w", id, image.ID, ErrLayerUsedByImage) + } if stringutils.InSlice(image.MappedTopLayers, id) { - if err = istore.removeMappedTopLayer(image.ID, id); err != nil { - return fmt.Errorf("remove mapped top layer %v from image %v: %w", id, image.ID, err) + // No write access to the image store, fail before the layer is deleted + if _, ok := ristore.(*imageStore); !ok { + return fmt.Errorf("layer %v used by image %v: %w", id, image.ID, ErrLayerUsedByImage) + } + } + } + containers, err := rcstore.Containers() + if err != nil { + return err + } + for _, container := range containers { + if container.LayerID == id { + return fmt.Errorf("layer %v used by container %v: %w", id, container.ID, ErrLayerUsedByContainer) + } + } + if err := rlstore.Delete(id); err != nil { + return fmt.Errorf("delete layer %v: %w", id, err) + } + + // The check here is used to avoid iterating the images if we don't need to. + // There is already a check above for the imageStore to be writeable when the layer is part of MappedTopLayers. + if istore, ok := ristore.(*imageStore); ok { + for _, image := range images { + if stringutils.InSlice(image.MappedTopLayers, id) { + if err = istore.removeMappedTopLayer(image.ID, id); err != nil { + return fmt.Errorf("remove mapped top layer %v from image %v: %w", id, image.ID, err) + } } } } + return nil } - return nil - } - return ErrNotALayer + return ErrNotALayer + }) } func (s *store) DeleteImage(id string, commit bool) (layers []string, err error) { - rlstore, err := s.getLayerStore() - if err != nil { - return nil, err - } - ristore, err := s.getImageStore() - if err != nil { - return nil, err - } - rcstore, err := s.getContainerStore() - if err != nil { - return nil, err - } - - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return nil, err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return nil, err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return nil, err - } layersToRemove := []string{} - if ristore.Exists(id) { - image, err := ristore.Get(id) - if err != nil { - return nil, err - } - id = image.ID - containers, err := rcstore.Containers() - if err != nil { - return nil, err - } - aContainerByImage := make(map[string]string) - for _, container := range containers { - aContainerByImage[container.ImageID] = container.ID - } - if container, ok := aContainerByImage[id]; ok { - return nil, fmt.Errorf("image used by %v: %w", container, ErrImageUsedByContainer) - } - images, err := ristore.Images() - if err != nil { - return nil, err - } - layers, err := rlstore.Layers() - if err != nil { - return nil, err - } - childrenByParent := make(map[string][]string) - for _, layer := range layers { - childrenByParent[layer.Parent] = append(childrenByParent[layer.Parent], layer.ID) - } - otherImagesTopLayers := make(map[string]struct{}) - for _, img := range images { - if img.ID != id { - otherImagesTopLayers[img.TopLayer] = struct{}{} - for _, layerID := range img.MappedTopLayers { - otherImagesTopLayers[layerID] = struct{}{} - } + if err := s.writeToAllStores(func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error { + if ristore.Exists(id) { + image, err := ristore.Get(id) + if err != nil { + return err } - } - if commit { - if err = ristore.Delete(id); err != nil { - return nil, err + id = image.ID + containers, err := rcstore.Containers() + if err != nil { + return err } - } - layer := image.TopLayer - layersToRemoveMap := make(map[string]struct{}) - layersToRemove = append(layersToRemove, image.MappedTopLayers...) - for _, mappedTopLayer := range image.MappedTopLayers { - layersToRemoveMap[mappedTopLayer] = struct{}{} - } - for layer != "" { - if rcstore.Exists(layer) { - break + aContainerByImage := make(map[string]string) + for _, container := range containers { + aContainerByImage[container.ImageID] = container.ID } - if _, used := otherImagesTopLayers[layer]; used { - break + if container, ok := aContainerByImage[id]; ok { + return fmt.Errorf("image used by %v: %w", container, ErrImageUsedByContainer) + } + images, err := ristore.Images() + if err != nil { + return err + } + layers, err := rlstore.Layers() + if err != nil { + return err + } + childrenByParent := make(map[string][]string) + for _, layer := range layers { + childrenByParent[layer.Parent] = append(childrenByParent[layer.Parent], layer.ID) + } + otherImagesTopLayers := make(map[string]struct{}) + for _, img := range images { + if img.ID != id { + otherImagesTopLayers[img.TopLayer] = struct{}{} + for _, layerID := range img.MappedTopLayers { + otherImagesTopLayers[layerID] = struct{}{} + } + } + } + if commit { + if err = ristore.Delete(id); err != nil { + return err + } } - parent := "" - if l, err := rlstore.Get(layer); err == nil { - parent = l.Parent + layer := image.TopLayer + layersToRemoveMap := make(map[string]struct{}) + layersToRemove = append(layersToRemove, image.MappedTopLayers...) + for _, mappedTopLayer := range image.MappedTopLayers { + layersToRemoveMap[mappedTopLayer] = struct{}{} } - hasChildrenNotBeingRemoved := func() bool { - layersToCheck := []string{layer} - if layer == image.TopLayer { - layersToCheck = append(layersToCheck, image.MappedTopLayers...) + for layer != "" { + if rcstore.Exists(layer) { + break + } + if _, used := otherImagesTopLayers[layer]; used { + break + } + parent := "" + if l, err := rlstore.Get(layer); err == nil { + parent = l.Parent } - for _, layer := range layersToCheck { - if childList := childrenByParent[layer]; len(childList) > 0 { - for _, child := range childList { - if _, childIsSlatedForRemoval := layersToRemoveMap[child]; childIsSlatedForRemoval { - continue + hasChildrenNotBeingRemoved := func() bool { + layersToCheck := []string{layer} + if layer == image.TopLayer { + layersToCheck = append(layersToCheck, image.MappedTopLayers...) + } + for _, layer := range layersToCheck { + if childList := childrenByParent[layer]; len(childList) > 0 { + for _, child := range childList { + if _, childIsSlatedForRemoval := layersToRemoveMap[child]; childIsSlatedForRemoval { + continue + } + return true } - return true } } + return false } - return false - } - if hasChildrenNotBeingRemoved() { - break + if hasChildrenNotBeingRemoved() { + break + } + layersToRemove = append(layersToRemove, layer) + layersToRemoveMap[layer] = struct{}{} + layer = parent } - layersToRemove = append(layersToRemove, layer) - layersToRemoveMap[layer] = struct{}{} - layer = parent + } else { + return ErrNotAnImage } - } else { - return nil, ErrNotAnImage - } - if commit { - for _, layer := range layersToRemove { - if err = rlstore.Delete(layer); err != nil { - return nil, err + if commit { + for _, layer := range layersToRemove { + if err = rlstore.Delete(layer); err != nil { + return err + } } } + return nil + }); err != nil { + return nil, err } return layersToRemove, nil } func (s *store) DeleteContainer(id string) error { - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - ristore, err := s.getImageStore() - if err != nil { - return err - } - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - - if !rcstore.Exists(id) { - return ErrNotAContainer - } + return s.writeToAllStores(func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error { + if !rcstore.Exists(id) { + return ErrNotAContainer + } - container, err := rcstore.Get(id) - if err != nil { - return ErrNotAContainer - } + container, err := rcstore.Get(id) + if err != nil { + return ErrNotAContainer + } - errChan := make(chan error) - var wg sync.WaitGroup + errChan := make(chan error) + var wg sync.WaitGroup - if rlstore.Exists(container.LayerID) { + if rlstore.Exists(container.LayerID) { + wg.Add(1) + go func() { + errChan <- rlstore.Delete(container.LayerID) + wg.Done() + }() + } wg.Add(1) go func() { - errChan <- rlstore.Delete(container.LayerID) + errChan <- rcstore.Delete(id) wg.Done() }() - } - wg.Add(1) - go func() { - errChan <- rcstore.Delete(id) - wg.Done() - }() - middleDir := s.graphDriverName + "-containers" - gcpath := filepath.Join(s.GraphRoot(), middleDir, container.ID) - wg.Add(1) - go func() { - defer wg.Done() - // attempt a simple rm -rf first - err := os.RemoveAll(gcpath) - if err == nil { - errChan <- nil - return - } - // and if it fails get to the more complicated cleanup - errChan <- system.EnsureRemoveAll(gcpath) - }() + middleDir := s.graphDriverName + "-containers" + gcpath := filepath.Join(s.GraphRoot(), middleDir, container.ID) + wg.Add(1) + go func() { + defer wg.Done() + // attempt a simple rm -rf first + err := os.RemoveAll(gcpath) + if err == nil { + errChan <- nil + return + } + // and if it fails get to the more complicated cleanup + errChan <- system.EnsureRemoveAll(gcpath) + }() - rcpath := filepath.Join(s.RunRoot(), middleDir, container.ID) - wg.Add(1) - go func() { - defer wg.Done() - // attempt a simple rm -rf first - err := os.RemoveAll(rcpath) - if err == nil { - errChan <- nil - return - } - // and if it fails get to the more complicated cleanup - errChan <- system.EnsureRemoveAll(rcpath) - }() + rcpath := filepath.Join(s.RunRoot(), middleDir, container.ID) + wg.Add(1) + go func() { + defer wg.Done() + // attempt a simple rm -rf first + err := os.RemoveAll(rcpath) + if err == nil { + errChan <- nil + return + } + // and if it fails get to the more complicated cleanup + errChan <- system.EnsureRemoveAll(rcpath) + }() - go func() { - wg.Wait() - close(errChan) - }() + go func() { + wg.Wait() + close(errChan) + }() - var errors []error - for err := range errChan { - if err != nil { - errors = append(errors, err) + var errors []error + for err := range errChan { + if err != nil { + errors = append(errors, err) + } } - } - return multierror.Append(nil, errors...).ErrorOrNil() + return multierror.Append(nil, errors...).ErrorOrNil() + }) } func (s *store) Delete(id string) error { - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - ristore, err := s.getImageStore() - if err != nil { - return err - } - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - - if rcstore.Exists(id) { - if container, err := rcstore.Get(id); err == nil { - if rlstore.Exists(container.LayerID) { - if err = rlstore.Delete(container.LayerID); err != nil { - return err - } - if err = rcstore.Delete(id); err != nil { - return err - } - middleDir := s.graphDriverName + "-containers" - gcpath := filepath.Join(s.GraphRoot(), middleDir, container.ID, "userdata") - if err = os.RemoveAll(gcpath); err != nil { - return err - } - rcpath := filepath.Join(s.RunRoot(), middleDir, container.ID, "userdata") - if err = os.RemoveAll(rcpath); err != nil { - return err + return s.writeToAllStores(func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error { + if rcstore.Exists(id) { + if container, err := rcstore.Get(id); err == nil { + if rlstore.Exists(container.LayerID) { + if err = rlstore.Delete(container.LayerID); err != nil { + return err + } + if err = rcstore.Delete(id); err != nil { + return err + } + middleDir := s.graphDriverName + "-containers" + gcpath := filepath.Join(s.GraphRoot(), middleDir, container.ID, "userdata") + if err = os.RemoveAll(gcpath); err != nil { + return err + } + rcpath := filepath.Join(s.RunRoot(), middleDir, container.ID, "userdata") + if err = os.RemoveAll(rcpath); err != nil { + return err + } + return nil } - return nil + return ErrNotALayer } - return ErrNotALayer } - } - if ristore.Exists(id) { - return ristore.Delete(id) - } - if rlstore.Exists(id) { - return rlstore.Delete(id) - } - return ErrLayerUnknown + if ristore.Exists(id) { + return ristore.Delete(id) + } + if rlstore.Exists(id) { + return rlstore.Delete(id) + } + return ErrLayerUnknown + }) } func (s *store) Wipe() error { - rcstore, err := s.getContainerStore() - if err != nil { - return err - } - ristore, err := s.getImageStore() - if err != nil { - return err - } - rlstore, err := s.getLayerStore() - if err != nil { - return err - } - - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { - return err - } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return err - } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { - return err - } - - if err = rcstore.Wipe(); err != nil { - return err - } - if err = ristore.Wipe(); err != nil { - return err - } - return rlstore.Wipe() + return s.writeToAllStores(func(rlstore rwLayerStore, ristore rwImageStore, rcstore rwContainerStore) error { + if err := rcstore.Wipe(); err != nil { + return err + } + if err := ristore.Wipe(); err != nil { + return err + } + return rlstore.Wipe() + }) } func (s *store) Status() ([][2]string, error) { From 6eb9793be0185a3d0d57cb677f1bd1a80cb31d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 11 Oct 2022 23:32:34 +0200 Subject: [PATCH 05/43] Add readAllLayerStores MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to remove the copy&pasted iteration and copy&pasted locking/ ReloadIfChanged. That will also make it much easier to add more locking in the future. Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 252 ++++++++++++++++++++++++++----------------------------- 1 file changed, 118 insertions(+), 134 deletions(-) diff --git a/store.go b/store.go index a9bbbf2e15..0440d201ae 100644 --- a/store.go +++ b/store.go @@ -965,6 +965,44 @@ func (s *store) allLayerStores() ([]roLayerStore, error) { return append([]roLayerStore{primary}, additional...), nil } +// readAllLayerStores processes allLayerStores() in order: +// It locks the store for reading, checks for updates, and calls +// +// (done, err) := fn(store) +// +// until the callback returns done == true, and returns the data from the callback. +// +// If reading any layer store fails, it immediately returns (true, err). +// +// If all layer stores are processed without setting done == true, it returns (false, nil). +// +// Typical usage: +// +// var res T = failureValue +// if done, err := s.readAllLayerStores(store, func(…) { +// … +// }; done { +// return res, err +// } +func (s *store) readAllLayerStores(fn func(store roLayerStore) (bool, error)) (bool, error) { + layerStores, err := s.allLayerStores() + if err != nil { + return true, err + } + for _, s := range layerStores { + store := s + store.RLock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return true, err + } + if done, err := fn(store); done { + return true, err + } + } + return false, nil +} + // getImageStore obtains and returns a handle to the writable image store object // used by the Store. func (s *store) getImageStore() (rwImageStore, error) { @@ -1516,20 +1554,16 @@ func (s *store) SetMetadata(id, metadata string) error { } func (s *store) Metadata(id string) (string, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return "", err - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return "", err - } + var res string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(id) { - return store.Metadata(id) + var err error + res, err = store.Metadata(id) + return true, err } + return false, nil + }); done { + return res, err } imageStores, err := s.allImageStores() @@ -1654,25 +1688,20 @@ func (s *store) ImageBigData(id, key string) ([]byte, error) { // ListLayerBigData retrieves a list of the (possibly large) chunks of // named data associated with an layer. func (s *store) ListLayerBigData(id string) ([]string, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return nil, err - } foundLayer := false - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res []string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { data, err := store.BigDataNames(id) if err == nil { - return data, nil + res = data + return true, nil } if store.Exists(id) { foundLayer = true } + return false, nil + }); done { + return res, err } if foundLayer { return nil, fmt.Errorf("locating big data for layer with ID %q: %w", id, os.ErrNotExist) @@ -1683,25 +1712,20 @@ func (s *store) ListLayerBigData(id string) ([]string, error) { // LayerBigData retrieves a (possibly large) chunk of named data // associated with a layer. func (s *store) LayerBigData(id, key string) (io.ReadCloser, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return nil, err - } foundLayer := false - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res io.ReadCloser + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { data, err := store.BigData(id, key) if err == nil { - return data, nil + res = data + return true, nil } if store.Exists(id) { foundLayer = true } + return false, nil + }); done { + return res, err } if foundLayer { return nil, fmt.Errorf("locating item named %q for layer with ID %q: %w", key, id, os.ErrNotExist) @@ -1993,20 +2017,15 @@ func (s *store) SetContainerBigData(id, key string, data []byte) error { } func (s *store) Exists(id string) bool { - layerStores, err := s.allLayerStores() - if err != nil { - return false - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return false - } + var res = false + if done, _ := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(id) { - return true + res = true + return true, nil } + return false, nil + }); done { + return res } imageStores, err := s.allImageStores() @@ -2163,20 +2182,15 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e } func (s *store) Names(id string) ([]string, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return nil, err - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res []string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if l, err := store.Get(id); l != nil && err == nil { - return l.Names, nil + res = l.Names + return true, nil } + return false, nil + }); done { + return res, err } imageStores, err := s.allImageStores() @@ -2211,20 +2225,15 @@ func (s *store) Names(id string) ([]string, error) { } func (s *store) Lookup(name string) (string, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return "", err - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return "", err - } + var res string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if l, err := store.Get(name); l != nil && err == nil { - return l.ID, nil + res = l.ID + return true, nil } + return false, nil + }); done { + return res, err } imageStores, err := s.allImageStores() @@ -2810,40 +2819,31 @@ func (s *store) Unmount(id string, force bool) (bool, error) { } func (s *store) Changes(from, to string) ([]archive.Change, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return nil, err - } - - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res []archive.Change + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(to) { - return store.Changes(from, to) + var err error + res, err = store.Changes(from, to) + return true, err } + return false, nil + }); done { + return res, err } return nil, ErrLayerUnknown } func (s *store) DiffSize(from, to string) (int64, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return -1, err - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return -1, err - } + var res int64 = -1 + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(to) { - return store.DiffSize(from, to) + var err error + res, err = store.DiffSize(from, to) + return true, err } + return false, nil + }); done { + return res, err } return -1, ErrLayerUnknown } @@ -2978,26 +2978,19 @@ func (s *store) ApplyDiff(to string, diff io.Reader) (int64, error) { } func (s *store) layersByMappedDigest(m func(roLayerStore, digest.Digest) ([]Layer, error), d digest.Digest) ([]Layer, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return nil, err - } var layers []Layer - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + if _, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { storeLayers, err := m(store, d) if err != nil { if !errors.Is(err, ErrLayerUnknown) { - return nil, err + return true, err } - continue + return false, nil } layers = append(layers, storeLayers...) + return false, nil + }); err != nil { + return nil, err } if len(layers) == 0 { return nil, ErrLayerUnknown @@ -3020,20 +3013,16 @@ func (s *store) LayersByUncompressedDigest(d digest.Digest) ([]Layer, error) { } func (s *store) LayerSize(id string) (int64, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return -1, err - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return -1, err - } + var res int64 = -1 + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(id) { - return store.Size(id) + var err error + res, err = store.Size(id) + return true, err } + return false, nil + }); done { + return res, err } return -1, ErrLayerUnknown } @@ -3160,21 +3149,16 @@ func (s *store) Containers() ([]Container, error) { } func (s *store) Layer(id string) (*Layer, error) { - layerStores, err := s.allLayerStores() - if err != nil { - return nil, err - } - for _, s := range layerStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res *Layer + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { layer, err := store.Get(id) if err == nil { - return layer, nil + res = layer + return true, nil } + return false, nil + }); done { + return res, err } return nil, ErrLayerUnknown } From 958058a0f1cd66e77267e91fcbf3d0c6823b9628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 03:05:36 +0200 Subject: [PATCH 06/43] Add readAllImageStores MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to remove the copy&pasted iteration and copy&pasted locking/ ReloadIfChanged. That will also make it much easier to add more locking in the future. Should not change behavior. Signed-off-by: Miloslav Trmač --- store.go | 261 +++++++++++++++++++++++++------------------------------ 1 file changed, 117 insertions(+), 144 deletions(-) diff --git a/store.go b/store.go index 0440d201ae..3fd1de434d 100644 --- a/store.go +++ b/store.go @@ -1036,6 +1036,44 @@ func (s *store) allImageStores() ([]roImageStore, error) { return append([]roImageStore{primary}, additional...), nil } +// readAllImageStores processes allImageStores() in order: +// It locks the store for reading, checks for updates, and calls +// +// (done, err) := fn(store) +// +// until the callback returns done == true, and returns the data from the callback. +// +// If reading any Image store fails, it immediately returns (true, err). +// +// If all Image stores are processed without setting done == true, it returns (false, nil). +// +// Typical usage: +// +// var res T = failureValue +// if done, err := s.readAllImageStores(store, func(…) { +// … +// }; done { +// return res, err +// } +func (s *store) readAllImageStores(fn func(store roImageStore) (bool, error)) (bool, error) { + ImageStores, err := s.allImageStores() + if err != nil { + return true, err + } + for _, s := range ImageStores { + store := s + store.RLock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return true, err + } + if done, err := fn(store); done { + return true, err + } + } + return false, nil +} + // getContainerStore obtains and returns a handle to the container store object // used by the Store. func (s *store) getContainerStore() (rwContainerStore, error) { @@ -1555,6 +1593,7 @@ func (s *store) SetMetadata(id, metadata string) error { func (s *store) Metadata(id string) (string, error) { var res string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(id) { var err error @@ -1566,20 +1605,15 @@ func (s *store) Metadata(id string) (string, error) { return res, err } - imageStores, err := s.allImageStores() - if err != nil { - return "", err - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return "", err - } + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { if store.Exists(id) { - return store.Metadata(id) + var err error + res, err = store.Metadata(id) + return true, err } + return false, nil + }); done { + return res, err } cstore, err := s.getContainerStore() @@ -1598,86 +1632,65 @@ func (s *store) Metadata(id string) (string, error) { } func (s *store) ListImageBigData(id string) ([]string, error) { - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res []string + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { bigDataNames, err := store.BigDataNames(id) if err == nil { - return bigDataNames, err + res = bigDataNames + return true, nil } + return false, nil + }); done { + return res, err } return nil, fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } func (s *store) ImageBigDataSize(id, key string) (int64, error) { - imageStores, err := s.allImageStores() - if err != nil { - return -1, err - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return -1, err - } + var res int64 = -1 + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { size, err := store.BigDataSize(id, key) if err == nil { - return size, nil + res = size + return true, nil } + return false, nil + }); done { + return res, err } return -1, ErrSizeUnknown } func (s *store) ImageBigDataDigest(id, key string) (digest.Digest, error) { - imageStores, err := s.allImageStores() - if err != nil { - return "", err - } - for _, r := range imageStores { - ristore := r - ristore.RLock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { - return "", err - } + var res digest.Digest + if done, err := s.readAllImageStores(func(ristore roImageStore) (bool, error) { d, err := ristore.BigDataDigest(id, key) if err == nil && d.Validate() == nil { - return d, nil + res = d + return true, nil } + return false, nil + }); done { + return res, err } return "", ErrDigestUnknown } func (s *store) ImageBigData(id, key string) ([]byte, error) { - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } - foundImage := false - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res []byte + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { data, err := store.BigData(id, key) if err == nil { - return data, nil + res = data + return true, nil } if store.Exists(id) { foundImage = true } + return false, nil + }); done { + return res, err } if foundImage { return nil, fmt.Errorf("locating item named %q for image with ID %q (consider removing the image to resolve the issue): %w", key, id, os.ErrNotExist) @@ -2018,6 +2031,7 @@ func (s *store) SetContainerBigData(id, key string, data []byte) error { func (s *store) Exists(id string) bool { var res = false + if done, _ := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if store.Exists(id) { res = true @@ -2028,20 +2042,14 @@ func (s *store) Exists(id string) bool { return res } - imageStores, err := s.allImageStores() - if err != nil { - return false - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return false - } + if done, _ := s.readAllImageStores(func(store roImageStore) (bool, error) { if store.Exists(id) { - return true + res = true + return true, nil } + return false, nil + }); done { + return res } rcstore, err := s.getContainerStore() @@ -2183,6 +2191,7 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e func (s *store) Names(id string) ([]string, error) { var res []string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if l, err := store.Get(id); l != nil && err == nil { res = l.Names @@ -2193,20 +2202,14 @@ func (s *store) Names(id string) ([]string, error) { return res, err } - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { if i, err := store.Get(id); i != nil && err == nil { - return i.Names, nil + res = i.Names + return true, nil } + return false, nil + }); done { + return res, err } rcstore, err := s.getContainerStore() @@ -2226,6 +2229,7 @@ func (s *store) Names(id string) ([]string, error) { func (s *store) Lookup(name string) (string, error) { var res string + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { if l, err := store.Get(name); l != nil && err == nil { res = l.ID @@ -2236,20 +2240,14 @@ func (s *store) Lookup(name string) (string, error) { return res, err } - imageStores, err := s.allImageStores() - if err != nil { - return "", err - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return "", err - } + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { if i, err := store.Get(name); i != nil && err == nil { - return i.ID, nil + res = i.ID + return true, nil } + return false, nil + }); done { + return res, err } cstore, err := s.getContainerStore() @@ -3112,23 +3110,16 @@ func (s *store) Layers() ([]Layer, error) { } func (s *store) Images() ([]Image, error) { - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } var images []Image - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + if _, err := s.readAllImageStores(func(store roImageStore) (bool, error) { storeImages, err := store.Images() if err != nil { - return nil, err + return true, err } images = append(images, storeImages...) + return false, nil + }); err != nil { + return nil, err } return images, nil } @@ -3245,21 +3236,16 @@ func (al *additionalLayer) Release() { } func (s *store) Image(id string) (*Image, error) { - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var res *Image + if done, err := s.readAllImageStores(func(store roImageStore) (bool, error) { image, err := store.Get(id) if err == nil { - return image, nil + res = image + return true, nil } + return false, nil + }); done { + return res, err } return nil, fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } @@ -3270,48 +3256,35 @@ func (s *store) ImagesByTopLayer(id string) ([]*Image, error) { return nil, err } - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } images := []*Image{} - for _, s := range imageStores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + if _, err := s.readAllImageStores(func(store roImageStore) (bool, error) { imageList, err := store.Images() if err != nil { - return nil, err + return true, err } for _, image := range imageList { if image.TopLayer == layer.ID || stringutils.InSlice(image.MappedTopLayers, layer.ID) { images = append(images, &image) } } + return false, nil + }); err != nil { + return nil, err } return images, nil } func (s *store) ImagesByDigest(d digest.Digest) ([]*Image, error) { - imageStores, err := s.allImageStores() - if err != nil { - return nil, err - } images := []*Image{} - for _, store := range imageStores { - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + if _, err := s.readAllImageStores(func(store roImageStore) (bool, error) { imageList, err := store.ByDigest(d) if err != nil && !errors.Is(err, ErrImageUnknown) { - return nil, err + return true, err } images = append(images, imageList...) + return false, nil + }); err != nil { + return nil, err } return images, nil } From 90a0dd43ce929498896245431386930959d47417 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:07:47 +0200 Subject: [PATCH 07/43] Remove a manual layerStore.Touch() call from store.Shutdown() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's completely unclear why this is necessary; the unmount calls actually write to the separate mountpoints.json file, with a separate lock. When this was originally introduced in b046fb5a9a1ad6d25bd6c854400f090131c8a21c , there wasn't a separate mountpoints.json file, and store.go was manually calling Touch() everywhere; so my best guess is that this is just an artifact of making those two API changes, and the Touch() is not actually necessary. Signed-off-by: Miloslav Trmač --- store.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/store.go b/store.go index e4990689e7..e55d8cd306 100644 --- a/store.go +++ b/store.go @@ -3307,7 +3307,6 @@ func (s *store) FromContainerRunDirectory(id, file string) ([]byte, error) { func (s *store) Shutdown(force bool) ([]string, error) { mounted := []string{} - modified := false rlstore, err := s.getLayerStore() if err != nil { @@ -3341,7 +3340,6 @@ func (s *store) Shutdown(force bool) ([]string, error) { } break } - modified = true } } } @@ -3357,16 +3355,6 @@ func (s *store) Shutdown(force bool) ([]string, error) { err = fmt.Errorf("(graphLock.Touch failed: %v) %w", err2, err) } } - modified = true - } - if modified { - if err2 := rlstore.Touch(); err2 != nil { - if err == nil { - err = err2 - } else { - err = fmt.Errorf("rlstore.Touch failed: %v) %w", err2, err) - } - } } return mounted, err } From 2eb434645a32b115b41071e4a0df9033cfc19b91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 22:30:10 +0200 Subject: [PATCH 08/43] Copy methods from included interfaces directly into rwContainerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we can modify/replace them. Signed-off-by: Miloslav Trmač --- containers.go | 44 +++++++++++++++++++++++++++++++++++++++++++- store.go | 7 ------- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/containers.go b/containers.go index 9d8b9fa038..627252aa86 100644 --- a/containers.go +++ b/containers.go @@ -68,11 +68,53 @@ type Container struct { // rwContainerStore provides bookkeeping for information about Containers. type rwContainerStore interface { - fileBasedStore metadataStore containerBigDataStore flaggableStore + // Acquire a writer lock. + // The default unix implementation panics if: + // - opening the lockfile failed + // - tried to lock a read-only lock-file + Lock() + + // Unlock the lock. + // The default unix implementation panics if: + // - unlocking an unlocked lock + // - if the lock counter is corrupted + Unlock() + + // Acquire a reader lock. + RLock() + + // Touch records, for others sharing the lock, that the caller was the + // last writer. It should only be called with the lock held. + Touch() error + + // Modified() checks if the most recent writer was a party other than the + // last recorded writer. It should only be called with the lock held. + Modified() (bool, error) + + // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. + TouchedSince(when time.Time) bool + + // IsReadWrite() checks if the lock file is read-write + IsReadWrite() bool + + // Locked() checks if lock is locked for writing by a thread in this process + Locked() bool + // Load reloads the contents of the store from disk. It should be called + // with the lock held. + Load() error + + // ReloadIfChanged reloads the contents of the store from disk if it is changed. + ReloadIfChanged() error + + // Save saves the contents of the store to disk. It should be called with + // the lock held, and Touch() should be called afterward before releasing the + // lock. + Save() error + // Create creates a container that has a specified ID (or generates a // random one if an empty value is supplied) and optional names, // based on the specified image, using the specified layer as its diff --git a/store.go b/store.go index e55d8cd306..95f18e07a5 100644 --- a/store.go +++ b/store.go @@ -73,13 +73,6 @@ type rwFileBasedStore interface { Save() error } -// fileBasedStore wraps up the common methods of various types of file-based -// data stores that we implement. -type fileBasedStore interface { - roFileBasedStore - rwFileBasedStore -} - // roMetadataStore wraps a method for reading metadata associated with an ID. type roMetadataStore interface { // Metadata reads metadata associated with an item with the specified ID. From 484fa71f2e783a0e5fca6656813921adeb10ec30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 22:38:00 +0200 Subject: [PATCH 09/43] Replace containerStore.{RLock,Unlock} with {startReading,stopReading} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This integrates ReloadIfChanges, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Signed-off-by: Miloslav Trmač --- containers.go | 36 +++++++++++++++++++---- store.go | 80 +++++++++++++++++++++------------------------------ 2 files changed, 62 insertions(+), 54 deletions(-) diff --git a/containers.go b/containers.go index 627252aa86..1b66f1a60e 100644 --- a/containers.go +++ b/containers.go @@ -84,8 +84,12 @@ type rwContainerStore interface { // - if the lock counter is corrupted Unlock() - // Acquire a reader lock. - RLock() + // startReading makes sure the store is fresh, and locks it for reading. + // If this succeeds, the caller MUST call stopReading(). + startReading() error + + // stopReading releases locks obtained by startReading. + stopReading() // Touch records, for others sharing the lock, that the caller was the // last writer. It should only be called with the lock held. @@ -215,6 +219,30 @@ func (c *Container) MountOpts() []string { } } +// startReading makes sure the store is fresh, and locks it for reading. +// If this succeeds, the caller MUST call stopReading(). +func (r *containerStore) startReading() error { + r.lockfile.RLock() + succeeded := false + defer func() { + if !succeeded { + r.lockfile.Unlock() + } + }() + + if err := r.ReloadIfChanged(); err != nil { + return err + } + + succeeded = true + return nil +} + +// stopReading releases locks obtained by startReading. +func (r *containerStore) stopReading() { + r.lockfile.Unlock() +} + func (r *containerStore) Containers() ([]Container, error) { containers := make([]Container, len(r.containers)) for i := range r.containers { @@ -665,10 +693,6 @@ func (r *containerStore) Lock() { r.lockfile.Lock() } -func (r *containerStore) RLock() { - r.lockfile.RLock() -} - func (r *containerStore) Unlock() { r.lockfile.Unlock() } diff --git a/store.go b/store.go index 95f18e07a5..6a41d3bdf1 100644 --- a/store.go +++ b/store.go @@ -1666,11 +1666,10 @@ func (s *store) Metadata(id string) (string, error) { if err != nil { return "", err } - cstore.RLock() - defer cstore.Unlock() - if err := cstore.ReloadIfChanged(); err != nil { + if err := cstore.startReading(); err != nil { return "", err } + defer cstore.stopReading() if cstore.Exists(id) { return cstore.Metadata(id) } @@ -1935,11 +1934,10 @@ func (s *store) ContainerSize(id string) (int64, error) { if err != nil { return -1, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return -1, err } + defer rcstore.stopReading() // Read the container record. container, err := rcstore.Get(id) @@ -1997,11 +1995,10 @@ func (s *store) ListContainerBigData(id string) ([]string, error) { return nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, err } + defer rcstore.stopReading() return rcstore.BigDataNames(id) } @@ -2011,11 +2008,10 @@ func (s *store) ContainerBigDataSize(id, key string) (int64, error) { if err != nil { return -1, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return -1, err } + defer rcstore.stopReading() return rcstore.BigDataSize(id, key) } @@ -2024,11 +2020,10 @@ func (s *store) ContainerBigDataDigest(id, key string) (digest.Digest, error) { if err != nil { return "", err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return "", err } + defer rcstore.stopReading() return rcstore.BigDataDigest(id, key) } @@ -2037,11 +2032,10 @@ func (s *store) ContainerBigData(id, key string) ([]byte, error) { if err != nil { return nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, err } + defer rcstore.stopReading() return rcstore.BigData(id, key) } @@ -2078,11 +2072,10 @@ func (s *store) Exists(id string) bool { if err != nil { return false } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return false } + defer rcstore.stopReading() if rcstore.Exists(id) { return true } @@ -2235,11 +2228,10 @@ func (s *store) Names(id string) ([]string, error) { if err != nil { return nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, err } + defer rcstore.stopReading() if c, err := rcstore.Get(id); c != nil && err == nil { return c.Names, nil } @@ -2273,11 +2265,10 @@ func (s *store) Lookup(name string) (string, error) { if err != nil { return "", err } - cstore.RLock() - defer cstore.Unlock() - if err := cstore.ReloadIfChanged(); err != nil { + if err := cstore.startReading(); err != nil { return "", err } + defer cstore.stopReading() if c, err := cstore.Get(name); c != nil && err == nil { return c.ID, nil } @@ -2917,11 +2908,10 @@ func (s *store) ContainerParentOwners(id string) ([]int, []int, error) { if err := rlstore.ReloadIfChanged(); err != nil { return nil, nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, nil, err } + defer rcstore.stopReading() container, err := rcstore.Get(id) if err != nil { return nil, nil, err @@ -2992,11 +2982,10 @@ func (s *store) Containers() ([]Container, error) { return nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, err } + defer rcstore.stopReading() return rcstore.Containers() } @@ -3156,11 +3145,10 @@ func (s *store) Container(id string) (*Container, error) { if err != nil { return nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, err } + defer rcstore.stopReading() return rcstore.Get(id) } @@ -3170,11 +3158,10 @@ func (s *store) ContainerLayerID(id string) (string, error) { if err != nil { return "", err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return "", err } + defer rcstore.stopReading() container, err := rcstore.Get(id) if err != nil { return "", err @@ -3191,11 +3178,10 @@ func (s *store) ContainerByLayer(id string) (*Container, error) { if err != nil { return nil, err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return nil, err } + defer rcstore.stopReading() containerList, err := rcstore.Containers() if err != nil { return nil, err @@ -3214,11 +3200,10 @@ func (s *store) ContainerDirectory(id string) (string, error) { if err != nil { return "", err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return "", err } + defer rcstore.stopReading() id, err = rcstore.Lookup(id) if err != nil { @@ -3239,11 +3224,10 @@ func (s *store) ContainerRunDirectory(id string) (string, error) { return "", err } - rcstore.RLock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startReading(); err != nil { return "", err } + defer rcstore.stopReading() id, err = rcstore.Lookup(id) if err != nil { From 987cac34ec60db96915991452a53714ea963d62c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 22:45:18 +0200 Subject: [PATCH 10/43] Replace containerStore.{Lock,Unlock} with {startWriting,stopWriting} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This integrates ReloadIfChanges, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Signed-off-by: Miloslav Trmač --- containers.go | 41 ++++++++++++++++++++++++++++++----------- store.go | 15 ++++++--------- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/containers.go b/containers.go index 1b66f1a60e..8f91a546a8 100644 --- a/containers.go +++ b/containers.go @@ -72,17 +72,12 @@ type rwContainerStore interface { containerBigDataStore flaggableStore - // Acquire a writer lock. - // The default unix implementation panics if: - // - opening the lockfile failed - // - tried to lock a read-only lock-file - Lock() - - // Unlock the lock. - // The default unix implementation panics if: - // - unlocking an unlocked lock - // - if the lock counter is corrupted - Unlock() + // startWriting makes sure the store is fresh, and locks it for writing. + // If this succeeds, the caller MUST call stopWriting(). + startWriting() error + + // stopWriting releases locks obtained by startWriting. + stopWriting() // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). @@ -219,6 +214,30 @@ func (c *Container) MountOpts() []string { } } +// startWriting makes sure the store is fresh, and locks it for writing. +// If this succeeds, the caller MUST call stopWriting(). +func (r *containerStore) startWriting() error { + r.lockfile.Lock() + succeeded := false + defer func() { + if !succeeded { + r.lockfile.Unlock() + } + }() + + if err := r.ReloadIfChanged(); err != nil { + return err + } + + succeeded = true + return nil +} + +// stopWriting releases locks obtained by startWriting. +func (r *containerStore) stopWriting() { + r.lockfile.Unlock() +} + // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). func (r *containerStore) startReading() error { diff --git a/store.go b/store.go index 6a41d3bdf1..4f75350617 100644 --- a/store.go +++ b/store.go @@ -1119,11 +1119,10 @@ func (s *store) writeToContainerStore(fn func(store rwContainerStore) error) err return err } - store.Lock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startWriting(); err != nil { return err } + defer store.stopWriting() return fn(store) } @@ -1154,11 +1153,10 @@ func (s *store) writeToAllStores(fn func(rlstore rwLayerStore, ristore rwImageSt if err := ristore.ReloadIfChanged(); err != nil { return err } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startWriting(); err != nil { return err } + defer rcstore.stopWriting() return fn(rlstore, ristore, rcstore) } @@ -1195,11 +1193,10 @@ func (s *store) PutLayer(id, parent string, names []string, mountLabel string, w if err := rlstore.ReloadIfChanged(); err != nil { return nil, -1, err } - rcstore.Lock() - defer rcstore.Unlock() - if err := rcstore.ReloadIfChanged(); err != nil { + if err := rcstore.startWriting(); err != nil { return nil, -1, err } + defer rcstore.stopWriting() if options == nil { options = &LayerOptions{} } From 72039cd1b2d6fc4850d32e0b32e5206a135f593a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 22:46:06 +0200 Subject: [PATCH 11/43] Avoid a warning that is now reported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- store.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/store.go b/store.go index 4f75350617..f205eca0e1 100644 --- a/store.go +++ b/store.go @@ -2073,11 +2073,7 @@ func (s *store) Exists(id string) bool { return false } defer rcstore.stopReading() - if rcstore.Exists(id) { - return true - } - - return false + return rcstore.Exists(id) } func dedupeNames(names []string) []string { From 1a03858b7ad21e212d9a947fb5eed6cfbc7da681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 23:00:48 +0200 Subject: [PATCH 12/43] Remove completely unused methods from rwContainerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exposing the internals of the lock is not necessary, and exposes too many implementation details. Signed-off-by: Miloslav Trmač --- containers.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/containers.go b/containers.go index 8f91a546a8..5a3c83a12a 100644 --- a/containers.go +++ b/containers.go @@ -94,12 +94,6 @@ type rwContainerStore interface { // last recorded writer. It should only be called with the lock held. Modified() (bool, error) - // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. - TouchedSince(when time.Time) bool - - // IsReadWrite() checks if the lock file is read-write - IsReadWrite() bool - // Locked() checks if lock is locked for writing by a thread in this process Locked() bool // Load reloads the contents of the store from disk. It should be called @@ -724,14 +718,6 @@ func (r *containerStore) Modified() (bool, error) { return r.lockfile.Modified() } -func (r *containerStore) IsReadWrite() bool { - return r.lockfile.IsReadWrite() -} - -func (r *containerStore) TouchedSince(when time.Time) bool { - return r.lockfile.TouchedSince(when) -} - func (r *containerStore) Locked() bool { return r.lockfile.Locked() } From 5df7cf0b06dcfb4ce9e5ee6443b71aab8c6fa34d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 23:05:14 +0200 Subject: [PATCH 13/43] Remove unused lockfile forwarders from rwContainerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only callers are internal, have them access r.lockfile directly. Signed-off-by: Miloslav Trmač --- containers.go | 28 +++------------------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/containers.go b/containers.go index 5a3c83a12a..a02aa455ba 100644 --- a/containers.go +++ b/containers.go @@ -86,16 +86,6 @@ type rwContainerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Touch records, for others sharing the lock, that the caller was the - // last writer. It should only be called with the lock held. - Touch() error - - // Modified() checks if the most recent writer was a party other than the - // last recorded writer. It should only be called with the lock held. - Modified() (bool, error) - - // Locked() checks if lock is locked for writing by a thread in this process - Locked() bool // Load reloads the contents of the store from disk. It should be called // with the lock held. Load() error @@ -315,7 +305,7 @@ func (r *containerStore) Load() error { } func (r *containerStore) Save() error { - if !r.Locked() { + if !r.lockfile.Locked() { return errors.New("container store is not locked") } rpath := r.containerspath() @@ -329,7 +319,7 @@ func (r *containerStore) Save() error { if err := ioutils.AtomicWriteFile(rpath, jdata, 0600); err != nil { return err } - return r.Touch() + return r.lockfile.Touch() } func newContainerStore(dir string) (rwContainerStore, error) { @@ -710,23 +700,11 @@ func (r *containerStore) Unlock() { r.lockfile.Unlock() } -func (r *containerStore) Touch() error { - return r.lockfile.Touch() -} - -func (r *containerStore) Modified() (bool, error) { - return r.lockfile.Modified() -} - -func (r *containerStore) Locked() bool { - return r.lockfile.Locked() -} - func (r *containerStore) ReloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() - modified, err := r.Modified() + modified, err := r.lockfile.Modified() if err == nil && modified { return r.Load() } From e25f4bd8772847ba4ab1f323a879f0e6240e4ab2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 23:07:39 +0200 Subject: [PATCH 14/43] Remove Load() from rwContainerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač --- containers.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/containers.go b/containers.go index a02aa455ba..08d61e24ab 100644 --- a/containers.go +++ b/containers.go @@ -86,10 +86,6 @@ type rwContainerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Load reloads the contents of the store from disk. It should be called - // with the lock held. - Load() error - // ReloadIfChanged reloads the contents of the store from disk if it is changed. ReloadIfChanged() error @@ -266,7 +262,9 @@ func (r *containerStore) datapath(id, key string) string { return filepath.Join(r.datadir(id), makeBigDataBaseName(key)) } -func (r *containerStore) Load() error { +// load reloads the contents of the store from disk. It should be called +// with the lock held. +func (r *containerStore) load() error { needSave := false rpath := r.containerspath() data, err := os.ReadFile(rpath) @@ -340,7 +338,7 @@ func newContainerStore(dir string) (rwContainerStore, error) { } cstore.Lock() defer cstore.Unlock() - if err := cstore.Load(); err != nil { + if err := cstore.load(); err != nil { return nil, err } return &cstore, nil @@ -706,7 +704,7 @@ func (r *containerStore) ReloadIfChanged() error { modified, err := r.lockfile.Modified() if err == nil && modified { - return r.Load() + return r.load() } return err } From f1547f96a527d9f3e7951de9d4f16bd97956d45c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 23:09:13 +0200 Subject: [PATCH 15/43] Remove ReloadIfChanged() from rwContainerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač --- containers.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/containers.go b/containers.go index 08d61e24ab..221e536aa6 100644 --- a/containers.go +++ b/containers.go @@ -86,9 +86,6 @@ type rwContainerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // ReloadIfChanged reloads the contents of the store from disk if it is changed. - ReloadIfChanged() error - // Save saves the contents of the store to disk. It should be called with // the lock held, and Touch() should be called afterward before releasing the // lock. @@ -205,7 +202,7 @@ func (r *containerStore) startWriting() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(); err != nil { return err } @@ -229,7 +226,7 @@ func (r *containerStore) startReading() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(); err != nil { return err } @@ -698,7 +695,8 @@ func (r *containerStore) Unlock() { r.lockfile.Unlock() } -func (r *containerStore) ReloadIfChanged() error { +// reloadIfChanged reloads the contents of the store from disk if it is changed. +func (r *containerStore) reloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() From c8ff4ffeb3b2767d07b7881be30b488cc8ef4585 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 23:13:02 +0200 Subject: [PATCH 16/43] Remove Save() from rwContainerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is done implicitly by all writers already. Also fix the documentation not to point at an explicit Touch(), which is not actually necessary. Signed-off-by: Miloslav Trmač --- containers.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/containers.go b/containers.go index 221e536aa6..7779e6708c 100644 --- a/containers.go +++ b/containers.go @@ -86,11 +86,6 @@ type rwContainerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Save saves the contents of the store to disk. It should be called with - // the lock held, and Touch() should be called afterward before releasing the - // lock. - Save() error - // Create creates a container that has a specified ID (or generates a // random one if an empty value is supplied) and optional names, // based on the specified image, using the specified layer as its @@ -294,12 +289,14 @@ func (r *containerStore) load() error { r.bylayer = layers r.byname = names if needSave { - return r.Save() + return r.save() } return nil } -func (r *containerStore) Save() error { +// save saves the contents of the store to disk. It should be called with +// the lock held. +func (r *containerStore) save() error { if !r.lockfile.Locked() { return errors.New("container store is not locked") } @@ -362,7 +359,7 @@ func (r *containerStore) ClearFlag(id string, flag string) error { return ErrContainerUnknown } delete(container.Flags, flag) - return r.Save() + return r.save() } func (r *containerStore) SetFlag(id string, flag string, value interface{}) error { @@ -374,7 +371,7 @@ func (r *containerStore) SetFlag(id string, flag string, value interface{}) erro container.Flags = make(map[string]interface{}) } container.Flags[flag] = value - return r.Save() + return r.save() } func (r *containerStore) Create(id string, names []string, image, layer, metadata string, options *ContainerOptions) (container *Container, err error) { @@ -431,7 +428,7 @@ func (r *containerStore) Create(id string, names []string, image, layer, metadat for _, name := range names { r.byname[name] = container } - err = r.Save() + err = r.save() container = copyContainer(container) } return container, err @@ -447,7 +444,7 @@ func (r *containerStore) Metadata(id string) (string, error) { func (r *containerStore) SetMetadata(id, metadata string) error { if container, ok := r.lookup(id); ok { container.Metadata = metadata - return r.Save() + return r.save() } return ErrContainerUnknown } @@ -489,7 +486,7 @@ func (r *containerStore) updateNames(id string, names []string, op updateNameOpe r.byname[name] = container } container.Names = names - return r.Save() + return r.save() } func (r *containerStore) Delete(id string) error { @@ -521,7 +518,7 @@ func (r *containerStore) Delete(id string) error { r.containers = append(r.containers[:toDeleteIndex], r.containers[toDeleteIndex+1:]...) } } - if err := r.Save(); err != nil { + if err := r.save(); err != nil { return err } if err := os.RemoveAll(r.datadir(id)); err != nil { @@ -668,7 +665,7 @@ func (r *containerStore) SetBigData(id, key string, data []byte) error { save = true } if save { - err = r.Save() + err = r.save() } } return err From 1c0149acbb60fe39213ca8116d01c11620377a4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 12 Oct 2022 23:19:19 +0200 Subject: [PATCH 17/43] Remove Lock/Unlock methods from rwContainerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They are now only used in the constructor, use a variant of startWriting instead. This code path is not performance-critical, so let's share as much code as possible to ensure consistency in locking. Signed-off-by: Miloslav Trmač --- containers.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/containers.go b/containers.go index 7779e6708c..5c030b2753 100644 --- a/containers.go +++ b/containers.go @@ -186,9 +186,12 @@ func (c *Container) MountOpts() []string { } } -// startWriting makes sure the store is fresh, and locks it for writing. +// startWritingWithReload makes sure the store is fresh if canReload, and locks it for writing. // If this succeeds, the caller MUST call stopWriting(). -func (r *containerStore) startWriting() error { +// +// This is an internal implementation detail of containerStore construction, every other caller +// should use startWriting() instead. +func (r *containerStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() succeeded := false defer func() { @@ -197,14 +200,22 @@ func (r *containerStore) startWriting() error { } }() - if err := r.reloadIfChanged(); err != nil { - return err + if canReload { + if err := r.reloadIfChanged(); err != nil { + return err + } } succeeded = true return nil } +// startWriting makes sure the store is fresh, and locks it for writing. +// If this succeeds, the caller MUST call stopWriting(). +func (r *containerStore) startWriting() error { + return r.startWritingWithReload(true) +} + // stopWriting releases locks obtained by startWriting. func (r *containerStore) stopWriting() { r.lockfile.Unlock() @@ -330,8 +341,10 @@ func newContainerStore(dir string) (rwContainerStore, error) { bylayer: make(map[string]*Container), byname: make(map[string]*Container), } - cstore.Lock() - defer cstore.Unlock() + if err := cstore.startWritingWithReload(false); err != nil { + return nil, err + } + defer cstore.stopWriting() if err := cstore.load(); err != nil { return nil, err } @@ -684,14 +697,6 @@ func (r *containerStore) Wipe() error { return nil } -func (r *containerStore) Lock() { - r.lockfile.Lock() -} - -func (r *containerStore) Unlock() { - r.lockfile.Unlock() -} - // reloadIfChanged reloads the contents of the store from disk if it is changed. func (r *containerStore) reloadIfChanged() error { r.loadMut.Lock() From 19716e687825833583588caca2e217bed56cf3f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 00:32:56 +0200 Subject: [PATCH 18/43] Move containerStore.reloadIfChanged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to be closer to the lock / load set of methods. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- containers.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/containers.go b/containers.go index 5c030b2753..d7af80ad29 100644 --- a/containers.go +++ b/containers.go @@ -245,6 +245,18 @@ func (r *containerStore) stopReading() { r.lockfile.Unlock() } +// reloadIfChanged reloads the contents of the store from disk if it is changed. +func (r *containerStore) reloadIfChanged() error { + r.loadMut.Lock() + defer r.loadMut.Unlock() + + modified, err := r.lockfile.Modified() + if err == nil && modified { + return r.load() + } + return err +} + func (r *containerStore) Containers() ([]Container, error) { containers := make([]Container, len(r.containers)) for i := range r.containers { @@ -696,15 +708,3 @@ func (r *containerStore) Wipe() error { } return nil } - -// reloadIfChanged reloads the contents of the store from disk if it is changed. -func (r *containerStore) reloadIfChanged() error { - r.loadMut.Lock() - defer r.loadMut.Unlock() - - modified, err := r.lockfile.Modified() - if err == nil && modified { - return r.load() - } - return err -} From 5e07241b0b6ff05295de29e4c0a31378a20d0cae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 00:36:37 +0200 Subject: [PATCH 19/43] Copy methods from included interfaces directly into *ImageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we can modify/replace them. Signed-off-by: Miloslav Trmač --- images.go | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/images.go b/images.go index 50559f0dca..b51e15d09b 100644 --- a/images.go +++ b/images.go @@ -96,10 +96,48 @@ type Image struct { // roImageStore provides bookkeeping for information about Images. type roImageStore interface { - roFileBasedStore roMetadataStore roBigDataStore + // Acquire a writer lock. + // The default unix implementation panics if: + // - opening the lockfile failed + // - tried to lock a read-only lock-file + Lock() + + // Unlock the lock. + // The default unix implementation panics if: + // - unlocking an unlocked lock + // - if the lock counter is corrupted + Unlock() + + // Acquire a reader lock. + RLock() + + // Touch records, for others sharing the lock, that the caller was the + // last writer. It should only be called with the lock held. + Touch() error + + // Modified() checks if the most recent writer was a party other than the + // last recorded writer. It should only be called with the lock held. + Modified() (bool, error) + + // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. + TouchedSince(when time.Time) bool + + // IsReadWrite() checks if the lock file is read-write + IsReadWrite() bool + + // Locked() checks if lock is locked for writing by a thread in this process + Locked() bool + + // Load reloads the contents of the store from disk. It should be called + // with the lock held. + Load() error + + // ReloadIfChanged reloads the contents of the store from disk if it is changed. + ReloadIfChanged() error + // Exists checks if there is an image with the given ID or name. Exists(id string) bool @@ -119,11 +157,15 @@ type roImageStore interface { // rwImageStore provides bookkeeping for information about Images. type rwImageStore interface { roImageStore - rwFileBasedStore rwMetadataStore rwImageBigDataStore flaggableStore + // Save saves the contents of the store to disk. It should be called with + // the lock held, and Touch() should be called afterward before releasing the + // lock. + Save() error + // Create creates an image that has a specified ID (or a random one) and // optional names, using the specified layer as its topmost (hopefully // read-only) layer. That layer can be referenced by multiple images. From 12ab1cf00809cb8723b872a0a24c82637e2fd37f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 00:44:02 +0200 Subject: [PATCH 20/43] Replace imageStore.{RLock,Unlock} with {startReading,stopReading} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Signed-off-by: Miloslav Trmač --- images.go | 32 ++++++++++++++++++++++++++++++-- store.go | 28 ++++++++++++++-------------- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/images.go b/images.go index b51e15d09b..1a57076360 100644 --- a/images.go +++ b/images.go @@ -111,8 +111,12 @@ type roImageStore interface { // - if the lock counter is corrupted Unlock() - // Acquire a reader lock. - RLock() + // startReading makes sure the store is fresh, and locks it for reading. + // If this succeeds, the caller MUST call stopReading(). + startReading() error + + // stopReading releases locks obtained by startReading. + stopReading() // Touch records, for others sharing the lock, that the caller was the // last writer. It should only be called with the lock held. @@ -235,6 +239,30 @@ func copyImageSlice(slice []*Image) []*Image { return nil } +// startReading makes sure the store is fresh, and locks it for reading. +// If this succeeds, the caller MUST call stopReading(). +func (r *imageStore) startReading() error { + r.lockfile.RLock() + succeeded := false + defer func() { + if !succeeded { + r.lockfile.Unlock() + } + }() + + if err := r.ReloadIfChanged(); err != nil { + return err + } + + succeeded = true + return nil +} + +// stopReading releases locks obtained by startReading. +func (r *imageStore) stopReading() { + r.lockfile.Unlock() +} + func (r *imageStore) Images() ([]Image, error) { images := make([]Image, len(r.images)) for i := range r.images { diff --git a/store.go b/store.go index f205eca0e1..4e090ad098 100644 --- a/store.go +++ b/store.go @@ -1072,11 +1072,10 @@ func (s *store) readAllImageStores(fn func(store roImageStore) (bool, error)) (b } for _, s := range ImageStores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return true, err } + defer store.stopReading() if done, err := fn(store); done { return true, err } @@ -1490,12 +1489,15 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat store := s if store == istore { store.Lock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return nil, err + } } else { - store.RLock() - } - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err + if err := store.startReading(); err != nil { + return nil, err + } + defer store.stopReading() } cimage, err = store.Get(image) if err == nil { @@ -1825,11 +1827,10 @@ func (s *store) ImageSize(id string) (int64, error) { var image *Image for _, s := range imageStores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return -1, err } + defer store.stopReading() if image, err = store.Get(id); err == nil { imageStore = store break @@ -2153,11 +2154,10 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e } for _, s := range ristores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return err } + defer store.stopReading() if i, err := store.Get(id); err == nil { if len(deduped) > 1 { // Do not want to create image name in R/W storage From 8d45c7ff4f38c6a47c0755c6c28a03565672bd36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 00:51:03 +0200 Subject: [PATCH 21/43] Replace imageStore.{Lock,Unlock} with {startWriting,stopWriting} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Signed-off-by: Miloslav Trmač --- images.go | 41 ++++++++++++++++++++++++++++++----------- images_test.go | 11 ++++++----- store.go | 20 ++++++++------------ 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/images.go b/images.go index 1a57076360..f864b895bc 100644 --- a/images.go +++ b/images.go @@ -99,17 +99,12 @@ type roImageStore interface { roMetadataStore roBigDataStore - // Acquire a writer lock. - // The default unix implementation panics if: - // - opening the lockfile failed - // - tried to lock a read-only lock-file - Lock() - - // Unlock the lock. - // The default unix implementation panics if: - // - unlocking an unlocked lock - // - if the lock counter is corrupted - Unlock() + // startWriting makes sure the store is fresh, and locks it for writing. + // If this succeeds, the caller MUST call stopWriting(). + startWriting() error + + // stopWriting releases locks obtained by startWriting. + stopWriting() // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). @@ -239,6 +234,30 @@ func copyImageSlice(slice []*Image) []*Image { return nil } +// startWriting makes sure the store is fresh, and locks it for writing. +// If this succeeds, the caller MUST call stopWriting(). +func (r *imageStore) startWriting() error { + r.lockfile.Lock() + succeeded := false + defer func() { + if !succeeded { + r.lockfile.Unlock() + } + }() + + if err := r.ReloadIfChanged(); err != nil { + return err + } + + succeeded = true + return nil +} + +// stopWriting releases locks obtained by startWriting. +func (r *imageStore) stopWriting() { + r.lockfile.Unlock() +} + // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). func (r *imageStore) startReading() error { diff --git a/images_test.go b/images_test.go index 01594c11fa..016c5639d2 100644 --- a/images_test.go +++ b/images_test.go @@ -16,10 +16,11 @@ func newTestImageStore(t *testing.T) rwImageStore { } func addTestImage(t *testing.T, store rwImageStore, id string, names []string) { - store.Lock() - defer store.Unlock() + err := store.startWriting() + require.NoError(t, err) + defer store.stopWriting() - _, err := store.Create( + _, err = store.Create( id, []string{}, "", "", time.Now(), digest.FromString(""), ) @@ -70,8 +71,8 @@ func TestHistoryNames(t *testing.T) { require.Equal(t, secondImage.NamesHistory[1], "2") // And When - store.Lock() - defer store.Unlock() + require.NoError(t, store.startWriting()) + defer store.stopWriting() require.Nil(t, store.SetNames(firstImageID, []string{"1", "2", "3", "4"})) // Then diff --git a/store.go b/store.go index 4e090ad098..b309dfadc6 100644 --- a/store.go +++ b/store.go @@ -1092,11 +1092,10 @@ func (s *store) writeToImageStore(fn func(store rwImageStore) error) error { return err } - store.Lock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startWriting(); err != nil { return err } + defer store.stopWriting() return fn(store) } @@ -1147,11 +1146,10 @@ func (s *store) writeToAllStores(fn func(rlstore rwLayerStore, ristore rwImageSt if err := rlstore.ReloadIfChanged(); err != nil { return err } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { + if err := ristore.startWriting(); err != nil { return err } + defer ristore.stopWriting() if err := rcstore.startWriting(); err != nil { return err } @@ -1488,11 +1486,10 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat for _, s := range append([]roImageStore{istore}, istores...) { store := s if store == istore { - store.Lock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startWriting(); err != nil { return nil, err } + defer store.stopWriting() } else { if err := store.startReading(); err != nil { return nil, err @@ -2129,11 +2126,10 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e if err != nil { return err } - ristore.Lock() - defer ristore.Unlock() - if err := ristore.ReloadIfChanged(); err != nil { + if err := ristore.startWriting(); err != nil { return err } + defer ristore.stopWriting() if ristore.Exists(id) { switch op { case setNames: From 16aaa1040fdc61f1bdbe65374a07604b7c4fe8dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 00:54:12 +0200 Subject: [PATCH 22/43] Move the writing lock methods from roImageStore to rwImageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... for a bit of extra safety. That requires us to be a bit more explicit in one of the users. Signed-off-by: Miloslav Trmač --- images.go | 14 +++++++------- store.go | 28 +++++++++++++++------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/images.go b/images.go index f864b895bc..b61a699348 100644 --- a/images.go +++ b/images.go @@ -99,13 +99,6 @@ type roImageStore interface { roMetadataStore roBigDataStore - // startWriting makes sure the store is fresh, and locks it for writing. - // If this succeeds, the caller MUST call stopWriting(). - startWriting() error - - // stopWriting releases locks obtained by startWriting. - stopWriting() - // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). startReading() error @@ -160,6 +153,13 @@ type rwImageStore interface { rwImageBigDataStore flaggableStore + // startWriting makes sure the store is fresh, and locks it for writing. + // If this succeeds, the caller MUST call stopWriting(). + startWriting() error + + // stopWriting releases locks obtained by startWriting. + stopWriting() + // Save saves the contents of the store to disk. It should be called with // the lock held, and Touch() should be called afterward before releasing the // lock. diff --git a/store.go b/store.go index b309dfadc6..d439f9287d 100644 --- a/store.go +++ b/store.go @@ -1483,23 +1483,25 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat if err := rlstore.ReloadIfChanged(); err != nil { return nil, err } - for _, s := range append([]roImageStore{istore}, istores...) { - store := s - if store == istore { - if err := store.startWriting(); err != nil { - return nil, err - } - defer store.stopWriting() - } else { + if err := istore.startWriting(); err != nil { + return nil, err + } + defer istore.stopWriting() + cimage, err = istore.Get(image) + if err == nil { + imageHomeStore = istore + } else { + for _, s := range istores { + store := s if err := store.startReading(); err != nil { return nil, err } defer store.stopReading() - } - cimage, err = store.Get(image) - if err == nil { - imageHomeStore = store - break + cimage, err = store.Get(image) + if err == nil { + imageHomeStore = store + break + } } } if cimage == nil { From 0978139906930de0b9ff4fdb57206ac003239917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 00:58:37 +0200 Subject: [PATCH 23/43] Remove a completely unused method from roImageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exposing the internals of the lock is not necessary, and exposes too many implementation details. Signed-off-by: Miloslav Trmač --- images.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/images.go b/images.go index b61a699348..1a9873edcb 100644 --- a/images.go +++ b/images.go @@ -114,9 +114,6 @@ type roImageStore interface { // last recorded writer. It should only be called with the lock held. Modified() (bool, error) - // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. - TouchedSince(when time.Time) bool - // IsReadWrite() checks if the lock file is read-write IsReadWrite() bool @@ -902,10 +899,6 @@ func (r *imageStore) IsReadWrite() bool { return r.lockfile.IsReadWrite() } -func (r *imageStore) TouchedSince(when time.Time) bool { - return r.lockfile.TouchedSince(when) -} - func (r *imageStore) Locked() bool { return r.lockfile.Locked() } From 068f7c256059a27894507dc68b36bea2a0c3c596 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:02:54 +0200 Subject: [PATCH 24/43] Remove unused lockfile forwarders from roImageStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only callers are internal, have them access r.lockfile directly. Signed-off-by: Miloslav Trmač --- images.go | 58 ++++++++++++++----------------------------------------- 1 file changed, 14 insertions(+), 44 deletions(-) diff --git a/images.go b/images.go index 1a9873edcb..b501a93f73 100644 --- a/images.go +++ b/images.go @@ -106,20 +106,6 @@ type roImageStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Touch records, for others sharing the lock, that the caller was the - // last writer. It should only be called with the lock held. - Touch() error - - // Modified() checks if the most recent writer was a party other than the - // last recorded writer. It should only be called with the lock held. - Modified() (bool, error) - - // IsReadWrite() checks if the lock file is read-write - IsReadWrite() bool - - // Locked() checks if lock is locked for writing by a thread in this process - Locked() bool - // Load reloads the contents of the store from disk. It should be called // with the lock held. Load() error @@ -374,10 +360,10 @@ func (r *imageStore) Load() error { list := digests[digest] digests[digest] = append(list, image) } - image.ReadOnly = !r.IsReadWrite() + image.ReadOnly = !r.lockfile.IsReadWrite() } } - if shouldSave && (!r.IsReadWrite() || !r.Locked()) { + if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) { return ErrDuplicateImageNames } r.images = images @@ -392,10 +378,10 @@ func (r *imageStore) Load() error { } func (r *imageStore) Save() error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the image store at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } - if !r.Locked() { + if !r.lockfile.Locked() { return errors.New("image store is not locked for writing") } rpath := r.imagespath() @@ -409,7 +395,7 @@ func (r *imageStore) Save() error { if err := ioutils.AtomicWriteFile(rpath, jdata, 0600); err != nil { return err } - return r.Touch() + return r.lockfile.Touch() } func newImageStore(dir string) (rwImageStore, error) { @@ -470,7 +456,7 @@ func (r *imageStore) lookup(id string) (*Image, bool) { } func (r *imageStore) ClearFlag(id string, flag string) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to clear flags on images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } image, ok := r.lookup(id) @@ -482,7 +468,7 @@ func (r *imageStore) ClearFlag(id string, flag string) error { } func (r *imageStore) SetFlag(id string, flag string, value interface{}) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to set flags on images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } image, ok := r.lookup(id) @@ -497,7 +483,7 @@ func (r *imageStore) SetFlag(id string, flag string, value interface{}) error { } func (r *imageStore) Create(id string, names []string, layer, metadata string, created time.Time, searchableDigest digest.Digest) (image *Image, err error) { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return nil, fmt.Errorf("not allowed to create new images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } if id == "" { @@ -584,7 +570,7 @@ func (r *imageStore) Metadata(id string) (string, error) { } func (r *imageStore) SetMetadata(id, metadata string) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify image metadata at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } if image, ok := r.lookup(id); ok { @@ -616,7 +602,7 @@ func (r *imageStore) RemoveNames(id string, names []string) error { } func (r *imageStore) updateNames(id string, names []string, op updateNameOperation) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to change image name assignments at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } image, ok := r.lookup(id) @@ -643,7 +629,7 @@ func (r *imageStore) updateNames(id string, names []string, op updateNameOperati } func (r *imageStore) Delete(id string) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } image, ok := r.lookup(id) @@ -779,7 +765,7 @@ func (r *imageStore) SetBigData(id, key string, data []byte, digestManifest func if key == "" { return fmt.Errorf("can't set empty name for image big data item: %w", ErrInvalidBigDataName) } - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to save data items associated with images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } image, ok := r.lookup(id) @@ -860,7 +846,7 @@ func (r *imageStore) SetBigData(id, key string, data []byte, digestManifest func } func (r *imageStore) Wipe() error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete images at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } ids := make([]string, 0, len(r.byid)) @@ -887,27 +873,11 @@ func (r *imageStore) Unlock() { r.lockfile.Unlock() } -func (r *imageStore) Touch() error { - return r.lockfile.Touch() -} - -func (r *imageStore) Modified() (bool, error) { - return r.lockfile.Modified() -} - -func (r *imageStore) IsReadWrite() bool { - return r.lockfile.IsReadWrite() -} - -func (r *imageStore) Locked() bool { - return r.lockfile.Locked() -} - func (r *imageStore) ReloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() - modified, err := r.Modified() + modified, err := r.lockfile.Modified() if err == nil && modified { return r.Load() } From 93b5cd4d0d1f3bf97f9a409d9cd3e2487292b6a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:04:21 +0200 Subject: [PATCH 25/43] Remove Load() from roImageStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač --- images.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/images.go b/images.go index b501a93f73..2f2983f445 100644 --- a/images.go +++ b/images.go @@ -106,10 +106,6 @@ type roImageStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Load reloads the contents of the store from disk. It should be called - // with the lock held. - Load() error - // ReloadIfChanged reloads the contents of the store from disk if it is changed. ReloadIfChanged() error @@ -325,7 +321,9 @@ func (i *Image) recomputeDigests() error { return nil } -func (r *imageStore) Load() error { +// load reloads the contents of the store from disk. It should be called +// with the lock held. +func (r *imageStore) load() error { shouldSave := false rpath := r.imagespath() data, err := os.ReadFile(rpath) @@ -416,7 +414,7 @@ func newImageStore(dir string) (rwImageStore, error) { } istore.Lock() defer istore.Unlock() - if err := istore.Load(); err != nil { + if err := istore.load(); err != nil { return nil, err } return &istore, nil @@ -437,7 +435,7 @@ func newROImageStore(dir string) (roImageStore, error) { } istore.RLock() defer istore.Unlock() - if err := istore.Load(); err != nil { + if err := istore.load(); err != nil { return nil, err } return &istore, nil @@ -879,7 +877,7 @@ func (r *imageStore) ReloadIfChanged() error { modified, err := r.lockfile.Modified() if err == nil && modified { - return r.Load() + return r.load() } return err } From b09619e2393d5cd716bbd44efbb4d5225d9199cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:05:15 +0200 Subject: [PATCH 26/43] Remove ReloadIfChanged() from roImageStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač --- images.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/images.go b/images.go index 2f2983f445..f0bfd4059f 100644 --- a/images.go +++ b/images.go @@ -106,9 +106,6 @@ type roImageStore interface { // stopReading releases locks obtained by startReading. stopReading() - // ReloadIfChanged reloads the contents of the store from disk if it is changed. - ReloadIfChanged() error - // Exists checks if there is an image with the given ID or name. Exists(id string) bool @@ -224,7 +221,7 @@ func (r *imageStore) startWriting() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(); err != nil { return err } @@ -248,7 +245,7 @@ func (r *imageStore) startReading() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(); err != nil { return err } @@ -871,7 +868,8 @@ func (r *imageStore) Unlock() { r.lockfile.Unlock() } -func (r *imageStore) ReloadIfChanged() error { +// reloadIfChanged reloads the contents of the store from disk if it is changed. +func (r *imageStore) reloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() From 363f5cdcb8b2a7ea11de20e52441e1b277bf8d0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:08:55 +0200 Subject: [PATCH 27/43] Remove a redundant rwImageStore.Save() call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rwImageStore.Create() already calls it. Signed-off-by: Miloslav Trmač --- store.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/store.go b/store.go index d439f9287d..8a6941fb40 100644 --- a/store.go +++ b/store.go @@ -2162,9 +2162,6 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e deduped = deduped[1:] } _, err := ristore.Create(id, deduped, i.TopLayer, i.Metadata, i.Created, i.Digest) - if err == nil { - return ristore.Save() - } return err } } From 1d6bb6157bbe017df2b8d44f424c39d4b1f6f4f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:10:11 +0200 Subject: [PATCH 28/43] Remove Save() from rwImageStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is done implicitly by all writers already. Also fix the documentation not to point at an explicit Touch(), which is not actually necessary. Signed-off-by: Miloslav Trmač --- images.go | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/images.go b/images.go index f0bfd4059f..c283dee80d 100644 --- a/images.go +++ b/images.go @@ -136,11 +136,6 @@ type rwImageStore interface { // stopWriting releases locks obtained by startWriting. stopWriting() - // Save saves the contents of the store to disk. It should be called with - // the lock held, and Touch() should be called afterward before releasing the - // lock. - Save() error - // Create creates an image that has a specified ID (or a random one) and // optional names, using the specified layer as its topmost (hopefully // read-only) layer. That layer can be referenced by multiple images. @@ -367,12 +362,14 @@ func (r *imageStore) load() error { r.byname = names r.bydigest = digests if shouldSave { - return r.Save() + return r.save() } return nil } -func (r *imageStore) Save() error { +// save saves the contents of the store to disk. It should be called with +// the lock held. +func (r *imageStore) save() error { if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the image store at %q: %w", r.imagespath(), ErrStoreIsReadOnly) } @@ -459,7 +456,7 @@ func (r *imageStore) ClearFlag(id string, flag string) error { return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } delete(image.Flags, flag) - return r.Save() + return r.save() } func (r *imageStore) SetFlag(id string, flag string, value interface{}) error { @@ -474,7 +471,7 @@ func (r *imageStore) SetFlag(id string, flag string, value interface{}) error { image.Flags = make(map[string]interface{}) } image.Flags[flag] = value - return r.Save() + return r.save() } func (r *imageStore) Create(id string, names []string, layer, metadata string, created time.Time, searchableDigest digest.Digest) (image *Image, err error) { @@ -531,7 +528,7 @@ func (r *imageStore) Create(id string, names []string, layer, metadata string, c list := r.bydigest[digest] r.bydigest[digest] = append(list, image) } - err = r.Save() + err = r.save() image = copyImage(image) return image, err } @@ -539,7 +536,7 @@ func (r *imageStore) Create(id string, names []string, layer, metadata string, c func (r *imageStore) addMappedTopLayer(id, layer string) error { if image, ok := r.lookup(id); ok { image.MappedTopLayers = append(image.MappedTopLayers, layer) - return r.Save() + return r.save() } return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } @@ -552,7 +549,7 @@ func (r *imageStore) removeMappedTopLayer(id, layer string) error { if initialLen == len(image.MappedTopLayers) { return nil } - return r.Save() + return r.save() } return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } @@ -570,7 +567,7 @@ func (r *imageStore) SetMetadata(id, metadata string) error { } if image, ok := r.lookup(id); ok { image.Metadata = metadata - return r.Save() + return r.save() } return fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } @@ -620,7 +617,7 @@ func (r *imageStore) updateNames(id string, names []string, op updateNameOperati image.addNameToHistory(name) } image.Names = names - return r.Save() + return r.save() } func (r *imageStore) Delete(id string) error { @@ -661,7 +658,7 @@ func (r *imageStore) Delete(id string) error { r.images = append(r.images[:toDeleteIndex], r.images[toDeleteIndex+1:]...) } } - if err := r.Save(); err != nil { + if err := r.save(); err != nil { return err } if err := os.RemoveAll(r.datadir(id)); err != nil { @@ -834,7 +831,7 @@ func (r *imageStore) SetBigData(id, key string, data []byte, digestManifest func } } if save { - err = r.Save() + err = r.save() } } return err From f109f006bb59857b6e578caa7421e316ff1b01cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:14:46 +0200 Subject: [PATCH 29/43] Remove Lock/Unlock methods from imageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They are now only used in the constructors, use a variant of startReading/startWriting instead. This code path is not performance-critical, so let's share as much code as possible to ensure consistency in locking. Signed-off-by: Miloslav Trmač --- images.go | 62 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/images.go b/images.go index c283dee80d..dddfed1954 100644 --- a/images.go +++ b/images.go @@ -205,9 +205,12 @@ func copyImageSlice(slice []*Image) []*Image { return nil } -// startWriting makes sure the store is fresh, and locks it for writing. +// startWritingWithReload makes sure the store is fresh if canReload, and locks it for writing. // If this succeeds, the caller MUST call stopWriting(). -func (r *imageStore) startWriting() error { +// +// This is an internal implementation detail of imageStore construction, every other caller +// should use startReading() instead. +func (r *imageStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() succeeded := false defer func() { @@ -216,22 +219,33 @@ func (r *imageStore) startWriting() error { } }() - if err := r.reloadIfChanged(); err != nil { - return err + if canReload { + if err := r.reloadIfChanged(); err != nil { + return err + } } succeeded = true return nil } +// startWriting makes sure the store is fresh, and locks it for writing. +// If this succeeds, the caller MUST call stopWriting(). +func (r *imageStore) startWriting() error { + return r.startWritingWithReload(false) +} + // stopWriting releases locks obtained by startWriting. func (r *imageStore) stopWriting() { r.lockfile.Unlock() } -// startReading makes sure the store is fresh, and locks it for reading. +// startReadingWithReload makes sure the store is fresh if canReload, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). -func (r *imageStore) startReading() error { +// +// This is an internal implementation detail of imageStore construction, every other caller +// should use startReading() instead. +func (r *imageStore) startReadingWithReload(canReload bool) error { r.lockfile.RLock() succeeded := false defer func() { @@ -240,14 +254,22 @@ func (r *imageStore) startReading() error { } }() - if err := r.reloadIfChanged(); err != nil { - return err + if canReload { + if err := r.reloadIfChanged(); err != nil { + return err + } } succeeded = true return nil } +// startReading makes sure the store is fresh, and locks it for reading. +// If this succeeds, the caller MUST call stopReading(). +func (r *imageStore) startReading() error { + return r.startReadingWithReload(true) +} + // stopReading releases locks obtained by startReading. func (r *imageStore) stopReading() { r.lockfile.Unlock() @@ -406,8 +428,10 @@ func newImageStore(dir string) (rwImageStore, error) { byname: make(map[string]*Image), bydigest: make(map[digest.Digest][]*Image), } - istore.Lock() - defer istore.Unlock() + if err := istore.startWritingWithReload(false); err != nil { + return nil, err + } + defer istore.stopWriting() if err := istore.load(); err != nil { return nil, err } @@ -427,8 +451,10 @@ func newROImageStore(dir string) (roImageStore, error) { byname: make(map[string]*Image), bydigest: make(map[digest.Digest][]*Image), } - istore.RLock() - defer istore.Unlock() + if err := istore.startReadingWithReload(false); err != nil { + return nil, err + } + defer istore.stopReading() if err := istore.load(); err != nil { return nil, err } @@ -853,18 +879,6 @@ func (r *imageStore) Wipe() error { return nil } -func (r *imageStore) Lock() { - r.lockfile.Lock() -} - -func (r *imageStore) RLock() { - r.lockfile.RLock() -} - -func (r *imageStore) Unlock() { - r.lockfile.Unlock() -} - // reloadIfChanged reloads the contents of the store from disk if it is changed. func (r *imageStore) reloadIfChanged() error { r.loadMut.Lock() From 668a9c95906d8d8123d8778c31ef8d2d4b647533 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:15:51 +0200 Subject: [PATCH 30/43] Move imageStore.reloadIfChanged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to be closer to the lock / load set of methods. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- images.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/images.go b/images.go index dddfed1954..6cb9a630bd 100644 --- a/images.go +++ b/images.go @@ -275,6 +275,18 @@ func (r *imageStore) stopReading() { r.lockfile.Unlock() } +// reloadIfChanged reloads the contents of the store from disk if it is changed. +func (r *imageStore) reloadIfChanged() error { + r.loadMut.Lock() + defer r.loadMut.Unlock() + + modified, err := r.lockfile.Modified() + if err == nil && modified { + return r.load() + } + return err +} + func (r *imageStore) Images() ([]Image, error) { images := make([]Image, len(r.images)) for i := range r.images { @@ -878,15 +890,3 @@ func (r *imageStore) Wipe() error { } return nil } - -// reloadIfChanged reloads the contents of the store from disk if it is changed. -func (r *imageStore) reloadIfChanged() error { - r.loadMut.Lock() - defer r.loadMut.Unlock() - - modified, err := r.lockfile.Modified() - if err == nil && modified { - return r.load() - } - return err -} From e3435f912a6c98a54f03701b68ed7faeb13bbef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:25:47 +0200 Subject: [PATCH 31/43] Copy methods from included interfaces directly into *ImageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we can modify/replace them. Signed-off-by: Miloslav Trmač --- layers.go | 46 ++++++++++++++++++++++++++++++++++++++++++++-- store.go | 23 ----------------------- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/layers.go b/layers.go index e1fa41ea60..da17a1bbe4 100644 --- a/layers.go +++ b/layers.go @@ -141,10 +141,48 @@ type DiffOptions struct { // name, and keeping track of parent-child relationships, along with a list of // all known layers. type roLayerStore interface { - roFileBasedStore roMetadataStore roLayerBigDataStore + // Acquire a writer lock. + // The default unix implementation panics if: + // - opening the lockfile failed + // - tried to lock a read-only lock-file + Lock() + + // Unlock the lock. + // The default unix implementation panics if: + // - unlocking an unlocked lock + // - if the lock counter is corrupted + Unlock() + + // Acquire a reader lock. + RLock() + + // Touch records, for others sharing the lock, that the caller was the + // last writer. It should only be called with the lock held. + Touch() error + + // Modified() checks if the most recent writer was a party other than the + // last recorded writer. It should only be called with the lock held. + Modified() (bool, error) + + // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. + TouchedSince(when time.Time) bool + + // IsReadWrite() checks if the lock file is read-write + IsReadWrite() bool + + // Locked() checks if lock is locked for writing by a thread in this process + Locked() bool + + // Load reloads the contents of the store from disk. It should be called + // with the lock held. + Load() error + + // ReloadIfChanged reloads the contents of the store from disk if it is changed. + ReloadIfChanged() error + // Exists checks if a layer with the specified name or ID is known. Exists(id string) bool @@ -194,11 +232,15 @@ type roLayerStore interface { // all known layers. type rwLayerStore interface { roLayerStore - rwFileBasedStore rwMetadataStore flaggableStore rwLayerBigDataStore + // Save saves the contents of the store to disk. It should be called with + // the lock held, and Touch() should be called afterward before releasing the + // lock. + Save() error + // Create creates a new layer, optionally giving it a specified ID rather than // a randomly-generated one, either inheriting data from another specified // layer or the empty base layer. The new layer can optionally be given names diff --git a/store.go b/store.go index 8a6941fb40..d87b209dc0 100644 --- a/store.go +++ b/store.go @@ -50,29 +50,6 @@ var ( storesLock sync.Mutex ) -// roFileBasedStore wraps up the methods of the various types of file-based -// data stores that we implement which are needed for both read-only and -// read-write files. -type roFileBasedStore interface { - Locker - - // Load reloads the contents of the store from disk. It should be called - // with the lock held. - Load() error - - // ReloadIfChanged reloads the contents of the store from disk if it is changed. - ReloadIfChanged() error -} - -// rwFileBasedStore wraps up the methods of various types of file-based data -// stores that we implement using read-write files. -type rwFileBasedStore interface { - // Save saves the contents of the store to disk. It should be called with - // the lock held, and Touch() should be called afterward before releasing the - // lock. - Save() error -} - // roMetadataStore wraps a method for reading metadata associated with an ID. type roMetadataStore interface { // Metadata reads metadata associated with an item with the specified ID. From c78fe4e015571431b2ed7a09c333a240084193fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:33:18 +0200 Subject: [PATCH 32/43] Replace layerStore.{RLock,Unlock} with {startReading,stopReading} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Signed-off-by: Miloslav Trmač --- layers.go | 32 ++++++++++++++++++++++-- store.go | 74 ++++++++++++++++++++++++------------------------------- 2 files changed, 62 insertions(+), 44 deletions(-) diff --git a/layers.go b/layers.go index da17a1bbe4..f5c5cf33bd 100644 --- a/layers.go +++ b/layers.go @@ -156,8 +156,12 @@ type roLayerStore interface { // - if the lock counter is corrupted Unlock() - // Acquire a reader lock. - RLock() + // startReading makes sure the store is fresh, and locks it for reading. + // If this succeeds, the caller MUST call stopReading(). + startReading() error + + // stopReading releases locks obtained by startReading. + stopReading() // Touch records, for others sharing the lock, that the caller was the // last writer. It should only be called with the lock held. @@ -356,6 +360,30 @@ func copyLayer(l *Layer) *Layer { } } +// startReading makes sure the store is fresh, and locks it for reading. +// If this succeeds, the caller MUST call stopReading(). +func (r *layerStore) startReading() error { + r.lockfile.RLock() + succeeded := false + defer func() { + if !succeeded { + r.lockfile.Unlock() + } + }() + + if err := r.ReloadIfChanged(); err != nil { + return err + } + + succeeded = true + return nil +} + +// stopReading releases locks obtained by startReading. +func (r *layerStore) stopReading() { + r.lockfile.Unlock() +} + func (r *layerStore) Layers() ([]Layer, error) { layers := make([]Layer, len(r.layers)) for i := range r.layers { diff --git a/store.go b/store.go index d87b209dc0..36269e49c0 100644 --- a/store.go +++ b/store.go @@ -961,11 +961,10 @@ func (s *store) readAllLayerStores(fn func(store roLayerStore) (bool, error)) (b } for _, s := range layerStores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return true, err } + defer store.stopReading() if done, err := fn(store); done { return true, err } @@ -1187,11 +1186,10 @@ func (s *store) PutLayer(id, parent string, names []string, mountLabel string, w for _, l := range append([]roLayerStore{rlstore}, rlstores...) { lstore := l if lstore != rlstore { - lstore.RLock() - defer lstore.Unlock() - if err := lstore.ReloadIfChanged(); err != nil { + if err := lstore.startReading(); err != nil { return nil, -1, err } + defer lstore.stopReading() } if l, err := lstore.Get(parent); err == nil && l != nil { ilayer = l @@ -1263,13 +1261,15 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, o store := s if store == lstore { store.Lock() + defer store.Unlock() + if err := store.ReloadIfChanged(); err != nil { + return nil, err + } } else { - store.RLock() - } - defer store.Unlock() - err := store.ReloadIfChanged() - if err != nil { - return nil, err + if err := store.startReading(); err != nil { + return nil, err + } + defer store.stopReading() } ilayer, err = store.Get(layer) if err == nil { @@ -1318,11 +1318,10 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, crea for _, s := range allStores { store := s if store != rlstore { - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return nil, err } + defer store.stopReading() } // Walk the top layer list. for _, candidate := range append([]string{image.TopLayer}, image.MappedTopLayers...) { @@ -1787,11 +1786,10 @@ func (s *store) ImageSize(id string) (int64, error) { } for _, s := range layerStores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return -1, err } + defer store.stopReading() } imageStores, err := s.allImageStores() @@ -1886,11 +1884,10 @@ func (s *store) ContainerSize(id string) (int64, error) { } for _, s := range layerStores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return -1, err } + defer store.stopReading() } // Get the location of the container directory and container run directory. @@ -2624,11 +2621,10 @@ func (s *store) Mounted(id string) (int, error) { if err != nil { return 0, err } - rlstore.RLock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startReading(); err != nil { return 0, err } + defer rlstore.stopReading() return rlstore.Mounted(id) } @@ -2716,9 +2712,7 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro for _, s := range layerStores { store := s - store.RLock() - if err := store.ReloadIfChanged(); err != nil { - store.Unlock() + if err := store.startReading(); err != nil { return nil, err } if store.Exists(to) { @@ -2726,15 +2720,15 @@ func (s *store) Diff(from, to string, options *DiffOptions) (io.ReadCloser, erro if rc != nil && err == nil { wrapped := ioutils.NewReadCloserWrapper(rc, func() error { err := rc.Close() - store.Unlock() + store.stopReading() return err }) return wrapped, nil } - store.Unlock() + store.stopReading() return rc, err } - store.Unlock() + store.stopReading() } return nil, ErrLayerUnknown } @@ -2848,11 +2842,10 @@ func (s *store) LayerParentOwners(id string) ([]int, []int, error) { if err != nil { return nil, nil, err } - rlstore.RLock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startReading(); err != nil { return nil, nil, err } + defer rlstore.stopReading() if rlstore.Exists(id) { return rlstore.ParentOwners(id) } @@ -2868,11 +2861,10 @@ func (s *store) ContainerParentOwners(id string) ([]int, []int, error) { if err != nil { return nil, nil, err } - rlstore.RLock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startReading(); err != nil { return nil, nil, err } + defer rlstore.stopReading() if err := rcstore.startReading(); err != nil { return nil, nil, err } @@ -2912,11 +2904,10 @@ func (s *store) Layers() ([]Layer, error) { for _, s := range lstores { store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startReading(); err != nil { return nil, err } + defer store.stopReading() storeLayers, err := store.Layers() if err != nil { return nil, err @@ -3028,11 +3019,10 @@ func (al *additionalLayer) PutAs(id, parent string, names []string) (*Layer, err if parent != "" { for _, lstore := range append([]roLayerStore{rlstore}, rlstores...) { if lstore != rlstore { - lstore.RLock() - defer lstore.Unlock() - if err := lstore.ReloadIfChanged(); err != nil { + if err := lstore.startReading(); err != nil { return nil, err } + defer lstore.stopReading() } parentLayer, err = lstore.Get(parent) if err == nil { From bc274683d7a4aa0328fc7acd87f0ab703bf265ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:43:40 +0200 Subject: [PATCH 33/43] Avoid an unnecessary complete reload in store.Layers() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use ReloadIfChanged like every other reader. Signed-off-by: Miloslav Trmač --- store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store.go b/store.go index 36269e49c0..cf3293f3ef 100644 --- a/store.go +++ b/store.go @@ -2888,7 +2888,7 @@ func (s *store) Layers() ([]Layer, error) { layers, err := func() ([]Layer, error) { lstore.Lock() defer lstore.Unlock() - if err := lstore.Load(); err != nil { + if err := lstore.ReloadIfChanged(); err != nil { return nil, err } return lstore.Layers() From 2c61bb87d6ad1202917bbf66843ca34660c2b5e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:44:31 +0200 Subject: [PATCH 34/43] Replace layerStore.{Lock,Unlock} with {startWriting,stopWriting} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Signed-off-by: Miloslav Trmač --- layers.go | 41 ++++++++++++++++++++++++++++++----------- store.go | 50 ++++++++++++++++++++------------------------------ 2 files changed, 50 insertions(+), 41 deletions(-) diff --git a/layers.go b/layers.go index f5c5cf33bd..fd96f1938b 100644 --- a/layers.go +++ b/layers.go @@ -144,17 +144,12 @@ type roLayerStore interface { roMetadataStore roLayerBigDataStore - // Acquire a writer lock. - // The default unix implementation panics if: - // - opening the lockfile failed - // - tried to lock a read-only lock-file - Lock() - - // Unlock the lock. - // The default unix implementation panics if: - // - unlocking an unlocked lock - // - if the lock counter is corrupted - Unlock() + // startWriting makes sure the store is fresh, and locks it for writing. + // If this succeeds, the caller MUST call stopWriting(). + startWriting() error + + // stopWriting releases locks obtained by startWriting. + stopWriting() // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). @@ -360,6 +355,30 @@ func copyLayer(l *Layer) *Layer { } } +// startWriting makes sure the store is fresh, and locks it for writing. +// If this succeeds, the caller MUST call stopWriting(). +func (r *layerStore) startWriting() error { + r.lockfile.Lock() + succeeded := false + defer func() { + if !succeeded { + r.lockfile.Unlock() + } + }() + + if err := r.ReloadIfChanged(); err != nil { + return err + } + + succeeded = true + return nil +} + +// stopWriting releases locks obtained by startWriting. +func (r *layerStore) stopWriting() { + r.lockfile.Unlock() +} + // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). func (r *layerStore) startReading() error { diff --git a/store.go b/store.go index cf3293f3ef..00c3a94b2e 100644 --- a/store.go +++ b/store.go @@ -981,11 +981,10 @@ func (s *store) writeToLayerStore(fn func(store rwLayerStore) error) error { return err } - store.Lock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startWriting(); err != nil { return err } + defer store.stopWriting() return fn(store) } @@ -1117,11 +1116,10 @@ func (s *store) writeToAllStores(fn func(rlstore rwLayerStore, ristore rwImageSt return err } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return err } + defer rlstore.stopWriting() if err := ristore.startWriting(); err != nil { return err } @@ -1161,11 +1159,10 @@ func (s *store) PutLayer(id, parent string, names []string, mountLabel string, w if err != nil { return nil, -1, err } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return nil, -1, err } + defer rlstore.stopWriting() if err := rcstore.startWriting(); err != nil { return nil, -1, err } @@ -1260,11 +1257,10 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, o for _, s := range append([]roLayerStore{lstore}, lstores...) { store := s if store == lstore { - store.Lock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { + if err := store.startWriting(); err != nil { return nil, err } + defer store.stopWriting() } else { if err := store.startReading(); err != nil { return nil, err @@ -1454,11 +1450,10 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat if err != nil { return nil, err } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return nil, err } + defer rlstore.stopWriting() if err := istore.startWriting(); err != nil { return nil, err } @@ -1515,11 +1510,10 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat } } } else { - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return nil, err } + defer rlstore.stopWriting() if !options.HostUIDMapping && len(options.UIDMap) == 0 { uidMap = s.uidMap } @@ -2541,11 +2535,10 @@ func (s *store) mount(id string, options drivers.MountOpts) (string, error) { s.graphLock.Lock() defer s.graphLock.Unlock() - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return "", err } + defer rlstore.stopWriting() modified, err := s.graphLock.Modified() if err != nil { @@ -2886,11 +2879,10 @@ func (s *store) Layers() ([]Layer, error) { } layers, err := func() ([]Layer, error) { - lstore.Lock() - defer lstore.Unlock() - if err := lstore.ReloadIfChanged(); err != nil { + if err := lstore.startWriting(); err != nil { return nil, err } + defer lstore.stopWriting() return lstore.Layers() }() if err != nil { @@ -3005,11 +2997,10 @@ func (al *additionalLayer) PutAs(id, parent string, names []string) (*Layer, err if err != nil { return nil, err } - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return nil, err } + defer rlstore.stopWriting() rlstores, err := al.s.getROLayerStores() if err != nil { return nil, err @@ -3248,11 +3239,10 @@ func (s *store) Shutdown(force bool) ([]string, error) { s.graphLock.Lock() defer s.graphLock.Unlock() - rlstore.Lock() - defer rlstore.Unlock() - if err := rlstore.ReloadIfChanged(); err != nil { + if err := rlstore.startWriting(); err != nil { return nil, err } + defer rlstore.stopWriting() layers, err := rlstore.Layers() if err != nil { From 7a53e95975d3b4efd3fd786982516cb4c4f9e927 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:48:29 +0200 Subject: [PATCH 35/43] Move the writing lock methods from roLayerStore to rwLayerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... for a bit of extra safety. That requires us to be a bit more explicit in one of the users. Signed-off-by: Miloslav Trmač --- layers.go | 14 +++++++------- store.go | 24 ++++++++++++------------ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/layers.go b/layers.go index fd96f1938b..f858663441 100644 --- a/layers.go +++ b/layers.go @@ -144,13 +144,6 @@ type roLayerStore interface { roMetadataStore roLayerBigDataStore - // startWriting makes sure the store is fresh, and locks it for writing. - // If this succeeds, the caller MUST call stopWriting(). - startWriting() error - - // stopWriting releases locks obtained by startWriting. - stopWriting() - // startReading makes sure the store is fresh, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). startReading() error @@ -235,6 +228,13 @@ type rwLayerStore interface { flaggableStore rwLayerBigDataStore + // startWriting makes sure the store is fresh, and locks it for writing. + // If this succeeds, the caller MUST call stopWriting(). + startWriting() error + + // stopWriting releases locks obtained by startWriting. + stopWriting() + // Save saves the contents of the store to disk. It should be called with // the lock held, and Touch() should be called afterward before releasing the // lock. diff --git a/store.go b/store.go index 00c3a94b2e..b94b2a7f83 100644 --- a/store.go +++ b/store.go @@ -1254,22 +1254,22 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, o return nil, err } var ilayer *Layer - for _, s := range append([]roLayerStore{lstore}, lstores...) { - store := s - if store == lstore { - if err := store.startWriting(); err != nil { - return nil, err - } - defer store.stopWriting() - } else { + if err := lstore.startWriting(); err != nil { + return nil, err + } + defer lstore.stopWriting() + ilayer, err = lstore.Get(layer) + if err != nil { + for _, s := range lstores { + store := s if err := store.startReading(); err != nil { return nil, err } defer store.stopReading() - } - ilayer, err = store.Get(layer) - if err == nil { - break + ilayer, err = store.Get(layer) + if err == nil { + break + } } } if ilayer == nil { From 8018452c15d55840e1b79f2f3d8da3e141187019 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 01:50:50 +0200 Subject: [PATCH 36/43] Remove a completely unused method from roImageStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Exposing the internals of the lock is not necessary, and exposes too many implementation details. Signed-off-by: Miloslav Trmač --- layers.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/layers.go b/layers.go index f858663441..ce69ce3c4a 100644 --- a/layers.go +++ b/layers.go @@ -159,9 +159,6 @@ type roLayerStore interface { // last recorded writer. It should only be called with the lock held. Modified() (bool, error) - // TouchedSince() checks if the most recent writer modified the file (likely using Touch()) after the specified time. - TouchedSince(when time.Time) bool - // IsReadWrite() checks if the lock file is read-write IsReadWrite() bool @@ -2032,10 +2029,6 @@ func (r *layerStore) IsReadWrite() bool { return r.lockfile.IsReadWrite() } -func (r *layerStore) TouchedSince(when time.Time) bool { - return r.lockfile.TouchedSince(when) -} - func (r *layerStore) Locked() bool { return r.lockfile.Locked() } From 87bad5fd3e5fe18a5dc36b5c9dbcbcbabc1a3b12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:17:28 +0200 Subject: [PATCH 37/43] Remove unused lockfile forwarders from roLayerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only callers are internal, have them access r.lockfile directly. Signed-off-by: Miloslav Trmač --- layers.go | 68 +++++++++++++++++++------------------------------------ 1 file changed, 23 insertions(+), 45 deletions(-) diff --git a/layers.go b/layers.go index ce69ce3c4a..a88ab7e5a3 100644 --- a/layers.go +++ b/layers.go @@ -151,20 +151,10 @@ type roLayerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Touch records, for others sharing the lock, that the caller was the - // last writer. It should only be called with the lock held. - Touch() error - // Modified() checks if the most recent writer was a party other than the // last recorded writer. It should only be called with the lock held. Modified() (bool, error) - // IsReadWrite() checks if the lock file is read-write - IsReadWrite() bool - - // Locked() checks if lock is locked for writing by a thread in this process - Locked() bool - // Load reloads the contents of the store from disk. It should be called // with the lock held. Load() error @@ -437,7 +427,7 @@ func (r *layerStore) Load() error { names := make(map[string]*Layer) compressedsums := make(map[digest.Digest][]string) uncompressedsums := make(map[digest.Digest][]string) - if r.IsReadWrite() { + if r.lockfile.IsReadWrite() { selinux.ClearLabels() } if err = json.Unmarshal(data, &layers); len(data) == 0 || err == nil { @@ -461,11 +451,11 @@ func (r *layerStore) Load() error { if layer.MountLabel != "" { selinux.ReserveLabel(layer.MountLabel) } - layer.ReadOnly = !r.IsReadWrite() + layer.ReadOnly = !r.lockfile.IsReadWrite() } err = nil } - if shouldSave && (!r.IsReadWrite() || !r.Locked()) { + if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) { return ErrDuplicateLayerNames } r.layers = layers @@ -476,7 +466,7 @@ func (r *layerStore) Load() error { r.byuncompressedsum = uncompressedsums // Load and merge information about which layers are mounted, and where. - if r.IsReadWrite() { + if r.lockfile.IsReadWrite() { r.mountsLockfile.RLock() defer r.mountsLockfile.Unlock() if err = r.loadMounts(); err != nil { @@ -486,7 +476,7 @@ func (r *layerStore) Load() error { // Last step: as we’re writable, try to remove anything that a previous // user of this storage area marked for deletion but didn't manage to // actually delete. - if r.Locked() { + if r.lockfile.Locked() { for _, layer := range r.layers { if layer.Flags == nil { layer.Flags = make(map[string]interface{}) @@ -556,10 +546,10 @@ func (r *layerStore) Save() error { } func (r *layerStore) saveLayers() error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } - if !r.Locked() { + if !r.lockfile.Locked() { return errors.New("layer store is not locked for writing") } rpath := r.layerspath() @@ -573,11 +563,11 @@ func (r *layerStore) saveLayers() error { if err := ioutils.AtomicWriteFile(rpath, jldata, 0600); err != nil { return err } - return r.Touch() + return r.lockfile.Touch() } func (r *layerStore) saveMounts() error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } if !r.mountsLockfile.Locked() { @@ -693,7 +683,7 @@ func (r *layerStore) Size(name string) (int64, error) { } func (r *layerStore) ClearFlag(id string, flag string) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to clear flags on layers at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } layer, ok := r.lookup(id) @@ -705,7 +695,7 @@ func (r *layerStore) ClearFlag(id string, flag string) error { } func (r *layerStore) SetFlag(id string, flag string, value interface{}) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to set flags on layers at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } layer, ok := r.lookup(id) @@ -780,7 +770,7 @@ func (r *layerStore) PutAdditionalLayer(id string, parentLayer *Layer, names []s } func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, flags map[string]interface{}, diff io.Reader) (*Layer, int64, error) { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return nil, -1, fmt.Errorf("not allowed to create new layers at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } if err := os.MkdirAll(r.rundir, 0700); err != nil { @@ -985,7 +975,7 @@ func (r *layerStore) Create(id string, parent *Layer, names []string, mountLabel } func (r *layerStore) Mounted(id string) (int, error) { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return 0, fmt.Errorf("no mount information for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } r.mountsLockfile.RLock() @@ -1015,7 +1005,7 @@ func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error) // You are not allowed to mount layers from readonly stores if they // are not mounted read/only. - if !r.IsReadWrite() && !hasReadOnlyOpt(options.Options) { + if !r.lockfile.IsReadWrite() && !hasReadOnlyOpt(options.Options) { return "", fmt.Errorf("not allowed to update mount locations for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } r.mountsLockfile.Lock() @@ -1065,7 +1055,7 @@ func (r *layerStore) Mount(id string, options drivers.MountOpts) (string, error) } func (r *layerStore) Unmount(id string, force bool) (bool, error) { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return false, fmt.Errorf("not allowed to update mount locations for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } r.mountsLockfile.Lock() @@ -1103,7 +1093,7 @@ func (r *layerStore) Unmount(id string, force bool) (bool, error) { } func (r *layerStore) ParentOwners(id string) (uids, gids []int, err error) { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return nil, nil, fmt.Errorf("no mount information for layers at %q: %w", r.mountspath(), ErrStoreIsReadOnly) } r.mountsLockfile.RLock() @@ -1191,7 +1181,7 @@ func (r *layerStore) RemoveNames(id string, names []string) error { } func (r *layerStore) updateNames(id string, names []string, op updateNameOperation) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to change layer name assignments at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } layer, ok := r.lookup(id) @@ -1239,7 +1229,7 @@ func (r *layerStore) SetBigData(id, key string, data io.Reader) error { if key == "" { return fmt.Errorf("can't set empty name for layer big data item: %w", ErrInvalidBigDataName) } - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to save data items associated with layers at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } layer, ok := r.lookup(id) @@ -1298,7 +1288,7 @@ func (r *layerStore) Metadata(id string) (string, error) { } func (r *layerStore) SetMetadata(id, metadata string) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to modify layer metadata at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } if layer, ok := r.lookup(id); ok { @@ -1324,7 +1314,7 @@ func layerHasIncompleteFlag(layer *Layer) bool { } func (r *layerStore) deleteInternal(id string) error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete layers at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } layer, ok := r.lookup(id) @@ -1456,7 +1446,7 @@ func (r *layerStore) Get(id string) (*Layer, error) { } func (r *layerStore) Wipe() error { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return fmt.Errorf("not allowed to delete layers at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } ids := make([]string, 0, len(r.byid)) @@ -1721,7 +1711,7 @@ func (r *layerStore) ApplyDiff(to string, diff io.Reader) (size int64, err error } func (r *layerStore) applyDiffWithOptions(to string, layerOptions *LayerOptions, diff io.Reader) (size int64, err error) { - if !r.IsReadWrite() { + if !r.lockfile.IsReadWrite() { return -1, fmt.Errorf("not allowed to modify layer contents at %q: %w", r.layerspath(), ErrStoreIsReadOnly) } @@ -1989,17 +1979,13 @@ func (r *layerStore) Unlock() { r.lockfile.Unlock() } -func (r *layerStore) Touch() error { - return r.lockfile.Touch() -} - func (r *layerStore) Modified() (bool, error) { var mmodified, tmodified bool lmodified, err := r.lockfile.Modified() if err != nil { return lmodified, err } - if r.IsReadWrite() { + if r.lockfile.IsReadWrite() { r.mountsLockfile.RLock() defer r.mountsLockfile.Unlock() mmodified, err = r.mountsLockfile.Modified() @@ -2025,14 +2011,6 @@ func (r *layerStore) Modified() (bool, error) { return tmodified, nil } -func (r *layerStore) IsReadWrite() bool { - return r.lockfile.IsReadWrite() -} - -func (r *layerStore) Locked() bool { - return r.lockfile.Locked() -} - func (r *layerStore) ReloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() From c2f5d23590d97dea121f5bc5137ff04468986356 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:19:07 +0200 Subject: [PATCH 38/43] Remove Modified() from the roLayerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is an internal helper for ReloadIfChanged, with no external users. Signed-off-by: Miloslav Trmač --- layers.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/layers.go b/layers.go index a88ab7e5a3..b4da4d61d6 100644 --- a/layers.go +++ b/layers.go @@ -151,10 +151,6 @@ type roLayerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Modified() checks if the most recent writer was a party other than the - // last recorded writer. It should only be called with the lock held. - Modified() (bool, error) - // Load reloads the contents of the store from disk. It should be called // with the lock held. Load() error @@ -1979,7 +1975,9 @@ func (r *layerStore) Unlock() { r.lockfile.Unlock() } -func (r *layerStore) Modified() (bool, error) { +// modified() checks if the most recent writer was a party other than the +// last recorded writer. It should only be called with the lock held. +func (r *layerStore) modified() (bool, error) { var mmodified, tmodified bool lmodified, err := r.lockfile.Modified() if err != nil { @@ -2015,7 +2013,7 @@ func (r *layerStore) ReloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() - modified, err := r.Modified() + modified, err := r.modified() if err == nil && modified { return r.Load() } From 3807ca20b34d3734bd97cb9f4d9cd7c6158fc8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:20:20 +0200 Subject: [PATCH 39/43] Remove Load() from roLayerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač --- layers.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/layers.go b/layers.go index b4da4d61d6..f7c3756e90 100644 --- a/layers.go +++ b/layers.go @@ -151,10 +151,6 @@ type roLayerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // Load reloads the contents of the store from disk. It should be called - // with the lock held. - Load() error - // ReloadIfChanged reloads the contents of the store from disk if it is changed. ReloadIfChanged() error @@ -402,7 +398,9 @@ func (r *layerStore) layerspath() string { return filepath.Join(r.layerdir, "layers.json") } -func (r *layerStore) Load() error { +// load reloads the contents of the store from disk. It should be called +// with the lock held. +func (r *layerStore) load() error { shouldSave := false rpath := r.layerspath() info, err := os.Stat(rpath) @@ -623,7 +621,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri } rlstore.Lock() defer rlstore.Unlock() - if err := rlstore.Load(); err != nil { + if err := rlstore.load(); err != nil { return nil, err } return &rlstore, nil @@ -646,7 +644,7 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL } rlstore.RLock() defer rlstore.Unlock() - if err := rlstore.Load(); err != nil { + if err := rlstore.load(); err != nil { return nil, err } return &rlstore, nil @@ -2015,7 +2013,7 @@ func (r *layerStore) ReloadIfChanged() error { modified, err := r.modified() if err == nil && modified { - return r.Load() + return r.load() } return err } From 611012ff3448d31617381c9f7a1e82102241c536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:21:02 +0200 Subject: [PATCH 40/43] Remove ReloadIfChanged() from roLayerStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač --- layers.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/layers.go b/layers.go index f7c3756e90..b3964ba060 100644 --- a/layers.go +++ b/layers.go @@ -151,9 +151,6 @@ type roLayerStore interface { // stopReading releases locks obtained by startReading. stopReading() - // ReloadIfChanged reloads the contents of the store from disk if it is changed. - ReloadIfChanged() error - // Exists checks if a layer with the specified name or ID is known. Exists(id string) bool @@ -345,7 +342,7 @@ func (r *layerStore) startWriting() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(); err != nil { return err } @@ -369,7 +366,7 @@ func (r *layerStore) startReading() error { } }() - if err := r.ReloadIfChanged(); err != nil { + if err := r.reloadIfChanged(); err != nil { return err } @@ -2007,7 +2004,8 @@ func (r *layerStore) modified() (bool, error) { return tmodified, nil } -func (r *layerStore) ReloadIfChanged() error { +// reloadIfChanged reloads the contents of the store from disk if it is changed. +func (r *layerStore) reloadIfChanged() error { r.loadMut.Lock() defer r.loadMut.Unlock() From b925c9ff3af41f6d78a422d5e55cd3729d7664ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:23:11 +0200 Subject: [PATCH 41/43] Remove Save() from rwImageStore API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is done implicitly by all writers already. Also fix the documentation not to point at an explicit Touch(), which is not actually necessary. Signed-off-by: Miloslav Trmač --- layers.go | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/layers.go b/layers.go index b3964ba060..3076f53905 100644 --- a/layers.go +++ b/layers.go @@ -211,11 +211,6 @@ type rwLayerStore interface { // stopWriting releases locks obtained by startWriting. stopWriting() - // Save saves the contents of the store to disk. It should be called with - // the lock held, and Touch() should be called afterward before releasing the - // lock. - Save() error - // Create creates a new layer, optionally giving it a specified ID rather than // a randomly-generated one, either inheriting data from another specified // layer or the empty base layer. The new layer can optionally be given names @@ -527,7 +522,9 @@ func (r *layerStore) loadMounts() error { return err } -func (r *layerStore) Save() error { +// save saves the contents of the store to disk. It should be called with +// the lock held. +func (r *layerStore) save() error { r.mountsLockfile.Lock() defer r.mountsLockfile.Unlock() if err := r.saveLayers(); err != nil { @@ -682,7 +679,7 @@ func (r *layerStore) ClearFlag(id string, flag string) error { return ErrLayerUnknown } delete(layer.Flags, flag) - return r.Save() + return r.save() } func (r *layerStore) SetFlag(id string, flag string, value interface{}) error { @@ -697,7 +694,7 @@ func (r *layerStore) SetFlag(id string, flag string, value interface{}) error { layer.Flags = make(map[string]interface{}) } layer.Flags[flag] = value - return r.Save() + return r.save() } func (r *layerStore) Status() ([][2]string, error) { @@ -751,7 +748,7 @@ func (r *layerStore) PutAdditionalLayer(id string, parentLayer *Layer, names []s if layer.UncompressedDigest != "" { r.byuncompressedsum[layer.UncompressedDigest] = append(r.byuncompressedsum[layer.UncompressedDigest], layer.ID) } - if err := r.Save(); err != nil { + if err := r.save(); err != nil { if err2 := r.driver.Remove(id); err2 != nil { logrus.Errorf("While recovering from a failure to save layers, error deleting layer %#v: %v", id, err2) } @@ -879,7 +876,7 @@ func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLab } }() - err := r.Save() + err := r.save() if err != nil { cleanupFailureContext = "saving incomplete layer metadata" return nil, -1, err @@ -945,7 +942,7 @@ func (r *layerStore) Put(id string, parentLayer *Layer, names []string, mountLab } } delete(layer.Flags, incompleteFlag) - err = r.Save() + err = r.save() if err != nil { cleanupFailureContext = "saving finished layer metadata" return nil, -1, err @@ -1194,7 +1191,7 @@ func (r *layerStore) updateNames(id string, names []string, op updateNameOperati r.byname[name] = layer } layer.Names = names - return r.Save() + return r.save() } func (r *layerStore) datadir(id string) string { @@ -1258,7 +1255,7 @@ func (r *layerStore) SetBigData(id, key string, data io.Reader) error { } if addName { layer.BigDataNames = append(layer.BigDataNames, key) - return r.Save() + return r.save() } return nil } @@ -1284,7 +1281,7 @@ func (r *layerStore) SetMetadata(id, metadata string) error { } if layer, ok := r.lookup(id); ok { layer.Metadata = metadata - return r.Save() + return r.save() } return ErrLayerUnknown } @@ -1318,7 +1315,7 @@ func (r *layerStore) deleteInternal(id string) error { layer.Flags = make(map[string]interface{}) } layer.Flags[incompleteFlag] = true - if err := r.Save(); err != nil { + if err := r.save(); err != nil { return err } } @@ -1421,7 +1418,7 @@ func (r *layerStore) Delete(id string) error { if err := r.deleteInternal(id); err != nil { return err } - return r.Save() + return r.save() } func (r *layerStore) Exists(id string) bool { @@ -1844,7 +1841,7 @@ func (r *layerStore) applyDiffWithOptions(to string, layerOptions *LayerOptions, return layer.GIDs[i] < layer.GIDs[j] }) - err = r.Save() + err = r.save() return size, err } @@ -1885,7 +1882,7 @@ func (r *layerStore) ApplyDiffFromStagingDirectory(id, stagingDirectory string, layer.UncompressedDigest = diffOutput.UncompressedDigest layer.UncompressedSize = diffOutput.Size layer.Metadata = diffOutput.Metadata - if err = r.Save(); err != nil { + if err = r.save(); err != nil { return err } for k, v := range diffOutput.BigData { @@ -1926,7 +1923,7 @@ func (r *layerStore) ApplyDiffWithDiffer(to string, options *drivers.ApplyDiffOp } layer.UIDs = output.UIDs layer.GIDs = output.GIDs - err = r.Save() + err = r.save() return &output, err } From fd7c5b93e630971187d5a089bc9d66a30892ccc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:33:45 +0200 Subject: [PATCH 42/43] Remove Lock/Unlock methods from layerStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They are now only used in the constructors, use a variant of startReading/startWriting instead. This code path is not performance-critical, so let's share as much code as possible to ensure consistency in locking. Signed-off-by: Miloslav Trmač --- layers.go | 62 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/layers.go b/layers.go index 3076f53905..062107f95a 100644 --- a/layers.go +++ b/layers.go @@ -326,9 +326,12 @@ func copyLayer(l *Layer) *Layer { } } -// startWriting makes sure the store is fresh, and locks it for writing. +// startWritingWithReload makes sure the store is fresh if canReload, and locks it for writing. // If this succeeds, the caller MUST call stopWriting(). -func (r *layerStore) startWriting() error { +// +// This is an internal implementation detail of layerStore construction, every other caller +// should use startWriting() instead. +func (r *layerStore) startWritingWithReload(canReload bool) error { r.lockfile.Lock() succeeded := false defer func() { @@ -337,22 +340,33 @@ func (r *layerStore) startWriting() error { } }() - if err := r.reloadIfChanged(); err != nil { - return err + if canReload { + if err := r.reloadIfChanged(); err != nil { + return err + } } succeeded = true return nil } +// startWriting makes sure the store is fresh, and locks it for writing. +// If this succeeds, the caller MUST call stopWriting(). +func (r *layerStore) startWriting() error { + return r.startWritingWithReload(false) +} + // stopWriting releases locks obtained by startWriting. func (r *layerStore) stopWriting() { r.lockfile.Unlock() } -// startReading makes sure the store is fresh, and locks it for reading. +// startReadingWithReload makes sure the store is fresh if canReload, and locks it for reading. // If this succeeds, the caller MUST call stopReading(). -func (r *layerStore) startReading() error { +// +// This is an internal implementation detail of layerStore construction, every other caller +// should use startReading() instead. +func (r *layerStore) startReadingWithReload(canReload bool) error { r.lockfile.RLock() succeeded := false defer func() { @@ -361,14 +375,22 @@ func (r *layerStore) startReading() error { } }() - if err := r.reloadIfChanged(); err != nil { - return err + if canReload { + if err := r.reloadIfChanged(); err != nil { + return err + } } succeeded = true return nil } +// startReading makes sure the store is fresh, and locks it for reading. +// If this succeeds, the caller MUST call stopReading(). +func (r *layerStore) startReading() error { + return r.startReadingWithReload(true) +} + // stopReading releases locks obtained by startReading. func (r *layerStore) stopReading() { r.lockfile.Unlock() @@ -613,8 +635,10 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri bymount: make(map[string]*Layer), byname: make(map[string]*Layer), } - rlstore.Lock() - defer rlstore.Unlock() + if err := rlstore.startWritingWithReload(false); err != nil { + return nil, err + } + defer rlstore.stopWriting() if err := rlstore.load(); err != nil { return nil, err } @@ -636,8 +660,10 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL bymount: make(map[string]*Layer), byname: make(map[string]*Layer), } - rlstore.RLock() - defer rlstore.Unlock() + if err := rlstore.startReadingWithReload(false); err != nil { + return nil, err + } + defer rlstore.stopReading() if err := rlstore.load(); err != nil { return nil, err } @@ -1955,18 +1981,6 @@ func (r *layerStore) LayersByUncompressedDigest(d digest.Digest) ([]Layer, error return r.layersByDigestMap(r.byuncompressedsum, d) } -func (r *layerStore) Lock() { - r.lockfile.Lock() -} - -func (r *layerStore) RLock() { - r.lockfile.RLock() -} - -func (r *layerStore) Unlock() { - r.lockfile.Unlock() -} - // modified() checks if the most recent writer was a party other than the // last recorded writer. It should only be called with the lock held. func (r *layerStore) modified() (bool, error) { From 30280a543985b775ffe68db7dfff6cf40b1555bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:34:38 +0200 Subject: [PATCH 43/43] Move layerStore.{reloadIfChanged,modified} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .. to be closer to the lock / load set of methods. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- layers.go | 92 +++++++++++++++++++++++++++---------------------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/layers.go b/layers.go index 062107f95a..f16e32727e 100644 --- a/layers.go +++ b/layers.go @@ -396,6 +396,52 @@ func (r *layerStore) stopReading() { r.lockfile.Unlock() } +// modified() checks if the most recent writer was a party other than the +// last recorded writer. It should only be called with the lock held. +func (r *layerStore) modified() (bool, error) { + var mmodified, tmodified bool + lmodified, err := r.lockfile.Modified() + if err != nil { + return lmodified, err + } + if r.lockfile.IsReadWrite() { + r.mountsLockfile.RLock() + defer r.mountsLockfile.Unlock() + mmodified, err = r.mountsLockfile.Modified() + if err != nil { + return lmodified, err + } + } + + if lmodified || mmodified { + return true, nil + } + + // If the layers.json file has been modified manually, then we have to + // reload the storage in any case. + info, err := os.Stat(r.layerspath()) + if err != nil && !os.IsNotExist(err) { + return false, fmt.Errorf("stat layers file: %w", err) + } + if info != nil { + tmodified = info.ModTime() != r.layerspathModified + } + + return tmodified, nil +} + +// reloadIfChanged reloads the contents of the store from disk if it is changed. +func (r *layerStore) reloadIfChanged() error { + r.loadMut.Lock() + defer r.loadMut.Unlock() + + modified, err := r.modified() + if err == nil && modified { + return r.load() + } + return err +} + func (r *layerStore) Layers() ([]Layer, error) { layers := make([]Layer, len(r.layers)) for i := range r.layers { @@ -1981,52 +2027,6 @@ func (r *layerStore) LayersByUncompressedDigest(d digest.Digest) ([]Layer, error return r.layersByDigestMap(r.byuncompressedsum, d) } -// modified() checks if the most recent writer was a party other than the -// last recorded writer. It should only be called with the lock held. -func (r *layerStore) modified() (bool, error) { - var mmodified, tmodified bool - lmodified, err := r.lockfile.Modified() - if err != nil { - return lmodified, err - } - if r.lockfile.IsReadWrite() { - r.mountsLockfile.RLock() - defer r.mountsLockfile.Unlock() - mmodified, err = r.mountsLockfile.Modified() - if err != nil { - return lmodified, err - } - } - - if lmodified || mmodified { - return true, nil - } - - // If the layers.json file has been modified manually, then we have to - // reload the storage in any case. - info, err := os.Stat(r.layerspath()) - if err != nil && !os.IsNotExist(err) { - return false, fmt.Errorf("stat layers file: %w", err) - } - if info != nil { - tmodified = info.ModTime() != r.layerspathModified - } - - return tmodified, nil -} - -// reloadIfChanged reloads the contents of the store from disk if it is changed. -func (r *layerStore) reloadIfChanged() error { - r.loadMut.Lock() - defer r.loadMut.Unlock() - - modified, err := r.modified() - if err == nil && modified { - return r.load() - } - return err -} - func closeAll(closes ...func() error) (rErr error) { for _, f := range closes { if err := f(); err != nil {