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

Fix invalid locking order and deadlock in rename operation #2656

Closed
wants to merge 1 commit into from
Closed

Fix invalid locking order and deadlock in rename operation #2656

wants to merge 1 commit into from

Conversation

seletskiy
Copy link
Contributor

This commit should prevent deadlock on dp_config_rwlock when running
zfs rename.

See #2652 for the description of that changes.

Minors renaming will be little out of sync with actual rename, but I don't think it's a big deal since snapshot creation works same way (in meaning of synchronization).

Fix #2652, #2525.

@dweeezil
Copy link
Contributor

dweeezil commented Sep 3, 2014

@seletskiy The buildbot import failures should go away if this is rebased on a current master. Also, I suspect the kmem_alloc() calls would no longer need KM_PUSHPAGE (or could use kmem_asprintf).

@ryao
Copy link
Contributor

ryao commented Sep 3, 2014

This looks good to me, but I would like to see the buildbot import failures resolved before it is merged. It should be possible to add the main repository as a remote, rebase against it and then force push an update. Something like this should work provided that you have SSH setup:

git remote add upstream [email protected]:zfsonlinux/zfs.git
git checkout fix-rename-deadlock
git pull --rebase upstream master
git push -f origin fix-rename-deadlock

@behlendorf
Copy link
Contributor

@seletskiy I like it. This is a nice clean way to sort out the lock inversion. As for the slightly out-of-sync renaming this was already effectively asynchronous so it's not a problem. Thanks for solving this one for us.

This commit should prevent deadlock on dp_config_rwlock when running
`zfs rename`.

Fix #2652.
Fix #2525.
@seletskiy
Copy link
Contributor Author

I've rebased branch on the latest master and it fails on buildbot, is it supposed to be that way?

KM_PUSHPAGE is removed in favor of kmem_asprintf.

@behlendorf: Glad to help, that was not so difficult, anyway.

@behlendorf
Copy link
Contributor

and it fails on buildbot, is it supposed to be that way?

There are still some known long standing issues (1/2 a dozen perhaps) which are occasionally hit by the buildbot testing. If any one of these things happens it will be reported as a failure but it doesn't always mean the patch introduced the issue. If you do see a failure you'll need to spot check the results and see if it's related. You can also always force update the branch you want to run the tests again.

In this case the failure looks like it happened because one of the builders ran out of free space for the file vdevs in its VM. That's obviously not related to this patch.

@ryao @dweeezil This change looks good to me and I'd like to merge it. Can I add your signed-off-by's to the patch as well?

@ryao
Copy link
Contributor

ryao commented Sep 4, 2014

@behlendorf Sure.

@behlendorf behlendorf added this to the 0.6.4 milestone Sep 4, 2014
@behlendorf behlendorf added the Bug label Sep 4, 2014
@dweeezil
Copy link
Contributor

dweeezil commented Sep 4, 2014

LGTM.

@behlendorf
Copy link
Contributor

Merged as:

2078f21 Fix invalid locking order in rename operation

@behlendorf behlendorf closed this Sep 4, 2014
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.

Deadlock on dp_config_rwlock on simultaneous snap/rename
4 participants