From 83195fc111be273de0e45a0d2bc25fd0245e766f Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sat, 23 May 2026 21:40:41 -0700 Subject: [PATCH] filer: reuse the caller's fetched entry in CreateEntry (#9645) CreateEntry starts with a FindEntry to load the current entry. A conditional CreateEntry already fetched that entry to evaluate the precondition under the per-path lock, so the create repeated the lookup. Add an existing *Entry parameter: when non-nil it is used as the current entry and the internal lookup is skipped; nil keeps the lookup. The gRPC CreateEntry handler passes the entry it fetched for the precondition, removing the redundant read while the lock is held. All other callers pass nil. --- weed/filer/filer.go | 11 +++++- weed/filer/filer_inode_test.go | 4 +- weed/filer/filer_lazy_remote.go | 2 +- weed/filer/filer_lazy_remote_listing.go | 4 +- weed/filer/filer_notify_append.go | 2 +- weed/filer/leveldb/leveldb_store_test.go | 2 +- weed/filer/leveldb2/leveldb2_store_test.go | 2 +- weed/filer/leveldb3/leveldb3_store_test.go | 2 +- weed/filer/rocksdb/rocksdb_store_test.go | 4 +- weed/server/filer_grpc_server.go | 10 +++-- .../filer_grpc_server_condition_test.go | 39 +++++++++++++++++++ weed/server/filer_grpc_server_rename.go | 2 +- weed/server/filer_server_handlers_copy.go | 2 +- weed/server/filer_server_handlers_tagging.go | 4 +- .../filer_server_handlers_write_autochunk.go | 4 +- weed/server/filer_server_tus_session.go | 8 ++-- 16 files changed, 76 insertions(+), 26 deletions(-) diff --git a/weed/filer/filer.go b/weed/filer/filer.go index 420aa1761..c4421ed37 100644 --- a/weed/filer/filer.go +++ b/weed/filer/filer.go @@ -209,7 +209,11 @@ func (f *Filer) RollbackTransaction(ctx context.Context) error { return f.Store.RollbackTransaction(ctx) } -func (f *Filer) CreateEntry(ctx context.Context, entry *Entry, o_excl bool, isFromOtherCluster bool, signatures []int32, skipCreateParentDir bool, maxFilenameLength uint32) error { +// CreateEntry creates or replaces an entry. When existing is non-nil the caller +// has already fetched the current entry at this path under a path lock, and it +// is reused instead of looking the store up again; pass nil to have CreateEntry +// look it up itself. +func (f *Filer) CreateEntry(ctx context.Context, entry *Entry, existing *Entry, o_excl bool, isFromOtherCluster bool, signatures []int32, skipCreateParentDir bool, maxFilenameLength uint32) error { if string(entry.FullPath) == "/" { return nil @@ -227,7 +231,10 @@ func (f *Filer) CreateEntry(ctx context.Context, entry *Entry, o_excl bool, isFr entry.Attr.Atime = entryInitialAtime(entry.Attr) } - oldEntry, _ := f.FindEntry(ctx, entry.FullPath) + oldEntry := existing + if oldEntry == nil { + oldEntry, _ = f.FindEntry(ctx, entry.FullPath) + } /* if !hasWritePermission(lastDirectoryEntry, entry) { diff --git a/weed/filer/filer_inode_test.go b/weed/filer/filer_inode_test.go index e965ab017..e19ae680a 100644 --- a/weed/filer/filer_inode_test.go +++ b/weed/filer/filer_inode_test.go @@ -28,7 +28,7 @@ func TestCreateEntryAssignsInodeWhenMissing(t *testing.T) { }, } - err := f.CreateEntry(context.Background(), entry, false, false, nil, false, f.MaxFilenameLength) + err := f.CreateEntry(context.Background(), entry, nil, false, false, nil, false, f.MaxFilenameLength) require.NoError(t, err) stored, findErr := store.FindEntry(context.Background(), entry.FullPath) @@ -48,7 +48,7 @@ func TestCreateEntryAssignsInodesToAutoCreatedParents(t *testing.T) { }, } - err := f.CreateEntry(context.Background(), entry, false, false, nil, false, f.MaxFilenameLength) + err := f.CreateEntry(context.Background(), entry, nil, false, false, nil, false, f.MaxFilenameLength) require.NoError(t, err) for _, path := range []string{"/a", "/a/b", "/a/b/c.txt"} { diff --git a/weed/filer/filer_lazy_remote.go b/weed/filer/filer_lazy_remote.go index a7025d304..77599ca8c 100644 --- a/weed/filer/filer_lazy_remote.go +++ b/weed/filer/filer_lazy_remote.go @@ -96,7 +96,7 @@ func (f *Filer) maybeLazyFetchFromRemote(ctx context.Context, p util.FullPath) ( persistBaseCtx, cancelPersist := context.WithTimeout(context.Background(), 30*time.Second) defer cancelPersist() persistCtx := context.WithValue(persistBaseCtx, lazyFetchContextKey{}, true) - saveErr := f.CreateEntry(persistCtx, entry, false, false, nil, true, f.MaxFilenameLength) + saveErr := f.CreateEntry(persistCtx, entry, nil, false, false, nil, true, f.MaxFilenameLength) if saveErr != nil { glog.Warningf("maybeLazyFetchFromRemote: failed to persist filer entry for %s: %v", p, saveErr) f.lazyFetchGroup.Forget(key) diff --git a/weed/filer/filer_lazy_remote_listing.go b/weed/filer/filer_lazy_remote_listing.go index c6d827157..59b2d2ea3 100644 --- a/weed/filer/filer_lazy_remote_listing.go +++ b/weed/filer/filer_lazy_remote_listing.go @@ -152,7 +152,7 @@ func (f *Filer) maybeLazyListFromRemote(ctx context.Context, p util.FullPath) { entry.Attr.FileSize = uint64(remoteEntry.RemoteSize) } } - if saveErr := f.CreateEntry(persistCtx, entry, false, false, nil, true, f.MaxFilenameLength); saveErr != nil { + if saveErr := f.CreateEntry(persistCtx, entry, nil, false, false, nil, true, f.MaxFilenameLength); saveErr != nil { glog.Warningf("maybeLazyListFromRemote: persist %s: %v", childPath, saveErr) } } @@ -193,7 +193,7 @@ func (f *Filer) updateDirectoryListingSyncedAt(ctx context.Context, p util.FullP dirEntry.Extended = make(map[string][]byte) } dirEntry.Extended[xattrRemoteListingSyncedAt] = []byte(fmt.Sprintf("%d", syncTime.Unix())) - if saveErr := f.CreateEntry(ctx, dirEntry, false, false, nil, true, f.MaxFilenameLength); saveErr != nil { + if saveErr := f.CreateEntry(ctx, dirEntry, nil, false, false, nil, true, f.MaxFilenameLength); saveErr != nil { glog.Warningf("maybeLazyListFromRemote: create dir synced_at for %s: %v", p, saveErr) } return diff --git a/weed/filer/filer_notify_append.go b/weed/filer/filer_notify_append.go index 036c825cf..432153dfb 100644 --- a/weed/filer/filer_notify_append.go +++ b/weed/filer/filer_notify_append.go @@ -43,7 +43,7 @@ func (f *Filer) appendToFile(targetFile string, data []byte) error { entry.Chunks = append(entry.GetChunks(), uploadResult.ToPbFileChunk(assignResult.Fid, offset, time.Now().UnixNano())) // update the entry - err = f.CreateEntry(context.Background(), entry, false, false, nil, false, f.MaxFilenameLength) + err = f.CreateEntry(context.Background(), entry, nil, false, false, nil, false, f.MaxFilenameLength) return err } diff --git a/weed/filer/leveldb/leveldb_store_test.go b/weed/filer/leveldb/leveldb_store_test.go index a491d969b..b5384fc77 100644 --- a/weed/filer/leveldb/leveldb_store_test.go +++ b/weed/filer/leveldb/leveldb_store_test.go @@ -32,7 +32,7 @@ func TestCreateAndFind(t *testing.T) { }, } - if err := testFiler.CreateEntry(ctx, entry1, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { + if err := testFiler.CreateEntry(ctx, entry1, nil, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { t.Errorf("create entry %v: %v", entry1.FullPath, err) return } diff --git a/weed/filer/leveldb2/leveldb2_store_test.go b/weed/filer/leveldb2/leveldb2_store_test.go index 6f6667422..5c6a3a4d7 100644 --- a/weed/filer/leveldb2/leveldb2_store_test.go +++ b/weed/filer/leveldb2/leveldb2_store_test.go @@ -29,7 +29,7 @@ func TestCreateAndFind(t *testing.T) { }, } - if err := testFiler.CreateEntry(ctx, entry1, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { + if err := testFiler.CreateEntry(ctx, entry1, nil, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { t.Errorf("create entry %v: %v", entry1.FullPath, err) return } diff --git a/weed/filer/leveldb3/leveldb3_store_test.go b/weed/filer/leveldb3/leveldb3_store_test.go index aea6edd04..8a3cd2d82 100644 --- a/weed/filer/leveldb3/leveldb3_store_test.go +++ b/weed/filer/leveldb3/leveldb3_store_test.go @@ -29,7 +29,7 @@ func TestCreateAndFind(t *testing.T) { }, } - if err := testFiler.CreateEntry(ctx, entry1, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { + if err := testFiler.CreateEntry(ctx, entry1, nil, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { t.Errorf("create entry %v: %v", entry1.FullPath, err) return } diff --git a/weed/filer/rocksdb/rocksdb_store_test.go b/weed/filer/rocksdb/rocksdb_store_test.go index 35181d601..00c772d4d 100644 --- a/weed/filer/rocksdb/rocksdb_store_test.go +++ b/weed/filer/rocksdb/rocksdb_store_test.go @@ -35,7 +35,7 @@ func TestCreateAndFind(t *testing.T) { }, } - if err := testFiler.CreateEntry(ctx, entry1, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { + if err := testFiler.CreateEntry(ctx, entry1, nil, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { t.Errorf("create entry %v: %v", entry1.FullPath, err) return } @@ -149,7 +149,7 @@ func TestListDirectoryWithPrefix(t *testing.T) { Gid: 1, }, } - if err := testFiler.CreateEntry(ctx, entry, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { + if err := testFiler.CreateEntry(ctx, entry, nil, false, false, nil, false, testFiler.MaxFilenameLength); err != nil { t.Fatalf("Failed to create entry %s: %v", fullpath, err) } } diff --git a/weed/server/filer_grpc_server.go b/weed/server/filer_grpc_server.go index 4aadf677a..ef67e2cb3 100644 --- a/weed/server/filer_grpc_server.go +++ b/weed/server/filer_grpc_server.go @@ -195,6 +195,9 @@ func (fs *FilerServer) CreateEntry(ctx context.Context, req *filer_pb.CreateEntr // Evaluate the optional precondition against the current entry while the // path lock is held, so the check and the write are atomic on this filer. + // The fetched entry is then handed to CreateEntry below so it does not look + // the same path up again under the lock. + var existing *filer.Entry if conditionIsSet(req.Condition) { current, findErr := fs.filer.FindEntry(ctx, fullpath) if findErr != nil && findErr != filer_pb.ErrNotFound { @@ -210,10 +213,11 @@ func (fs *FilerServer) CreateEntry(ctx context.Context, req *filer_pb.CreateEntr ErrorCode: filer_pb.FilerError_PRECONDITION_FAILED, }, nil } + existing = current } ctx, eventSink := filer.WithMetadataEventSink(ctx) - createErr := fs.filer.CreateEntry(ctx, newEntry, req.OExcl, req.IsFromOtherCluster, req.Signatures, req.SkipCheckParentDirectory, so.MaxFileNameLength) + createErr := fs.filer.CreateEntry(ctx, newEntry, existing, req.OExcl, req.IsFromOtherCluster, req.Signatures, req.SkipCheckParentDirectory, so.MaxFileNameLength) if createErr == nil { fs.filer.DeleteChunksNotRecursive(garbage) @@ -327,7 +331,7 @@ func (fs *FilerServer) applyObjectMutation(ctx context.Context, m *filer_pb.Obje return fmt.Errorf("PUT requires an entry") } newEntry := filer.FromPbEntry(m.Directory, m.Entry) - return fs.filer.CreateEntry(ctx, newEntry, false, fromOtherCluster, signatures, false, fs.filer.MaxFilenameLength) + return fs.filer.CreateEntry(ctx, newEntry, nil, false, fromOtherCluster, signatures, false, fs.filer.MaxFilenameLength) case filer_pb.ObjectMutation_DELETE: fullpath := util.NewFullPath(m.Directory, m.Name) @@ -592,7 +596,7 @@ func (fs *FilerServer) AppendToEntry(ctx context.Context, req *filer_pb.AppendTo glog.V(0).InfofCtx(ctx, "MaybeManifestize: %v", err) } - err = fs.filer.CreateEntry(context.Background(), entry, false, false, nil, false, fs.filer.MaxFilenameLength) + err = fs.filer.CreateEntry(context.Background(), entry, nil, false, false, nil, false, fs.filer.MaxFilenameLength) return &filer_pb.AppendToEntryResponse{}, err } diff --git a/weed/server/filer_grpc_server_condition_test.go b/weed/server/filer_grpc_server_condition_test.go index 9ee4ea6d7..ce9007a55 100644 --- a/weed/server/filer_grpc_server_condition_test.go +++ b/weed/server/filer_grpc_server_condition_test.go @@ -234,3 +234,42 @@ func TestCreateEntryConditionEnforced(t *testing.T) { t.Fatalf("matching etag should overwrite, got error %q", resp.Error) } } + +// CreateEntry reuses a provided existing entry instead of reading the store +// again; passing nil makes it look the path up itself. +func TestCreateEntryReusesProvidedExisting(t *testing.T) { + existing := &filer.Entry{ + FullPath: "/test/obj", + Attr: filer.Attr{Inode: 1, Mtime: time.Unix(1700000000, 0), Crtime: time.Unix(1700000000, 0)}, + Extended: map[string][]byte{s3_constants.ExtETagKey: []byte("abc")}, + } + newEntry := func() *filer.Entry { + return &filer.Entry{ + FullPath: "/test/obj", + Attr: filer.Attr{Inode: 1, Mtime: time.Unix(1700000001, 0)}, + } + } + + // Provided existing vs nil: the only difference is CreateEntry's own lookup, + // so providing it must save exactly one path read (other store-layer reads + // during the overwrite are the same in both runs). + store := newRenameTestStore() + store.entries["/test/obj"] = existing.ShallowClone() + f := newRenameTestFiler(store) + if err := f.CreateEntry(context.Background(), newEntry(), existing, false, false, nil, true, f.MaxFilenameLength); err != nil { + t.Fatalf("create with existing: %v", err) + } + withExisting := store.findEntryCallCount("/test/obj") + + store2 := newRenameTestStore() + store2.entries["/test/obj"] = existing.ShallowClone() + f2 := newRenameTestFiler(store2) + if err := f2.CreateEntry(context.Background(), newEntry(), nil, false, false, nil, true, f2.MaxFilenameLength); err != nil { + t.Fatalf("create with nil: %v", err) + } + withNil := store2.findEntryCallCount("/test/obj") + + if withNil != withExisting+1 { + t.Fatalf("providing existing should save one path lookup: existing=%d nil=%d", withExisting, withNil) + } +} diff --git a/weed/server/filer_grpc_server_rename.go b/weed/server/filer_grpc_server_rename.go index 3c8aae6b1..3b60e9e75 100644 --- a/weed/server/filer_grpc_server_rename.go +++ b/weed/server/filer_grpc_server_rename.go @@ -245,7 +245,7 @@ func (fs *FilerServer) moveSelfEntry(ctx context.Context, stream filer_pb.Seawee return fmt.Errorf("insert entry %s: %v", newEntry.FullPath, createErr) } } else { - if createErr := fs.filer.CreateEntry(filer.WithSuppressedMetadataEvents(ctx), newEntry, false, false, signatures, false, fs.filer.MaxFilenameLength); createErr != nil { + if createErr := fs.filer.CreateEntry(filer.WithSuppressedMetadataEvents(ctx), newEntry, nil, false, false, signatures, false, fs.filer.MaxFilenameLength); createErr != nil { return createErr } } diff --git a/weed/server/filer_server_handlers_copy.go b/weed/server/filer_server_handlers_copy.go index 2577c41b2..041c19d89 100644 --- a/weed/server/filer_server_handlers_copy.go +++ b/weed/server/filer_server_handlers_copy.go @@ -169,7 +169,7 @@ func (fs *FilerServer) copy(ctx context.Context, w http.ResponseWriter, r *http. // Pass o_excl = !overwrite so the default copy refuses to replace an // existing destination, while overwrite=true updates the pre-created target. - if createErr := fs.filer.CreateEntry(ctx, newEntry, !overwrite, false, nil, false, fs.filer.MaxFilenameLength); createErr != nil { + if createErr := fs.filer.CreateEntry(ctx, newEntry, nil, !overwrite, false, nil, false, fs.filer.MaxFilenameLength); createErr != nil { err = fmt.Errorf("failed to create copied entry from '%s' to '%s': %w", src, dst, createErr) writeJsonError(w, r, http.StatusInternalServerError, err) return diff --git a/weed/server/filer_server_handlers_tagging.go b/weed/server/filer_server_handlers_tagging.go index 8d6961959..06cd1295e 100644 --- a/weed/server/filer_server_handlers_tagging.go +++ b/weed/server/filer_server_handlers_tagging.go @@ -42,7 +42,7 @@ func (fs *FilerServer) PutTaggingHandler(w http.ResponseWriter, r *http.Request) } } - if dbErr := fs.filer.CreateEntry(ctx, existingEntry, false, false, nil, false, fs.filer.MaxFilenameLength); dbErr != nil { + if dbErr := fs.filer.CreateEntry(ctx, existingEntry, nil, false, false, nil, false, fs.filer.MaxFilenameLength); dbErr != nil { glog.V(0).InfofCtx(ctx, "failing to update %s tagging : %v", path, dbErr) writeJsonError(w, r, http.StatusInternalServerError, dbErr) return @@ -108,7 +108,7 @@ func (fs *FilerServer) DeleteTaggingHandler(w http.ResponseWriter, r *http.Reque return } - if dbErr := fs.filer.CreateEntry(ctx, existingEntry, false, false, nil, false, fs.filer.MaxFilenameLength); dbErr != nil { + if dbErr := fs.filer.CreateEntry(ctx, existingEntry, nil, false, false, nil, false, fs.filer.MaxFilenameLength); dbErr != nil { glog.V(0).InfofCtx(ctx, "failing to delete %s tagging : %v", path, dbErr) writeJsonError(w, r, http.StatusInternalServerError, dbErr) return diff --git a/weed/server/filer_server_handlers_write_autochunk.go b/weed/server/filer_server_handlers_write_autochunk.go index 94af39b8d..c7959682a 100644 --- a/weed/server/filer_server_handlers_write_autochunk.go +++ b/weed/server/filer_server_handlers_write_autochunk.go @@ -339,7 +339,7 @@ func (fs *FilerServer) saveMetaData(ctx context.Context, r *http.Request, fileNa } } - dbErr := fs.filer.CreateEntry(context.WithoutCancel(ctx), entry, false, false, nil, skipCheckParentDirEntry(r), so.MaxFileNameLength) + dbErr := fs.filer.CreateEntry(context.WithoutCancel(ctx), entry, nil, false, false, nil, skipCheckParentDirEntry(r), so.MaxFileNameLength) if dbErr != nil { replyerr = dbErr filerResult.Error = dbErr.Error() @@ -437,7 +437,7 @@ func (fs *FilerServer) mkdir(ctx context.Context, w http.ResponseWriter, r *http Name: util.FullPath(path).Name(), } - if dbErr := fs.filer.CreateEntry(context.WithoutCancel(ctx), entry, false, false, nil, false, so.MaxFileNameLength); dbErr != nil { + if dbErr := fs.filer.CreateEntry(context.WithoutCancel(ctx), entry, nil, false, false, nil, false, so.MaxFileNameLength); dbErr != nil { replyerr = dbErr filerResult.Error = dbErr.Error() glog.V(0).InfofCtx(ctx, "failing to create dir %s on filer server : %v", path, dbErr) diff --git a/weed/server/filer_server_tus_session.go b/weed/server/filer_server_tus_session.go index 90aba9c99..2b804b3d8 100644 --- a/weed/server/filer_server_tus_session.go +++ b/weed/server/filer_server_tus_session.go @@ -132,7 +132,7 @@ func (fs *FilerServer) createTusSession(ctx context.Context, uploadID, targetPat Uid: OS_UID, Gid: OS_GID, }, - }, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { + }, nil, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { return nil, fmt.Errorf("create session directory: %w", err) } @@ -167,7 +167,7 @@ func (fs *FilerServer) saveTusSession(ctx context.Context, session *TusSession) Content: sessionData, } - if err := fs.filer.CreateEntry(ctx, entry, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { + if err := fs.filer.CreateEntry(ctx, entry, nil, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { return fmt.Errorf("save session info entry: %w", err) } @@ -265,7 +265,7 @@ func (fs *FilerServer) saveTusChunk(ctx context.Context, uploadID string, chunk Uid: OS_UID, Gid: OS_GID, }, - }, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { + }, nil, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { return fmt.Errorf("save chunk info: %w", err) } @@ -371,7 +371,7 @@ func (fs *FilerServer) completeTusUpload(ctx context.Context, session *TusSessio } // Ensure parent directory exists - if err := fs.filer.CreateEntry(ctx, entry, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { + if err := fs.filer.CreateEntry(ctx, entry, nil, false, false, nil, false, fs.filer.MaxFilenameLength); err != nil { return fmt.Errorf("create final file entry: %w", err) }