Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new file to type cache #2303

Merged
merged 6 commits into from
Sep 2, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions internal/fs/fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -1650,16 +1650,19 @@ func (fs *fileSystem) createFile(
// Creates localFileInode with the given name under the parent inode.
// LOCKS_EXCLUDED(fs.mu)
// UNLOCK_FUNCTION(fs.mu)
// LOCK_FUNCTION(in)
// LOCK_FUNCTION(child)
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()

Expand All @@ -1670,8 +1673,6 @@ func (fs *fileSystem) createLocalFile(
}
}()

fs.mu.Lock()

fullName := inode.NewFileName(parent.Name(), name)
child, ok := fs.localFileInodes[fullName]

Expand All @@ -1680,23 +1681,28 @@ 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.
var result *inode.Core
result, err = parent.CreateLocalChildFile(name)
core, err := parent.CreateLocalChildFileCore(name)
if err != nil {
return
}

child = fs.mintInode(*result)
child = fs.mintInode(core)
vadlakondaswetha marked this conversation as resolved.
Show resolved Hide resolved
fs.localFileInodes[child.Name()] = child

// Empty file is created to be able to set attributes on the file.
fileInode := child.(*inode.FileInode)
err = fileInode.CreateEmptyTempFile()
if err != nil {
return
if err := fileInode.CreateEmptyTempFile(); err != nil {
return nil, err
}

return
fs.mu.Unlock()
raj-prince marked this conversation as resolved.
Show resolved Hide resolved
parent.Lock()
defer func() {
kislaykishore marked this conversation as resolved.
Show resolved Hide resolved
parent.Unlock()
}()
parent.InsertFileIntoTypeCache(name)
vadlakondaswetha marked this conversation as resolved.
Show resolved Hide resolved
// 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
}

// LOCKS_EXCLUDED(fs.mu)
Expand Down
8 changes: 6 additions & 2 deletions internal/fs/inode/base_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,12 @@ func (d *baseDirInode) CreateChildFile(ctx context.Context, name string) (*Core,
return nil, fuse.ENOSYS
}

func (d *baseDirInode) CreateLocalChildFile(name string) (*Core, error) {
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) CloneToChildFile(ctx context.Context, name string, src *gcs.MinObject) (*Core, error) {
Expand Down
29 changes: 21 additions & 8 deletions internal/fs/inode/dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,14 @@ 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)

// 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)
// 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)

// Like CreateChildFile, except clone the supplied source object instead of
// creating an empty object.
Expand Down Expand Up @@ -806,17 +811,25 @@ func (d *dirInode) CreateChildFile(ctx context.Context, name string) (*Core, err
}, nil
}

func (d *dirInode) CreateLocalChildFile(name string) (*Core, error) {
fullName := NewFileName(d.Name(), name)

return &Core{
func (d *dirInode) CreateLocalChildFileCore(name string) (Core, error) {
return Core{
Bucket: d.Bucket(),
FullName: fullName,
FullName: NewFileName(d.Name(), name),
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.
Expand Down
35 changes: 25 additions & 10 deletions internal/fs/inode/dir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1436,21 +1436,36 @@ func (t *DirTest) DeleteChildDir_ImplicitDirTrue() {
ExpectFalse(dirIn.IsUnlinked())
}

func (t *DirTest) CreateLocalChildFile_ShouldnotCreateObjectInGCS() {
const name = "qux"

// Create the local file inode.
core, err := t.in.CreateLocalChildFile(name)
func (t *DirTest) LocalChildFileCore() {
core, err := t.in.CreateLocalChildFileCore("qux")

AssertEq(nil, err)
AssertEq(true, core.Local)
AssertEq(t.bucket.Name(), core.Bucket.Name())
AssertEq("foo/bar/qux", core.FullName.objectName)
AssertTrue(core.Local)
AssertEq(nil, core.MinObject)
kislaykishore marked this conversation as resolved.
Show resolved Hide resolved

// Object shouldn't get created in GCS.
result, err := t.in.LookUpChild(t.ctx, name)
result, err := t.in.LookUpChild(t.ctx, "qux")
AssertEq(nil, err)
kislaykishore marked this conversation as resolved.
Show resolved Hide resolved
AssertEq(nil, result)
ExpectEq(metadata.UnknownType, t.getTypeFromCache(name))
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)
kislaykishore marked this conversation as resolved.
Show resolved Hide resolved
}

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)
}

func (t *DirTest) LocalFileEntriesEmpty() {
Expand Down
22 changes: 22 additions & 0 deletions internal/fs/local_file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -857,3 +857,25 @@ 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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,54 @@ 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)
kislaykishore marked this conversation as resolved.
Show resolved Hide resolved

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)
////////////////////////////////////////////////////////////////////////
Expand Down
Loading