From fcb426a66a1923c2591699c6f01b91897cc68978 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Tue, 10 Dec 2024 06:15:39 +0000 Subject: [PATCH 01/10] fixing conflicts --- internal/fs/inode/file_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/fs/inode/file_test.go b/internal/fs/inode/file_test.go index 3c7f69985c..780fad7b44 100644 --- a/internal/fs/inode/file_test.go +++ b/internal/fs/inode/file_test.go @@ -193,7 +193,7 @@ func (t *FileTest) TestInitialAttributes_MtimeFromObjectMetadata_Gcsfuse() { // Ask it for its attributes. attrs, err := t.in.Attributes(t.ctx) - assert.Nil(t.T(), err) + assert.Equal(t.T(), nil, err) assert.Equal(t.T(), attrs.Mtime, mtime) } From e389abfe26d7f6988ec54fef489921f5893fc925 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Tue, 10 Dec 2024 06:17:46 +0000 Subject: [PATCH 02/10] fixing nil assert # Conflicts: # internal/fs/inode/file_test.go --- internal/fs/inode/file_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/fs/inode/file_test.go b/internal/fs/inode/file_test.go index 780fad7b44..63e21aa7b2 100644 --- a/internal/fs/inode/file_test.go +++ b/internal/fs/inode/file_test.go @@ -193,7 +193,7 @@ func (t *FileTest) TestInitialAttributes_MtimeFromObjectMetadata_Gcsfuse() { // Ask it for its attributes. attrs, err := t.in.Attributes(t.ctx) - assert.Equal(t.T(), nil, err) + assert.Nil(t.T(), err) assert.Equal(t.T(), attrs.Mtime, mtime) } @@ -384,7 +384,7 @@ func (t *FileTest) TestWriteThenSync() { m, _, err := t.bucket.StatObject(t.ctx, statReq) assert.Nil(t.T(), err) - assert.NotNil(t.T(), m) + assert.NotEqual(t.T(), nil, m) assert.Equal(t.T(), t.in.SourceGeneration().Object, m.Generation) assert.Equal(t.T(), t.in.SourceGeneration().Metadata, m.MetaGeneration) assert.Equal(t.T(), uint64(len("paco")), m.Size) @@ -431,7 +431,7 @@ func (t *FileTest) TestWriteToLocalFileThenSync() { statReq := &gcs.StatObjectRequest{Name: t.in.Name().GcsObjectName()} m, _, err := t.bucket.StatObject(t.ctx, statReq) assert.Nil(t.T(), err) - assert.NotNil(t.T(), m) + assert.NotEqual(t.T(), nil, m) assert.Equal(t.T(), t.in.SourceGeneration().Object, m.Generation) assert.Equal(t.T(), t.in.SourceGeneration().Metadata, m.MetaGeneration) assert.Equal(t.T(), uint64(len("tacos")), m.Size) From 1d297950c50653b51aed1af41b076c8f28097181 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Tue, 10 Dec 2024 00:32:30 +0000 Subject: [PATCH 03/10] not nil fix --- internal/fs/inode/file_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/fs/inode/file_test.go b/internal/fs/inode/file_test.go index 63e21aa7b2..3c7f69985c 100644 --- a/internal/fs/inode/file_test.go +++ b/internal/fs/inode/file_test.go @@ -384,7 +384,7 @@ func (t *FileTest) TestWriteThenSync() { m, _, err := t.bucket.StatObject(t.ctx, statReq) assert.Nil(t.T(), err) - assert.NotEqual(t.T(), nil, m) + assert.NotNil(t.T(), m) assert.Equal(t.T(), t.in.SourceGeneration().Object, m.Generation) assert.Equal(t.T(), t.in.SourceGeneration().Metadata, m.MetaGeneration) assert.Equal(t.T(), uint64(len("paco")), m.Size) @@ -431,7 +431,7 @@ func (t *FileTest) TestWriteToLocalFileThenSync() { statReq := &gcs.StatObjectRequest{Name: t.in.Name().GcsObjectName()} m, _, err := t.bucket.StatObject(t.ctx, statReq) assert.Nil(t.T(), err) - assert.NotEqual(t.T(), nil, m) + assert.NotNil(t.T(), m) assert.Equal(t.T(), t.in.SourceGeneration().Object, m.Generation) assert.Equal(t.T(), t.in.SourceGeneration().Metadata, m.MetaGeneration) assert.Equal(t.T(), uint64(len("tacos")), m.Size) From 4c83720fa52799df71f5bcb3a59bd05bf022ee93 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Tue, 17 Dec 2024 16:24:34 +0000 Subject: [PATCH 04/10] storing fileHandle open semantics --- internal/fs/fs.go | 4 ++-- internal/fs/handle/file.go | 12 +++++++++++- internal/fs/inode/file.go | 27 +++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 3 deletions(-) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 922bb0c65e..5e9f920ac1 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -1737,7 +1737,7 @@ func (fs *fileSystem) CreateFile( handleID := fs.nextHandleID fs.nextHandleID++ - fs.handles[handleID] = handle.NewFileHandle(child.(*inode.FileInode), fs.fileCacheHandler, fs.cacheFileForRangeRead, fs.metricHandle) + fs.handles[handleID] = handle.NewFileHandle(child.(*inode.FileInode), fs.fileCacheHandler, fs.cacheFileForRangeRead, fs.metricHandle, false) op.Handle = handleID fs.mu.Unlock() @@ -2381,7 +2381,7 @@ func (fs *fileSystem) OpenFile( handleID := fs.nextHandleID fs.nextHandleID++ - fs.handles[handleID] = handle.NewFileHandle(in, fs.fileCacheHandler, fs.cacheFileForRangeRead, fs.metricHandle) + fs.handles[handleID] = handle.NewFileHandle(in, fs.fileCacheHandler, fs.cacheFileForRangeRead, fs.metricHandle, op.OpenFlags.IsReadOnly()) op.Handle = handleID // When we observe object generations that we didn't create, we assign them diff --git a/internal/fs/handle/file.go b/internal/fs/handle/file.go index 0d5c77962c..0af059ab8e 100644 --- a/internal/fs/handle/file.go +++ b/internal/fs/handle/file.go @@ -47,14 +47,24 @@ type FileHandle struct { // will be downloaded for random reads as well too. cacheFileForRangeRead bool metricHandle common.MetricHandle + // For now, we will consider the files which are open in append mode also as write, + // as we are not doing anything special for append. When required we will + // define an enum instead of boolean to hold the type of open. + readOnly bool } -func NewFileHandle(inode *inode.FileInode, fileCacheHandler *file.CacheHandler, cacheFileForRangeRead bool, metricHandle common.MetricHandle) (fh *FileHandle) { +func NewFileHandle( + inode *inode.FileInode, + fileCacheHandler *file.CacheHandler, + cacheFileForRangeRead bool, + metricHandle common.MetricHandle, + readOnly bool) (fh *FileHandle) { fh = &FileHandle{ inode: inode, fileCacheHandler: fileCacheHandler, cacheFileForRangeRead: cacheFileForRangeRead, metricHandle: metricHandle, + readOnly: readOnly, } fh.mu = syncutil.NewInvariantMutex(fh.checkInvariants) diff --git a/internal/fs/inode/file.go b/internal/fs/inode/file.go index 9c60fbf4fd..dc8b30e4b5 100644 --- a/internal/fs/inode/file.go +++ b/internal/fs/inode/file.go @@ -27,6 +27,7 @@ import ( "github.com/googlecloudplatform/gcsfuse/v2/internal/contentcache" "github.com/googlecloudplatform/gcsfuse/v2/internal/fs/gcsfuse_errors" "github.com/googlecloudplatform/gcsfuse/v2/internal/gcsx" + "github.com/googlecloudplatform/gcsfuse/v2/internal/logger" "github.com/googlecloudplatform/gcsfuse/v2/internal/storage/gcs" "github.com/googlecloudplatform/gcsfuse/v2/internal/storage/storageutil" "github.com/jacobsa/fuse/fuseops" @@ -95,6 +96,7 @@ type FileInode struct { bwh *bufferedwrites.BufferedWriteHandler config *cfg.Config + writeHandleCount int32 } var _ Inode = &FileInode{} @@ -369,6 +371,31 @@ func (f *FileInode) DecrementLookupCount(n uint64) (destroy bool) { return } +// LOCKS_REQUIRED(f.mu) +func (f *FileInode) RegisterFileHandle(readOnly bool) { + if !readOnly { + f.writeHandleCount++ + } +} + +// LOCKS_REQUIRED(f.mu) +func (f *FileInode) DeRegisterFileHandle(readOnly bool) { + if readOnly { + return + } + + f.writeHandleCount-- + + if f.writeHandleCount < 0 { + logger.Errorf("Mismatch in number of write file handles for inode :%d", f.id) + } + + // All write fileHandles associated with bwh are closed. So safe to set bwh to nil. + if f.writeHandleCount == 0 { + f.bwh = nil + } +} + // LOCKS_REQUIRED(f.mu) func (f *FileInode) Destroy() (err error) { f.destroyed = true From b845fefbdbe9dc6a992c894c4fabf5ae0d43b6c7 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Tue, 17 Dec 2024 16:56:19 +0000 Subject: [PATCH 05/10] Added unit tests for fileInode --- internal/fs/inode/file.go | 17 ++++--- internal/fs/inode/file_test.go | 93 ++++++++++++++++++++++++++++++++-- 2 files changed, 101 insertions(+), 9 deletions(-) diff --git a/internal/fs/inode/file.go b/internal/fs/inode/file.go index dc8b30e4b5..89bfc41b78 100644 --- a/internal/fs/inode/file.go +++ b/internal/fs/inode/file.go @@ -94,9 +94,9 @@ type FileInode struct { // Represents if local file has been unlinked. unlinked bool - bwh *bufferedwrites.BufferedWriteHandler - config *cfg.Config - writeHandleCount int32 + bwh *bufferedwrites.BufferedWriteHandler + config *cfg.Config + writeHandleCount int32 } var _ Inode = &FileInode{} @@ -384,12 +384,12 @@ func (f *FileInode) DeRegisterFileHandle(readOnly bool) { return } - f.writeHandleCount-- - - if f.writeHandleCount < 0 { + if f.writeHandleCount <= 0 { logger.Errorf("Mismatch in number of write file handles for inode :%d", f.id) } + f.writeHandleCount-- + // All write fileHandles associated with bwh are closed. So safe to set bwh to nil. if f.writeHandleCount == 0 { f.bwh = nil @@ -829,6 +829,11 @@ func (f *FileInode) CreateBufferedOrTempWriter(ctx context.Context) (err error) } func (f *FileInode) ensureBufferedWriteHandler(ctx context.Context) error { + // bwh already initialized, do nothing. + if f.bwh != nil { + return nil + } + var err error var latestGcsObj *gcs.Object if !f.local { diff --git a/internal/fs/inode/file_test.go b/internal/fs/inode/file_test.go index 3c7f69985c..1e36ffb05f 100644 --- a/internal/fs/inode/file_test.go +++ b/internal/fs/inode/file_test.go @@ -36,6 +36,7 @@ import ( "github.com/jacobsa/syncutil" "github.com/jacobsa/timeutil" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "golang.org/x/net/context" ) @@ -1311,7 +1312,7 @@ func (t *FileTest) TestMultipleWritesToLocalFileWhenStreamingWritesAreEnabled() assert.WithinDuration(t.T(), attrs.Mtime, createTime, Delta) } -func (t *FileTest) WriteToEmptyGCSFileWhenStreamingWritesAreEnabled() { +func (t *FileTest) TestWriteToEmptyGCSFileWhenStreamingWritesAreEnabled() { t.createInodeWithEmptyObject() t.in.config = &cfg.Config{Write: *getWriteConfig()} createTime := t.in.mtimeClock.Now() @@ -1330,7 +1331,7 @@ func (t *FileTest) WriteToEmptyGCSFileWhenStreamingWritesAreEnabled() { assert.WithinDuration(t.T(), attrs.Mtime, createTime, Delta) } -func (t *FileTest) SetMtimeOnEmptyGCSFileWhenStreamingWritesAreEnabled() { +func (t *FileTest) TestSetMtimeOnEmptyGCSFileWhenStreamingWritesAreEnabled() { t.createInodeWithEmptyObject() t.in.config = &cfg.Config{Write: *getWriteConfig()} assert.Nil(t.T(), t.in.bwh) @@ -1342,7 +1343,7 @@ func (t *FileTest) SetMtimeOnEmptyGCSFileWhenStreamingWritesAreEnabled() { assert.Nil(t.T(), t.in.bwh) } -func (t *FileTest) SetMtimeOnEmptyGCSFileAfterWritesWhenStreamingWritesAreEnabled() { +func (t *FileTest) TestSetMtimeOnEmptyGCSFileAfterWritesWhenStreamingWritesAreEnabled() { t.createInodeWithEmptyObject() t.in.config = &cfg.Config{Write: *getWriteConfig()} assert.Nil(t.T(), t.in.bwh) @@ -1366,6 +1367,92 @@ func (t *FileTest) SetMtimeOnEmptyGCSFileAfterWritesWhenStreamingWritesAreEnable assert.Equal(t.T(), attrs.Atime, mtime) } +func (t *FileTest) TestRegisterFileHandle() { + tbl := []struct { + name string + readonly bool + currentVal int32 + expectedVal int32 + }{ + { + name: "ReadOnlyHandle", + readonly: true, + currentVal: 0, + expectedVal: 0, + }, + { + name: "ZeroCurrentValueForWriteHandle", + readonly: false, + currentVal: 0, + expectedVal: 1, + }, + { + name: "NonZeroCurrentValueForWriteHandle", + readonly: false, + currentVal: 5, + expectedVal: 6, + }, + } + for _, tc := range tbl { + t.Run(tc.name, func() { + t.in.writeHandleCount = tc.currentVal + + t.in.RegisterFileHandle(tc.readonly) + + assert.Equal(t.T(), tc.expectedVal, t.in.writeHandleCount) + }) + } +} + +func (t *FileTest) TestDeRegisterFileHandle() { + tbl := []struct { + name string + readonly bool + currentVal int32 + expectedVal int32 + isBwhNil bool + }{ + { + name: "ReadOnlyHandle", + readonly: true, + currentVal: 10, + expectedVal: 10, + isBwhNil: false, + }, + { + name: "NonZeroCurrentValueForWriteHandle", + readonly: false, + currentVal: 10, + expectedVal: 9, + isBwhNil: false, + }, + { + name: "LastWriteHandleToDeregister", + readonly: false, + currentVal: 1, + expectedVal: 0, + isBwhNil: true, + }, + } + for _, tc := range tbl { + t.Run(tc.name, func() { + t.in.config = &cfg.Config{Write: *getWriteConfig()} + t.in.writeHandleCount = tc.currentVal + err := t.in.ensureBufferedWriteHandler(t.ctx) + require.NoError(t.T(), err) + + t.in.DeRegisterFileHandle(tc.readonly) + + assert.Equal(t.T(), tc.expectedVal, t.in.writeHandleCount) + if tc.isBwhNil { + assert.Nil(t.T(), t.in.bwh) + } else { + assert.NotNil(t.T(), t.in.bwh) + } + }) + } +} + func getWriteConfig() *cfg.WriteConfig { return &cfg.WriteConfig{ MaxBlocksPerFile: 10, From 8feaafc22eb2edefc8ebef24f7b2c6bb96d19ce6 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Thu, 19 Dec 2024 09:20:31 +0000 Subject: [PATCH 06/10] Registering and deregistering. --- internal/fs/fs.go | 8 ++++++++ internal/fs/handle/file.go | 3 +++ 2 files changed, 11 insertions(+) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 5e9f920ac1..4ac3645ac7 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -2376,6 +2376,14 @@ func (fs *fileSystem) OpenFile( // Find the inode. in := fs.fileInodeOrDie(op.Inode) + // Follow lock ordering rules to get inode lock. + // Inode lock is required to register fileHandle with the inode. + fs.mu.Unlock() + in.Lock() + + // Get the fs lock again. + fs.mu.Lock() + defer fs.mu.Unlock() // Allocate a handle. handleID := fs.nextHandleID diff --git a/internal/fs/handle/file.go b/internal/fs/handle/file.go index 0af059ab8e..ea28688138 100644 --- a/internal/fs/handle/file.go +++ b/internal/fs/handle/file.go @@ -67,6 +67,7 @@ func NewFileHandle( readOnly: readOnly, } + fh.inode.RegisterFileHandle(fh.readOnly) fh.mu = syncutil.NewInvariantMutex(fh.checkInvariants) return @@ -75,6 +76,8 @@ func NewFileHandle( // Destroy any resources associated with the handle, which must not be used // again. func (fh *FileHandle) Destroy() { + // Deregister the fileHandle with the inode. + fh.inode.DeRegisterFileHandle(fh.readOnly) if fh.reader != nil { fh.reader.Destroy() } From 30e18443b3f5d2e80fe24c45ee30a7a8bff397d1 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Thu, 19 Dec 2024 09:30:13 +0000 Subject: [PATCH 07/10] ADDED COMMENTS --- internal/fs/fs.go | 1 + internal/fs/inode/file.go | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 4ac3645ac7..3709dfc1e1 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -1737,6 +1737,7 @@ func (fs *fileSystem) CreateFile( handleID := fs.nextHandleID fs.nextHandleID++ + // Creating new file is always a write operation, hence passing readOnly as false. fs.handles[handleID] = handle.NewFileHandle(child.(*inode.FileInode), fs.fileCacheHandler, fs.cacheFileForRangeRead, fs.metricHandle, false) op.Handle = handleID diff --git a/internal/fs/inode/file.go b/internal/fs/inode/file.go index 89bfc41b78..78aa009c16 100644 --- a/internal/fs/inode/file.go +++ b/internal/fs/inode/file.go @@ -94,8 +94,16 @@ type FileInode struct { // Represents if local file has been unlinked. unlinked bool - bwh *bufferedwrites.BufferedWriteHandler - config *cfg.Config + bwh *bufferedwrites.BufferedWriteHandler + config *cfg.Config + + // Once write is started on the file i.e, bwh is initialized, any fileHandles + // opened in write mode before or after this and not yet closed are considered + // as writing to the file even though they are not writing. + // In case of successful flush, we will set bwh to nil. But in case of error, + // we will keep returning that error to all the fileHandles open during that time + // and set bwh to nil after all fileHandlers are closed. + // writeHandleCount tracks the count of open fileHandles in write mode. writeHandleCount int32 } From 945924a54b0806223addaa9601ccd8796f577944 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Mon, 23 Dec 2024 11:51:58 +0000 Subject: [PATCH 08/10] fixing panic --- internal/fs/fs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/fs/fs.go b/internal/fs/fs.go index 3709dfc1e1..c1cafc1524 100644 --- a/internal/fs/fs.go +++ b/internal/fs/fs.go @@ -2373,7 +2373,6 @@ func (fs *fileSystem) OpenFile( ctx context.Context, op *fuseops.OpenFileOp) (err error) { fs.mu.Lock() - defer fs.mu.Unlock() // Find the inode. in := fs.fileInodeOrDie(op.Inode) @@ -2381,6 +2380,7 @@ func (fs *fileSystem) OpenFile( // Inode lock is required to register fileHandle with the inode. fs.mu.Unlock() in.Lock() + defer in.Unlock() // Get the fs lock again. fs.mu.Lock() From de7dfb48490d3e67ac8440c21c5405906a7d7ce2 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Mon, 23 Dec 2024 12:38:29 +0000 Subject: [PATCH 09/10] fixing tests --- internal/fs/inode/file_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/fs/inode/file_test.go b/internal/fs/inode/file_test.go index 1e36ffb05f..0666f8846e 100644 --- a/internal/fs/inode/file_test.go +++ b/internal/fs/inode/file_test.go @@ -1327,7 +1327,7 @@ func (t *FileTest) TestWriteToEmptyGCSFileWhenStreamingWritesAreEnabled() { // The inode should agree about the new mtime. attrs, err := t.in.Attributes(t.ctx) assert.Nil(t.T(), err) - assert.Equal(t.T(), int64(2), attrs.Size) + assert.Equal(t.T(), uint64(2), attrs.Size) assert.WithinDuration(t.T(), attrs.Mtime, createTime, Delta) } @@ -1352,7 +1352,7 @@ func (t *FileTest) TestSetMtimeOnEmptyGCSFileAfterWritesWhenStreamingWritesAreEn assert.Nil(t.T(), err) assert.NotNil(t.T(), t.in.bwh) writeFileInfo := t.in.bwh.WriteFileInfo() - assert.Equal(t.T(), 2, writeFileInfo.TotalSize) + assert.Equal(t.T(), int64(2), writeFileInfo.TotalSize) // Set mtime. mtime := time.Now().UTC().Add(123 * time.Second) From 17066b22b4693a8c8c41f190a0e7a3ce8c2947d3 Mon Sep 17 00:00:00 2001 From: vadlakondaswetha Date: Tue, 24 Dec 2024 06:42:24 +0000 Subject: [PATCH 10/10] PR comment --- internal/fs/handle/file.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/fs/handle/file.go b/internal/fs/handle/file.go index ea28688138..d2d417ce92 100644 --- a/internal/fs/handle/file.go +++ b/internal/fs/handle/file.go @@ -53,12 +53,8 @@ type FileHandle struct { readOnly bool } -func NewFileHandle( - inode *inode.FileInode, - fileCacheHandler *file.CacheHandler, - cacheFileForRangeRead bool, - metricHandle common.MetricHandle, - readOnly bool) (fh *FileHandle) { +// LOCKS_REQUIRED(fh.inode.mu) +func NewFileHandle(inode *inode.FileInode, fileCacheHandler *file.CacheHandler, cacheFileForRangeRead bool, metricHandle common.MetricHandle, readOnly bool) (fh *FileHandle) { fh = &FileHandle{ inode: inode, fileCacheHandler: fileCacheHandler,