Skip to content

Commit

Permalink
Merge pull request ipfs#45 from ipfs/fix/flush/remove-fullsync
Browse files Browse the repository at this point in the history
remove the `fullSync` option from `updateChildEntry`
  • Loading branch information
schomatis authored Dec 27, 2018
2 parents e5a375d + 8d26210 commit 4fb6dc4
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 63 deletions.
70 changes: 19 additions & 51 deletions dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,45 +76,27 @@ func (d *Directory) SetCidBuilder(b cid.Builder) {
d.unixfsDir.SetCidBuilder(b)
}

// This method implements the `parent` interface. It first updates
// the child entry in the underlying UnixFS directory and then, if `fullSync`
// is set, it:
// 1. DAG: saves the newly created directory node with the updated entry.
// 2. MFS: propagates the update upwards (through this same interface)
// repeating the whole process in the parent.
func (d *Directory) updateChildEntry(c child, fullSync bool) error {

// There's a local flush (`closeChildUpdate`) and a propagated flush (`updateChildEntry`).

newDirNode, err := d.closeChildUpdate(c, fullSync)
// This method implements the `parent` interface. It first does the local
// update of the child entry in the underlying UnixFS directory and saves
// the newly created directory node with the updated entry in the DAG
// service. Then it propagates the update upwards (through this same
// interface) repeating the whole process in the parent.
func (d *Directory) updateChildEntry(c child) error {
newDirNode, err := d.localUpdate(c)
if err != nil {
return err
}

// TODO: The `sync` seems to be tightly coupling this two pieces of code,
// we use the node returned by `closeChildUpdate` (which entails a copy)
// only if `sync` is set, and we are discarding it otherwise. At the very
// least the `if sync {` clause at the end of `closeChildUpdate` should
// be merged with this one (the use of the `lock` is stopping this at the
// moment, re-evaluate when its purpose has been better understood).

if fullSync {
return d.parent.updateChildEntry(child{d.name, newDirNode}, true)
// Setting `fullSync` to true here means, if the original child that
// initiated the update process wanted to propagate it upwards then
// continue to do so all the way up to the root, that is, the only
// time `fullSync` can be false is in the first call (which will be
// the *only* call), we either update the first parent entry or *all*
// the parent's.
}

return nil
// Continue to propagate the update process upwards
// (all the way up to the root).
return d.parent.updateChildEntry(child{d.name, newDirNode})
}

// This method implements the part of `updateChildEntry` that needs
// to be locked around: in charge of updating the UnixFS layer and
// generating the new node reflecting the update.
func (d *Directory) closeChildUpdate(c child, fullSync bool) (*dag.ProtoNode, error) {
// generating the new node reflecting the update. It also stores the
// new node in the DAG layer.
func (d *Directory) localUpdate(c child) (*dag.ProtoNode, error) {
d.lock.Lock()
defer d.lock.Unlock()

Expand All @@ -125,34 +107,20 @@ func (d *Directory) closeChildUpdate(c child, fullSync bool) (*dag.ProtoNode, er
// TODO: Clearly define how are we propagating changes to lower layers
// like UnixFS.

if fullSync {
return d.flushCurrentNode()
}
return nil, nil
}

// Recreate the underlying UnixFS directory node and save it in the DAG layer.
func (d *Directory) flushCurrentNode() (*dag.ProtoNode, error) {
nd, err := d.unixfsDir.GetNode()
if err != nil {
return nil, err
}

err = d.dagService.Add(d.ctx, nd)
if err != nil {
return nil, err
}
// TODO: This method is called in `closeChildUpdate` while the lock is
// taken, we need the lock while operating on `unixfsDir` to create the
// new node but do we also need to keep the lock while adding it to the
// DAG service? Evaluate refactoring these two methods together and better
// redistributing the node.

pbnd, ok := nd.(*dag.ProtoNode)
if !ok {
return nil, dag.ErrNotProtobuf
}
// TODO: Why do we check the node *after* adding it to the DAG service?

err = d.dagService.Add(d.ctx, nd)
if err != nil {
return nil, err
}

return pbnd.Copy().(*dag.ProtoNode), nil
// TODO: Why do we need a copy?
Expand Down Expand Up @@ -374,7 +342,7 @@ func (d *Directory) Flush() error {
return err
}

return d.parent.updateChildEntry(child{d.name, nd}, true)
return d.parent.updateChildEntry(child{d.name, nd})
}

// AddChild adds the node 'nd' under this directory giving it the name 'name'
Expand Down
11 changes: 7 additions & 4 deletions fd.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,8 @@ func (fi *fileDescriptor) Flush() error {

// flushUp syncs the file and adds it to the dagservice
// it *must* be called with the File's lock taken
// TODO: What is `fullsync`? Propagate the changes upward
// to the root flushing every node in the path (the "up"
// part of `flushUp`).
// If `fullSync` is set the changes are propagated upwards
// (the `Up` part of `flushUp`).
func (fi *fileDescriptor) flushUp(fullSync bool) error {
nd, err := fi.mod.GetNode()
if err != nil {
Expand All @@ -151,7 +150,11 @@ func (fi *fileDescriptor) flushUp(fullSync bool) error {
fi.inode.nodeLock.Unlock()
// TODO: Maybe all this logic should happen in `File`.

return parent.updateChildEntry(child{name, nd}, fullSync)
if fullSync {
return parent.updateChildEntry(child{name, nd})
}

return nil
}

// Seek implements io.Seeker
Expand Down
15 changes: 7 additions & 8 deletions root.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,12 @@ type child struct {
// the entry/link of the directory that pointed to the modified node).
type parent interface {
// Method called by a child to its parent to signal to update the content
// pointed to in the entry by that child's name. The child sends as
// arguments its own information (under the `child` structure) and a flag
// (`fullsync`) indicating whether or not to propagate the update upwards:
// modifying a directory entry entails modifying its contents which means
// that its parent (the parent's parent) will also need to be updated (and
// so on).
updateChildEntry(c child, fullSync bool) error
// pointed to in the entry by that child's name. The child sends its own
// information in the `child` structure. As modifying a directory entry
// entails modifying its contents the parent will also call *its* parent's
// `updateChildEntry` to update the entry pointing to the new directory,
// this mechanism is in turn repeated until reaching the `Root`.
updateChildEntry(c child) error
}

type NodeType int
Expand Down Expand Up @@ -189,7 +188,7 @@ func (kr *Root) FlushMemFree(ctx context.Context) error {
// TODO: The `sync` argument isn't used here (we've already reached
// the top), document it and maybe make it an anonymous variable (if
// that's possible).
func (kr *Root) updateChildEntry(c child, fullSync bool) error {
func (kr *Root) updateChildEntry(c child) error {
err := kr.GetDirectory().dagService.Add(context.TODO(), c.Node)
if err != nil {
return err
Expand Down

0 comments on commit 4fb6dc4

Please sign in to comment.