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

Avoid closeChild calls altogether if flush/sync is not set, remove our internal sync option #39

Closed
schomatis opened this issue Dec 17, 2018 · 5 comments
Assignees
Labels
topic/technical debt Topic technical debt

Comments

@schomatis
Copy link
Contributor

With the logic of the flush/sync option of the MFS API discussed and clarified in #37 it seems that we either want to (when the user changes a file in the MFS) update all of the directories up to the root or do nothing (letting the final Close of the root to do the full update at the end to make sure nothing is lost) depending on whether the user of the API specifies the flush/sync option or not when opening a file.

At the moment, our alternative to an up-to-the-root update is not nothing but rather a kind of half-update that does just part of the job but doesn't guarantee to leave the MFS in sync at the end (we are still relying on the final Close). What the closeChild (now renamed updateChildEntry in #36) method does when called with the sync/flush options unset is to just call updateChild,

go-mfs/dir.go

Lines 124 to 127 in 2c1b835

err := d.updateChild(name, nd)
if err != nil {
return nil, err
}

which only updates the entry at the UnixFS layer in its internal (mutable) state,

go-mfs/dir.go

Lines 421 to 424 in 2c1b835

err := d.unixfsDir.AddChild(d.ctx, name, node)
if err != nil {
return err
}

that is, it doesn't regenerate the final node representing the new state with the updated entry (and hence it also doesn't add it to the DAG service nor update its parent with the new version).

That by itself it would seem to serve no purpose and it's already taken care of in sync (called by the root's Close method),

go-mfs/dir.go

Lines 432 to 443 in 2c1b835

func (d *Directory) sync() error {
for name, dir := range d.childDirs {
nd, err := dir.GetNode()
if err != nil {
return err
}
err = d.updateChild(name, nd)
if err != nil {
return err
}
}

So the proposed solution would be to remove the sync option in the closeChild interface effectively converting it into an up-to-the-root-sync/flush mechanism that it's either called in its entirety (one childCloser calls its parent and its parent calls its parent's parent and so on all the way up to the root) or not at all (depending if the file was open with the sync/flush option or not).

@schomatis schomatis added the topic/technical debt Topic technical debt label Dec 17, 2018
@schomatis schomatis added this to the Review the MFS interface milestone Dec 17, 2018
@schomatis schomatis self-assigned this Dec 17, 2018
@schomatis
Copy link
Contributor Author

/cc @Stebalien

1 similar comment
@schomatis
Copy link
Contributor Author

/cc @Stebalien

@schomatis
Copy link
Contributor Author

/cc @Stebalien (the main question is: is it worth doing a flushUp without sync? If not, we can remove that flag which would greatly simplify the update mechanism)

@Stebalien
Copy link
Member

That sounds fine but I didn't write the code so I really don't know any more than you in this case. However, make sure to update the file's inode when closing the file descriptor, even if we're not flushing.

@schomatis
Copy link
Contributor Author

make sure to update the file's inode when closing the file descriptor, even if we're not flushing.

Agreed, that's handled in the initial part of flushUp (which is always called),

go-mfs/fd.go

Lines 145 to 154 in e5a375d

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.nodeLock.Unlock()
// TODO: Maybe all this logic should happen in `File`.
return parent.updateChildEntry(child{name, nd}, fullSync)

before starting the updateChildEntry chain call, the only receiver of the fullSync argument that this issue proposes to eliminate.

@ghost ghost added the status/in-progress In progress label Dec 21, 2018
@ghost ghost removed the status/in-progress In progress label Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic/technical debt Topic technical debt
Projects
None yet
Development

No branches or pull requests

2 participants