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

curvefs/mds: fix deadlock in UpdateClientAliveTime #2311

Closed
wants to merge 1 commit into from

Conversation

surpassly
Copy link

What problem does this PR solve?

Fix deadlock caused by UmontFs and UpdateClientAliveTime

Issue Number: #2308

Problem Summary: A deadlock happens when two bthread block each other.

What is changed and how it works?

What's Changed: Change the functions UpdateClientAliveTime and RefreshSession

How it Works: Make sure that the program acquires NameLock before WriteLock

Side effects(Breaking backward compatibility? Performance regression?): Unknown

Check List

  • Relevant documentation/comments is changed or added
  • I acknowledge that all my contributions will be made under the project's license

@Cyber-SiKu
Copy link
Contributor

cicheck

@surpassly
Copy link
Author

cicheck

@wuhongsong wuhongsong requested a review from SeanHai March 15, 2023 11:51
@SeanHai
Copy link
Contributor

SeanHai commented Mar 15, 2023

  1. There is no NameLock acquires in function 'AddMountPoint', so now there is no deadlock in the code, but NameLock really need added in.
  2. If deal with AddMountPoint in RefreshSession, the third param 'addMountPoint' of functon 'UpdateClientAliveTime' is needn't.
  3. The code you fixed in 'RefreshSession' still have some issues, if a UmountFs request get namelock and wait for WriteLock and another refresh session request get ReadLock here,then UmountFs request finished and this request will add mountpoint to the file which have umounted.

bool found;
{
ReadLockGuard rlock(recorderMutex_);
auto iter = mpTimeRecorder_.find(mountpath);
found = (iter == mpTimeRecorder_.end());
}
if (!found) {
// client hang timeout and recover later
// need add mountpoint to fsInfo
AddMountPoint(request->mountpoint(), request->fsname());
} else {
// update this client's alive time
UpdateClientAliveTime(request->mountpoint(), request->fsname(), false);
}

@surpassly
Copy link
Author

  1. There is no NameLock acquires in function 'AddMountPoint', so now there is no deadlock in the code, but NameLock really need added in.
  2. If deal with AddMountPoint in RefreshSession, the third param 'addMountPoint' of functon 'UpdateClientAliveTime' is needn't.
  3. The code you fixed in 'RefreshSession' still have some issues, if a UmountFs request get namelock and wait for WriteLock and another refresh session request get ReadLock here,then UmountFs request finished and this request will add mountpoint to the file which have umounted.

bool found; { ReadLockGuard rlock(recorderMutex_); auto iter = mpTimeRecorder_.find(mountpath); found = (iter == mpTimeRecorder_.end()); } if (!found) { // client hang timeout and recover later // need add mountpoint to fsInfo AddMountPoint(request->mountpoint(), request->fsname()); } else { // update this client's alive time UpdateClientAliveTime(request->mountpoint(), request->fsname(), false); }

I see, I closed this PR for now, maybe revisit it for future improvements.

@surpassly surpassly closed this Mar 15, 2023
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