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

Fixing the creation of multiple inode creation for the same name when… #572

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skuppa-veeva
Copy link

Fixing the creation of multiple inode creation for the same name when MkDir executed simultaneously.

… MkDir or CreateFile executed simultaneously.

Get parent lock before creating inode from `Goofys.MkDir` or `Goofys.CreateFile` to avoid creating a duplicate inode.
Remove get lock from `Inode.Create` and `Inode.MkDir` since it lock already acquired by the caller.
@skuppa skuppa force-pushed the prevent-duplicate-inode-creation branch from d714e44 to 1ff2c07 Compare October 27, 2020 18:54
Comment on lines -908 to -910
parent.mu.Lock()
defer parent.mu.Unlock()

Copy link
Author

@skuppa-veeva skuppa-veeva Oct 28, 2020

Choose a reason for hiding this comment

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

We don't need to parent lock since it is already locked from caller.

Comment on lines -951 to -953
parent.mu.Lock()
defer parent.mu.Unlock()

Copy link
Author

Choose a reason for hiding this comment

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

We need lock here as well, since caller already has a lock.

@@ -1013,10 +1013,10 @@ func (fs *Goofys) CreateFile(
parent := fs.getInodeOrDie(op.Parent)
fs.mu.RUnlock()

inode, fh := parent.Create(op.Name, op.Metadata)

parent.mu.Lock()

Copy link
Author

Choose a reason for hiding this comment

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

The folder creation is moved after acquiring the parent lock. This lock needs to be atomic between creating inode and inserting into map of inodes.

Comment on lines -1058 to -1059
parent.mu.Lock()

Copy link
Author

Choose a reason for hiding this comment

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

The folder creation is moved after acquiring the parent lock. This lock needs to be atomic between creating inode and inserting into map of inodes.

@skuppa-veeva
Copy link
Author

skuppa-veeva commented Oct 28, 2020

@kahing This PR fixes multiple issues:
Fix #353 #377 #571

Could you review and make sure the order of lock is correct and it works as expected. We are waiting for this fix and I would like to get you feedback.

@skuppa-veeva
Copy link
Author

@kahing could you review this PR and give us your expert advice.

@skuppa-veeva
Copy link
Author

@kahing could you review this PR?

@monthonk
Copy link
Contributor

monthonk commented Aug 8, 2022

This PR seems to fix multiple issues but we need a rebase to make it work with current code base. I'm not sure you're still tracking it but we probably want to try to get this PR merged as part of the next release.

@94jskim
Copy link

94jskim commented Apr 18, 2023

@kahing @gaul @monthonk I need this fix, when will it be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants