From 8783e3b754348396dd76dc895d134a831daf8968 Mon Sep 17 00:00:00 2001 From: Adam Hughes <9903835+tri-adam@users.noreply.github.com> Date: Thu, 4 Jul 2024 19:33:53 +0000 Subject: [PATCH 1/2] refactor: improve compaction logic for AddObject Previously, an add operation would simply append the data object to the end of the data section. If one or more object(s) had previously been deleted from the image, this could result in wasted space. Now, we trim the data section based on in-use objects before adding the new object. Update tests to reflect this new behaviour. --- pkg/sif/create.go | 18 +++++++++++++++++- .../NoCompact.golden | Bin 32185 -> 32182 bytes .../TestDeleteObjectAndAddObject/Zero.golden | Bin 32185 -> 32182 bytes 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/sif/create.go b/pkg/sif/create.go index 0f41e0a9..0eb1e1d1 100644 --- a/pkg/sif/create.go +++ b/pkg/sif/create.go @@ -1,4 +1,4 @@ -// Copyright (c) 2018-2023, Sylabs Inc. All rights reserved. +// Copyright (c) 2018-2024, Sylabs Inc. All rights reserved. // Copyright (c) 2017, SingularityWare, LLC. All rights reserved. // Copyright (c) 2017, Yannick Cote All rights reserved. // This software is licensed under a 3-clause BSD license. Please consult the @@ -55,6 +55,20 @@ func writeDataObjectAt(ws io.WriteSeeker, offsetUnaligned int64, di DescriptorIn return nil } +// calculatedDataSize calculates the size of the data section based on the in-use descriptors. +func (f *FileImage) calculatedDataSize() int64 { + dataEnd := f.DataOffset() + + f.WithDescriptors(func(d Descriptor) bool { + if objectEnd := d.Offset() + d.Size(); dataEnd < objectEnd { + dataEnd = objectEnd + } + return false + }) + + return dataEnd - f.DataOffset() +} + var ( errInsufficientCapacity = errors.New("insufficient descriptor capacity to add data object(s) to image") errPrimaryPartition = errors.New("image already contains a primary partition") @@ -80,6 +94,8 @@ func (f *FileImage) writeDataObject(i int, di DescriptorInput, t time.Time) erro d := &f.rds[i] d.ID = uint32(i) + 1 + f.h.DataSize = f.calculatedDataSize() + if err := writeDataObjectAt(f.rw, f.h.DataOffset+f.h.DataSize, di, t, d); err != nil { return err } diff --git a/pkg/sif/testdata/TestDeleteObjectAndAddObject/NoCompact.golden b/pkg/sif/testdata/TestDeleteObjectAndAddObject/NoCompact.golden index 7042b4ace3e0f0757bbc594fc47461e457e84dfe..483ef82e1e456df6e124e5615b31adfe2edccaa6 100644 GIT binary patch delta 30 mcmdn_n{nH3#t9XSY?~(vwexQhh+<;gyg6DZBAYoqBNG6_DGO5o delta 33 pcmdn?n{nrF#t9XSoSP>KwexQhh+<;gwmDiTBAYEGH7z|O69DJl42u8& diff --git a/pkg/sif/testdata/TestDeleteObjectAndAddObject/Zero.golden b/pkg/sif/testdata/TestDeleteObjectAndAddObject/Zero.golden index bacfe7aff919f467af09461be1bef713e4bd987d..483ef82e1e456df6e124e5615b31adfe2edccaa6 100644 GIT binary patch delta 30 mcmdn_n{nH3#t9XSY?~(vwexQhh+<;gyg6DZBAYoqBNG6_DGO5o delta 33 pcmdn?n{nrF#t9XSoSP>KwexQhh+<;gwmDiTBAbnYfgwF369D1$3tRvI From e8dad67f88b7af7d7d3d4933d1c301428c17a319 Mon Sep 17 00:00:00 2001 From: Adam Hughes <9903835+tri-adam@users.noreply.github.com> Date: Thu, 4 Jul 2024 19:54:32 +0000 Subject: [PATCH 2/2] refactor: improve delete compaction logic Improve compaction via OptDeleteCompact. Do not fail when compaction is requested on a data object that is not at the end of the data section. When compaction is requested, always calculate the end of the data section based on the in-use descriptors. --- pkg/sif/delete.go | 49 ++-------- pkg/sif/delete_test.go | 90 +++++++++++++++--- .../TestDeleteObject/OneCompact.golden | Bin 0 -> 32180 bytes .../testdata/TestDeleteObject/OneZero.golden | Bin 0 -> 32180 bytes .../TestDeleteObject/OneZeroCompact.golden | Bin 0 -> 32180 bytes .../{Zero.golden => TwoCompact.golden} | Bin 32178 -> 32178 bytes .../testdata/TestDeleteObject/TwoZero.golden | Bin 0 -> 32180 bytes ...roCompact.golden => TwoZeroCompact.golden} | Bin 32176 -> 32178 bytes 8 files changed, 86 insertions(+), 53 deletions(-) create mode 100644 pkg/sif/testdata/TestDeleteObject/OneCompact.golden create mode 100644 pkg/sif/testdata/TestDeleteObject/OneZero.golden create mode 100644 pkg/sif/testdata/TestDeleteObject/OneZeroCompact.golden rename pkg/sif/testdata/TestDeleteObject/{Zero.golden => TwoCompact.golden} (99%) create mode 100644 pkg/sif/testdata/TestDeleteObject/TwoZero.golden rename pkg/sif/testdata/TestDeleteObject/{ZeroCompact.golden => TwoZeroCompact.golden} (99%) diff --git a/pkg/sif/delete.go b/pkg/sif/delete.go index c0f0aba3..41c0a726 100644 --- a/pkg/sif/delete.go +++ b/pkg/sif/delete.go @@ -1,4 +1,4 @@ -// Copyright (c) 2018-2023, Sylabs Inc. All rights reserved. +// Copyright (c) 2018-2024, Sylabs Inc. All rights reserved. // Copyright (c) 2017, SingularityWare, LLC. All rights reserved. // Copyright (c) 2017, Yannick Cote All rights reserved. // This software is licensed under a 3-clause BSD license. Please consult the @@ -8,32 +8,16 @@ package sif import ( - "errors" "fmt" "io" "time" ) -// isLast return true if the data object associated with d is the last in f. -func (f *FileImage) isLast(d *rawDescriptor) bool { - isLast := true - - end := d.Offset + d.Size - f.WithDescriptors(func(d Descriptor) bool { - isLast = d.Offset()+d.Size() <= end - return !isLast - }) - - return isLast -} - // zeroReader is an io.Reader that returns a stream of zero-bytes. type zeroReader struct{} func (zeroReader) Read(b []byte) (int, error) { - for i := range b { - b[i] = 0 - } + clear(b) return len(b), nil } @@ -47,13 +31,6 @@ func (f *FileImage) zero(d *rawDescriptor) error { return err } -// truncateAt truncates f at the start of the padded data object described by d. -func (f *FileImage) truncateAt(d *rawDescriptor) error { - start := d.Offset + d.Size - d.SizeWithPadding - - return f.rw.Truncate(start) -} - // deleteOpts accumulates object deletion options. type deleteOpts struct { zero bool @@ -97,8 +74,6 @@ func OptDeleteWithTime(t time.Time) DeleteOpt { } } -var errCompactNotImplemented = errors.New("compact not implemented for non-last object") - // DeleteObject deletes the data object with id, according to opts. // // To zero the data region of the deleted object, use OptDeleteZero. To compact the file following @@ -125,24 +100,12 @@ func (f *FileImage) DeleteObject(id uint32, opts ...DeleteOpt) error { return fmt.Errorf("%w", err) } - if do.compact && !f.isLast(d) { - return fmt.Errorf("%w", errCompactNotImplemented) - } - if do.zero { if err := f.zero(d); err != nil { return fmt.Errorf("%w", err) } } - if do.compact { - if err := f.truncateAt(d); err != nil { - return fmt.Errorf("%w", err) - } - - f.h.DataSize -= d.SizeWithPadding - } - f.h.DescriptorsFree++ f.h.ModifiedAt = do.t.Unix() @@ -156,6 +119,14 @@ func (f *FileImage) DeleteObject(id uint32, opts ...DeleteOpt) error { // Reset rawDescripter with empty struct *d = rawDescriptor{} + if do.compact { + f.h.DataSize = f.calculatedDataSize() + + if err := f.rw.Truncate(f.h.DataOffset + f.h.DataSize); err != nil { + return fmt.Errorf("%w", err) + } + } + if err := f.writeDescriptors(); err != nil { return fmt.Errorf("%w", err) } diff --git a/pkg/sif/delete_test.go b/pkg/sif/delete_test.go index 683e00dd..a7c2a165 100644 --- a/pkg/sif/delete_test.go +++ b/pkg/sif/delete_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2018-2023, Sylabs Inc. All rights reserved. +// Copyright (c) 2018-2024, Sylabs Inc. All rights reserved. // This software is licensed under a 3-clause BSD license. Please consult the // LICENSE file distributed with the sources of this project regarding your // rights to use or distribute this software. @@ -17,7 +17,7 @@ func TestDeleteObject(t *testing.T) { tests := []struct { name string createOpts []CreateOpt - id uint32 + ids []uint32 opts []DeleteOpt wantErr error }{ @@ -26,44 +26,104 @@ func TestDeleteObject(t *testing.T) { createOpts: []CreateOpt{ OptCreateDeterministic(), }, - id: 1, + ids: []uint32{1}, wantErr: ErrObjectNotFound, }, { - name: "Zero", + name: "Compact", + createOpts: []CreateOpt{ + OptCreateDeterministic(), + OptCreateWithDescriptors( + getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), + ), + }, + ids: []uint32{1, 2}, + opts: []DeleteOpt{ + OptDeleteCompact(true), + }, + }, + { + name: "OneZero", createOpts: []CreateOpt{ OptCreateDeterministic(), OptCreateWithDescriptors( getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), ), }, - id: 1, + ids: []uint32{1}, opts: []DeleteOpt{ OptDeleteZero(true), }, }, { - name: "Compact", + name: "OneCompact", createOpts: []CreateOpt{ OptCreateDeterministic(), OptCreateWithDescriptors( getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), ), }, - id: 1, + ids: []uint32{1}, opts: []DeleteOpt{ OptDeleteCompact(true), }, }, { - name: "ZeroCompact", + name: "OneZeroCompact", + createOpts: []CreateOpt{ + OptCreateDeterministic(), + OptCreateWithDescriptors( + getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), + ), + }, + ids: []uint32{1}, + opts: []DeleteOpt{ + OptDeleteZero(true), + OptDeleteCompact(true), + }, + }, + { + name: "TwoZero", + createOpts: []CreateOpt{ + OptCreateDeterministic(), + OptCreateWithDescriptors( + getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), + ), + }, + ids: []uint32{2}, + opts: []DeleteOpt{ + OptDeleteZero(true), + }, + }, + { + name: "TwoCompact", + createOpts: []CreateOpt{ + OptCreateDeterministic(), + OptCreateWithDescriptors( + getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), + ), + }, + ids: []uint32{2}, + opts: []DeleteOpt{ + OptDeleteCompact(true), + }, + }, + { + name: "TwoZeroCompact", createOpts: []CreateOpt{ OptCreateDeterministic(), OptCreateWithDescriptors( getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), + getDescriptorInput(t, DataGeneric, []byte{0xfe, 0xed}), ), }, - id: 1, + ids: []uint32{2}, opts: []DeleteOpt{ OptDeleteZero(true), OptDeleteCompact(true), @@ -78,7 +138,7 @@ func TestDeleteObject(t *testing.T) { ), OptCreateWithTime(time.Unix(946702800, 0)), }, - id: 1, + ids: []uint32{1}, opts: []DeleteOpt{ OptDeleteDeterministic(), }, @@ -91,7 +151,7 @@ func TestDeleteObject(t *testing.T) { getDescriptorInput(t, DataGeneric, []byte{0xfa, 0xce}), ), }, - id: 1, + ids: []uint32{1}, opts: []DeleteOpt{ OptDeleteWithTime(time.Unix(946702800, 0)), }, @@ -106,7 +166,7 @@ func TestDeleteObject(t *testing.T) { ), ), }, - id: 1, + ids: []uint32{1}, }, } @@ -119,8 +179,10 @@ func TestDeleteObject(t *testing.T) { t.Fatal(err) } - if got, want := f.DeleteObject(tt.id, tt.opts...), tt.wantErr; !errors.Is(got, want) { - t.Errorf("got error %v, want %v", got, want) + for _, id := range tt.ids { + if got, want := f.DeleteObject(id, tt.opts...), tt.wantErr; !errors.Is(got, want) { + t.Errorf("got error %v, want %v", got, want) + } } if err := f.UnloadContainer(); err != nil { diff --git a/pkg/sif/testdata/TestDeleteObject/OneCompact.golden b/pkg/sif/testdata/TestDeleteObject/OneCompact.golden new file mode 100644 index 0000000000000000000000000000000000000000..cb86f62d5719233811f5bdc7e2527fed9897d562 GIT binary patch literal 32180 zcmeI$u?fOZ5C-6j2x8|ZJ~vPVC54p(IDu|p?>OQhZX#%FVPeQj8o|y&_^x|ANBwM literal 0 HcmV?d00001 diff --git a/pkg/sif/testdata/TestDeleteObject/OneZero.golden b/pkg/sif/testdata/TestDeleteObject/OneZero.golden new file mode 100644 index 0000000000000000000000000000000000000000..ec7f18be315a571430b57f408ff038f1365911de GIT binary patch literal 32180 zcmeI$u?Ye}5CzaV5yZ?+JUdVXFM%JDMiV7nHVN#xp6&~Jm(yjIYEE`0RjXF5FkK+009C7 z2oNCfCjygYL^W!cts)-X&bM;u>3UuD*!D1PBlyK!5-N0t5&UAV7cs0RjXF z5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0zVOW-QQ7d B9xea? literal 0 HcmV?d00001 diff --git a/pkg/sif/testdata/TestDeleteObject/OneZeroCompact.golden b/pkg/sif/testdata/TestDeleteObject/OneZeroCompact.golden new file mode 100644 index 0000000000000000000000000000000000000000..ec7f18be315a571430b57f408ff038f1365911de GIT binary patch literal 32180 zcmeI$u?Ye}5CzaV5yZ?+JUdVXFM%JDMiV7nHVN#xp6&~Jm(yjIYEE`0RjXF5FkK+009C7 z2oNCfCjygYL^W!cts)-X&bM;u>3UuD*!D1PBlyK!5-N0t5&UAV7cs0RjXF z5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0zVOW-QQ7d B9xea? literal 0 HcmV?d00001 diff --git a/pkg/sif/testdata/TestDeleteObject/Zero.golden b/pkg/sif/testdata/TestDeleteObject/TwoCompact.golden similarity index 99% rename from pkg/sif/testdata/TestDeleteObject/Zero.golden rename to pkg/sif/testdata/TestDeleteObject/TwoCompact.golden index 036b3690a02c7a69c1fd86dd0562ce64f7395f3f..74b6489cc62b849d79558a5c92de01b40fb09315 100644 GIT binary patch delta 79 zcmdn=n{m@`#t8w8`Wpk+^INhzFfcGOG5`q%h7UlBVM8s5WME)|P;i=oGq2<0|NsAC M^v1^WT&7>=0O*$%ivR!s delta 22 ecmdn=n{m@`#t8w81{(v{^KT9iNXVTyAqW6@&Z4c&nIpMt1Utdv3nV{D9z6KE z7x8@^o89TK**Dv$S5eoW#q-PU*>zo%T$YNla*-d}bIEJYX{i$g2oNAZfB*pk1PBly zK!CvS1!n7rs!Dy*e)R3yGN4cTOIkw?1OAx#+ev@`0RjXF5FkK+009C72oNAZfB*pk z1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs z0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+0D*rMc-3apfith; R