mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-11-07 04:27:49 +01:00
filer: no lazy remote fetch inside a store transaction (#12439)
* filer: no lazy remote fetch inside a store transaction A rename onto an existing target deletes the target inside the store transaction, then CreateEntry's FindEntry misses the row and runs the lazy remote fetch, which persists the archive copy on a second connection. The store wrapper drops that call's deadline, so the UPDATE waits without bound on the row lock of the transaction's own uncommitted DELETE while the rename goroutine holds the path's exclusive locks: every later lookup, rename and delete of the path on that filer waits forever. A lookup whose context carries the store transaction skips the fetch. (cherry picked from commit b69e9535c2597d0c8e856f031d3778fddfd4159c) * filer: materialize remote-only rename paths before the transaction With the lazy fetch off inside a store transaction, a source or target the filer holds only on the remote would read as absent to AtomicRenameEntry and StreamRenameEntry: a rename with FailIfTargetExists could overwrite it, and a remote-only source would skip the deferred copy. PrefetchRemoteEntries runs the fetch for both paths before BeginTransaction, under the path locks. (cherry picked from commit e61f9718018669ef59768cd5eda25b6466d69597) * filer: skip the lazy remote fetch only in CreateEntry's lookup inside a transaction The guard on every lookup inside a store transaction hid a source or target the filer holds only on the remote from the rename, and the prefetch before the transaction did not reach the children of a directory rename. The deadlock needs a write: CreateEntry's lookup after the transaction deleted the path. That lookup alone skips the fetch; reads inside the transaction keep it, and their persist on a second connection holds no lock the transaction waits for. (cherry picked from commit f555092c324780a8fe5314b238694551e3ef6be5) * filer: mark a store transaction independently of the store's own key ArangoDB and FoundationDB keep their transaction under other context keys, so the CreateEntry guard keyed on "tx" missed them. The filer now marks every context returned by BeginTransaction and the guard checks that marker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commitbd0505eb80) * filer: keep the remote check for an exclusive create in a transaction Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commitb6aac15c3b) * filer: test that a create outside a transaction still consults the remote Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit972daddd84) --------- Co-authored-by: james <james@medable.com> Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
2 files changed
+59
-2
No files matched your search
+17
-2
@@ -451,8 +451,16 @@ func (fs *Filer) GetMaster(ctx context.Context) pb.ServerAddress {
|
||||
return fs.MasterClient.GetMaster(ctx)
|
||||
}
|
||||
|
||||
// storeTransactionKey marks a context inside BeginTransaction, whatever key
|
||||
// the store keeps its own transaction under.
|
||||
type storeTransactionKey struct{}
|
||||
|
||||
func (f *Filer) BeginTransaction(ctx context.Context) (context.Context, error) {
|
||||
return f.Store.BeginTransaction(ctx)
|
||||
txCtx, err := f.Store.BeginTransaction(ctx)
|
||||
if err != nil {
|
||||
return txCtx, err
|
||||
}
|
||||
return context.WithValue(txCtx, storeTransactionKey{}, true), nil
|
||||
}
|
||||
|
||||
func (f *Filer) CommitTransaction(ctx context.Context) error {
|
||||
@@ -516,7 +524,14 @@ func (f *Filer) CreateEntry(ctx context.Context, entry *Entry, existing *Entry,
|
||||
oldEntry := existing
|
||||
if oldEntry == nil {
|
||||
var findErr error
|
||||
oldEntry, findErr = f.FindEntry(ctx, entry.FullPath)
|
||||
// A lazy fetch here would persist outside the transaction and wait on
|
||||
// the lock it holds for a target it just deleted; a non-exclusive
|
||||
// create writes the path either way.
|
||||
findCtx := ctx
|
||||
if ctx.Value(storeTransactionKey{}) != nil && !o_excl {
|
||||
findCtx = context.WithValue(ctx, lazyFetchContextKey{}, true)
|
||||
}
|
||||
oldEntry, findErr = f.FindEntry(findCtx, entry.FullPath)
|
||||
if findErr != nil && !errors.Is(findErr, filer_pb.ErrNotFound) {
|
||||
// A create cannot decide whether the path exists when the lookup
|
||||
// itself failed; proceeding would upsert over it.
|
||||
|
||||
@@ -734,6 +734,48 @@ func TestMaybeLazyFetchFromRemote_ContextGuardPreventsRecursion(t *testing.T) {
|
||||
assert.Equal(t, 0, countingStub.statCalls, "guard should prevent StatFile from being called")
|
||||
}
|
||||
|
||||
func TestCreateEntry_SkipsTheLazyFetchInsideAStoreTransaction(t *testing.T) {
|
||||
const storageType = "stub_lazy_tx"
|
||||
countingStub := &countingRemoteClient{
|
||||
stubRemoteClient: stubRemoteClient{
|
||||
statResult: &filer_pb.RemoteEntry{RemoteMtime: 1, RemoteSize: 1},
|
||||
},
|
||||
}
|
||||
f, store := newMountedTestFiler(t, storageType, countingStub, 0)
|
||||
putDirEntry(store, "/buckets/mybucket/dir")
|
||||
// the stub opens no transaction of its own; the filer marks it anyway
|
||||
txCtx, err := f.BeginTransaction(context.Background())
|
||||
require.NoError(t, err)
|
||||
|
||||
// a create inside the transaction: its lookup does not consult the
|
||||
// remote, the entry is inserted as given
|
||||
created := &Entry{FullPath: "/buckets/mybucket/dir/a.txt", Attr: Attr{Mode: 0644, Mtime: time.Now(), Crtime: time.Now()}}
|
||||
require.NoError(t, f.CreateEntry(txCtx, created, nil, false, false, nil, false, f.MaxFilenameLength))
|
||||
assert.Equal(t, 0, countingStub.statCalls, "a create inside a transaction must not consult the remote")
|
||||
got, err := f.FindEntry(txCtx, "/buckets/mybucket/dir/a.txt")
|
||||
require.NoError(t, err)
|
||||
assert.False(t, got.IsInRemoteOnly())
|
||||
|
||||
// a read inside the transaction keeps the fetch: a target the filer only
|
||||
// holds on the remote is still seen by a rename's target check
|
||||
entry, err := f.FindEntry(txCtx, "/buckets/mybucket/dir/b.txt")
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, entry)
|
||||
assert.True(t, entry.IsInRemoteOnly())
|
||||
assert.NotZero(t, countingStub.statCalls)
|
||||
|
||||
// an exclusive create inside the transaction still sees the remote object
|
||||
excl := &Entry{FullPath: "/buckets/mybucket/dir/c.txt", Attr: Attr{Mode: 0644, Mtime: time.Now(), Crtime: time.Now()}}
|
||||
err = f.CreateEntry(txCtx, excl, nil, true, false, nil, false, f.MaxFilenameLength)
|
||||
assert.ErrorIs(t, err, filer_pb.ErrEntryAlreadyExists)
|
||||
|
||||
// outside a transaction a create still consults the remote
|
||||
statCalls := countingStub.statCalls
|
||||
plain := &Entry{FullPath: "/buckets/mybucket/dir/d.txt", Attr: Attr{Mode: 0644, Mtime: time.Now(), Crtime: time.Now()}}
|
||||
require.NoError(t, f.CreateEntry(context.Background(), plain, nil, false, false, nil, false, f.MaxFilenameLength))
|
||||
assert.Greater(t, countingStub.statCalls, statCalls)
|
||||
}
|
||||
|
||||
func TestMaybeLazyFetchFromRemote_GuardedClientRejectsEndpoint(t *testing.T) {
|
||||
const storageType = "stub_lazy_guarded"
|
||||
countingStub := &countingRemoteClient{
|
||||
|
||||
Reference in new issue
Block a user