Skip to content

Commit

Permalink
objstorage: fix preallocated handle initialization
Browse files Browse the repository at this point in the history
We were not initializing the readahead state when using a preallocated
handle. We add an assertion that would have failed and improve an
existing test to use the preallocated handle sometimes.

We also use a preallocated handle for the single level iterator (using
it only for the two level was an omission).
  • Loading branch information
RaduBerinde committed Jun 11, 2024
1 parent 6b251de commit 4a5d7e3
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 8 deletions.
9 changes: 8 additions & 1 deletion objstorage/objstorageprovider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,14 @@ func TestProvider(t *testing.T) {
if err != nil {
return err.Error()
}
rh := r.NewReadHandle(ctx)
var rh objstorage.ReadHandle
// Test both ways of getting a handle.
if rand.Intn(2) == 0 {
rh = r.NewReadHandle(ctx)
} else {
var prealloc PreallocatedReadHandle
rh = UsePreallocatedReadHandle(ctx, r, &prealloc)
}
if forCompaction {
rh.SetupForCompaction()
}
Expand Down
5 changes: 5 additions & 0 deletions objstorage/objstorageprovider/readahead.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

package objstorageprovider

import "github.com/cockroachdb/pebble/internal/invariants"

const (
// Constants for dynamic readahead of data blocks. Note that the size values
// make sense as some multiple of the default block size; and they should
Expand Down Expand Up @@ -81,6 +83,9 @@ func (rs *readaheadState) recordCacheHit(offset, blockLength int64) {
// Returns a size value (greater than 0) that should be prefetched if readahead
// would be beneficial.
func (rs *readaheadState) maybeReadahead(offset, blockLength int64) int64 {
if invariants.Enabled && rs.maxReadaheadSize == 0 {
panic("readaheadState not initialized")
}
currentReadEnd := offset + blockLength
if rs.numReads >= minFileReadsForReadahead {
// The minimum threshold of sequential reads to justify reading ahead
Expand Down
19 changes: 17 additions & 2 deletions objstorage/objstorageprovider/testdata/provider/local_readahead
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ create 1 local 1 2000000
<local fs> sync-data: p1/000001.sst
<local fs> close: p1/000001.sst

# We should see prefetch calls, and eventually a reopen
# (with sequential reads option).
# We should see prefetch calls, and eventually a reopen with sequential reads
# option.
read 1
0 1000
1000 15000
Expand Down Expand Up @@ -49,3 +49,18 @@ size: 2000000
140000 80000: ok (salt 1)
<local fs> close: p1/000001.sst
<local fs> close: p1/000001.sst

# We should directly see a reopen with sequential reads option.
read 1 for-compaction
0 1000
1000 15000
----
<local fs> open: p1/000001.sst (options: *vfs.randomReadsOption)
<local fs> open: p1/000001.sst (options: *vfs.sequentialReadsOption)
size: 2000000
<local fs> read-at(0, 1000): p1/000001.sst
0 1000: ok (salt 1)
<local fs> read-at(1000, 15000): p1/000001.sst
1000 15000: ok (salt 1)
<local fs> close: p1/000001.sst
<local fs> close: p1/000001.sst
13 changes: 9 additions & 4 deletions objstorage/objstorageprovider/vfs_readable.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ func (r *fileReadable) Size() int64 {
// NewReadHandle is part of the objstorage.Readable interface.
func (r *fileReadable) NewReadHandle(_ context.Context) objstorage.ReadHandle {
rh := readHandlePool.Get().(*vfsReadHandle)
rh.r = r
rh.rs = makeReadaheadState(fileMaxReadaheadSize)
rh.init(r)
return rh
}

Expand Down Expand Up @@ -108,6 +107,13 @@ var readHandlePool = sync.Pool{
},
}

func (rh *vfsReadHandle) init(r *fileReadable) {
*rh = vfsReadHandle{
r: r,
rs: makeReadaheadState(fileMaxReadaheadSize),
}
}

// Close is part of the objstorage.ReadHandle interface.
func (rh *vfsReadHandle) Close() error {
var err error
Expand Down Expand Up @@ -208,8 +214,7 @@ func UsePreallocatedReadHandle(
ctx context.Context, readable objstorage.Readable, rh *PreallocatedReadHandle,
) objstorage.ReadHandle {
if r, ok := readable.(*fileReadable); ok {
// See fileReadable.NewReadHandle.
rh.vfsReadHandle = vfsReadHandle{r: r}
rh.init(r)
return rh
}
return readable.NewReadHandle(ctx)
Expand Down
3 changes: 2 additions & 1 deletion sstable/reader_iter_two_lvl.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"

"github.com/cockroachdb/pebble/internal/base"
"github.com/cockroachdb/pebble/objstorage/objstorageprovider"
"github.com/cockroachdb/pebble/objstorage/objstorageprovider/objiotracing"
)

Expand Down Expand Up @@ -177,7 +178,7 @@ func (i *twoLevelIterator) init(
_ = i.topLevelIndex.Close()
return err
}
i.dataRH = r.readable.NewReadHandle(ctx)
i.dataRH = objstorageprovider.UsePreallocatedReadHandle(ctx, r.readable, &i.dataRHPrealloc)
if r.tableFormat >= TableFormatPebblev3 {
if r.Properties.NumValueBlocks > 0 {
i.vbReader = &valueBlockReader{
Expand Down

0 comments on commit 4a5d7e3

Please sign in to comment.