From 5d1a876b6312b3f8c69481551ffa744e4f93754c Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Tue, 21 Oct 2025 04:31:37 +0000 Subject: [PATCH 1/9] Base btrfs snapshot Implement creating, closing of btrfs snapshots. Use reflinks instead of hardlinks. --- .vscode/settings.json | 6 + internal/git/objectpool/link.go | 2 +- .../storagemgr/partition/apply_operations.go | 59 ++- .../storagemgr/partition/snapshot/snapshot.go | 407 ++++++++++++++++-- internal/gitaly/storage/wal/entry.go | 57 ++- 5 files changed, 503 insertions(+), 28 deletions(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000000..705cb2f46a --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,6 @@ +{ + "go.testEnvVars": { + "TEST_TMP_DIR": "/mnt/git-repositories/tmp" + }, + "go.testFlags": ["-v"] +} diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go index e284493196..5924277a3e 100644 --- a/internal/git/objectpool/link.go +++ b/internal/git/objectpool/link.go @@ -76,7 +76,7 @@ func Link(ctx context.Context, pool, repo *localrepo.Repo, txManager transaction } if err := tx.FS().RecordFile(alternatesRelativePath); err != nil { - return fmt.Errorf("record alternates file") + return fmt.Errorf("record alternates file: %w", err) } } diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index c5ba4c0d8e..5ff3fdb496 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -4,12 +4,16 @@ import ( "context" "errors" "fmt" + "io" "io/fs" "os" "path/filepath" + "syscall" + // "syscall" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/keyvalue" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" + "golang.org/x/sys/unix" ) // applyOperations applies the operations from the log entry to the storage. @@ -34,11 +38,12 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err } destinationPath := string(op.GetDestinationPath()) - if err := os.Link( + + if err := LinkOrReflink( filepath.Join(basePath, string(op.GetSourcePath())), filepath.Join(storageRoot, destinationPath), ); err != nil && !errors.Is(err, fs.ErrExist) { - return fmt.Errorf("link: %w", err) + return fmt.Errorf("linkOrReflink: %w", err) } // Sync the parent directory of the newly created directory entry. @@ -94,3 +99,53 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err return nil } +// LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, +// falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. +func LinkOrReflink(src, dst string) error { + // Try hard link first (works within same subvolume) + if err := os.Link(src, dst); err == nil { + return nil + } else if !errors.Is(err, syscall.EXDEV) { + // Some other error, not cross-device + return err + } + + // Hard link failed due to cross-device - try reflink + if err := reflink(src, dst); err != nil { + return fmt.Errorf("reflink: %w", err) + } + + return nil +} + +// reflink creates a copy-on-write copy of src to dst using btrfs reflink. +// This is space-efficient like hard links but works across subvolumes. +func reflink(src, dst string) error { + srcFile, err := os.Open(src) + if err != nil { + return fmt.Errorf("open source: %w", err) + } + defer srcFile.Close() + + srcInfo, err := srcFile.Stat() + if err != nil { + return fmt.Errorf("stat source: %w", err) + } + + dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) + if err != nil { + return fmt.Errorf("create destination: %w", err) + } + defer dstFile.Close() + + // Use FICLONE ioctl to create a reflink (COW copy) + err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) + if err != nil { + // Reflink failed - fall back to regular copy + if _, err := io.Copy(dstFile, srcFile); err != nil { + return fmt.Errorf("copy data: %w", err) + } + } + + return nil +} \ No newline at end of file diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index d7377a29e4..7a88d44b4b 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -6,6 +6,7 @@ import ( "fmt" "io/fs" "os" + "os/exec" "path/filepath" "strings" "time" @@ -56,6 +57,9 @@ type snapshot struct { readOnly bool // stats contains statistics related to the snapshot. stats snapshotStatistics + // repositoryPaths contains the paths of individual repository subvolumes + // that need to be cleaned up. The root itself is just a regular directory. + repositoryPaths []string } // Root returns the root of the snapshot's file system. @@ -74,22 +78,70 @@ func (s *snapshot) RelativePath(relativePath string) string { return filepath.Join(s.prefix, relativePath) } +// Closes removes the snapshot. +// func (s *snapshot) Close() error { +// if s.readOnly { +// // Make the directories writable again so we can remove the snapshot. +// if err := s.setDirectoryMode(mode.Directory); err != nil { +// return fmt.Errorf("make writable: %w", err) +// } +// } + +// if err := os.RemoveAll(s.root); err != nil { +// return fmt.Errorf("remove all: %w", err) +// } + +// return nil +// } + // Closes removes the snapshot. func (s *snapshot) Close() error { + var errs []error + + // If this was a read-only snapshot, we need to restore write permissions + // before we can delete anything if s.readOnly { - // Make the directories writable again so we can remove the snapshot. - if err := s.setDirectoryMode(mode.Directory); err != nil { - return fmt.Errorf("make writable: %w", err) + // First, make all btrfs subvolumes writable + for _, repoPath := range s.repositoryPaths { + // Make the subvolume writable + _ = setBtrfsReadOnly(context.Background(), repoPath, false) + + // Restore write permissions to directories inside the subvolume + // so we can delete files + _ = storage.SetDirectoryMode(repoPath, mode.Directory) + } + + // Restore write permissions to parent directories + _ = os.Chmod(s.root, mode.Directory) + for _, repoPath := range s.repositoryPaths { + dir := filepath.Dir(repoPath) + for dir != s.root && dir != "." && dir != "/" { + _ = os.Chmod(dir, mode.Directory) + dir = filepath.Dir(dir) + } + } + } + + // Now delete individual repository subvolumes + for _, repoPath := range s.repositoryPaths { + if err := deleteBtrfsSnapshot(context.Background(), repoPath, false); err != nil { + errs = append(errs, fmt.Errorf("delete repository snapshot %s: %w", repoPath, err)) } } + // Finally delete the root if err := os.RemoveAll(s.root); err != nil { - return fmt.Errorf("remove all: %w", err) + errs = append(errs, fmt.Errorf("delete snapshot root %s: %w", s.root, err)) + } + + if len(errs) > 0 { + return errors.Join(errs...) } return nil } + // setDirectoryMode walks the snapshot and sets each directory's mode to the given mode. func (s *snapshot) setDirectoryMode(mode fs.FileMode) error { return storage.SetDirectoryMode(s.root, mode) @@ -120,15 +172,17 @@ func newSnapshot(ctx context.Context, storageRoot, snapshotRoot string, relative } }() - if err := createRepositorySnapshots(ctx, storageRoot, snapshotRoot, relativePaths, snapshotFilter, &s.stats); err != nil { + // Create all repository snapshots (they handle their own read-only conversion internally) + if err := createRepositorySnapshots(ctx, storageRoot, snapshotRoot, relativePaths, snapshotFilter, readOnly, s); err != nil { return nil, fmt.Errorf("create repository snapshots: %w", err) } + if readOnly { // Now that we've finished creating the snapshot, change the directory permissions to read-only // to prevent writing in the snapshot. - if err := s.setDirectoryMode(ModeReadOnlyDirectory); err != nil { - return nil, fmt.Errorf("make read-only: %w", err) + if err := setParentDirectoriesReadOnly(s); err != nil { + return nil, fmt.Errorf("make parent directories read-only: %w", err) } } @@ -136,14 +190,51 @@ func newSnapshot(ctx context.Context, storageRoot, snapshotRoot string, relative return s, nil } -// createRepositorySnapshots creates a snapshot of the partition containing all repositories at the given relative paths -// and their alternates. + +// setParentDirectoriesReadOnly makes all parent directories (non-subvolume directories) read-only. +// This is called after all repository snapshots have been created. +func setParentDirectoriesReadOnly(s *snapshot) error { + // Walk the snapshot root and make directories read-only, but skip the repository subvolumes + // since they were already made read-only by createBtrfsSnapshotAndFilter + repoPathSet := make(map[string]struct{}) + for _, repoPath := range s.repositoryPaths { + repoPathSet[repoPath] = struct{}{} + } + + return filepath.Walk(s.root, func(path string, info fs.FileInfo, err error) error { + if err != nil { + return err + } + + if !info.IsDir() { + return nil + } + + // Skip repository subvolumes - they're already read-only + if _, isRepoPath := repoPathSet[path]; isRepoPath { + return fs.SkipDir + } + + // Make this parent directory read-only + if err := os.Chmod(path, ModeReadOnlyDirectory); err != nil { + return fmt.Errorf("chmod %s: %w", path, err) + } + + return nil + }) +} +// createRepositorySnapshots creates btrfs snapshots of all repositories at the given relative paths +// and their alternates, then applies filtering to remove unwanted files. func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot string, relativePaths []string, - snapshotFilter Filter, stats *snapshotStatistics, -) error { + snapshotFilter Filter, readOnly bool, s *snapshot, + ) error { // Create the root directory always to as the storage would also exist always. - stats.directoryCount++ - if err := os.Mkdir(snapshotRoot, mode.Directory); err != nil { + s.stats.directoryCount++ + // if err := os.Mkdir(snapshotRoot, 0755); err != nil { + // return fmt.Errorf("mkdir snapshot root: %w", err) + // } + + if err := ensureDir0700(snapshotRoot); err != nil { return fmt.Errorf("mkdir snapshot root: %w", err) } @@ -153,7 +244,7 @@ func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot st continue } - if err := createParentDirectories(storageRoot, snapshotRoot, relativePath, stats); err != nil { + if err := createParentDirectories(storageRoot, snapshotRoot, relativePath, &s.stats); err != nil { return fmt.Errorf("create parent directories: %w", err) } @@ -171,7 +262,7 @@ func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot st return fmt.Errorf("validate git directory: %w", err) } - if err := createRepositorySnapshot(ctx, storageRoot, snapshotRoot, relativePath, snapshotFilter, stats); err != nil { + if err := createRepositorySnapshot(ctx, storageRoot, snapshotRoot, relativePath, snapshotFilter, readOnly, s); err != nil { return fmt.Errorf("create snapshot: %w", err) } @@ -190,7 +281,7 @@ func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot st continue } - if err := createParentDirectories(storageRoot, snapshotRoot, alternateRelativePath, stats); err != nil { + if err := createParentDirectories(storageRoot, snapshotRoot, alternateRelativePath, &s.stats); err != nil { return fmt.Errorf("create parent directories: %w", err) } @@ -200,7 +291,8 @@ func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot st snapshotRoot, alternateRelativePath, snapshotFilter, - stats, + readOnly, + s, ); err != nil { return fmt.Errorf("create alternate snapshot: %w", err) } @@ -208,7 +300,6 @@ func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot st snapshottedRepositories[alternateRelativePath] = struct{}{} } } - return nil } @@ -253,23 +344,291 @@ func createParentDirectories(storageRoot, snapshotRoot, relativePath string, sta return nil } - +// ensureDir0700 creates path (and parents) and enforces 0700 permissions. +// This guarantees the created directory has owner-only permissions regardless of umask. +func ensureDir0700(path string) error { + if err := os.MkdirAll(path, 0700); err != nil { + return fmt.Errorf("create parent directories: %w", err) + } + // Explicitly set permissions to 0700 to avoid umask influence and ensure tests that + // assert specific directory modes are stable. + if err := os.Chmod(path, 0700); err != nil { + return fmt.Errorf("set directory permissions: %w", err) + } + return nil +} // createRepositorySnapshot snapshots a repository's current state at snapshotPath. This is done by // recreating the repository's directory structure and hard linking the repository's files in their // correct locations there. This effectively does a copy-free clone of the repository. Since the files // are shared between the snapshot and the repository, they must not be modified. Git doesn't modify // existing files but writes new ones so this property is upheld. +// func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, relativePath string, +// snapshotFilter Filter, stats *snapshotStatistics, +// ) error { +// if err := createDirectorySnapshot(ctx, filepath.Join(storageRoot, relativePath), +// filepath.Join(snapshotRoot, relativePath), +// snapshotFilter, stats); err != nil { +// return fmt.Errorf("create directory snapshot: %w", err) +// } +// return nil +// } + +// createRepositorySnapshot creates a snapshot of a repository using btrfs snapshots. +// The source repository is expected to be a btrfs subvolume for optimal performance. +// If the source is not a btrfs subvolume, it will be converted to one first. func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, relativePath string, - snapshotFilter Filter, stats *snapshotStatistics, + snapshotFilter Filter, readOnly bool, s *snapshot, ) error { - if err := createDirectorySnapshot(ctx, filepath.Join(storageRoot, relativePath), - filepath.Join(snapshotRoot, relativePath), - snapshotFilter, stats); err != nil { - return fmt.Errorf("create directory snapshot: %w", err) + originalPath := filepath.Join(storageRoot, relativePath) + snapshotPath := filepath.Join(snapshotRoot, relativePath) + + // Check if the original directory exists + if _, err := os.Stat(originalPath); err != nil { + if errors.Is(err, fs.ErrNotExist) { + // The directory being snapshotted does not exist. This is fine as the transaction + // may be about to create it. + return nil + } + return fmt.Errorf("stat original directory: %w", err) } + + // Check if the source is a btrfs subvolume + isBtrfsSubvolume, err := isBtrfsSubvolume(ctx, originalPath) + if err != nil { + return fmt.Errorf("check if btrfs subvolume: %w", err) + } + + // If it's not a subvolume, convert it to one + if !isBtrfsSubvolume { + s.repositoryPaths = append(s.repositoryPaths, originalPath) + if err := convertToSubvolume(ctx, originalPath); err != nil { + return fmt.Errorf("convert to subvolume: %w", err) + } + } + + // Now create the btrfs snapshot and apply filtering + if err := createBtrfsSnapshotAndFilter(ctx, originalPath, snapshotPath, snapshotFilter, readOnly, s); err != nil { + return fmt.Errorf("create btrfs snapshot and filter: %w", err) + } + + // Track this repository subvolume for cleanup + s.repositoryPaths = append(s.repositoryPaths, snapshotPath) + return nil } + +// createBtrfsSnapshotAndFilter creates a btrfs snapshot and applies filtering. +func createBtrfsSnapshotAndFilter(ctx context.Context, originalPath, snapshotPath string, + snapshotFilter Filter, readOnly bool, s *snapshot, +) error { + // Create btrfs snapshot - always create as writable initially + if err := createBtrfsSnapshot(ctx, originalPath, snapshotPath, false); err != nil { + return fmt.Errorf("create btrfs snapshot: %w", err) + } + + // Apply the filter by removing unwanted files/directories from the snapshot + if err := applyFilterToSnapshot(ctx, snapshotPath, snapshotFilter, &s.stats); err != nil { + return fmt.Errorf("apply filter to snapshot: %w", err) + } + + // If we're making a read-only snapshot, make the btrfs subvolume read-only + if readOnly { + // First, set all directories within this subvolume to read-only permissions + // This changes the actual permission bits that will be visible to users + if err := storage.SetDirectoryMode(snapshotPath, ModeReadOnlyDirectory); err != nil { + return fmt.Errorf("set directory mode: %w", err) + } + // Make the btrfs subvolume read-only + // This protects the files without needing to change directory permissions + if err := setBtrfsReadOnly(ctx, snapshotPath, true); err != nil { + return fmt.Errorf("make snapshot read-only: %w", err) + } + } + + return nil +} +// applyFilterToSnapshot walks through the snapshot and removes files/directories that don't match the filter. +func applyFilterToSnapshot(ctx context.Context, snapshotPath string, matcher Filter, stats *snapshotStatistics) error { + // We need to walk the snapshot and remove files that don't match the filter + // We collect paths to remove first, then remove them in reverse order (deepest first) + var pathsToRemove []string + + err := filepath.Walk(snapshotPath, func(path string, info fs.FileInfo, err error) error { + if err != nil { + return err + } + + if err := ctx.Err(); err != nil { + return err + } + + relativePath, err := filepath.Rel(snapshotPath, path) + if err != nil { + return fmt.Errorf("rel: %w", err) + } + + // Skip the root directory + if relativePath == "." { + return nil + } + + if !matcher.Matches(relativePath) { + pathsToRemove = append(pathsToRemove, path) + if info.IsDir() { + return fs.SkipDir + } + } else { + // Count the files and directories that we're keeping + if info.IsDir() { + stats.directoryCount++ + } else if info.Mode().IsRegular() { + stats.fileCount++ + } + } + + return nil + }) + if err != nil { + return fmt.Errorf("walk snapshot: %w", err) + } + + // Remove paths in reverse order (deepest first) + for i := len(pathsToRemove) - 1; i >= 0; i-- { + if err := os.RemoveAll(pathsToRemove[i]); err != nil { + return fmt.Errorf("remove filtered path %s: %w", pathsToRemove[i], err) + } + } + + return nil +} + + + +// createBtrfsSnapshot creates a btrfs subvolume snapshot from source to destination. +// If readOnly is true, the snapshot will be created as a read-only subvolume. +func createBtrfsSnapshot(ctx context.Context, source, destination string, readOnly bool) error { + // Build the btrfs subvolume snapshot command + args := []string{"subvolume", "snapshot"} + + // Add read-only flag if requested + if readOnly { + args = append(args, "-r") + } + + args = append(args, source, destination) + + cmd := exec.CommandContext(ctx, "btrfs", args...) + + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("btrfs snapshot failed: %w, output: %s", err, string(output)) + } + + return nil +} + + +// isBtrfsSubvolume checks if the given path is a btrfs subvolume. +func isBtrfsSubvolume(ctx context.Context, path string) (bool, error) { + cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "show", path) + err := cmd.Run() + if err != nil { + // If the command fails, it's not a subvolume (or not on btrfs at all) + return false, nil + } + return true, nil +} + + +// convertToSubvolume converts a regular directory to a btrfs subvolume in place. +func convertToSubvolume(ctx context.Context, path string) error { + tempPath := path + ".subvol-temp" + + // Create new subvolume with temporary name + cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "create", tempPath) + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("create subvolume: %w, output: %s", err, string(output)) + } + + // Copy contents from old directory to new subvolume + cmd = exec.CommandContext(ctx, "cp", "-R", "-a", path+"/.", tempPath+"/") + if output, err := cmd.CombinedOutput(); err != nil { + // Clean up the temp subvolume if copy fails + _ = deleteBtrfsSnapshot(ctx, tempPath, false) + return fmt.Errorf("copy contents: %w, output: %s", err, string(output)) + } + + // Remove old directory + if err := os.RemoveAll(path); err != nil { + return fmt.Errorf("remove old directory: %w", err) + } + + // Rename subvolume to original path + if err := os.Rename(tempPath, path); err != nil { + return fmt.Errorf("rename subvolume: %w", err) + } + + return nil +} + + +// setBtrfsReadOnly sets or unsets the read-only property of a btrfs subvolume. +func setBtrfsReadOnly(ctx context.Context, path string, readOnly bool) error { + value := "false" + if readOnly { + value = "true" + } + + cmd := exec.CommandContext(ctx, "btrfs", "property", "set", "-ts", path, "ro", value) + + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("btrfs property set ro=%s failed: %w for path: %s, output: %s", value, err, path, string(output)) + } + + return nil +} + + + +// deleteBtrfsSnapshot deletes a btrfs subvolume snapshot. +// If the snapshot is read-only, it first changes it to read-write before deletion. +func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly bool) error { + // Check if the snapshot exists + if _, err := os.Stat(snapshotPath); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return fmt.Errorf("stat snapshot: %w", err) + } + + // If it was a read-only snapshot, ensure it's writable before deletion + if wasReadOnly { + // Try to make it writable (ignore errors - it might not be a subvolume) + _ = setBtrfsReadOnly(ctx, snapshotPath, false) + + // Also restore directory write permissions + _ = storage.SetDirectoryMode(snapshotPath, mode.Directory) + } + + // Try to delete as a btrfs subvolume first + cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "delete", snapshotPath) + if err := cmd.Run(); err == nil { + // Successfully deleted as a subvolume + return nil + } + + // If that failed, try regular removal + if err := os.RemoveAll(snapshotPath); err != nil { + return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) + } + + return nil +} + + + + + // createDirectorySnapshot recursively recreates the directory structure from originalDirectory into // snapshotDirectory and hard links files into the same locations in snapshotDirectory. // diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index f398447628..8599709a71 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -1,15 +1,20 @@ package wal import ( + "errors" "fmt" + "io" "io/fs" "os" "path/filepath" "strconv" + "syscall" + // "syscall" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" + "golang.org/x/sys/unix" ) // Entry represents a write-ahead log entry. @@ -76,7 +81,7 @@ func (e *Entry) stageFile(path string) (string, error) { // The file names within the log entry are not important as the manifest records the // actual name the file will be linked as. fileName := strconv.FormatUint(e.fileIDSequence, 36) - if err := os.Link(path, filepath.Join(e.stateDirectory, fileName)); err != nil { + if err := LinkOrReflink(path, filepath.Join(e.stateDirectory, fileName)); err != nil { return "", fmt.Errorf("link: %w", err) } @@ -119,3 +124,53 @@ func (e *Entry) CreateLink(sourceRelativePath, destinationRelativePath string) { func (e *Entry) RemoveDirectoryEntry(relativePath string) { e.operations.removeDirectoryEntry(relativePath) } +// LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, +// falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. +func LinkOrReflink(src, dst string) error { + // Try hard link first (works within same subvolume) + if err := os.Link(src, dst); err == nil { + return nil + } else if !errors.Is(err, syscall.EXDEV) { + // Some other error, not cross-device + return err + } + + // Hard link failed due to cross-device - try reflink + if err := reflink(src, dst); err != nil { + return fmt.Errorf("reflink: %w", err) + } + + return nil +} + +// reflink creates a copy-on-write copy of src to dst using btrfs reflink. +// This is space-efficient like hard links but works across subvolumes. +func reflink(src, dst string) error { + srcFile, err := os.Open(src) + if err != nil { + return fmt.Errorf("open source: %w", err) + } + defer srcFile.Close() + + srcInfo, err := srcFile.Stat() + if err != nil { + return fmt.Errorf("stat source: %w", err) + } + + dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) + if err != nil { + return fmt.Errorf("create destination: %w", err) + } + defer dstFile.Close() + + // Use FICLONE ioctl to create a reflink (COW copy) + err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) + if err != nil { + // Reflink failed - fall back to regular copy + if _, err := io.Copy(dstFile, srcFile); err != nil { + return fmt.Errorf("copy data: %w", err) + } + } + + return nil +} \ No newline at end of file -- GitLab From 8f1bf06b05a6c04402c27cbb221324857b98e031 Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Wed, 22 Oct 2025 12:54:21 +0000 Subject: [PATCH 2/9] Refactor with reflink library --- go.mod | 1 + go.sum | 2 + internal/git/objectpool/disconnect.go | 6 +- internal/git/quarantine/quarantine.go | 6 +- internal/gitaly/repoutil/custom_hooks.go | 67 ++++++++++- .../gitaly/service/repository/replicate.go | 3 +- internal/gitaly/storage/fs.go | 3 +- .../storagemgr/partition/apply_operations.go | 98 ++++++++-------- .../storagemgr/partition/snapshot/snapshot.go | 3 +- .../partition/transaction_manager_test.go | 30 ++--- internal/gitaly/storage/wal/entry.go | 110 ++++++++++-------- .../gitaly/storage/wal/reference_recorder.go | 3 +- internal/helper/reflink.go | 76 ++++++++++++ 13 files changed, 290 insertions(+), 118 deletions(-) create mode 100644 internal/helper/reflink.go diff --git a/go.mod b/go.mod index 8854dc3f85..66ade46969 100644 --- a/go.mod +++ b/go.mod @@ -86,6 +86,7 @@ require ( github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.48.1 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.48.1 // indirect github.com/HdrHistogram/hdrhistogram-go v1.1.2 // indirect + github.com/KarpelesLab/reflink v1.0.2 // indirect github.com/Microsoft/go-winio v0.6.1 // indirect github.com/alexbrainman/sspi v0.0.0-20210105120005-909beea2cc74 // indirect github.com/avast/retry-go v3.0.0+incompatible // indirect diff --git a/go.sum b/go.sum index 2dd632e8c4..21ee872e9e 100644 --- a/go.sum +++ b/go.sum @@ -108,6 +108,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.48.1/go.mod h1:viRWSEhtMZqz1rhwmOVKkWl6SwmVowfL9O2YR5gI2PE= github.com/HdrHistogram/hdrhistogram-go v1.1.2 h1:5IcZpTvzydCQeHzK4Ef/D5rrSqwxob0t8PQPMybUNFM= github.com/HdrHistogram/hdrhistogram-go v1.1.2/go.mod h1:yDgFjdqOqDEKOvasDdhWNXYg9BVp4O+o5f6V/ehm6Oo= +github.com/KarpelesLab/reflink v1.0.2 h1:hQ1aM3TmjU2kTNUx5p/HaobDoADYk+a6AuEinG4Cv88= +github.com/KarpelesLab/reflink v1.0.2/go.mod h1:WGkTOKNjd1FsJKBw3mu4JvrPEDJyJJ+JPtxBkbPoCok= github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY= github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migciow= github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM= diff --git a/internal/git/objectpool/disconnect.go b/internal/git/objectpool/disconnect.go index 8f776ed38b..dd369c48c5 100644 --- a/internal/git/objectpool/disconnect.go +++ b/internal/git/objectpool/disconnect.go @@ -11,11 +11,13 @@ import ( "strings" "time" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v18/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v18/internal/git/stats" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v18/internal/log" "gitlab.com/gitlab-org/gitaly/v18/internal/transaction/voting" @@ -225,12 +227,12 @@ func removeAlternatesIfOk(ctx context.Context, repo *localrepo.Repo, altFile, ba // preserved for possible forensic use. tmp := backupFile + ".2" - if err := os.Link(backupFile, tmp); err != nil { + if err := reflink.Auto(backupFile, tmp); err != nil { logger.WithError(err).ErrorContext(ctx, "copy backup alternates file") return } - if err := os.Rename(tmp, altFile); err != nil { + if err := helper.RenameDirectoryWithReflink(tmp, altFile); err != nil { logger.WithError(err).ErrorContext(ctx, "restore backup alternates file") } }() diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go index 23c7354eef..d742649203 100644 --- a/internal/git/quarantine/quarantine.go +++ b/internal/git/quarantine/quarantine.go @@ -9,8 +9,10 @@ import ( "sort" "strings" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/log" "gitlab.com/gitlab-org/gitaly/v18/internal/safe" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" @@ -178,12 +180,12 @@ func migrate(ctx context.Context, sourcePath, targetPath string) error { func finalizeObjectFile(sourcePath, targetPath string) error { // We first try to link the file via a hardlink. The benefit compared to doing a rename is // that in case of a collision, we do not replace the target. - err := os.Link(sourcePath, targetPath) + err := reflink.Auto(sourcePath, targetPath) // In case the hardlink failed, we fall back to a rename. renamed := false if err != nil && !errors.Is(err, os.ErrExist) { - err = os.Rename(sourcePath, targetPath) + err = helper.RenameDirectoryWithReflink(sourcePath, targetPath) renamed = err == nil } diff --git a/internal/gitaly/repoutil/custom_hooks.go b/internal/gitaly/repoutil/custom_hooks.go index 9e7955ff49..e59f22c409 100644 --- a/internal/gitaly/repoutil/custom_hooks.go +++ b/internal/gitaly/repoutil/custom_hooks.go @@ -17,6 +17,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/log" "gitlab.com/gitlab-org/gitaly/v18/internal/safe" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" @@ -107,7 +108,7 @@ func SetCustomHooks( ) error { repoPath, err := locator.GetRepoPath(ctx, repo) if err != nil { - return fmt.Errorf("getting repo path: %w", err) + return fmt.Errorf("getting repo path %s: %w", repo.GetRelativePath(), err) } var originalCustomHooksRelativePath string @@ -192,7 +193,7 @@ func SetCustomHooks( // If the `custom_hooks` directory exists in the repository, move the // current hooks to `previous_hooks` in the temporary directory. - if err := os.Rename(repoHooksPath, prevHooksPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + if err := helper.RenameDirectoryWithReflink(repoHooksPath, prevHooksPath); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("moving current hooks to temp: %w", err) } @@ -209,7 +210,7 @@ func SetCustomHooks( } // Move `custom_hooks` from the temporary directory to the repository. - if err := os.Rename(tempHooksPath, repoHooksPath); err != nil { + if err := helper.RenameDirectoryWithReflink(tempHooksPath, repoHooksPath); err != nil { return fmt.Errorf("moving new hooks to repo: %w", err) } @@ -320,3 +321,63 @@ func voteCustomHooks( return nil } +// moveDirectory moves a directory from src to dst, working across btrfs subvolumes. +// func moveDirectory(ctx context.Context, src, dst string) error { +// // Check if source exists +// if _, err := os.Stat(src); err != nil { +// return err +// } + +// // Try simple rename first (fast path for same subvolume) +// if err := os.Rename(src, dst); err == nil { +// return nil +// } else if !errors.Is(err, syscall.EXDEV) { +// return err +// } + +// // Cross-device move - ensure parent dir exists +// if err := os.MkdirAll(filepath.Dir(dst), mode.Directory); err != nil { +// return fmt.Errorf("create destination parent: %w", err) +// } + +// // Use cp -r -a with reflinks +// cmd := exec.CommandContext(ctx, "cp", "-r", "-a", "--reflink=auto", src, dst) +// if output, err := cmd.CombinedOutput(); err != nil { +// return fmt.Errorf("cp failed: %w, output: %s", err, string(output)) +// } + +// // Remove source after successful copy +// return os.RemoveAll(src) +// } + +// func copyWithReflink(src, dst string) error { +// // Get source info +// srcInfo, err := os.Stat(src) +// if err != nil { +// return err +// } + +// // If source is a directory, copy its contents +// if srcInfo.IsDir() { +// // Create destination if it doesn't exist +// err = os.MkdirAll(dst, srcInfo.Mode()) +// if err != nil { +// return err +// } + +// cmd := exec.Command("cp", "--reflink=always", "-a", "-T", src+"/.", dst) +// output, err := cmd.CombinedOutput() +// if err != nil { +// return fmt.Errorf("copy failed: %v, output: %s", err, output) +// } +// } else { +// // Single file copy +// cmd := exec.Command("cp", "--reflink=auto", "-a", src, dst) +// output, err := cmd.CombinedOutput() +// if err != nil { +// return fmt.Errorf("copy failed: %v, output: %s", err, output) +// } +// } + +// return os.RemoveAll(src) +// } \ No newline at end of file diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go index e9310c0a83..629518ab41 100644 --- a/internal/gitaly/service/repository/replicate.go +++ b/internal/gitaly/service/repository/replicate.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v18/internal/git" "gitlab.com/gitlab-org/gitaly/v18/internal/git/gitcmd" @@ -406,7 +407,7 @@ func (s *server) extractTarToDirectory(ctx context.Context, reader io.Reader, ta return fmt.Errorf("removing existing file for hard link %s: %w", targetPath, err) } - if err := os.Link(linkTarget, targetPath); err != nil { + if err := reflink.Auto(linkTarget, targetPath); err != nil { return fmt.Errorf("creating hard link %s -> %s: %w", targetPath, linkTarget, err) } diff --git a/internal/gitaly/storage/fs.go b/internal/gitaly/storage/fs.go index 691ec47801..4d849369c3 100644 --- a/internal/gitaly/storage/fs.go +++ b/internal/gitaly/storage/fs.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" ) @@ -40,7 +41,7 @@ func newTargetIsFileError(path string) error { // Link creates a hard link from source to destination and records the operation. func Link(f FS, source, destination string) error { - if err := os.Link(filepath.Join(f.Root(), source), filepath.Join(f.Root(), destination)); err != nil { + if err := reflink.Auto(filepath.Join(f.Root(), source), filepath.Join(f.Root(), destination)); err != nil { return fmt.Errorf("link: %w", err) } diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index 5ff3fdb496..f87cdac2c8 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -4,16 +4,12 @@ import ( "context" "errors" "fmt" - "io" "io/fs" "os" "path/filepath" - "syscall" - - // "syscall" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/keyvalue" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" - "golang.org/x/sys/unix" ) // applyOperations applies the operations from the log entry to the storage. @@ -38,7 +34,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err } destinationPath := string(op.GetDestinationPath()) - if err := LinkOrReflink( filepath.Join(basePath, string(op.GetSourcePath())), filepath.Join(storageRoot, destinationPath), @@ -92,26 +87,37 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // Sync all the dirty directories. for relativePath := range dirtyDirectories { - if err := sync(ctx, filepath.Join(storageRoot, relativePath)); err != nil { + absPath := filepath.Join(storageRoot, relativePath) + + // If the target directory does not exist, skip syncing it. + if _, err := os.Stat(absPath); err != nil { + if errors.Is(err, fs.ErrNotExist) { + continue + } + return fmt.Errorf("stat: %w", err) + } + + if err := sync(ctx, absPath); err != nil { return fmt.Errorf("sync: %w", err) } + + parent := filepath.Dir(absPath) + if _, err := os.Stat(parent); err == nil { + if err := sync(ctx, parent); err != nil { + return fmt.Errorf("sync parent: %w", err) + } + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("stat parent: %w", err) + } } return nil } + // LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { - // Try hard link first (works within same subvolume) - if err := os.Link(src, dst); err == nil { - return nil - } else if !errors.Is(err, syscall.EXDEV) { - // Some other error, not cross-device - return err - } - - // Hard link failed due to cross-device - try reflink - if err := reflink(src, dst); err != nil { + if err := reflink.Auto(src, dst); err != nil { return fmt.Errorf("reflink: %w", err) } @@ -120,32 +126,32 @@ func LinkOrReflink(src, dst string) error { // reflink creates a copy-on-write copy of src to dst using btrfs reflink. // This is space-efficient like hard links but works across subvolumes. -func reflink(src, dst string) error { - srcFile, err := os.Open(src) - if err != nil { - return fmt.Errorf("open source: %w", err) - } - defer srcFile.Close() - - srcInfo, err := srcFile.Stat() - if err != nil { - return fmt.Errorf("stat source: %w", err) - } - - dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) - if err != nil { - return fmt.Errorf("create destination: %w", err) - } - defer dstFile.Close() - - // Use FICLONE ioctl to create a reflink (COW copy) - err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) - if err != nil { - // Reflink failed - fall back to regular copy - if _, err := io.Copy(dstFile, srcFile); err != nil { - return fmt.Errorf("copy data: %w", err) - } - } - - return nil -} \ No newline at end of file +// func reflink(src, dst string) error { +// srcFile, err := os.Open(src) +// if err != nil { +// return fmt.Errorf("open source: %w", err) +// } +// defer srcFile.Close() + +// srcInfo, err := srcFile.Stat() +// if err != nil { +// return fmt.Errorf("stat source: %w", err) +// } + +// dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) +// if err != nil { +// return fmt.Errorf("create destination: %w", err) +// } +// defer dstFile.Close() + +// // Use FICLONE ioctl to create a reflink (COW copy) +// err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) +// if err != nil { +// // Reflink failed - fall back to regular copy +// if _, err := io.Copy(dstFile, srcFile); err != nil { +// return fmt.Errorf("copy data: %w", err) +// } +// } + +// return nil +// } diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index 7a88d44b4b..92a6b6e438 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/gitstorage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" @@ -665,7 +666,7 @@ func createDirectorySnapshot(ctx context.Context, originalDirectory, snapshotDir } } else if info.Mode().IsRegular() { stats.fileCount++ - if err := os.Link(oldPath, newPath); err != nil { + if err := reflink.Auto(oldPath, newPath); err != nil { return fmt.Errorf("link file: %w", err) } } else { diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go index f5432f2259..862b6319bb 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go @@ -358,22 +358,22 @@ func TestTransactionManager(t *testing.T) { setup := setupTest(t, ctx, testPartitionID, relativePath) subTests := map[string][]transactionTestCase{ - "Common": generateCommonTests(t, ctx, setup), - "CommittedEntries": generateCommittedEntriesTests(t, setup), - "ModifyReferences": generateModifyReferencesTests(t, setup), - "CreateRepository": generateCreateRepositoryTests(t, setup), - "DeleteRepository": generateDeleteRepositoryTests(t, setup), - "DefaultBranch": generateDefaultBranchTests(t, setup), - "Alternate": generateAlternateTests(t, setup), + // "Common": generateCommonTests(t, ctx, setup), + // "CommittedEntries": generateCommittedEntriesTests(t, setup), + // "ModifyReferences": generateModifyReferencesTests(t, setup), + // "CreateRepository": generateCreateRepositoryTests(t, setup), + // "DeleteRepository": generateDeleteRepositoryTests(t, setup), + // "DefaultBranch": generateDefaultBranchTests(t, setup), + // "Alternate": generateAlternateTests(t, setup), "CustomHooks": generateCustomHooksTests(t, setup), - "Housekeeping/PackRefs": generateHousekeepingPackRefsTests(t, ctx, testPartitionID, relativePath), - "Housekeeping/RepackingStrategy": generateHousekeepingRepackingStrategyTests(t, ctx, testPartitionID, relativePath), - "Housekeeping/RepackingConcurrent": generateHousekeepingRepackingConcurrentTests(t, ctx, setup), - "Housekeeping/CommitGraphs": generateHousekeepingCommitGraphsTests(t, ctx, setup), - "Consumer": generateConsumerTests(t, setup), - "KeyValue": generateKeyValueTests(t, setup), - "Offloading": generateOffloadingTests(t, ctx, testPartitionID, relativePath), - "Rehydrating": generateRehydratingTests(t, ctx, testPartitionID, relativePath), + // "Housekeeping/PackRefs": generateHousekeepingPackRefsTests(t, ctx, testPartitionID, relativePath), + // "Housekeeping/RepackingStrategy": generateHousekeepingRepackingStrategyTests(t, ctx, testPartitionID, relativePath), + // "Housekeeping/RepackingConcurrent": generateHousekeepingRepackingConcurrentTests(t, ctx, setup), + // "Housekeeping/CommitGraphs": generateHousekeepingCommitGraphsTests(t, ctx, setup), + // "Consumer": generateConsumerTests(t, setup), + // "KeyValue": generateKeyValueTests(t, setup), + // "Offloading": generateOffloadingTests(t, ctx, testPartitionID, relativePath), + // "Rehydrating": generateRehydratingTests(t, ctx, testPartitionID, relativePath), } for desc, tests := range subTests { diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index 8599709a71..481cf319cf 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -1,20 +1,15 @@ package wal import ( - "errors" "fmt" - "io" "io/fs" "os" "path/filepath" "strconv" - "syscall" - - // "syscall" + rf "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" - "golang.org/x/sys/unix" ) // Entry represents a write-ahead log entry. @@ -82,7 +77,7 @@ func (e *Entry) stageFile(path string) (string, error) { // actual name the file will be linked as. fileName := strconv.FormatUint(e.fileIDSequence, 36) if err := LinkOrReflink(path, filepath.Join(e.stateDirectory, fileName)); err != nil { - return "", fmt.Errorf("link: %w", err) + return "", fmt.Errorf("LinkOrReflink: %w", err) } return fileName, nil @@ -127,16 +122,7 @@ func (e *Entry) RemoveDirectoryEntry(relativePath string) { // LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { - // Try hard link first (works within same subvolume) - if err := os.Link(src, dst); err == nil { - return nil - } else if !errors.Is(err, syscall.EXDEV) { - // Some other error, not cross-device - return err - } - - // Hard link failed due to cross-device - try reflink - if err := reflink(src, dst); err != nil { + if err := rf.Auto(src, dst); err != nil { return fmt.Errorf("reflink: %w", err) } @@ -145,32 +131,64 @@ func LinkOrReflink(src, dst string) error { // reflink creates a copy-on-write copy of src to dst using btrfs reflink. // This is space-efficient like hard links but works across subvolumes. -func reflink(src, dst string) error { - srcFile, err := os.Open(src) - if err != nil { - return fmt.Errorf("open source: %w", err) - } - defer srcFile.Close() - - srcInfo, err := srcFile.Stat() - if err != nil { - return fmt.Errorf("stat source: %w", err) - } - - dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) - if err != nil { - return fmt.Errorf("create destination: %w", err) - } - defer dstFile.Close() - - // Use FICLONE ioctl to create a reflink (COW copy) - err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) - if err != nil { - // Reflink failed - fall back to regular copy - if _, err := io.Copy(dstFile, srcFile); err != nil { - return fmt.Errorf("copy data: %w", err) - } - } - - return nil -} \ No newline at end of file +// func reflink(src, dst string) error { +// srcFile, err := os.Open(src) +// if err != nil { +// return fmt.Errorf("open source: %w", err) +// } +// defer srcFile.Close() + +// srcInfo, err := srcFile.Stat() +// if err != nil { +// return fmt.Errorf("stat source: %w", err) +// } + +// dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) +// if err != nil { +// return fmt.Errorf("create destination: %w", err) +// } +// defer dstFile.Close() + +// // Use FICLONE ioctl to create a reflink (COW copy) +// err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) +// if err != nil { +// // Reflink failed - fall back to regular copy +// if _, err := io.Copy(dstFile, srcFile); err != nil { +// return fmt.Errorf("copy data: %w", err) +// } +// } + +// return nil +// } + +// func copyWithReflink(src, dst string) error { +// // Get source info +// srcInfo, err := os.Stat(src) +// if err != nil { +// return err +// } + +// // If source is a directory, copy its contents +// if srcInfo.IsDir() { +// // Create destination if it doesn't exist +// err = os.MkdirAll(dst, srcInfo.Mode()) +// if err != nil { +// return err +// } + +// cmd := exec.Command("cp", "-R", "--reflink=auto", "-a", src+"/.", dst) +// output, err := cmd.CombinedOutput() +// if err != nil { +// return fmt.Errorf("copy failed: %v, output: %s", err, output) +// } +// } else { +// // Single file copy +// cmd := exec.Command("cp", "--reflink=auto", "-a", src, dst) +// output, err := cmd.CombinedOutput() +// if err != nil { +// return fmt.Errorf("copy failed: %v, output: %s", err, output) +// } +// } + +// return os.RemoveAll(src) +// } \ No newline at end of file diff --git a/internal/gitaly/storage/wal/reference_recorder.go b/internal/gitaly/storage/wal/reference_recorder.go index fede4d5e3a..a41dd6b573 100644 --- a/internal/gitaly/storage/wal/reference_recorder.go +++ b/internal/gitaly/storage/wal/reference_recorder.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/git" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/wal/reftree" ) @@ -65,7 +66,7 @@ func NewReferenceRecorder(tmpDir string, entry *Entry, snapshotRoot, relativePat preImagePackedRefsPath := filepath.Join(tmpDir, "packed-refs") postImagePackedRefsPath := filepath.Join(repoRoot, "packed-refs") - if err := os.Link(postImagePackedRefsPath, preImagePackedRefsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + if err := reflink.Auto(postImagePackedRefsPath, preImagePackedRefsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("record pre-image packed-refs: %w", err) } diff --git a/internal/helper/reflink.go b/internal/helper/reflink.go new file mode 100644 index 0000000000..f2cadf7bea --- /dev/null +++ b/internal/helper/reflink.go @@ -0,0 +1,76 @@ +package helper + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/KarpelesLab/reflink" +) + +// RenameDirectoryWithReflink recursively copies a directory using reflinks when possible +func RenameDirectoryWithReflink(src, dst string) error { + srcInfo, err := os.Stat(src) + if err != nil { + return fmt.Errorf("stat source: %w", err) + } + + // Create destination directory with same permissions as source + if err := os.MkdirAll(dst, srcInfo.Mode()); err != nil { + return fmt.Errorf("create destination: %w", err) + } + + entries, err := os.ReadDir(src) + if err != nil { + return fmt.Errorf("read directory: %w", err) + } + + for _, entry := range entries { + srcPath := filepath.Join(src, entry.Name()) + dstPath := filepath.Join(dst, entry.Name()) + + info, err := entry.Info() + if err != nil { + return fmt.Errorf("get entry info: %w", err) + } + + if entry.IsDir() { + // Recursively copy subdirectory + if err := RenameDirectoryWithReflink(srcPath, dstPath); err != nil { + return err + } + } else if info.Mode()&os.ModeSymlink != 0 { + // Copy symlink + target, err := os.Readlink(srcPath) + if err != nil { + return fmt.Errorf("read symlink: %w", err) + } + if err := os.Symlink(target, dstPath); err != nil { + return fmt.Errorf("create symlink: %w", err) + } + } else if info.Mode().IsRegular() { + // Copy regular file with reflink + // reflink.Auto tries reflink first, falls back to regular copy + if err := reflinkRename(srcPath, dstPath); err != nil { + return fmt.Errorf("reflink rename %s: %w", srcPath, err) + } + } + // Skip other file types (devices, sockets, etc.) + } + + return nil +} + +func reflinkRename(src, dst string) error { + // cp to new loc on different subvolume + err := reflink.Auto(src, dst) + if err != nil { + return fmt.Errorf("reflink copy failed: %w", err) + } + // remove original after successful reflink + err = os.RemoveAll(src) + if err != nil { + return fmt.Errorf("removing source after reflink failed: %w", err) + } + return nil +} \ No newline at end of file -- GitLab From 37127c756892a2b87d4afd23826a529d32a43d83 Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Thu, 23 Oct 2025 02:19:40 +0000 Subject: [PATCH 3/9] Fix apply_operations test --- .../storagemgr/partition/apply_operations.go | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index f87cdac2c8..945f60ac14 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -7,8 +7,11 @@ import ( "io/fs" "os" "path/filepath" + "syscall" + "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/keyvalue" + "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" ) @@ -47,7 +50,7 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err op := wrapper.CreateDirectory path := string(op.GetPath()) - if err := os.Mkdir(filepath.Join(storageRoot, path), fs.FileMode(op.GetMode())); err != nil && !errors.Is(err, fs.ErrExist) { + if err := os.MkdirAll(filepath.Join(storageRoot, path), fs.FileMode(op.GetMode())); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("mkdir: %w", err) } @@ -66,8 +69,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // Remove the dirty marker from the removed directory entry if it exists. There's // no need to sync it anymore as it doesn't exist. delete(dirtyDirectories, path) - // Sync the parent directory where directory entry was removed from. - dirtyDirectories[filepath.Dir(path)] = struct{}{} case *gitalypb.LogEntry_Operation_SetKey_: op := wrapper.SetKey @@ -100,15 +101,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err if err := sync(ctx, absPath); err != nil { return fmt.Errorf("sync: %w", err) } - - parent := filepath.Dir(absPath) - if _, err := os.Stat(parent); err == nil { - if err := sync(ctx, parent); err != nil { - return fmt.Errorf("sync parent: %w", err) - } - } else if !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("stat parent: %w", err) - } } return nil @@ -117,6 +109,21 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { + // Ensure the parent directory exists + dstDir := filepath.Dir(dst) + if err := os.MkdirAll(dstDir, mode.Directory); err != nil { + return fmt.Errorf("create parent directory: %w", err) + } + + // Try hard link first (fast path for same subvolume) + if err := os.Link(src, dst); err == nil { + return nil + } else if !errors.Is(err, syscall.EXDEV) && !errors.Is(err, fs.ErrExist) { + // Some error other than cross-device or already exists + return fmt.Errorf("link: %w", err) + } + + // Hard link failed due to cross-device - use reflink if err := reflink.Auto(src, dst); err != nil { return fmt.Errorf("reflink: %w", err) } -- GitLab From 4829e894a4fc4534f0648822253da550ba643523 Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Fri, 24 Oct 2025 05:30:46 +0000 Subject: [PATCH 4/9] working pt2: got pass majority of failing tests in txn manager --- .vscode/settings.json | 2 +- internal/git/objectpool/disconnect.go | 5 +- internal/git/quarantine/quarantine.go | 3 +- internal/gitaly/repoutil/custom_hooks.go | 73 ++--------- internal/gitaly/repoutil/remove.go | 3 +- .../gitaly/service/repository/replicate.go | 4 +- internal/gitaly/storage/fs.go | 6 +- .../storagemgr/partition/apply_operations.go | 26 ++-- .../storagemgr/partition/snapshot/snapshot.go | 46 +++---- .../partition/transaction_manager.go | 2 +- .../partition/transaction_manager_test.go | 30 ++--- .../storage/storagemgr/partition_manager.go | 1 + internal/gitaly/storage/wal/entry.go | 37 +++++- .../gitaly/storage/wal/reference_recorder.go | 4 +- internal/helper/reflink.go | 120 ++++++++++++------ 15 files changed, 182 insertions(+), 180 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 705cb2f46a..98c116c452 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,6 @@ { "go.testEnvVars": { - "TEST_TMP_DIR": "/mnt/git-repositories/tmp" + "TEST_TMP_DIR": "/mnt/git-repositories/tmp_sv" }, "go.testFlags": ["-v"] } diff --git a/internal/git/objectpool/disconnect.go b/internal/git/objectpool/disconnect.go index dd369c48c5..63f046d2e0 100644 --- a/internal/git/objectpool/disconnect.go +++ b/internal/git/objectpool/disconnect.go @@ -11,7 +11,6 @@ import ( "strings" "time" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/git/gitcmd" "gitlab.com/gitlab-org/gitaly/v18/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v18/internal/git/stats" @@ -211,7 +210,7 @@ func removeAlternatesIfOk(ctx context.Context, repo *localrepo.Repo, altFile, ba return fmt.Errorf("preparatory vote for disconnecting alternate: %w", err) } - if err := os.Rename(altFile, backupFile); err != nil { + if err := helper.RenameDirectoryWithReflink(altFile, backupFile); err != nil { return err } @@ -227,7 +226,7 @@ func removeAlternatesIfOk(ctx context.Context, repo *localrepo.Repo, altFile, ba // preserved for possible forensic use. tmp := backupFile + ".2" - if err := reflink.Auto(backupFile, tmp); err != nil { + if err := helper.ReflinkWithPermissions(backupFile, tmp); err != nil { logger.WithError(err).ErrorContext(ctx, "copy backup alternates file") return } diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go index d742649203..de8e41f6a8 100644 --- a/internal/git/quarantine/quarantine.go +++ b/internal/git/quarantine/quarantine.go @@ -9,7 +9,6 @@ import ( "sort" "strings" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/helper" @@ -180,7 +179,7 @@ func migrate(ctx context.Context, sourcePath, targetPath string) error { func finalizeObjectFile(sourcePath, targetPath string) error { // We first try to link the file via a hardlink. The benefit compared to doing a rename is // that in case of a collision, we do not replace the target. - err := reflink.Auto(sourcePath, targetPath) + err := helper.ReflinkWithPermissions(sourcePath, targetPath) // In case the hardlink failed, we fall back to a rename. renamed := false diff --git a/internal/gitaly/repoutil/custom_hooks.go b/internal/gitaly/repoutil/custom_hooks.go index e59f22c409..dd9427b0e0 100644 --- a/internal/gitaly/repoutil/custom_hooks.go +++ b/internal/gitaly/repoutil/custom_hooks.go @@ -108,7 +108,7 @@ func SetCustomHooks( ) error { repoPath, err := locator.GetRepoPath(ctx, repo) if err != nil { - return fmt.Errorf("getting repo path %s: %w", repo.GetRelativePath(), err) + return fmt.Errorf("getting repo path: %s:: %w", repo.GetRelativePath(), err) } var originalCustomHooksRelativePath string @@ -199,11 +199,14 @@ func SetCustomHooks( syncer := safe.NewSyncer() + // Ensure parent directories exist before syncing + if err := os.MkdirAll(filepath.Dir(tempHooksPath), mode.Directory); err != nil { + return fmt.Errorf("making repo hooks parent directories: %w", err) + } + if storage.NeedsSync(ctx) { - // Sync the custom hooks in the temporary directory before being moved into - // the repository. This makes the move atomic as there is no state where the - // move succeeds, but the hook files themselves are not yet on the disk, or - // are partially written. + + // Then sync all the hook files recursively if err := syncer.SyncRecursive(ctx, tempHooksPath); err != nil { return fmt.Errorf("syncing extracted custom hooks: %w", err) } @@ -321,63 +324,3 @@ func voteCustomHooks( return nil } -// moveDirectory moves a directory from src to dst, working across btrfs subvolumes. -// func moveDirectory(ctx context.Context, src, dst string) error { -// // Check if source exists -// if _, err := os.Stat(src); err != nil { -// return err -// } - -// // Try simple rename first (fast path for same subvolume) -// if err := os.Rename(src, dst); err == nil { -// return nil -// } else if !errors.Is(err, syscall.EXDEV) { -// return err -// } - -// // Cross-device move - ensure parent dir exists -// if err := os.MkdirAll(filepath.Dir(dst), mode.Directory); err != nil { -// return fmt.Errorf("create destination parent: %w", err) -// } - -// // Use cp -r -a with reflinks -// cmd := exec.CommandContext(ctx, "cp", "-r", "-a", "--reflink=auto", src, dst) -// if output, err := cmd.CombinedOutput(); err != nil { -// return fmt.Errorf("cp failed: %w, output: %s", err, string(output)) -// } - -// // Remove source after successful copy -// return os.RemoveAll(src) -// } - -// func copyWithReflink(src, dst string) error { -// // Get source info -// srcInfo, err := os.Stat(src) -// if err != nil { -// return err -// } - -// // If source is a directory, copy its contents -// if srcInfo.IsDir() { -// // Create destination if it doesn't exist -// err = os.MkdirAll(dst, srcInfo.Mode()) -// if err != nil { -// return err -// } - -// cmd := exec.Command("cp", "--reflink=always", "-a", "-T", src+"/.", dst) -// output, err := cmd.CombinedOutput() -// if err != nil { -// return fmt.Errorf("copy failed: %v, output: %s", err, output) -// } -// } else { -// // Single file copy -// cmd := exec.Command("cp", "--reflink=auto", "-a", src, dst) -// output, err := cmd.CombinedOutput() -// if err != nil { -// return fmt.Errorf("copy failed: %v, output: %s", err, output) -// } -// } - -// return os.RemoveAll(src) -// } \ No newline at end of file diff --git a/internal/gitaly/repoutil/remove.go b/internal/gitaly/repoutil/remove.go index fca1f80a27..b7a0fc8263 100644 --- a/internal/gitaly/repoutil/remove.go +++ b/internal/gitaly/repoutil/remove.go @@ -11,6 +11,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/counter" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/log" "gitlab.com/gitlab-org/gitaly/v18/internal/safe" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" @@ -126,7 +127,7 @@ func remove( // We move the repository into our temporary directory first before we start to // delete it. This is done such that we don't leave behind a partially-removed and // thus likely corrupt repository. - if err := os.Rename(path, filepath.Join(destDir, "repo")); err != nil { + if err := helper.RenameDirectoryWithReflink(path, filepath.Join(destDir, "repo")); err != nil { return structerr.NewInternal("staging repository for removal: %w", err) } diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go index 629518ab41..2a41565355 100644 --- a/internal/gitaly/service/repository/replicate.go +++ b/internal/gitaly/service/repository/replicate.go @@ -11,7 +11,6 @@ import ( "path/filepath" "strings" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/featureflag" "gitlab.com/gitlab-org/gitaly/v18/internal/git" "gitlab.com/gitlab-org/gitaly/v18/internal/git/gitcmd" @@ -25,6 +24,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v18/internal/grpc/client" "gitlab.com/gitlab-org/gitaly/v18/internal/grpc/metadata" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/safe" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" "gitlab.com/gitlab-org/gitaly/v18/internal/tempdir" @@ -407,7 +407,7 @@ func (s *server) extractTarToDirectory(ctx context.Context, reader io.Reader, ta return fmt.Errorf("removing existing file for hard link %s: %w", targetPath, err) } - if err := reflink.Auto(linkTarget, targetPath); err != nil { + if err := helper.ReflinkWithPermissions(linkTarget, targetPath); err != nil { return fmt.Errorf("creating hard link %s -> %s: %w", targetPath, linkTarget, err) } diff --git a/internal/gitaly/storage/fs.go b/internal/gitaly/storage/fs.go index 4d849369c3..a2fca8eea3 100644 --- a/internal/gitaly/storage/fs.go +++ b/internal/gitaly/storage/fs.go @@ -8,8 +8,8 @@ import ( "path/filepath" "strings" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" ) @@ -41,8 +41,8 @@ func newTargetIsFileError(path string) error { // Link creates a hard link from source to destination and records the operation. func Link(f FS, source, destination string) error { - if err := reflink.Auto(filepath.Join(f.Root(), source), filepath.Join(f.Root(), destination)); err != nil { - return fmt.Errorf("link: %w", err) + if err := helper.ReflinkWithPermissions(filepath.Join(f.Root(), source), filepath.Join(f.Root(), destination)); err != nil { + return fmt.Errorf("reflink auto: %w", err) } if err := f.RecordLink(source, destination); err != nil { diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index 945f60ac14..f803cb3608 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -7,11 +7,13 @@ import ( "io/fs" "os" "path/filepath" + // "sort" + // "strings" "syscall" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/keyvalue" - "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" + + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" ) @@ -50,7 +52,9 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err op := wrapper.CreateDirectory path := string(op.GetPath()) - if err := os.MkdirAll(filepath.Join(storageRoot, path), fs.FileMode(op.GetMode())); err != nil && !errors.Is(err, fs.ErrExist) { + fmt.Printf("applyOperations: CreateDirectory for path: %s\n", path) + + if err := os.Mkdir(filepath.Join(storageRoot, path), fs.FileMode(op.GetMode())); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("mkdir: %w", err) } @@ -62,6 +66,7 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err op := wrapper.RemoveDirectoryEntry path := string(op.GetPath()) + fmt.Println("apply ops: Removing" , filepath.Join(storageRoot, path)) if err := os.Remove(filepath.Join(storageRoot, path)); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove: %w", err) } @@ -90,14 +95,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err for relativePath := range dirtyDirectories { absPath := filepath.Join(storageRoot, relativePath) - // If the target directory does not exist, skip syncing it. - if _, err := os.Stat(absPath); err != nil { - if errors.Is(err, fs.ErrNotExist) { - continue - } - return fmt.Errorf("stat: %w", err) - } - if err := sync(ctx, absPath); err != nil { return fmt.Errorf("sync: %w", err) } @@ -111,8 +108,9 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err func LinkOrReflink(src, dst string) error { // Ensure the parent directory exists dstDir := filepath.Dir(dst) - if err := os.MkdirAll(dstDir, mode.Directory); err != nil { - return fmt.Errorf("create parent directory: %w", err) + fmt.Println("ensuring parent directory exists", dstDir) + if _, err := os.Stat(dstDir); err != nil { + return fmt.Errorf("parent directory does not exist: %w", err) } // Try hard link first (fast path for same subvolume) @@ -124,7 +122,7 @@ func LinkOrReflink(src, dst string) error { } // Hard link failed due to cross-device - use reflink - if err := reflink.Auto(src, dst); err != nil { + if err := helper.ReflinkWithPermissions(src, dst); err != nil { return fmt.Errorf("reflink: %w", err) } diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index 92a6b6e438..61ca5a3c8b 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -11,11 +11,11 @@ import ( "strings" "time" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/gitstorage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode/permission" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" ) // ModeReadOnlyDirectory is the mode given to directories in read-only snapshots. @@ -142,7 +142,6 @@ func (s *snapshot) Close() error { return nil } - // setDirectoryMode walks the snapshot and sets each directory's mode to the given mode. func (s *snapshot) setDirectoryMode(mode fs.FileMode) error { return storage.SetDirectoryMode(s.root, mode) @@ -178,7 +177,6 @@ func newSnapshot(ctx context.Context, storageRoot, snapshotRoot string, relative return nil, fmt.Errorf("create repository snapshots: %w", err) } - if readOnly { // Now that we've finished creating the snapshot, change the directory permissions to read-only // to prevent writing in the snapshot. @@ -191,7 +189,6 @@ func newSnapshot(ctx context.Context, storageRoot, snapshotRoot string, relative return s, nil } - // setParentDirectoriesReadOnly makes all parent directories (non-subvolume directories) read-only. // This is called after all repository snapshots have been created. func setParentDirectoriesReadOnly(s *snapshot) error { @@ -224,11 +221,12 @@ func setParentDirectoriesReadOnly(s *snapshot) error { return nil }) } + // createRepositorySnapshots creates btrfs snapshots of all repositories at the given relative paths // and their alternates, then applies filtering to remove unwanted files. func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot string, relativePaths []string, snapshotFilter Filter, readOnly bool, s *snapshot, - ) error { +) error { // Create the root directory always to as the storage would also exist always. s.stats.directoryCount++ // if err := os.Mkdir(snapshotRoot, 0755); err != nil { @@ -345,19 +343,21 @@ func createParentDirectories(storageRoot, snapshotRoot, relativePath string, sta return nil } + // ensureDir0700 creates path (and parents) and enforces 0700 permissions. // This guarantees the created directory has owner-only permissions regardless of umask. func ensureDir0700(path string) error { - if err := os.MkdirAll(path, 0700); err != nil { - return fmt.Errorf("create parent directories: %w", err) - } - // Explicitly set permissions to 0700 to avoid umask influence and ensure tests that - // assert specific directory modes are stable. - if err := os.Chmod(path, 0700); err != nil { - return fmt.Errorf("set directory permissions: %w", err) - } - return nil + if err := os.MkdirAll(path, 0o700); err != nil { + return fmt.Errorf("create parent directories: %w", err) + } + // Explicitly set permissions to 0700 to avoid umask influence and ensure tests that + // assert specific directory modes are stable. + if err := os.Chmod(path, 0o700); err != nil { + return fmt.Errorf("set directory permissions: %w", err) + } + return nil } + // createRepositorySnapshot snapshots a repository's current state at snapshotPath. This is done by // recreating the repository's directory structure and hard linking the repository's files in their // correct locations there. This effectively does a copy-free clone of the repository. Since the files @@ -401,7 +401,6 @@ func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, re // If it's not a subvolume, convert it to one if !isBtrfsSubvolume { - s.repositoryPaths = append(s.repositoryPaths, originalPath) if err := convertToSubvolume(ctx, originalPath); err != nil { return fmt.Errorf("convert to subvolume: %w", err) } @@ -418,7 +417,6 @@ func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, re return nil } - // createBtrfsSnapshotAndFilter creates a btrfs snapshot and applies filtering. func createBtrfsSnapshotAndFilter(ctx context.Context, originalPath, snapshotPath string, snapshotFilter Filter, readOnly bool, s *snapshot, @@ -449,6 +447,7 @@ func createBtrfsSnapshotAndFilter(ctx context.Context, originalPath, snapshotPat return nil } + // applyFilterToSnapshot walks through the snapshot and removes files/directories that don't match the filter. func applyFilterToSnapshot(ctx context.Context, snapshotPath string, matcher Filter, stats *snapshotStatistics) error { // We need to walk the snapshot and remove files that don't match the filter @@ -504,8 +503,6 @@ func applyFilterToSnapshot(ctx context.Context, snapshotPath string, matcher Fil return nil } - - // createBtrfsSnapshot creates a btrfs subvolume snapshot from source to destination. // If readOnly is true, the snapshot will be created as a read-only subvolume. func createBtrfsSnapshot(ctx context.Context, source, destination string, readOnly bool) error { @@ -528,7 +525,6 @@ func createBtrfsSnapshot(ctx context.Context, source, destination string, readOn return nil } - // isBtrfsSubvolume checks if the given path is a btrfs subvolume. func isBtrfsSubvolume(ctx context.Context, path string) (bool, error) { cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "show", path) @@ -540,7 +536,6 @@ func isBtrfsSubvolume(ctx context.Context, path string) (bool, error) { return true, nil } - // convertToSubvolume converts a regular directory to a btrfs subvolume in place. func convertToSubvolume(ctx context.Context, path string) error { tempPath := path + ".subvol-temp" @@ -572,7 +567,6 @@ func convertToSubvolume(ctx context.Context, path string) error { return nil } - // setBtrfsReadOnly sets or unsets the read-only property of a btrfs subvolume. func setBtrfsReadOnly(ctx context.Context, path string, readOnly bool) error { value := "false" @@ -589,8 +583,6 @@ func setBtrfsReadOnly(ctx context.Context, path string, readOnly bool) error { return nil } - - // deleteBtrfsSnapshot deletes a btrfs subvolume snapshot. // If the snapshot is read-only, it first changes it to read-write before deletion. func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly bool) error { @@ -613,8 +605,10 @@ func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly b // Try to delete as a btrfs subvolume first cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "delete", snapshotPath) + fmt.Println("Deleting btrfs subvolume:", snapshotPath) if err := cmd.Run(); err == nil { // Successfully deleted as a subvolume + fmt.Println("Successfully deleted btrfs subvolume:", snapshotPath) return nil } @@ -626,10 +620,6 @@ func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly b return nil } - - - - // createDirectorySnapshot recursively recreates the directory structure from originalDirectory into // snapshotDirectory and hard links files into the same locations in snapshotDirectory. // @@ -666,7 +656,7 @@ func createDirectorySnapshot(ctx context.Context, originalDirectory, snapshotDir } } else if info.Mode().IsRegular() { stats.fileCount++ - if err := reflink.Auto(oldPath, newPath); err != nil { + if err := helper.ReflinkWithPermissions(oldPath, newPath); err != nil { return fmt.Errorf("link file: %w", err) } } else { diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go index 8056d30a85..4be9cb0e98 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go @@ -2183,7 +2183,7 @@ func (mgr *TransactionManager) applyLogEntry(ctx context.Context, lsn storage.LS if err := mgr.db.Update(func(tx keyvalue.ReadWriter) error { if err := applyOperations(ctx, safe.NewSyncer().Sync, mgr.storagePath, mgr.logManager.GetEntryPath(lsn), manifest.GetOperations(), tx); err != nil { - return fmt.Errorf("apply operations: %w", err) + return fmt.Errorf("apply operations for lsn %d: %w", lsn, err) } return nil diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go index 862b6319bb..f5432f2259 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_test.go @@ -358,22 +358,22 @@ func TestTransactionManager(t *testing.T) { setup := setupTest(t, ctx, testPartitionID, relativePath) subTests := map[string][]transactionTestCase{ - // "Common": generateCommonTests(t, ctx, setup), - // "CommittedEntries": generateCommittedEntriesTests(t, setup), - // "ModifyReferences": generateModifyReferencesTests(t, setup), - // "CreateRepository": generateCreateRepositoryTests(t, setup), - // "DeleteRepository": generateDeleteRepositoryTests(t, setup), - // "DefaultBranch": generateDefaultBranchTests(t, setup), - // "Alternate": generateAlternateTests(t, setup), + "Common": generateCommonTests(t, ctx, setup), + "CommittedEntries": generateCommittedEntriesTests(t, setup), + "ModifyReferences": generateModifyReferencesTests(t, setup), + "CreateRepository": generateCreateRepositoryTests(t, setup), + "DeleteRepository": generateDeleteRepositoryTests(t, setup), + "DefaultBranch": generateDefaultBranchTests(t, setup), + "Alternate": generateAlternateTests(t, setup), "CustomHooks": generateCustomHooksTests(t, setup), - // "Housekeeping/PackRefs": generateHousekeepingPackRefsTests(t, ctx, testPartitionID, relativePath), - // "Housekeeping/RepackingStrategy": generateHousekeepingRepackingStrategyTests(t, ctx, testPartitionID, relativePath), - // "Housekeeping/RepackingConcurrent": generateHousekeepingRepackingConcurrentTests(t, ctx, setup), - // "Housekeeping/CommitGraphs": generateHousekeepingCommitGraphsTests(t, ctx, setup), - // "Consumer": generateConsumerTests(t, setup), - // "KeyValue": generateKeyValueTests(t, setup), - // "Offloading": generateOffloadingTests(t, ctx, testPartitionID, relativePath), - // "Rehydrating": generateRehydratingTests(t, ctx, testPartitionID, relativePath), + "Housekeeping/PackRefs": generateHousekeepingPackRefsTests(t, ctx, testPartitionID, relativePath), + "Housekeeping/RepackingStrategy": generateHousekeepingRepackingStrategyTests(t, ctx, testPartitionID, relativePath), + "Housekeeping/RepackingConcurrent": generateHousekeepingRepackingConcurrentTests(t, ctx, setup), + "Housekeeping/CommitGraphs": generateHousekeepingCommitGraphsTests(t, ctx, setup), + "Consumer": generateConsumerTests(t, setup), + "KeyValue": generateKeyValueTests(t, setup), + "Offloading": generateOffloadingTests(t, ctx, testPartitionID, relativePath), + "Rehydrating": generateRehydratingTests(t, ctx, testPartitionID, relativePath), } for desc, tests := range subTests { diff --git a/internal/gitaly/storage/storagemgr/partition_manager.go b/internal/gitaly/storage/storagemgr/partition_manager.go index 7854becb84..51432f0adb 100644 --- a/internal/gitaly/storage/storagemgr/partition_manager.go +++ b/internal/gitaly/storage/storagemgr/partition_manager.go @@ -536,6 +536,7 @@ func (sm *StorageManager) startPartition(ctx context.Context, partitionID storag } stagingDir, err := os.MkdirTemp(sm.stagingDirectory, "") + fmt.Println("Created Staging dir:", stagingDir) if err != nil { return fmt.Errorf("create staging directory: %w", err) } diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index 481cf319cf..46c45a72d4 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -1,13 +1,16 @@ package wal import ( + "errors" "fmt" "io/fs" "os" "path/filepath" "strconv" - rf "github.com/KarpelesLab/reflink" + "syscall" + "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" "gitlab.com/gitlab-org/gitaly/v18/proto/go/gitalypb" ) @@ -95,6 +98,7 @@ func (e *Entry) DeleteKey(key []byte) { // CreateDirectory records creation of a single directory. func (e *Entry) CreateDirectory(relativePath string) { + fmt.Printf("create directory %s\n", relativePath) e.operations.createDirectory(relativePath) } @@ -105,24 +109,43 @@ func (e *Entry) CreateFile(sourceAbsolutePath string, relativePath string) error if err != nil { return fmt.Errorf("stage file: %w", err) } - + fmt.Printf("create file hardlinking %s to %s\n", sourceAbsolutePath, relativePath) e.operations.createHardLink(stagedFile, relativePath, false) return nil } // CreateLink records a creation of a hard link to an exisiting file in the partition. func (e *Entry) CreateLink(sourceRelativePath, destinationRelativePath string) { + fmt.Printf("hardlinking %s to %s\n", sourceRelativePath, destinationRelativePath) e.operations.createHardLink(sourceRelativePath, destinationRelativePath, true) } // RemoveDirectoryEntry records the removal of the directory entry at the given path. func (e *Entry) RemoveDirectoryEntry(relativePath string) { + fmt.Printf("removing directory entry %s\n", relativePath) e.operations.removeDirectoryEntry(relativePath) } + // LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { - if err := rf.Auto(src, dst); err != nil { + // Ensure the parent directory exists + dstDir := filepath.Dir(dst) + if _, err := os.Stat(dstDir); err != nil { + fmt.Printf("LinkOrReflink: parent directory check failed: %v\n", err) + return fmt.Errorf("parent directory does not exist: %w", err) + } + + // Try hard link first (fast path for same subvolume) + if err := os.Link(src, dst); err == nil { + return nil + } else if !errors.Is(err, syscall.EXDEV) && !errors.Is(err, fs.ErrExist) { + // Some error other than cross-device or already exists + return fmt.Errorf("link: %w", err) + } + + // Hard link failed due to cross-device - use reflink + if err := helper.ReflinkWithPermissions(src, dst); err != nil { return fmt.Errorf("reflink: %w", err) } @@ -167,7 +190,7 @@ func LinkOrReflink(src, dst string) error { // if err != nil { // return err // } - + // // If source is a directory, copy its contents // if srcInfo.IsDir() { // // Create destination if it doesn't exist @@ -175,7 +198,7 @@ func LinkOrReflink(src, dst string) error { // if err != nil { // return err // } - + // cmd := exec.Command("cp", "-R", "--reflink=auto", "-a", src+"/.", dst) // output, err := cmd.CombinedOutput() // if err != nil { @@ -189,6 +212,6 @@ func LinkOrReflink(src, dst string) error { // return fmt.Errorf("copy failed: %v, output: %s", err, output) // } // } - + // return os.RemoveAll(src) -// } \ No newline at end of file +// } diff --git a/internal/gitaly/storage/wal/reference_recorder.go b/internal/gitaly/storage/wal/reference_recorder.go index a41dd6b573..e72cccbee9 100644 --- a/internal/gitaly/storage/wal/reference_recorder.go +++ b/internal/gitaly/storage/wal/reference_recorder.go @@ -9,9 +9,9 @@ import ( "path/filepath" "strings" - "github.com/KarpelesLab/reflink" "gitlab.com/gitlab-org/gitaly/v18/internal/git" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/wal/reftree" + "gitlab.com/gitlab-org/gitaly/v18/internal/helper" ) // ReferenceRecorder records the file system operations performed by reference transactions. @@ -66,7 +66,7 @@ func NewReferenceRecorder(tmpDir string, entry *Entry, snapshotRoot, relativePat preImagePackedRefsPath := filepath.Join(tmpDir, "packed-refs") postImagePackedRefsPath := filepath.Join(repoRoot, "packed-refs") - if err := reflink.Auto(postImagePackedRefsPath, preImagePackedRefsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + if err := helper.ReflinkWithPermissions(postImagePackedRefsPath, preImagePackedRefsPath); err != nil && !errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("record pre-image packed-refs: %w", err) } diff --git a/internal/helper/reflink.go b/internal/helper/reflink.go index f2cadf7bea..4975f37a01 100644 --- a/internal/helper/reflink.go +++ b/internal/helper/reflink.go @@ -8,66 +8,114 @@ import ( "github.com/KarpelesLab/reflink" ) -// RenameDirectoryWithReflink recursively copies a directory using reflinks when possible +// RenameDirectoryWithReflink recursively copies a directory or file using reflinks when possible +// RenameDirectoryWithReflink recursively copies a directory or file using reflinks when possible +// then deletes the source (simulating a rename operation) func RenameDirectoryWithReflink(src, dst string) error { - srcInfo, err := os.Stat(src) - if err != nil { - return fmt.Errorf("stat source: %w", err) + // Remove destination if it exists (not the source!) + if _, err := os.Lstat(dst); err == nil { + if err := os.RemoveAll(dst); err != nil { + return fmt.Errorf("remove existing destination: %w", err) + } + } else if !os.IsNotExist(err) { + return fmt.Errorf("stat destination: %w", err) } - // Create destination directory with same permissions as source - if err := os.MkdirAll(dst, srcInfo.Mode()); err != nil { - return fmt.Errorf("create destination: %w", err) + // Copy source to destination + if err := copyWithReflink(src, dst); err != nil { + return err } - entries, err := os.ReadDir(src) + // Delete the source after successful copy + return os.RemoveAll(src) +} + +func copyWithReflink(src, dst string) error { + srcInfo, err := os.Stat(src) if err != nil { - return fmt.Errorf("read directory: %w", err) + return fmt.Errorf("stat source: %w", err) } - for _, entry := range entries { - srcPath := filepath.Join(src, entry.Name()) - dstPath := filepath.Join(dst, entry.Name()) + // Handle directory + if srcInfo.IsDir() { + if err := os.MkdirAll(dst, srcInfo.Mode()); err != nil { + return fmt.Errorf("create destination: %w", err) + } - info, err := entry.Info() + entries, err := os.ReadDir(src) if err != nil { - return fmt.Errorf("get entry info: %w", err) + return fmt.Errorf("read directory: %w", err) } - if entry.IsDir() { - // Recursively copy subdirectory - if err := RenameDirectoryWithReflink(srcPath, dstPath); err != nil { + for _, entry := range entries { + srcPath := filepath.Join(src, entry.Name()) + dstPath := filepath.Join(dst, entry.Name()) + + if err := copyWithReflink(srcPath, dstPath); err != nil { return err } - } else if info.Mode()&os.ModeSymlink != 0 { - // Copy symlink - target, err := os.Readlink(srcPath) - if err != nil { - return fmt.Errorf("read symlink: %w", err) - } - if err := os.Symlink(target, dstPath); err != nil { - return fmt.Errorf("create symlink: %w", err) - } - } else if info.Mode().IsRegular() { - // Copy regular file with reflink - // reflink.Auto tries reflink first, falls back to regular copy - if err := reflinkRename(srcPath, dstPath); err != nil { - return fmt.Errorf("reflink rename %s: %w", srcPath, err) - } } - // Skip other file types (devices, sockets, etc.) + return nil + } + + // Handle symlink + if srcInfo.Mode()&os.ModeSymlink != 0 { + target, err := os.Readlink(src) + if err != nil { + return fmt.Errorf("read symlink: %w", err) + } + return os.Symlink(target, dst) + } + + // Handle regular file + if srcInfo.Mode().IsRegular() { + return reflinkRename(src, dst) + } + + // Skip other file types + return nil +} + +func ReflinkWithPermissions(src, dst string) error { + srcInfo, err := os.Stat(src) + if err != nil { + return fmt.Errorf("stat source: %w", err) + } + + if err := reflink.Always(src, dst); err != nil { + return fmt.Errorf("reflink copy: %w", err) + } + + if err := os.Chmod(dst, srcInfo.Mode()); err != nil { + return fmt.Errorf("chmod destination: %w", err) } return nil } func reflinkRename(src, dst string) error { - // cp to new loc on different subvolume - err := reflink.Auto(src, dst) + // Check source permissions before reflink + srcInfo, err := os.Stat(src) + if err != nil { + return fmt.Errorf("stat source before reflink: %w", err) + } + fmt.Printf("Source %s permissions before reflink: %v\n", src, srcInfo.Mode()) + + // cp to new loc on different subvolume with preserved permissions + err = ReflinkWithPermissions(src, dst) if err != nil { - return fmt.Errorf("reflink copy failed: %w", err) + return fmt.Errorf("reflink with permissions failed: %w", err) } + + // Check destination permissions after reflink + dstInfo, err := os.Stat(dst) + if err != nil { + return fmt.Errorf("stat destination after reflink: %w", err) + } + fmt.Printf("Destination %s permissions after reflink: %v\n", dst, dstInfo.Mode()) + // remove original after successful reflink + fmt.Println("Renamed src, removing ", src) err = os.RemoveAll(src) if err != nil { return fmt.Errorf("removing source after reflink failed: %w", err) -- GitLab From 2d24959fef8a347c17cd5db09f50ecff1d070915 Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Sun, 26 Oct 2025 11:12:03 +0000 Subject: [PATCH 5/9] working pt3: all tests in transaction manager passing --- .../storagemgr/partition/apply_operations.go | 13 +++++++------ .../partition/conflict/fshistory/errors.go | 6 ++++-- .../storagemgr/partition/snapshot/snapshot.go | 16 ++++++---------- .../storagemgr/partition/transaction_manager.go | 12 ++++++++++-- .../transaction_manager_housekeeping.go | 6 ++++++ .../partition/transaction_manager_refs_test.go | 4 +++- 6 files changed, 36 insertions(+), 21 deletions(-) diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index f803cb3608..8fa276d41b 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -64,9 +64,9 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err dirtyDirectories[filepath.Dir(path)] = struct{}{} case *gitalypb.LogEntry_Operation_RemoveDirectoryEntry_: op := wrapper.RemoveDirectoryEntry - path := string(op.GetPath()) fmt.Println("apply ops: Removing" , filepath.Join(storageRoot, path)) + if err := os.Remove(filepath.Join(storageRoot, path)); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove: %w", err) } @@ -93,6 +93,7 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // Sync all the dirty directories. for relativePath := range dirtyDirectories { + fmt.Println("synced path", filepath.Join(storageRoot, relativePath)) absPath := filepath.Join(storageRoot, relativePath) if err := sync(ctx, absPath); err != nil { @@ -107,11 +108,11 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { // Ensure the parent directory exists - dstDir := filepath.Dir(dst) - fmt.Println("ensuring parent directory exists", dstDir) - if _, err := os.Stat(dstDir); err != nil { - return fmt.Errorf("parent directory does not exist: %w", err) - } + // dstDir := filepath.Dir(dst) + // fmt.Println("ensuring parent directory exists", dstDir) + // if _, err := os.Stat(dstDir); err != nil { + // return fmt.Errorf("parent directory does not exist: %w", err) + // } // Try hard link first (fast path for same subvolume) if err := os.Link(src, dst); err == nil { diff --git a/internal/gitaly/storage/storagemgr/partition/conflict/fshistory/errors.go b/internal/gitaly/storage/storagemgr/partition/conflict/fshistory/errors.go index 5de3b1cfda..212efd9aa0 100644 --- a/internal/gitaly/storage/storagemgr/partition/conflict/fshistory/errors.go +++ b/internal/gitaly/storage/storagemgr/partition/conflict/fshistory/errors.go @@ -1,6 +1,8 @@ package fshistory import ( + "fmt" + "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v18/internal/structerr" ) @@ -14,8 +16,8 @@ type ReadWriteConflictError struct { } // Error returns the error message. -func (ReadWriteConflictError) Error() string { - return "path was modified after read" +func (r ReadWriteConflictError) Error() string { + return fmt.Sprintf("path %s was modified after read at lsn %d and write lsn %d\n", r.Path, r.ReadLSN.ToProto().Value, r.WriteLSN.ToProto().Value) } // NewReadWriteConflictError returns an error detailing a conflicting read. diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index 61ca5a3c8b..d25964bd37 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -605,17 +605,13 @@ func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly b // Try to delete as a btrfs subvolume first cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "delete", snapshotPath) - fmt.Println("Deleting btrfs subvolume:", snapshotPath) - if err := cmd.Run(); err == nil { - // Successfully deleted as a subvolume - fmt.Println("Successfully deleted btrfs subvolume:", snapshotPath) - return nil - } - + // fmt.Println("Deleting btrfs subvolume:", snapshotPath) + if err := cmd.Run(); err != nil { // If that failed, try regular removal - if err := os.RemoveAll(snapshotPath); err != nil { - return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) - } + if err := os.RemoveAll(snapshotPath); err != nil { + return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) + } + } return nil } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go index 4be9cb0e98..45af48b915 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go @@ -1572,6 +1572,7 @@ func (mgr *TransactionManager) preparePackRefsReftable(ctx context.Context, tran // https://gitlab.com/gitlab-org/git/-/issues/222 func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transaction *Transaction) error { runPackRefs := transaction.runHousekeeping.packRefs + for _, lock := range []string{".new", ".lock"} { lockRelativePath := filepath.Join(transaction.relativePath, "packed-refs"+lock) lockAbsolutePath := filepath.Join(transaction.snapshot.Root(), lockRelativePath) @@ -1595,7 +1596,7 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac if err != nil { return err } - + fmt.Println("walking path:", path) relPath, err := filepath.Rel(repoPath, path) if err != nil { return fmt.Errorf("extracting ref name: %w", err) @@ -1617,6 +1618,7 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac }); err != nil { return fmt.Errorf("initial walking refs directory: %w", err) } + fmt.Printf("loose refs %+v", looseReferences) // Execute git-pack-refs command. The command runs in the scope of the snapshot repository. Thus, we can // let it prune the ref references without causing any impact to other concurrent transactions. @@ -1642,6 +1644,8 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac } } + fmt.Printf("pruned refs %+v", runPackRefs.PrunedRefs) + return nil } @@ -1906,6 +1910,10 @@ func (mgr *TransactionManager) verifyFileSystemOperations(ctx context.Context, t isTablesList := func(path string) bool { return path == filepath.Join(tx.relativePath, "reftable", "tables.list") } + // // isPackedRefs returns true if this is the packed-ref file + // isPackedRefs := func(path string) bool { + // return path == filepath.Join(tx.relativePath, "packed-refs") + // } for _, op := range tx.walEntry.Operations() { switch op.GetOperation().(type) { @@ -1951,7 +1959,7 @@ func (mgr *TransactionManager) verifyFileSystemOperations(ctx context.Context, t // been resolved at this point. Don't conflict check it. if !isTablesList(path) { if err := fsTX.Read(path); err != nil { - return nil, fmt.Errorf("read: %w", err) + return nil, fmt.Errorf("read path %s: %w", path, err) } } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go index e1d130b8d4..4da002635c 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go @@ -336,6 +336,9 @@ func (mgr *TransactionManager) prepareRepacking(ctx context.Context, transaction for file := range beforeFiles { // We delete the files only if it's missing from the before set. if _, exist := afterFiles[file]; !exist || (file == midxFileName && newMidxInode != oldMidxInode) { + fmt.Println("prepareRepacking: Removing pack file", filepath.Join( + objectsDirRelativePath, "pack", file, + )) transaction.walEntry.RemoveDirectoryEntry(filepath.Join( objectsDirRelativePath, "pack", file, )) @@ -371,6 +374,7 @@ func (mgr *TransactionManager) prepareRepacking(ctx context.Context, transaction if info != nil { // The file existed and needs to be removed first. + fmt.Println("prepareRepacking: Removing existing full repack timestamp file", timestampAbsolutePath) transaction.walEntry.RemoveDirectoryEntry(timestampRelativePath) } @@ -807,6 +811,7 @@ func (mgr *TransactionManager) verifyPackRefsFiles(ctx context.Context, transact } deletedPaths[relativePath] = struct{}{} + fmt.Println("verifyPackRefsFiles: remove ", relativePath) transaction.walEntry.RemoveDirectoryEntry(relativePath) return nil @@ -934,6 +939,7 @@ func (mgr *TransactionManager) prepareOffloading(ctx context.Context, transactio // Record WAL entry for file := range oldPackFiles { + fmt.Println("prepareOffloading: Removing pack file", filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) transaction.walEntry.RemoveDirectoryEntry(filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go index 1e57b7d959..51d886ade2 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go @@ -1211,11 +1211,12 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t ReferenceUpdates: git.ReferenceUpdates{ "refs/heads/branch-loose": {OldOID: setup.Commits.First.OID, NewOID: setup.ObjectHash.ZeroOID}, }, + ExpectedError: fshistory.NewReadWriteConflictError(filepath.Join(setup.RelativePath, "packed-refs"), 3, 4), }, }, expectedState: StateAssertion{ Database: DatabaseState{ - string(keyAppliedLSN): storage.LSN(5).ToProto(), + string(keyAppliedLSN): storage.LSN(4).ToProto(), }, Repositories: RepositoryStates{ setup.RelativePath: { @@ -1227,6 +1228,7 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t }, LooseReferences: map[git.ReferenceName]git.ObjectID{ "refs/heads/sentinel-loose": setup.Commits.First.OID, + "refs/heads/branch-loose": setup.Commits.First.OID, }, }, }, -- GitLab From 78d7fdb91b32f7a1efbf4c8469db20cdecc252fd Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Mon, 27 Oct 2025 00:36:34 +0000 Subject: [PATCH 6/9] Fix inode checks for staging packed refs --- .../storagemgr/partition/apply_operations.go | 40 +------------ .../partition/transaction_manager.go | 5 +- .../transaction_manager_refs_test.go | 5 +- internal/gitaly/storage/wal/entry.go | 3 + .../gitaly/storage/wal/reference_recorder.go | 60 ++++++++++++++++++- internal/helper/reflink.go | 38 ++---------- 6 files changed, 73 insertions(+), 78 deletions(-) diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index 8fa276d41b..46063fe6be 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -74,6 +74,8 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // Remove the dirty marker from the removed directory entry if it exists. There's // no need to sync it anymore as it doesn't exist. delete(dirtyDirectories, path) + // Sync the parent directory where directory entry was removed from. + dirtyDirectories[filepath.Dir(path)] = struct{}{} case *gitalypb.LogEntry_Operation_SetKey_: op := wrapper.SetKey @@ -107,12 +109,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { - // Ensure the parent directory exists - // dstDir := filepath.Dir(dst) - // fmt.Println("ensuring parent directory exists", dstDir) - // if _, err := os.Stat(dstDir); err != nil { - // return fmt.Errorf("parent directory does not exist: %w", err) - // } // Try hard link first (fast path for same subvolume) if err := os.Link(src, dst); err == nil { @@ -129,35 +125,3 @@ func LinkOrReflink(src, dst string) error { return nil } - -// reflink creates a copy-on-write copy of src to dst using btrfs reflink. -// This is space-efficient like hard links but works across subvolumes. -// func reflink(src, dst string) error { -// srcFile, err := os.Open(src) -// if err != nil { -// return fmt.Errorf("open source: %w", err) -// } -// defer srcFile.Close() - -// srcInfo, err := srcFile.Stat() -// if err != nil { -// return fmt.Errorf("stat source: %w", err) -// } - -// dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) -// if err != nil { -// return fmt.Errorf("create destination: %w", err) -// } -// defer dstFile.Close() - -// // Use FICLONE ioctl to create a reflink (COW copy) -// err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) -// if err != nil { -// // Reflink failed - fall back to regular copy -// if _, err := io.Copy(dstFile, srcFile); err != nil { -// return fmt.Errorf("copy data: %w", err) -// } -// } - -// return nil -// } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go index 45af48b915..2cfce95787 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go @@ -1596,7 +1596,6 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac if err != nil { return err } - fmt.Println("walking path:", path) relPath, err := filepath.Rel(repoPath, path) if err != nil { return fmt.Errorf("extracting ref name: %w", err) @@ -1618,7 +1617,7 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac }); err != nil { return fmt.Errorf("initial walking refs directory: %w", err) } - fmt.Printf("loose refs %+v", looseReferences) + fmt.Printf("loose refs %+v\n", looseReferences) // Execute git-pack-refs command. The command runs in the scope of the snapshot repository. Thus, we can // let it prune the ref references without causing any impact to other concurrent transactions. @@ -1644,7 +1643,7 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac } } - fmt.Printf("pruned refs %+v", runPackRefs.PrunedRefs) + fmt.Printf("pruned refs %+v\n", runPackRefs.PrunedRefs) return nil } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go index 51d886ade2..0be6a8e385 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go @@ -1211,12 +1211,12 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t ReferenceUpdates: git.ReferenceUpdates{ "refs/heads/branch-loose": {OldOID: setup.Commits.First.OID, NewOID: setup.ObjectHash.ZeroOID}, }, - ExpectedError: fshistory.NewReadWriteConflictError(filepath.Join(setup.RelativePath, "packed-refs"), 3, 4), + // ExpectedError: fshistory.NewReadWriteConflictError(filepath.Join(setup.RelativePath, "packed-refs"), 3, 4), }, }, expectedState: StateAssertion{ Database: DatabaseState{ - string(keyAppliedLSN): storage.LSN(4).ToProto(), + string(keyAppliedLSN): storage.LSN(5).ToProto(), }, Repositories: RepositoryStates{ setup.RelativePath: { @@ -1228,7 +1228,6 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t }, LooseReferences: map[git.ReferenceName]git.ObjectID{ "refs/heads/sentinel-loose": setup.Commits.First.OID, - "refs/heads/branch-loose": setup.Commits.First.OID, }, }, }, diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index 46c45a72d4..a3f63ce72f 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -82,6 +82,8 @@ func (e *Entry) stageFile(path string) (string, error) { if err := LinkOrReflink(path, filepath.Join(e.stateDirectory, fileName)); err != nil { return "", fmt.Errorf("LinkOrReflink: %w", err) } + fmt.Printf("staged file %s as %s\n", path, fileName) + return fileName, nil } @@ -141,6 +143,7 @@ func LinkOrReflink(src, dst string) error { return nil } else if !errors.Is(err, syscall.EXDEV) && !errors.Is(err, fs.ErrExist) { // Some error other than cross-device or already exists + fmt.Println("cross device error") return fmt.Errorf("link: %w", err) } diff --git a/internal/gitaly/storage/wal/reference_recorder.go b/internal/gitaly/storage/wal/reference_recorder.go index e72cccbee9..8fcbb73874 100644 --- a/internal/gitaly/storage/wal/reference_recorder.go +++ b/internal/gitaly/storage/wal/reference_recorder.go @@ -1,9 +1,12 @@ package wal import ( + "bytes" "context" + "crypto/sha256" "errors" "fmt" + "io" "io/fs" "os" "path/filepath" @@ -259,13 +262,36 @@ func (r *ReferenceRecorder) StagePackedRefs() error { if err != nil { return fmt.Errorf("pre-image inode: %w", err) } + fmt.Println("preImageInode", preImageInode) postImageInode, err := GetInode(r.postImagePackedRefsPath) if err != nil { - return fmt.Errorf("post-imaga inode: %w", err) + return fmt.Errorf("post-image inode: %w", err) + } + fmt.Println("postImageInode", postImageInode) + + // With BTRFS reflinks, inode numbers differ even for identical content. + // We need to compare content instead of just inodes. + unchanged := false + + if preImageInode == postImageInode && preImageInode > 0 { + // Same inode means definitely unchanged (works for hard links) + unchanged = true + } else if preImageInode > 0 && postImageInode > 0 { + // Different inodes - need to check if content actually changed + // This handles BTRFS reflinks where inodes differ but content may be same + contentEqual, err := filesHaveSameContent(r.preImagePackedRefsPath, r.postImagePackedRefsPath) + if err != nil { + return fmt.Errorf("compare file contents: %w", err) + } + unchanged = contentEqual + } else if preImageInode == 0 && postImageInode == 0 { + // Both don't exist + unchanged = true } - if preImageInode == postImageInode { + if unchanged { + fmt.Println("packed-refs unchanged") return nil } @@ -275,6 +301,7 @@ func (r *ReferenceRecorder) StagePackedRefs() error { } if postImageInode > 0 { + fmt.Println("staging packed-refs change") fileID, err := r.entry.stageFile(r.postImagePackedRefsPath) if err != nil { return fmt.Errorf("stage packed-refs: %w", err) @@ -285,3 +312,32 @@ func (r *ReferenceRecorder) StagePackedRefs() error { return nil } + +func filesHaveSameContent(path1, path2 string) (bool, error) { + hash1, err := hashFile(path1) + if err != nil { + return false, err + } + + hash2, err := hashFile(path2) + if err != nil { + return false, err + } + + return bytes.Equal(hash1, hash2), nil +} + +func hashFile(path string) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() + + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return nil, err + } + + return h.Sum(nil), nil +} \ No newline at end of file diff --git a/internal/helper/reflink.go b/internal/helper/reflink.go index 4975f37a01..e82a87d71f 100644 --- a/internal/helper/reflink.go +++ b/internal/helper/reflink.go @@ -8,12 +8,12 @@ import ( "github.com/KarpelesLab/reflink" ) -// RenameDirectoryWithReflink recursively copies a directory or file using reflinks when possible // RenameDirectoryWithReflink recursively copies a directory or file using reflinks when possible // then deletes the source (simulating a rename operation) func RenameDirectoryWithReflink(src, dst string) error { // Remove destination if it exists (not the source!) if _, err := os.Lstat(dst); err == nil { + fmt.Println("RenameDirectoryWithReflink: Destination exists, removing ", dst) if err := os.RemoveAll(dst); err != nil { return fmt.Errorf("remove existing destination: %w", err) } @@ -69,7 +69,11 @@ func copyWithReflink(src, dst string) error { // Handle regular file if srcInfo.Mode().IsRegular() { - return reflinkRename(src, dst) + // cp to new loc on different subvolume with preserved permissions + err := ReflinkWithPermissions(src, dst) + if err != nil { + return fmt.Errorf("reflink with permissions failed: %w", err) + } } // Skip other file types @@ -92,33 +96,3 @@ func ReflinkWithPermissions(src, dst string) error { return nil } - -func reflinkRename(src, dst string) error { - // Check source permissions before reflink - srcInfo, err := os.Stat(src) - if err != nil { - return fmt.Errorf("stat source before reflink: %w", err) - } - fmt.Printf("Source %s permissions before reflink: %v\n", src, srcInfo.Mode()) - - // cp to new loc on different subvolume with preserved permissions - err = ReflinkWithPermissions(src, dst) - if err != nil { - return fmt.Errorf("reflink with permissions failed: %w", err) - } - - // Check destination permissions after reflink - dstInfo, err := os.Stat(dst) - if err != nil { - return fmt.Errorf("stat destination after reflink: %w", err) - } - fmt.Printf("Destination %s permissions after reflink: %v\n", dst, dstInfo.Mode()) - - // remove original after successful reflink - fmt.Println("Renamed src, removing ", src) - err = os.RemoveAll(src) - if err != nil { - return fmt.Errorf("removing source after reflink failed: %w", err) - } - return nil -} \ No newline at end of file -- GitLab From 676d0c52dd3985162b6e742ef008552bb22dd1a0 Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Mon, 27 Oct 2025 02:42:09 +0000 Subject: [PATCH 7/9] Refactor subvolume Fix the check to test for subvolumes in btrfs and cleanup. Subvolumes created should be unique --- .../storagemgr/partition/apply_operations.go | 6 +-- .../storagemgr/partition/snapshot/snapshot.go | 42 ++++++++++++++----- .../transaction_manager_housekeeping.go | 12 +++--- .../storage/storagemgr/partition_manager.go | 2 +- internal/gitaly/storage/wal/entry.go | 2 +- .../gitaly/storage/wal/reference_recorder.go | 8 ++-- internal/helper/reflink.go | 2 +- 7 files changed, 47 insertions(+), 27 deletions(-) diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index 46063fe6be..e317642f73 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -52,7 +52,7 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err op := wrapper.CreateDirectory path := string(op.GetPath()) - fmt.Printf("applyOperations: CreateDirectory for path: %s\n", path) + // fmt.Printf("applyOperations: CreateDirectory for path: %s\n", path) if err := os.Mkdir(filepath.Join(storageRoot, path), fs.FileMode(op.GetMode())); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("mkdir: %w", err) @@ -65,7 +65,7 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err case *gitalypb.LogEntry_Operation_RemoveDirectoryEntry_: op := wrapper.RemoveDirectoryEntry path := string(op.GetPath()) - fmt.Println("apply ops: Removing" , filepath.Join(storageRoot, path)) + // fmt.Println("apply ops: Removing" , filepath.Join(storageRoot, path)) if err := os.Remove(filepath.Join(storageRoot, path)); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove: %w", err) @@ -95,7 +95,7 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // Sync all the dirty directories. for relativePath := range dirtyDirectories { - fmt.Println("synced path", filepath.Join(storageRoot, relativePath)) + // fmt.Println("synced path", filepath.Join(storageRoot, relativePath)) absPath := filepath.Join(storageRoot, relativePath) if err := sync(ctx, absPath); err != nil { diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index d25964bd37..57ed37b6b8 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "strings" + "syscall" "time" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage" @@ -394,7 +395,7 @@ func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, re } // Check if the source is a btrfs subvolume - isBtrfsSubvolume, err := isBtrfsSubvolume(ctx, originalPath) + isBtrfsSubvolume, err := isBtrfsSubvolume(originalPath) if err != nil { return fmt.Errorf("check if btrfs subvolume: %w", err) } @@ -526,31 +527,48 @@ func createBtrfsSnapshot(ctx context.Context, source, destination string, readOn } // isBtrfsSubvolume checks if the given path is a btrfs subvolume. -func isBtrfsSubvolume(ctx context.Context, path string) (bool, error) { - cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "show", path) - err := cmd.Run() - if err != nil { - // If the command fails, it's not a subvolume (or not on btrfs at all) - return false, nil +func isBtrfsSubvolume(path string) (bool, error) { + var stat syscall.Stat_t + if err := syscall.Stat(path, &stat); err != nil { + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + return false, fmt.Errorf("stat path: %w", err) } - return true, nil + + // Btrfs subvolumes always have inode number 256 + // This is BTRFS_FIRST_FREE_OBJECTID + return stat.Ino == 256, nil } // convertToSubvolume converts a regular directory to a btrfs subvolume in place. func convertToSubvolume(ctx context.Context, path string) error { tempPath := path + ".subvol-temp" + // Clean up any existing temp subvolume from previous failed attempts + if _, err := os.Stat(tempPath); err == nil { + if err := deleteBtrfsSnapshot(ctx, tempPath, false); err != nil { + return fmt.Errorf("cleanup existing temp subvolume: %w", err) + } + } + // Create new subvolume with temporary name cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "create", tempPath) if output, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("create subvolume: %w, output: %s", err, string(output)) } + // Ensure cleanup on any subsequent failure + var cleanupTemp bool = true + defer func() { + if cleanupTemp { + _ = deleteBtrfsSnapshot(ctx, tempPath, false) + } + }() + // Copy contents from old directory to new subvolume - cmd = exec.CommandContext(ctx, "cp", "-R", "-a", path+"/.", tempPath+"/") + cmd = exec.CommandContext(ctx, "cp", "-a", path+"/.", tempPath+"/") if output, err := cmd.CombinedOutput(); err != nil { - // Clean up the temp subvolume if copy fails - _ = deleteBtrfsSnapshot(ctx, tempPath, false) return fmt.Errorf("copy contents: %w, output: %s", err, string(output)) } @@ -564,6 +582,8 @@ func convertToSubvolume(ctx context.Context, path string) error { return fmt.Errorf("rename subvolume: %w", err) } + // Success - don't clean up the temp path + cleanupTemp = false return nil } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go index 4da002635c..59172fe9cd 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go @@ -336,9 +336,9 @@ func (mgr *TransactionManager) prepareRepacking(ctx context.Context, transaction for file := range beforeFiles { // We delete the files only if it's missing from the before set. if _, exist := afterFiles[file]; !exist || (file == midxFileName && newMidxInode != oldMidxInode) { - fmt.Println("prepareRepacking: Removing pack file", filepath.Join( - objectsDirRelativePath, "pack", file, - )) + // fmt.Println("prepareRepacking: Removing pack file", filepath.Join( + // objectsDirRelativePath, "pack", file, + // )) transaction.walEntry.RemoveDirectoryEntry(filepath.Join( objectsDirRelativePath, "pack", file, )) @@ -374,7 +374,7 @@ func (mgr *TransactionManager) prepareRepacking(ctx context.Context, transaction if info != nil { // The file existed and needs to be removed first. - fmt.Println("prepareRepacking: Removing existing full repack timestamp file", timestampAbsolutePath) + // fmt.Println("prepareRepacking: Removing existing full repack timestamp file", timestampAbsolutePath) transaction.walEntry.RemoveDirectoryEntry(timestampRelativePath) } @@ -811,7 +811,7 @@ func (mgr *TransactionManager) verifyPackRefsFiles(ctx context.Context, transact } deletedPaths[relativePath] = struct{}{} - fmt.Println("verifyPackRefsFiles: remove ", relativePath) + // fmt.Println("verifyPackRefsFiles: remove ", relativePath) transaction.walEntry.RemoveDirectoryEntry(relativePath) return nil @@ -939,7 +939,7 @@ func (mgr *TransactionManager) prepareOffloading(ctx context.Context, transactio // Record WAL entry for file := range oldPackFiles { - fmt.Println("prepareOffloading: Removing pack file", filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) + // fmt.Println("prepareOffloading: Removing pack file", filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) transaction.walEntry.RemoveDirectoryEntry(filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) } diff --git a/internal/gitaly/storage/storagemgr/partition_manager.go b/internal/gitaly/storage/storagemgr/partition_manager.go index 51432f0adb..d45044db9c 100644 --- a/internal/gitaly/storage/storagemgr/partition_manager.go +++ b/internal/gitaly/storage/storagemgr/partition_manager.go @@ -536,7 +536,7 @@ func (sm *StorageManager) startPartition(ctx context.Context, partitionID storag } stagingDir, err := os.MkdirTemp(sm.stagingDirectory, "") - fmt.Println("Created Staging dir:", stagingDir) + // fmt.Println("Created Staging dir:", stagingDir) if err != nil { return fmt.Errorf("create staging directory: %w", err) } diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index a3f63ce72f..cc9936e611 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -143,7 +143,7 @@ func LinkOrReflink(src, dst string) error { return nil } else if !errors.Is(err, syscall.EXDEV) && !errors.Is(err, fs.ErrExist) { // Some error other than cross-device or already exists - fmt.Println("cross device error") + // fmt.Println("cross device error") return fmt.Errorf("link: %w", err) } diff --git a/internal/gitaly/storage/wal/reference_recorder.go b/internal/gitaly/storage/wal/reference_recorder.go index 8fcbb73874..e69d4a2908 100644 --- a/internal/gitaly/storage/wal/reference_recorder.go +++ b/internal/gitaly/storage/wal/reference_recorder.go @@ -262,13 +262,13 @@ func (r *ReferenceRecorder) StagePackedRefs() error { if err != nil { return fmt.Errorf("pre-image inode: %w", err) } - fmt.Println("preImageInode", preImageInode) + // fmt.Println("preImageInode", preImageInode) postImageInode, err := GetInode(r.postImagePackedRefsPath) if err != nil { return fmt.Errorf("post-image inode: %w", err) } - fmt.Println("postImageInode", postImageInode) + // fmt.Println("postImageInode", postImageInode) // With BTRFS reflinks, inode numbers differ even for identical content. // We need to compare content instead of just inodes. @@ -291,7 +291,7 @@ func (r *ReferenceRecorder) StagePackedRefs() error { } if unchanged { - fmt.Println("packed-refs unchanged") + // fmt.Println("packed-refs unchanged") return nil } @@ -301,7 +301,7 @@ func (r *ReferenceRecorder) StagePackedRefs() error { } if postImageInode > 0 { - fmt.Println("staging packed-refs change") + // fmt.Println("staging packed-refs change") fileID, err := r.entry.stageFile(r.postImagePackedRefsPath) if err != nil { return fmt.Errorf("stage packed-refs: %w", err) diff --git a/internal/helper/reflink.go b/internal/helper/reflink.go index e82a87d71f..7aff44234f 100644 --- a/internal/helper/reflink.go +++ b/internal/helper/reflink.go @@ -13,7 +13,7 @@ import ( func RenameDirectoryWithReflink(src, dst string) error { // Remove destination if it exists (not the source!) if _, err := os.Lstat(dst); err == nil { - fmt.Println("RenameDirectoryWithReflink: Destination exists, removing ", dst) + // fmt.Println("RenameDirectoryWithReflink: Destination exists, removing ", dst) if err := os.RemoveAll(dst); err != nil { return fmt.Errorf("remove existing destination: %w", err) } -- GitLab From 7b6f76249c2309fc79f03e52d2fc636df6952e6e Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Wed, 29 Oct 2025 09:41:22 +0000 Subject: [PATCH 8/9] Concurrent delete btrfs snapshots Cleanup snapshots in parallel. General cleanup of unused --- .../storagemgr/partition/snapshot/snapshot.go | 166 +++++++----------- internal/gitaly/storage/wal/entry.go | 70 -------- 2 files changed, 62 insertions(+), 174 deletions(-) diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index 57ed37b6b8..87151f56f1 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "syscall" "time" @@ -16,7 +17,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/gitstorage" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/mode/permission" - "gitlab.com/gitlab-org/gitaly/v18/internal/helper" ) // ModeReadOnlyDirectory is the mode given to directories in read-only snapshots. @@ -96,23 +96,19 @@ func (s *snapshot) RelativePath(relativePath string) string { // return nil // } -// Closes removes the snapshot. func (s *snapshot) Close() error { var errs []error - - // If this was a read-only snapshot, we need to restore write permissions - // before we can delete anything if s.readOnly { // First, make all btrfs subvolumes writable for _, repoPath := range s.repositoryPaths { // Make the subvolume writable _ = setBtrfsReadOnly(context.Background(), repoPath, false) - // Restore write permissions to directories inside the subvolume // so we can delete files _ = storage.SetDirectoryMode(repoPath, mode.Directory) } + } // Restore write permissions to parent directories _ = os.Chmod(s.root, mode.Directory) for _, repoPath := range s.repositoryPaths { @@ -121,21 +117,35 @@ func (s *snapshot) Close() error { _ = os.Chmod(dir, mode.Directory) dir = filepath.Dir(dir) } - } } - // Now delete individual repository subvolumes + + // Use a WaitGroup for concurrency + var wg sync.WaitGroup + errChan := make(chan error, len(s.repositoryPaths)) + + // Now delete individual repository subvolumes concurrently for _, repoPath := range s.repositoryPaths { - if err := deleteBtrfsSnapshot(context.Background(), repoPath, false); err != nil { - errs = append(errs, fmt.Errorf("delete repository snapshot %s: %w", repoPath, err)) - } + wg.Add(1) + go func(path string) { + defer wg.Done() + // Use the btrfs subvolume delete command directly, it is designed for this + if err := deleteBtrfsSnapshot(context.Background(), path, false); err != nil { + errChan <- fmt.Errorf("delete repository snapshot %s: %w", path, err) + } + }(repoPath) + } + wg.Wait() + close(errChan) + + for err := range errChan { + errs = append(errs, err) } // Finally delete the root if err := os.RemoveAll(s.root); err != nil { errs = append(errs, fmt.Errorf("delete snapshot root %s: %w", s.root, err)) } - if len(errs) > 0 { return errors.Join(errs...) } @@ -143,6 +153,7 @@ func (s *snapshot) Close() error { return nil } + // setDirectoryMode walks the snapshot and sets each directory's mode to the given mode. func (s *snapshot) setDirectoryMode(mode fs.FileMode) error { return storage.SetDirectoryMode(s.root, mode) @@ -230,9 +241,6 @@ func createRepositorySnapshots(ctx context.Context, storageRoot, snapshotRoot st ) error { // Create the root directory always to as the storage would also exist always. s.stats.directoryCount++ - // if err := os.Mkdir(snapshotRoot, 0755); err != nil { - // return fmt.Errorf("mkdir snapshot root: %w", err) - // } if err := ensureDir0700(snapshotRoot); err != nil { return fmt.Errorf("mkdir snapshot root: %w", err) @@ -359,21 +367,6 @@ func ensureDir0700(path string) error { return nil } -// createRepositorySnapshot snapshots a repository's current state at snapshotPath. This is done by -// recreating the repository's directory structure and hard linking the repository's files in their -// correct locations there. This effectively does a copy-free clone of the repository. Since the files -// are shared between the snapshot and the repository, they must not be modified. Git doesn't modify -// existing files but writes new ones so this property is upheld. -// func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, relativePath string, -// snapshotFilter Filter, stats *snapshotStatistics, -// ) error { -// if err := createDirectorySnapshot(ctx, filepath.Join(storageRoot, relativePath), -// filepath.Join(snapshotRoot, relativePath), -// snapshotFilter, stats); err != nil { -// return fmt.Errorf("create directory snapshot: %w", err) -// } -// return nil -// } // createRepositorySnapshot creates a snapshot of a repository using btrfs snapshots. // The source repository is expected to be a btrfs subvolume for optimal performance. @@ -387,8 +380,6 @@ func createRepositorySnapshot(ctx context.Context, storageRoot, snapshotRoot, re // Check if the original directory exists if _, err := os.Stat(originalPath); err != nil { if errors.Is(err, fs.ErrNotExist) { - // The directory being snapshotted does not exist. This is fine as the transaction - // may be about to create it. return nil } return fmt.Errorf("stat original directory: %w", err) @@ -537,12 +528,23 @@ func isBtrfsSubvolume(path string) (bool, error) { } // Btrfs subvolumes always have inode number 256 - // This is BTRFS_FIRST_FREE_OBJECTID return stat.Ino == 256, nil } // convertToSubvolume converts a regular directory to a btrfs subvolume in place. func convertToSubvolume(ctx context.Context, path string) error { + // fmt.Println("Converting to btrfs subvolume:", path) + // Acquire an exclusive lock on the repository + lockPath := path + ".lock" + lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0644) + if err != nil { + return fmt.Errorf("acquire lock: %w", err) + } + defer func() { + lockFile.Close() + os.Remove(lockPath) + }() + tempPath := path + ".subvol-temp" // Clean up any existing temp subvolume from previous failed attempts @@ -566,16 +568,31 @@ func convertToSubvolume(ctx context.Context, path string) error { } }() - // Copy contents from old directory to new subvolume +// Copy contents from old directory to new subvolume cmd = exec.CommandContext(ctx, "cp", "-a", path+"/.", tempPath+"/") if output, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("copy contents: %w, output: %s", err, string(output)) } - // Remove old directory - if err := os.RemoveAll(path); err != nil { - return fmt.Errorf("remove old directory: %w", err) + + // Atomic swap using rename + backupPath := path + ".old-backup" + + // Rename old to backup + if err := os.Rename(path, backupPath); err != nil { + return fmt.Errorf("backup old directory: %w", err) } + + // Ensure cleanup of backup + defer func() { + if cleanupTemp { + // Restore backup if something failed + _ = os.Rename(backupPath, path) + } else { + // Remove backup on success + _ = os.RemoveAll(backupPath) + } + }() // Rename subvolume to original path if err := os.Rename(tempPath, path); err != nil { @@ -606,83 +623,24 @@ func setBtrfsReadOnly(ctx context.Context, path string, readOnly bool) error { // deleteBtrfsSnapshot deletes a btrfs subvolume snapshot. // If the snapshot is read-only, it first changes it to read-write before deletion. func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly bool) error { - // Check if the snapshot exists - if _, err := os.Stat(snapshotPath); err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return fmt.Errorf("stat snapshot: %w", err) - } - // If it was a read-only snapshot, ensure it's writable before deletion if wasReadOnly { // Try to make it writable (ignore errors - it might not be a subvolume) _ = setBtrfsReadOnly(ctx, snapshotPath, false) // Also restore directory write permissions - _ = storage.SetDirectoryMode(snapshotPath, mode.Directory) + // _ = storage.SetDirectoryMode(snapshotPath, mode.Directory) } // Try to delete as a btrfs subvolume first - cmd := exec.CommandContext(ctx, "btrfs", "subvolume", "delete", snapshotPath) - // fmt.Println("Deleting btrfs subvolume:", snapshotPath) - if err := cmd.Run(); err != nil { - // If that failed, try regular removal - if err := os.RemoveAll(snapshotPath); err != nil { - return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) - } - } - - return nil -} - -// createDirectorySnapshot recursively recreates the directory structure from originalDirectory into -// snapshotDirectory and hard links files into the same locations in snapshotDirectory. -// -// matcher is needed to track which paths we want to include in the snapshot. -func createDirectorySnapshot(ctx context.Context, originalDirectory, snapshotDirectory string, matcher Filter, stats *snapshotStatistics) error { - if err := filepath.Walk(originalDirectory, func(oldPath string, info fs.FileInfo, err error) error { - if err != nil { - if errors.Is(err, fs.ErrNotExist) && oldPath == originalDirectory { - // The directory being snapshotted does not exist. This is fine as the transaction - // may be about to create it. - return nil - } - - return err - } - - relativePath, err := filepath.Rel(originalDirectory, oldPath) - if err != nil { - return fmt.Errorf("rel: %w", err) - } - - if !matcher.Matches(relativePath) { - if info.IsDir() { - return fs.SkipDir - } - return nil - } - - newPath := filepath.Join(snapshotDirectory, relativePath) - if info.IsDir() { - stats.directoryCount++ - if err := os.Mkdir(newPath, info.Mode().Perm()); err != nil { - return fmt.Errorf("create dir: %w", err) - } - } else if info.Mode().IsRegular() { - stats.fileCount++ - if err := helper.ReflinkWithPermissions(oldPath, newPath); err != nil { - return fmt.Errorf("link file: %w", err) - } - } else { - return fmt.Errorf("unsupported file mode: %q", info.Mode()) - } - - return nil - }); err != nil { - return fmt.Errorf("walk: %w", err) +cmd := exec.CommandContext(ctx, "sudo", "btrfs", "subvolume", "delete", snapshotPath) +_, err := cmd.CombinedOutput() +if err != nil { + // Try regular removal + if err := os.RemoveAll(snapshotPath); err != nil { + return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) } +} return nil } diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index cc9936e611..e530ff8a4e 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -82,7 +82,6 @@ func (e *Entry) stageFile(path string) (string, error) { if err := LinkOrReflink(path, filepath.Join(e.stateDirectory, fileName)); err != nil { return "", fmt.Errorf("LinkOrReflink: %w", err) } - fmt.Printf("staged file %s as %s\n", path, fileName) return fileName, nil @@ -100,7 +99,6 @@ func (e *Entry) DeleteKey(key []byte) { // CreateDirectory records creation of a single directory. func (e *Entry) CreateDirectory(relativePath string) { - fmt.Printf("create directory %s\n", relativePath) e.operations.createDirectory(relativePath) } @@ -111,20 +109,17 @@ func (e *Entry) CreateFile(sourceAbsolutePath string, relativePath string) error if err != nil { return fmt.Errorf("stage file: %w", err) } - fmt.Printf("create file hardlinking %s to %s\n", sourceAbsolutePath, relativePath) e.operations.createHardLink(stagedFile, relativePath, false) return nil } // CreateLink records a creation of a hard link to an exisiting file in the partition. func (e *Entry) CreateLink(sourceRelativePath, destinationRelativePath string) { - fmt.Printf("hardlinking %s to %s\n", sourceRelativePath, destinationRelativePath) e.operations.createHardLink(sourceRelativePath, destinationRelativePath, true) } // RemoveDirectoryEntry records the removal of the directory entry at the given path. func (e *Entry) RemoveDirectoryEntry(relativePath string) { - fmt.Printf("removing directory entry %s\n", relativePath) e.operations.removeDirectoryEntry(relativePath) } @@ -134,7 +129,6 @@ func LinkOrReflink(src, dst string) error { // Ensure the parent directory exists dstDir := filepath.Dir(dst) if _, err := os.Stat(dstDir); err != nil { - fmt.Printf("LinkOrReflink: parent directory check failed: %v\n", err) return fmt.Errorf("parent directory does not exist: %w", err) } @@ -154,67 +148,3 @@ func LinkOrReflink(src, dst string) error { return nil } - -// reflink creates a copy-on-write copy of src to dst using btrfs reflink. -// This is space-efficient like hard links but works across subvolumes. -// func reflink(src, dst string) error { -// srcFile, err := os.Open(src) -// if err != nil { -// return fmt.Errorf("open source: %w", err) -// } -// defer srcFile.Close() - -// srcInfo, err := srcFile.Stat() -// if err != nil { -// return fmt.Errorf("stat source: %w", err) -// } - -// dstFile, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, srcInfo.Mode()) -// if err != nil { -// return fmt.Errorf("create destination: %w", err) -// } -// defer dstFile.Close() - -// // Use FICLONE ioctl to create a reflink (COW copy) -// err = unix.IoctlFileClone(int(dstFile.Fd()), int(srcFile.Fd())) -// if err != nil { -// // Reflink failed - fall back to regular copy -// if _, err := io.Copy(dstFile, srcFile); err != nil { -// return fmt.Errorf("copy data: %w", err) -// } -// } - -// return nil -// } - -// func copyWithReflink(src, dst string) error { -// // Get source info -// srcInfo, err := os.Stat(src) -// if err != nil { -// return err -// } - -// // If source is a directory, copy its contents -// if srcInfo.IsDir() { -// // Create destination if it doesn't exist -// err = os.MkdirAll(dst, srcInfo.Mode()) -// if err != nil { -// return err -// } - -// cmd := exec.Command("cp", "-R", "--reflink=auto", "-a", src+"/.", dst) -// output, err := cmd.CombinedOutput() -// if err != nil { -// return fmt.Errorf("copy failed: %v, output: %s", err, output) -// } -// } else { -// // Single file copy -// cmd := exec.Command("cp", "--reflink=auto", "-a", src, dst) -// output, err := cmd.CombinedOutput() -// if err != nil { -// return fmt.Errorf("copy failed: %v, output: %s", err, output) -// } -// } - -// return os.RemoveAll(src) -// } -- GitLab From 6b68d6166fbb14d1463f158b1d559a68e6cdca30 Mon Sep 17 00:00:00 2001 From: Emily Chui Date: Wed, 29 Oct 2025 11:51:40 +0000 Subject: [PATCH 9/9] General cleanup --- internal/gitaly/repoutil/custom_hooks.go | 1 - .../storagemgr/partition/apply_operations.go | 6 -- .../storagemgr/partition/snapshot/snapshot.go | 59 ++++++------------- .../partition/transaction_manager.go | 5 +- .../transaction_manager_housekeeping.go | 6 -- .../transaction_manager_refs_test.go | 1 - .../storage/storagemgr/partition_manager.go | 1 - internal/gitaly/storage/wal/entry.go | 1 - .../gitaly/storage/wal/reference_recorder.go | 16 ++--- 9 files changed, 26 insertions(+), 70 deletions(-) diff --git a/internal/gitaly/repoutil/custom_hooks.go b/internal/gitaly/repoutil/custom_hooks.go index dd9427b0e0..93e76384a0 100644 --- a/internal/gitaly/repoutil/custom_hooks.go +++ b/internal/gitaly/repoutil/custom_hooks.go @@ -205,7 +205,6 @@ func SetCustomHooks( } if storage.NeedsSync(ctx) { - // Then sync all the hook files recursively if err := syncer.SyncRecursive(ctx, tempHooksPath); err != nil { return fmt.Errorf("syncing extracted custom hooks: %w", err) diff --git a/internal/gitaly/storage/storagemgr/partition/apply_operations.go b/internal/gitaly/storage/storagemgr/partition/apply_operations.go index e317642f73..694eef774b 100644 --- a/internal/gitaly/storage/storagemgr/partition/apply_operations.go +++ b/internal/gitaly/storage/storagemgr/partition/apply_operations.go @@ -7,8 +7,6 @@ import ( "io/fs" "os" "path/filepath" - // "sort" - // "strings" "syscall" "gitlab.com/gitlab-org/gitaly/v18/internal/gitaly/storage/keyvalue" @@ -52,7 +50,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err op := wrapper.CreateDirectory path := string(op.GetPath()) - // fmt.Printf("applyOperations: CreateDirectory for path: %s\n", path) if err := os.Mkdir(filepath.Join(storageRoot, path), fs.FileMode(op.GetMode())); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("mkdir: %w", err) @@ -65,7 +62,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err case *gitalypb.LogEntry_Operation_RemoveDirectoryEntry_: op := wrapper.RemoveDirectoryEntry path := string(op.GetPath()) - // fmt.Println("apply ops: Removing" , filepath.Join(storageRoot, path)) if err := os.Remove(filepath.Join(storageRoot, path)); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("remove: %w", err) @@ -95,7 +91,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // Sync all the dirty directories. for relativePath := range dirtyDirectories { - // fmt.Println("synced path", filepath.Join(storageRoot, relativePath)) absPath := filepath.Join(storageRoot, relativePath) if err := sync(ctx, absPath); err != nil { @@ -109,7 +104,6 @@ func applyOperations(ctx context.Context, sync func(context.Context, string) err // LinkOrReflink attempts to create a hard link, and if that fails due to cross-device, // falls back to creating a reflink (COW copy) on btrfs, or a regular copy as last resort. func LinkOrReflink(src, dst string) error { - // Try hard link first (fast path for same subvolume) if err := os.Link(src, dst); err == nil { return nil diff --git a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go index 87151f56f1..87b5ed48cb 100644 --- a/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go +++ b/internal/gitaly/storage/storagemgr/partition/snapshot/snapshot.go @@ -80,22 +80,6 @@ func (s *snapshot) RelativePath(relativePath string) string { return filepath.Join(s.prefix, relativePath) } -// Closes removes the snapshot. -// func (s *snapshot) Close() error { -// if s.readOnly { -// // Make the directories writable again so we can remove the snapshot. -// if err := s.setDirectoryMode(mode.Directory); err != nil { -// return fmt.Errorf("make writable: %w", err) -// } -// } - -// if err := os.RemoveAll(s.root); err != nil { -// return fmt.Errorf("remove all: %w", err) -// } - -// return nil -// } - func (s *snapshot) Close() error { var errs []error if s.readOnly { @@ -107,19 +91,17 @@ func (s *snapshot) Close() error { // so we can delete files _ = storage.SetDirectoryMode(repoPath, mode.Directory) } - + } + // Restore write permissions to parent directories + _ = os.Chmod(s.root, mode.Directory) + for _, repoPath := range s.repositoryPaths { + dir := filepath.Dir(repoPath) + for dir != s.root && dir != "." && dir != "/" { + _ = os.Chmod(dir, mode.Directory) + dir = filepath.Dir(dir) } - // Restore write permissions to parent directories - _ = os.Chmod(s.root, mode.Directory) - for _, repoPath := range s.repositoryPaths { - dir := filepath.Dir(repoPath) - for dir != s.root && dir != "." && dir != "/" { - _ = os.Chmod(dir, mode.Directory) - dir = filepath.Dir(dir) - } } - // Use a WaitGroup for concurrency var wg sync.WaitGroup errChan := make(chan error, len(s.repositoryPaths)) @@ -153,7 +135,6 @@ func (s *snapshot) Close() error { return nil } - // setDirectoryMode walks the snapshot and sets each directory's mode to the given mode. func (s *snapshot) setDirectoryMode(mode fs.FileMode) error { return storage.SetDirectoryMode(s.root, mode) @@ -367,7 +348,6 @@ func ensureDir0700(path string) error { return nil } - // createRepositorySnapshot creates a snapshot of a repository using btrfs snapshots. // The source repository is expected to be a btrfs subvolume for optimal performance. // If the source is not a btrfs subvolume, it will be converted to one first. @@ -536,7 +516,7 @@ func convertToSubvolume(ctx context.Context, path string) error { // fmt.Println("Converting to btrfs subvolume:", path) // Acquire an exclusive lock on the repository lockPath := path + ".lock" - lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0644) + lockFile, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o644) if err != nil { return fmt.Errorf("acquire lock: %w", err) } @@ -568,21 +548,20 @@ func convertToSubvolume(ctx context.Context, path string) error { } }() -// Copy contents from old directory to new subvolume + // Copy contents from old directory to new subvolume cmd = exec.CommandContext(ctx, "cp", "-a", path+"/.", tempPath+"/") if output, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("copy contents: %w, output: %s", err, string(output)) } - // Atomic swap using rename backupPath := path + ".old-backup" - + // Rename old to backup if err := os.Rename(path, backupPath); err != nil { return fmt.Errorf("backup old directory: %w", err) } - + // Ensure cleanup of backup defer func() { if cleanupTemp { @@ -633,14 +612,14 @@ func deleteBtrfsSnapshot(ctx context.Context, snapshotPath string, wasReadOnly b } // Try to delete as a btrfs subvolume first -cmd := exec.CommandContext(ctx, "sudo", "btrfs", "subvolume", "delete", snapshotPath) -_, err := cmd.CombinedOutput() -if err != nil { - // Try regular removal - if err := os.RemoveAll(snapshotPath); err != nil { - return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) + cmd := exec.CommandContext(ctx, "sudo", "btrfs", "subvolume", "delete", snapshotPath) + _, err := cmd.CombinedOutput() + if err != nil { + // Try regular removal + if err := os.RemoveAll(snapshotPath); err != nil { + return fmt.Errorf("failed to delete %s as subvolume or directory: %w", snapshotPath, err) + } } -} return nil } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go index 2cfce95787..cda591aa49 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager.go @@ -1617,7 +1617,6 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac }); err != nil { return fmt.Errorf("initial walking refs directory: %w", err) } - fmt.Printf("loose refs %+v\n", looseReferences) // Execute git-pack-refs command. The command runs in the scope of the snapshot repository. Thus, we can // let it prune the ref references without causing any impact to other concurrent transactions. @@ -1643,8 +1642,6 @@ func (mgr *TransactionManager) preparePackRefsFiles(ctx context.Context, transac } } - fmt.Printf("pruned refs %+v\n", runPackRefs.PrunedRefs) - return nil } @@ -1958,7 +1955,7 @@ func (mgr *TransactionManager) verifyFileSystemOperations(ctx context.Context, t // been resolved at this point. Don't conflict check it. if !isTablesList(path) { if err := fsTX.Read(path); err != nil { - return nil, fmt.Errorf("read path %s: %w", path, err) + return nil, fmt.Errorf("read path %s: %w", path, err) } } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go index 59172fe9cd..e1d130b8d4 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_housekeeping.go @@ -336,9 +336,6 @@ func (mgr *TransactionManager) prepareRepacking(ctx context.Context, transaction for file := range beforeFiles { // We delete the files only if it's missing from the before set. if _, exist := afterFiles[file]; !exist || (file == midxFileName && newMidxInode != oldMidxInode) { - // fmt.Println("prepareRepacking: Removing pack file", filepath.Join( - // objectsDirRelativePath, "pack", file, - // )) transaction.walEntry.RemoveDirectoryEntry(filepath.Join( objectsDirRelativePath, "pack", file, )) @@ -374,7 +371,6 @@ func (mgr *TransactionManager) prepareRepacking(ctx context.Context, transaction if info != nil { // The file existed and needs to be removed first. - // fmt.Println("prepareRepacking: Removing existing full repack timestamp file", timestampAbsolutePath) transaction.walEntry.RemoveDirectoryEntry(timestampRelativePath) } @@ -811,7 +807,6 @@ func (mgr *TransactionManager) verifyPackRefsFiles(ctx context.Context, transact } deletedPaths[relativePath] = struct{}{} - // fmt.Println("verifyPackRefsFiles: remove ", relativePath) transaction.walEntry.RemoveDirectoryEntry(relativePath) return nil @@ -939,7 +934,6 @@ func (mgr *TransactionManager) prepareOffloading(ctx context.Context, transactio // Record WAL entry for file := range oldPackFiles { - // fmt.Println("prepareOffloading: Removing pack file", filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) transaction.walEntry.RemoveDirectoryEntry(filepath.Join(transaction.relativePath, objectsDir, packFileDir, file)) } diff --git a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go index 0be6a8e385..1e57b7d959 100644 --- a/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go +++ b/internal/gitaly/storage/storagemgr/partition/transaction_manager_refs_test.go @@ -1211,7 +1211,6 @@ func generateModifyReferencesTests(t *testing.T, setup testTransactionSetup) []t ReferenceUpdates: git.ReferenceUpdates{ "refs/heads/branch-loose": {OldOID: setup.Commits.First.OID, NewOID: setup.ObjectHash.ZeroOID}, }, - // ExpectedError: fshistory.NewReadWriteConflictError(filepath.Join(setup.RelativePath, "packed-refs"), 3, 4), }, }, expectedState: StateAssertion{ diff --git a/internal/gitaly/storage/storagemgr/partition_manager.go b/internal/gitaly/storage/storagemgr/partition_manager.go index d45044db9c..7854becb84 100644 --- a/internal/gitaly/storage/storagemgr/partition_manager.go +++ b/internal/gitaly/storage/storagemgr/partition_manager.go @@ -536,7 +536,6 @@ func (sm *StorageManager) startPartition(ctx context.Context, partitionID storag } stagingDir, err := os.MkdirTemp(sm.stagingDirectory, "") - // fmt.Println("Created Staging dir:", stagingDir) if err != nil { return fmt.Errorf("create staging directory: %w", err) } diff --git a/internal/gitaly/storage/wal/entry.go b/internal/gitaly/storage/wal/entry.go index e530ff8a4e..0fb9f3c80a 100644 --- a/internal/gitaly/storage/wal/entry.go +++ b/internal/gitaly/storage/wal/entry.go @@ -83,7 +83,6 @@ func (e *Entry) stageFile(path string) (string, error) { return "", fmt.Errorf("LinkOrReflink: %w", err) } - return fileName, nil } diff --git a/internal/gitaly/storage/wal/reference_recorder.go b/internal/gitaly/storage/wal/reference_recorder.go index e69d4a2908..b185c716a0 100644 --- a/internal/gitaly/storage/wal/reference_recorder.go +++ b/internal/gitaly/storage/wal/reference_recorder.go @@ -262,18 +262,16 @@ func (r *ReferenceRecorder) StagePackedRefs() error { if err != nil { return fmt.Errorf("pre-image inode: %w", err) } - // fmt.Println("preImageInode", preImageInode) postImageInode, err := GetInode(r.postImagePackedRefsPath) if err != nil { return fmt.Errorf("post-image inode: %w", err) } - // fmt.Println("postImageInode", postImageInode) // With BTRFS reflinks, inode numbers differ even for identical content. // We need to compare content instead of just inodes. unchanged := false - + if preImageInode == postImageInode && preImageInode > 0 { // Same inode means definitely unchanged (works for hard links) unchanged = true @@ -291,7 +289,6 @@ func (r *ReferenceRecorder) StagePackedRefs() error { } if unchanged { - // fmt.Println("packed-refs unchanged") return nil } @@ -301,7 +298,6 @@ func (r *ReferenceRecorder) StagePackedRefs() error { } if postImageInode > 0 { - // fmt.Println("staging packed-refs change") fileID, err := r.entry.stageFile(r.postImagePackedRefsPath) if err != nil { return fmt.Errorf("stage packed-refs: %w", err) @@ -318,12 +314,12 @@ func filesHaveSameContent(path1, path2 string) (bool, error) { if err != nil { return false, err } - + hash2, err := hashFile(path2) if err != nil { return false, err } - + return bytes.Equal(hash1, hash2), nil } @@ -333,11 +329,11 @@ func hashFile(path string) ([]byte, error) { return nil, err } defer f.Close() - + h := sha256.New() if _, err := io.Copy(h, f); err != nil { return nil, err } - + return h.Sum(nil), nil -} \ No newline at end of file +} -- GitLab