Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

use RW lock for the File's lock #43

Merged
merged 2 commits into from
Dec 19, 2018
Merged

use RW lock for the File's lock #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36363fb
rename and document `File`'s node lock
schomatis Dec 19, 2018
00b7d5c
use RW lock for the `File`'s node
schomatis Dec 19, 2018
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
4 changes: 2 additions & 2 deletions fd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,13 @@ func (fi *fileDescriptor) flushUp(fullsync bool) error {
return err
}

fi.inode.nodelk.Lock()
fi.inode.nodeLock.Lock()
fi.inode.node = nd
// TODO: Create a `SetNode` method.
name := fi.inode.name
parent := fi.inode.parent
// TODO: Can the parent be modified? Do we need to do this inside the lock?
fi.inode.nodelk.Unlock()
fi.inode.nodeLock.Unlock()
// TODO: Maybe all this logic should happen in `File`.

return parent.closeChild(name, nd, fullsync)
Expand Down
19 changes: 10 additions & 9 deletions file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ type File struct {
// of a particular sub-DAG that abstract an upper layer's entity.
node ipld.Node

// TODO: Rename.
nodelk sync.Mutex
// Lock around the `node` that represents this file, necessary because
// there may be many `FileDescriptor`s operating on this `File`.
nodeLock sync.RWMutex

RawLeaves bool
}
Expand Down Expand Up @@ -58,9 +59,9 @@ const (
)

func (fi *File) Open(flags int, sync bool) (FileDescriptor, error) {
fi.nodelk.Lock()
fi.nodeLock.RLock()
node := fi.node
fi.nodelk.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also call GetNode here.

fi.nodeLock.RUnlock()

// TODO: Move this `switch` logic outside (maybe even
// to another package, this seems like a job of UnixFS),
Expand Down Expand Up @@ -118,8 +119,8 @@ func (fi *File) Open(flags int, sync bool) (FileDescriptor, error) {
// pretty much the same thing as here, we should at least call
// that function and wrap the `ErrNotUnixfs` with an MFS text.
func (fi *File) Size() (int64, error) {
fi.nodelk.Lock()
defer fi.nodelk.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR but, ideally, we'd call GetNode() here instead of holding the lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have a couple of TODOs to refactor that (including also adding a SetNode() method).

fi.nodeLock.RLock()
defer fi.nodeLock.RUnlock()
switch nd := fi.node.(type) {
case *dag.ProtoNode:
fsn, err := ft.FSNodeFromBytes(nd.Data())
Expand All @@ -135,10 +136,10 @@ func (fi *File) Size() (int64, error) {
}

// GetNode returns the dag node associated with this file
// TODO: Use this method and do not access the `nodelk` directly anywhere else.
// TODO: Use this method and do not access the `nodeLock` directly anywhere else.
func (fi *File) GetNode() (ipld.Node, error) {
fi.nodelk.Lock()
defer fi.nodelk.Unlock()
fi.nodeLock.RLock()
defer fi.nodeLock.RUnlock()
return fi.node, nil
}

Expand Down