Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

More small locking fixes #1430

Merged
merged 6 commits into from
Nov 14, 2022
Merged

More small locking fixes #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34ab9ed
Remove unnecessary writes to BigDataDigests and BigDataSizes
mtrmac Nov 8, 2022
e2b414c
Fix locking of containerStore.BigDataSize
mtrmac Nov 8, 2022
66f06a8
Fix locking of containerStore.BigDataDigest
mtrmac Nov 8, 2022
49ca156
Remove unnecessary writes to BigDataDigests and BigDataSizes
mtrmac Nov 8, 2022
0d8e117
Pass a rwImageStore to imageTopLayerForMapping
mtrmac Nov 11, 2022
d1e7432
Remove incorrect (*imageStore) type checks
mtrmac Nov 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions containers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,7 @@ func (r *containerStore) BigData(id, key string) ([]byte, error) {
return os.ReadFile(r.datapath(c.ID, key))
}

// Requires startWriting. Yes, really, WRITING (see SetBigData).
func (r *containerStore) BigDataSize(id, key string) (int64, error) {
if key == "" {
return -1, fmt.Errorf("can't retrieve size of container big data with empty name: %w", ErrInvalidBigDataName)
Expand All @@ -609,10 +610,7 @@ func (r *containerStore) BigDataSize(id, key string) (int64, error) {
if !ok {
return -1, ErrContainerUnknown
}
if c.BigDataSizes == nil {
c.BigDataSizes = make(map[string]int64)
}
if size, ok := c.BigDataSizes[key]; ok {
if size, ok := c.BigDataSizes[key]; ok { // This is valid, and returns ok == false, for BigDataSizes == nil.
return size, nil
}
if data, err := r.BigData(id, key); err == nil && data != nil {
Expand All @@ -631,6 +629,7 @@ func (r *containerStore) BigDataSize(id, key string) (int64, error) {
return -1, ErrSizeUnknown
}

// Requires startWriting. Yes, really, WRITING (see SetBigData).
func (r *containerStore) BigDataDigest(id, key string) (digest.Digest, error) {
if key == "" {
return "", fmt.Errorf("can't retrieve digest of container big data value with empty name: %w", ErrInvalidBigDataName)
Expand All @@ -639,10 +638,7 @@ func (r *containerStore) BigDataDigest(id, key string) (digest.Digest, error) {
if !ok {
return "", ErrContainerUnknown
}
if c.BigDataDigests == nil {
c.BigDataDigests = make(map[string]digest.Digest)
}
if d, ok := c.BigDataDigests[key]; ok {
if d, ok := c.BigDataDigests[key]; ok { // This is valid, and returns ok == false, for BigDataSizes == nil.
return d, nil
}
if data, err := r.BigData(id, key); err == nil && data != nil {
Expand Down
13 changes: 5 additions & 8 deletions images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ type rwImageStore interface {
// Delete removes the record of the image.
Delete(id string) error

addMappedTopLayer(id, layer string) error
removeMappedTopLayer(id, layer string) error

// Wipe removes records of all images.
Wipe() error
}
Expand Down Expand Up @@ -763,10 +766,7 @@ func (r *imageStore) BigDataSize(id, key string) (int64, error) {
if !ok {
return -1, fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown)
}
if image.BigDataSizes == nil {
image.BigDataSizes = make(map[string]int64)
}
if size, ok := image.BigDataSizes[key]; ok {
if size, ok := image.BigDataSizes[key]; ok { // This is valid, and returns ok == false, for BigDataSizes == nil.
return size, nil
}
if data, err := r.BigData(id, key); err == nil && data != nil {
Expand All @@ -783,10 +783,7 @@ func (r *imageStore) BigDataDigest(id, key string) (digest.Digest, error) {
if !ok {
return "", fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown)
}
if image.BigDataDigests == nil {
image.BigDataDigests = make(map[string]digest.Digest)
}
if d, ok := image.BigDataDigests[key]; ok {
if d, ok := image.BigDataDigests[key]; ok { // This is valid, and returns ok == false, for BigDataDigests == nil.
return d, nil
}
return "", ErrDigestUnknown
Expand Down
212 changes: 96 additions & 116 deletions store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1282,9 +1282,10 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, o
// imageTopLayerForMapping does ???
// On entry:
// - ristore must be locked EITHER for reading or writing
// - primaryImageStore must be locked for writing; it might be identical to ristore.
// - rlstore must be locked for writing
// - lstores must all be locked for reading
func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, createMappedLayer bool, rlstore rwLayerStore, lstores []roLayerStore, options types.IDMappingOptions) (*Layer, error) {
func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, primaryImageStore rwImageStore, rlstore rwLayerStore, lstores []roLayerStore, options types.IDMappingOptions) (*Layer, error) {
layerMatchesMappingOptions := func(layer *Layer, options types.IDMappingOptions) bool {
// If the driver supports shifting and the layer has no mappings, we can use it.
if s.canUseShifting(options.UIDMap, options.GIDMap) && len(layer.UIDMap) == 0 && len(layer.GIDMap) == 0 {
Expand All @@ -1303,6 +1304,7 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, crea
var layer, parentLayer *Layer
allStores := append([]roLayerStore{rlstore}, lstores...)
// Locate the image's top layer and its parent, if it has one.
createMappedLayer := ristore == primaryImageStore
for _, s := range allStores {
store := s
// Walk the top layer list.
Expand Down Expand Up @@ -1350,44 +1352,41 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, crea
return layer, nil
}
// The top layer's mappings don't match the ones we want, and it's in an image store
// that lets us edit image metadata...
if istore, ok := ristore.(*imageStore); ok {
// ... so create a duplicate of the layer with the desired mappings, and
// register it as an alternate top layer in the image.
var layerOptions LayerOptions
if s.canUseShifting(options.UIDMap, options.GIDMap) {
layerOptions = LayerOptions{
IDMappingOptions: types.IDMappingOptions{
HostUIDMapping: true,
HostGIDMapping: true,
UIDMap: nil,
GIDMap: nil,
},
}
} else {
layerOptions = LayerOptions{
IDMappingOptions: types.IDMappingOptions{
HostUIDMapping: options.HostUIDMapping,
HostGIDMapping: options.HostGIDMapping,
UIDMap: copyIDMap(options.UIDMap),
GIDMap: copyIDMap(options.GIDMap),
},
}
// that lets us edit image metadata, so create a duplicate of the layer with the desired
// mappings, and register it as an alternate top layer in the image.
var layerOptions LayerOptions
if s.canUseShifting(options.UIDMap, options.GIDMap) {
layerOptions = LayerOptions{
IDMappingOptions: types.IDMappingOptions{
HostUIDMapping: true,
HostGIDMapping: true,
UIDMap: nil,
GIDMap: nil,
},
}
layerOptions.TemplateLayer = layer.ID
mappedLayer, _, err := rlstore.Put("", parentLayer, nil, layer.MountLabel, nil, &layerOptions, false, nil, nil)
if err != nil {
return nil, fmt.Errorf("creating an ID-mapped copy of layer %q: %w", layer.ID, err)
} else {
layerOptions = LayerOptions{
IDMappingOptions: types.IDMappingOptions{
HostUIDMapping: options.HostUIDMapping,
HostGIDMapping: options.HostGIDMapping,
UIDMap: copyIDMap(options.UIDMap),
GIDMap: copyIDMap(options.GIDMap),
},
}
if err = istore.addMappedTopLayer(image.ID, mappedLayer.ID); err != nil {
if err2 := rlstore.Delete(mappedLayer.ID); err2 != nil {
err = fmt.Errorf("deleting layer %q: %v: %w", mappedLayer.ID, err2, err)
}
return nil, fmt.Errorf("registering ID-mapped layer with image %q: %w", image.ID, err)
}
layerOptions.TemplateLayer = layer.ID
mappedLayer, _, err := rlstore.Put("", parentLayer, nil, layer.MountLabel, nil, &layerOptions, false, nil, nil)
if err != nil {
return nil, fmt.Errorf("creating an ID-mapped copy of layer %q: %w", layer.ID, err)
}
// By construction, createMappedLayer can only be true if ristore == primaryImageStore.
if err = primaryImageStore.addMappedTopLayer(image.ID, mappedLayer.ID); err != nil {
if err2 := rlstore.Delete(mappedLayer.ID); err2 != nil {
err = fmt.Errorf("deleting layer %q: %v: %w", mappedLayer.ID, err2, err)
}
layer = mappedLayer
return nil, fmt.Errorf("registering ID-mapped layer with image %q: %w", image.ID, err)
}
return layer, nil
return mappedLayer, nil
}

func (s *store) CreateContainer(id string, names []string, image, layer, metadata string, options *ContainerOptions) (*Container, error) {
Expand Down Expand Up @@ -1488,8 +1487,7 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat
idMappingsOptions := options.IDMappingOptions
if image != "" {
if cimage.TopLayer != "" {
createMappedLayer := imageHomeStore == istore
ilayer, err := s.imageTopLayerForMapping(cimage, imageHomeStore, createMappedLayer, rlstore, lstores, idMappingsOptions)
ilayer, err := s.imageTopLayerForMapping(cimage, imageHomeStore, istore, rlstore, lstores, idMappingsOptions)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1888,63 +1886,59 @@ func (s *store) ContainerSize(id string) (int64, error) {
return -1, err
}

rcstore, err := s.getContainerStore()
if err != nil {
return -1, err
}
if err := rcstore.startReading(); err != nil {
return -1, err
}
defer rcstore.stopReading()
var res int64 = -1
err = s.writeToContainerStore(func(rcstore rwContainerStore) error { // Yes, rcstore.BigDataSize requires a write lock.
// Read the container record.
container, err := rcstore.Get(id)
if err != nil {
return err
}

// Read the container record.
container, err := rcstore.Get(id)
if err != nil {
return -1, err
}
// Read the container's layer's size.
var layer *Layer
var size int64
for _, store := range layerStores {
if layer, err = store.Get(container.LayerID); err == nil {
size, err = store.DiffSize("", layer.ID)
if err != nil {
return fmt.Errorf("determining size of layer with ID %q: %w", layer.ID, err)
}
break
}
}
if layer == nil {
return fmt.Errorf("locating layer with ID %q: %w", container.LayerID, ErrLayerUnknown)
}

// Read the container's layer's size.
var layer *Layer
var size int64
for _, store := range layerStores {
if layer, err = store.Get(container.LayerID); err == nil {
size, err = store.DiffSize("", layer.ID)
// Count big data items.
names, err := rcstore.BigDataNames(id)
if err != nil {
return fmt.Errorf("reading list of big data items for container %q: %w", container.ID, err)
}
for _, name := range names {
n, err := rcstore.BigDataSize(id, name)
if err != nil {
return -1, fmt.Errorf("determining size of layer with ID %q: %w", layer.ID, err)
return fmt.Errorf("reading size of big data item %q for container %q: %w", name, id, err)
}
break
size += n
}
}
if layer == nil {
return -1, fmt.Errorf("locating layer with ID %q: %w", container.LayerID, ErrLayerUnknown)
}

// Count big data items.
names, err := rcstore.BigDataNames(id)
if err != nil {
return -1, fmt.Errorf("reading list of big data items for container %q: %w", container.ID, err)
}
for _, name := range names {
n, err := rcstore.BigDataSize(id, name)
// Count the size of our container directory and container run directory.
n, err := directory.Size(cdir)
if err != nil {
return -1, fmt.Errorf("reading size of big data item %q for container %q: %w", name, id, err)
return err
}
size += n
n, err = directory.Size(rdir)
if err != nil {
return err
}
size += n
}

// Count the size of our container directory and container run directory.
n, err := directory.Size(cdir)
if err != nil {
return -1, err
}
size += n
n, err = directory.Size(rdir)
if err != nil {
return -1, err
}
size += n

return size, nil
res = size
return nil
})
return res, err
}

func (s *store) ListContainerBigData(id string) ([]string, error) {
Expand All @@ -1962,27 +1956,23 @@ func (s *store) ListContainerBigData(id string) ([]string, error) {
}

func (s *store) ContainerBigDataSize(id, key string) (int64, error) {
rcstore, err := s.getContainerStore()
if err != nil {
return -1, err
}
if err := rcstore.startReading(); err != nil {
return -1, err
}
defer rcstore.stopReading()
return rcstore.BigDataSize(id, key)
var res int64 = -1
err := s.writeToContainerStore(func(store rwContainerStore) error { // Yes, BigDataSize requires a write lock.
var err error
res, err = store.BigDataSize(id, key)
return err
})
return res, err
}

func (s *store) ContainerBigDataDigest(id, key string) (digest.Digest, error) {
rcstore, err := s.getContainerStore()
if err != nil {
return "", err
}
if err := rcstore.startReading(); err != nil {
return "", err
}
defer rcstore.stopReading()
return rcstore.BigDataDigest(id, key)
var res digest.Digest
err := s.writeToContainerStore(func(store rwContainerStore) error { // Yes, BigDataDigest requires a write lock.
var err error
res, err = store.BigDataDigest(id, key)
return err
})
return res, err
}

func (s *store) ContainerBigData(id, key string) ([]byte, error) {
Expand Down Expand Up @@ -2222,12 +2212,6 @@ func (s *store) DeleteLayer(id string) error {
if image.TopLayer == id {
return fmt.Errorf("layer %v used by image %v: %w", id, image.ID, ErrLayerUsedByImage)
}
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)
}
}
}
containers, err := rcstore.Containers()
if err != nil {
Expand All @@ -2242,14 +2226,10 @@ func (s *store) DeleteLayer(id string) error {
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)
}
for _, image := range images {
if stringutils.InSlice(image.MappedTopLayers, id) {
if err = ristore.removeMappedTopLayer(image.ID, id); err != nil {
return fmt.Errorf("remove mapped top layer %v from image %v: %w", id, image.ID, err)
}
}
}
Expand Down