diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 747fc94c2f..63bd341571 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -1642,19 +1642,16 @@ func (fs *fileSystem) createFile( // Creates localFileInode with the given name under the parent inode. // LOCKS_EXCLUDED(fs.mu) // UNLOCK_FUNCTION(fs.mu) -// LOCK_FUNCTION(child) +// LOCK_FUNCTION(in) func (fs *fileSystem) createLocalFile( parentID fuseops.InodeID, name string) (child inode.Inode, err error) { // Find the parent. fs.mu.Lock() parent := fs.dirInodeOrDie(parentID) + fs.mu.Unlock() defer func() { - if err != nil { - // fs.mu lock is already taken - delete(fs.localFileInodes, child.Name()) - } // We need to release the filesystem lock before acquiring the inode lock. fs.mu.Unlock() @@ -1665,6 +1662,8 @@ func (fs *fileSystem) createLocalFile( } }() + fs.mu.Lock() + fullName := inode.NewFileName(parent.Name(), name) child, ok := fs.localFileInodes[fullName] @@ -1673,26 +1672,23 @@ func (fs *fileSystem) createLocalFile( } // Create a new inode when a file is created first time, or when a local file is unlinked and then recreated with the same name. - core, err := parent.CreateLocalChildFileCore(name) + var result *inode.Core + result, err = parent.CreateLocalChildFile(name) if err != nil { return } - child = fs.mintInode(core) + + child = fs.mintInode(*result) fs.localFileInodes[child.Name()] = child // Empty file is created to be able to set attributes on the file. fileInode := child.(*inode.FileInode) - if err := fileInode.CreateEmptyTempFile(); err != nil { - return nil, err + err = fileInode.CreateEmptyTempFile() + if err != nil { + return } - fs.mu.Unlock() - parent.Lock() - defer parent.Unlock() - parent.InsertFileIntoTypeCache(name) - // Even though there is no action here that requires locking, adding locking - // so that the defer call that unlocks the mutex doesn't fail. - fs.mu.Lock() - return child, nil + + return } // LOCKS_EXCLUDED(fs.mu) diff --git a/internal/fs/inode/base_dir.go b/internal/fs/inode/base_dir.go index 8c7b52ab43..2b02273ebf 100644 --- a/internal/fs/inode/base_dir.go +++ b/internal/fs/inode/base_dir.go @@ -199,12 +199,8 @@ func (d *baseDirInode) CreateChildFile(ctx context.Context, name string) (*Core, return nil, fuse.ENOSYS } -func (d *baseDirInode) InsertFileIntoTypeCache(_ string) {} - -func (d *baseDirInode) EraseFromTypeCache(_ string) {} - -func (d *baseDirInode) CreateLocalChildFileCore(_ string) (Core, error) { - return Core{}, fuse.ENOSYS +func (d *baseDirInode) CreateLocalChildFile(name string) (*Core, error) { + return nil, fuse.ENOSYS } func (d *baseDirInode) CloneToChildFile(ctx context.Context, name string, src *gcs.MinObject) (*Core, error) { diff --git a/internal/fs/inode/dir.go b/internal/fs/inode/dir.go index 52bc961c33..84ada0e848 100644 --- a/internal/fs/inode/dir.go +++ b/internal/fs/inode/dir.go @@ -92,14 +92,9 @@ type DirInode interface { // Return the full name of the child and the GCS object it backs up. CreateChildFile(ctx context.Context, name string) (*Core, error) - // CreateLocalChildFileCore returns an empty local child file core. - CreateLocalChildFileCore(name string) (Core, error) - - // InsertFileIntoTypeCache adds the given name to type-cache - InsertFileIntoTypeCache(name string) - - // EraseFromTypeCache removes the given name from type-cache - EraseFromTypeCache(name string) + // Create an empty local child file with the supplied (relative) name. Local + // file means the object is not yet created in GCS. + CreateLocalChildFile(name string) (*Core, error) // Like CreateChildFile, except clone the supplied source object instead of // creating an empty object. @@ -811,25 +806,17 @@ func (d *dirInode) CreateChildFile(ctx context.Context, name string) (*Core, err }, nil } -func (d *dirInode) CreateLocalChildFileCore(name string) (Core, error) { - return Core{ +func (d *dirInode) CreateLocalChildFile(name string) (*Core, error) { + fullName := NewFileName(d.Name(), name) + + return &Core{ Bucket: d.Bucket(), - FullName: NewFileName(d.Name(), name), + FullName: fullName, MinObject: nil, Local: true, }, nil } -// LOCKS_REQUIRED(d) -func (d *dirInode) InsertFileIntoTypeCache(name string) { - d.cache.Insert(d.cacheClock.Now(), name, metadata.RegularFileType) -} - -// LOCKS_REQUIRED(d) -func (d *dirInode) EraseFromTypeCache(name string) { - d.cache.Erase(name) -} - // LOCKS_REQUIRED(d) func (d *dirInode) CloneToChildFile(ctx context.Context, name string, src *gcs.MinObject) (*Core, error) { // Erase any existing type information for this name. diff --git a/internal/fs/inode/dir_test.go b/internal/fs/inode/dir_test.go index 3af10c5875..9c88626a5c 100644 --- a/internal/fs/inode/dir_test.go +++ b/internal/fs/inode/dir_test.go @@ -1436,36 +1436,21 @@ func (t *DirTest) DeleteChildDir_ImplicitDirTrue() { ExpectFalse(dirIn.IsUnlinked()) } -func (t *DirTest) LocalChildFileCore() { - core, err := t.in.CreateLocalChildFileCore("qux") +func (t *DirTest) CreateLocalChildFile_ShouldnotCreateObjectInGCS() { + const name = "qux" + + // Create the local file inode. + core, err := t.in.CreateLocalChildFile(name) AssertEq(nil, err) - AssertEq(t.bucket.Name(), core.Bucket.Name()) - AssertEq("foo/bar/qux", core.FullName.objectName) - AssertTrue(core.Local) + AssertEq(true, core.Local) AssertEq(nil, core.MinObject) - result, err := t.in.LookUpChild(t.ctx, "qux") + + // Object shouldn't get created in GCS. + result, err := t.in.LookUpChild(t.ctx, name) AssertEq(nil, err) AssertEq(nil, result) - ExpectEq(metadata.UnknownType, t.getTypeFromCache("qux")) -} - -func (t *DirTest) InsertIntoTypeCache() { - t.in.InsertFileIntoTypeCache("abc") - - d := t.in.(*dirInode) - tp := t.tc.Get(d.cacheClock.Now(), "abc") - AssertEq(2, tp) -} - -func (t *DirTest) EraseFromTypeCache() { - t.in.InsertFileIntoTypeCache("abc") - - t.in.EraseFromTypeCache("abc") - - d := t.in.(*dirInode) - tp := d.cache.Get(d.cacheClock.Now(), "abc") - AssertEq(0, tp) + ExpectEq(metadata.UnknownType, t.getTypeFromCache(name)) } func (t *DirTest) LocalFileEntriesEmpty() { diff --git a/internal/fs/local_file_test.go b/internal/fs/local_file_test.go index 117aba2712..93856c67ba 100644 --- a/internal/fs/local_file_test.go +++ b/internal/fs/local_file_test.go @@ -857,25 +857,3 @@ func (t *LocalFileTest) TestStatLocalFileAfterRecreatingItWithSameName() { ExpectEq("test.txt", f.Name()) ExpectFalse(f.IsDir()) } - -func (t *LocalFileTest) TestStatFailsOnNewFileAfterDeletion() { - t.serverCfg.NewConfig = &cfg.Config{ - ImplicitDirs: true, - MetadataCache: cfg.MetadataCacheConfig{ - TtlSecs: -1, - TypeCacheMaxSizeMb: -1, - StatCacheMaxSizeMb: -1, - }, - Logging: cfg.DefaultLoggingConfig(), - } - filePath := path.Join(mntDir, "test.txt") - AssertEq(nil, err) - f1, err := os.Create(filePath) - AssertEq(nil, err) - defer AssertEq(nil, f1.Close()) - AssertEq(nil, os.Remove(filePath)) - - _, err = os.Stat(filePath) - - AssertNe(nil, err) -} diff --git a/tools/integration_tests/concurrent_operations/concurrent_listing_test.go b/tools/integration_tests/concurrent_operations/concurrent_listing_test.go index 2664a2be6c..3f997b7ad7 100644 --- a/tools/integration_tests/concurrent_operations/concurrent_listing_test.go +++ b/tools/integration_tests/concurrent_operations/concurrent_listing_test.go @@ -623,54 +623,6 @@ func (s *concurrentListingTest) Test_ListWithMoveDir(t *testing.T) { } } -// Test_StatWithNewFileWrite tests for potential deadlocks or race conditions when -// statting and creating a new file happen concurrently. -func (s *concurrentListingTest) Test_StatWithNewFileWrite(t *testing.T) { - t.Parallel() - testCaseDir := "Test_StatWithNewFileWrite" - createDirectoryStructureForTestCase(t, testCaseDir) - targetDir := path.Join(testDirPath, testCaseDir, "explicitDir") - var wg sync.WaitGroup - wg.Add(2) - timeout := 400 * time.Second // Adjust timeout as needed - - // Goroutine 1: Repeatedly calls Stat - go func() { - defer wg.Done() - for i := 0; i < iterationsForLightOperations; i++ { - _, err := os.Stat(targetDir) - - assert.NoError(t, err) - } - }() - - // Goroutine 2: Repeatedly create a file. - go func() { - defer wg.Done() - for i := 0; i < iterationsForLightOperations; i++ { - // Create file - filePath := path.Join(targetDir, fmt.Sprintf("tmp_file_%d.txt", i)) - err := os.WriteFile(filePath, []byte("Hello, world!"), setup.FilePermission_0600) - - assert.NoError(t, err) - } - }() - - // Wait for goroutines or timeout - done := make(chan bool) - go func() { - wg.Wait() - done <- true - }() - - select { - case <-done: - // Success: Both operations finished before timeout - case <-time.After(timeout): - assert.FailNow(t, "Possible deadlock or race condition detected") - } -} - //////////////////////////////////////////////////////////////////////// // Test Function (Runs once before all tests) ////////////////////////////////////////////////////////////////////////