From 121de29e89cb753ebc392f0fc660fde5bcb8f4f8 Mon Sep 17 00:00:00 2001 From: paul cannon Date: Mon, 5 Feb 2024 17:40:08 -0600 Subject: [PATCH] storagenode/{piece,blob}store: avoid extra trash checks We check to see if missing pieces/blobs are in the trash so that we can get an idea via metrics if things are going into the trash incorrectly. However, we've been doing that twice for every time we get a not-found error, which is unnecessary. The first check happens in the blobstore. We currently use that to increment a metric counter, then we throw the information away. Afterward, at the piecestore endpoint layer, we check again, then try to restore the blob out of the trash if possible. Instead of doing that, we will remove the check from the blobstore, then increment the same metric counter from the piecestore side if appropriate. Change-Id: I4513baf5a98c1df11d0d903ddf6658d024176303 --- storagenode/blobstore/filestore/dir.go | 23 +---------------------- storagenode/blobstore/filestore/store.go | 6 +++++- storagenode/piecestore/endpoint.go | 10 ++++------ 3 files changed, 10 insertions(+), 29 deletions(-) diff --git a/storagenode/blobstore/filestore/dir.go b/storagenode/blobstore/filestore/dir.go index 31c4888f8b6e..794dd3fc61e6 100644 --- a/storagenode/blobstore/filestore/dir.go +++ b/storagenode/blobstore/filestore/dir.go @@ -173,18 +173,6 @@ func (dir *Dir) refToDirPath(ref blobstore.BlobRef, subDir string) (string, erro return filepath.Join(subDir, namespace, key[:2], key[2:]), nil } -// fileConfirmedInTrash returns true if it is able to confirm the file is in -// the trash. On errors, or if the file is not in the trash, it returns false. -func (dir *Dir) fileConfirmedInTrash(ctx context.Context, ref blobstore.BlobRef, formatVer blobstore.FormatVersion) bool { - trashBasePath, err := dir.refToDirPath(ref, dir.trashdir()) - if err != nil { - return false - } - trashVerPath := blobPathForFormatVersion(trashBasePath, formatVer) - _, err = os.Stat(trashVerPath) - return err == nil -} - // blobPathForFormatVersion adjusts a bare blob path (as might have been generated by a call to // blobToBasePath()) to what it should be for the given storage format version. func blobPathForFormatVersion(path string, formatVersion blobstore.FormatVersion) string { @@ -258,12 +246,7 @@ func (dir *Dir) Open(ctx context.Context, ref blobstore.BlobRef) (_ *os.File, _ if err == nil { return file, formatVer, nil } - if os.IsNotExist(err) { - // Check and monitor if the file is in the trash - if dir.fileConfirmedInTrash(ctx, ref, formatVer) { - monFileInTrash(ref.Namespace).Mark(1) - } - } else { + if !os.IsNotExist(err) { return nil, FormatV0, Error.New("unable to open %q: %v", vPath, err) } } @@ -284,10 +267,6 @@ func (dir *Dir) OpenWithStorageFormat(ctx context.Context, ref blobstore.BlobRef return file, nil } if os.IsNotExist(err) { - // Check and monitor if the file is in the trash - if dir.fileConfirmedInTrash(ctx, ref, formatVer) { - monFileInTrash(ref.Namespace).Mark(1) - } return nil, err } return nil, Error.New("unable to open %q: %v", vPath, err) diff --git a/storagenode/blobstore/filestore/store.go b/storagenode/blobstore/filestore/store.go index 57b85726ed94..2d72aa4cc10b 100644 --- a/storagenode/blobstore/filestore/store.go +++ b/storagenode/blobstore/filestore/store.go @@ -36,7 +36,11 @@ var ( _ blobstore.Blobs = (*blobStore)(nil) ) -func monFileInTrash(namespace []byte) *monkit.Meter { +// MonFileInTrash returns a monkit meter which counts the times a requested blob is +// found in the trash. It is exported so that it can be activated from outside this +// package for backwards compatibility. (It is no longer activated from inside this +// package.) +func MonFileInTrash(namespace []byte) *monkit.Meter { return monStorage.Meter("open_file_in_trash", monkit.NewSeriesTag("namespace", hex.EncodeToString(namespace))) //mon:locked } diff --git a/storagenode/piecestore/endpoint.go b/storagenode/piecestore/endpoint.go index 574f271d37d7..f803830df4b4 100644 --- a/storagenode/piecestore/endpoint.go +++ b/storagenode/piecestore/endpoint.go @@ -34,6 +34,7 @@ import ( "storj.io/drpc" "storj.io/drpc/drpcctx" "storj.io/storj/storagenode/bandwidth" + "storj.io/storj/storagenode/blobstore/filestore" "storj.io/storj/storagenode/monitor" "storj.io/storj/storagenode/orders" "storj.io/storj/storagenode/orders/ordersfile" @@ -680,16 +681,13 @@ func (endpoint *Endpoint) Download(stream pb.DRPCPiecestore_DownloadStream) (err // continue serving the download request. tryRestoreErr := endpoint.store.TryRestoreTrashPiece(ctx, limit.SatelliteId, limit.PieceId) if tryRestoreErr != nil { - if !errs.Is(tryRestoreErr, fs.ErrNotExist) { - // file is not in trash, and we don't want to return a file rename error, - // so we return the original "file does not exist" error - tryRestoreErr = err - } endpoint.monitor.VerifyDirReadableLoop.TriggerWait() - return rpcstatus.Wrap(rpcstatus.NotFound, tryRestoreErr) + // we want to return the original "file does not exist" error to the rpc client + return rpcstatus.Wrap(rpcstatus.NotFound, err) } restoredFromTrash = true mon.Meter("download_file_in_trash", monkit.NewSeriesTag("namespace", limit.SatelliteId.String()), monkit.NewSeriesTag("piece_id", limit.PieceId.String())).Mark(1) + filestore.MonFileInTrash(limit.SatelliteId[:]).Mark(1) log.Warn("file found in trash") // try to open the file again