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.

Sign up for GitHub

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

Jump to bottom

mds: set the correct WRLOCK flag always in wrlock_force() #57085

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

mds: set the correct WRLOCK flag always in wrlock_force() #57085

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/mds/Locker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1832,7 +1832,8 @@ void Locker::wrlock_force(SimpleLock *lock, MutationRef& mut)
dout(7) << "wrlock_force on " << *lock
<< " on " << *lock->get_parent() << dendl;
lock->get_wrlock(true);
mut->emplace_lock(lock, MutationImpl::LockOp::WRLOCK);
auto it = mut->emplace_lock(lock, MutationImpl::LockOp::WRLOCK);
it->flags |= MutationImpl::LockOp::WRLOCK; // may already remote_wrlocked
Copy link
Member

Choose a reason for hiding this comment

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

I was looking at Server::rdlock_two_paths_xlock_destdn and found it strange that this method is grabbing a remote_wrlock at all on an inode the MDS may be auth for. It's checking if the directory auth is different (as in the case in the bug) but then grabs a remote_wrlock on the inode's nestlock -- but it is auth for the inode. The check seems odd but maybe that's just how it's supposed to work; it's been that way for a while. I don't get it though.

Copy link
Member Author

@lxbsz lxbsz Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

The directory is fragmented and the fragmented CDirs' auths could be different with the CInode's. In Server::rdlock_two_paths_xlock_destdn() it will try to grab remote_wrlock for the CDir's in the remote auth MDS. While the remote MDS isn't the auth of the CInode.

While the local wrlock is for the CInode's auth in local MDS.

Copy link
Member

Choose a reason for hiding this comment

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

The directory is fragmented and the fragmented CDirs' auths could be different with the CInode's.

Correct.

In Server::rdlock_two_paths_xlock_destdn() it will try to grab remote_wrlock for the CDir's in the remote auth MDS. While the remote MDS isn't the auth of the CInode.

That's the the thing though: a CDir has no locks. Those "remote wrlocks" are actually for the inode which the MDS may be auth for. So why would it request a remote_wrlock on a lock with a parent (CInode) that it is auth for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, this is beyond my knowledge about this. What I get from the code before is that the remote_wrlock will always in the CInode's auth. I need to check the history of the CDir case to get why.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was first introduced by:

commit d72bdab7d7c74148cbb119a0e7c1eb14a0432d46
Author: Sage Weil <sweil@redhat.com>
Date:   Fri Jul 8 09:32:04 2011 -0700

    mds: take a remote_wrlock on srcdir for cross-mds rename
    
    This ensures that we hold a wrlock on the srcdn auth when the slave
    makes it's changes to the src directory, and prevents us from corrupting
    the scatterlock state.
    
    Signed-off-by: Sage Weil <sage.weil@dreamhost.com>

And by going through the code the wrlock is not like the xlock, which need to do it in the CInode's auth always. While the wrlock is based on the CDir's auths instead.

Copy link
Member

Choose a reason for hiding this comment

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

Very good. Please update the code with a comment explanation Xiubo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

}

bool Locker::wrlock_try(SimpleLock *lock, const MutationRef& mut, client_t client)
Expand Down Expand Up @@ -2054,7 +2055,8 @@ bool Locker::xlock_start(SimpleLock *lock, const MDRequestRef& mut)
in && in->issued_caps_need_gather(lock))) { // xlocker does not hold shared cap
lock->set_state(LOCK_XLOCK);
lock->get_xlock(mut, client);
mut->emplace_lock(lock, MutationImpl::LockOp::XLOCK);
auto it = mut->emplace_lock(lock, MutationImpl::LockOp::XLOCK);
ceph_assert(it->is_xlock());
mut->finish_locking(lock);
return true;
}
Expand Down Expand Up @@ -5138,7 +5140,8 @@ void Locker::scatter_writebehind(ScatterLock *lock)

// forcefully take a wrlock
lock->get_wrlock(true);
mut->emplace_lock(lock, MutationImpl::LockOp::WRLOCK);
auto it = mut->emplace_lock(lock, MutationImpl::LockOp::WRLOCK);
ceph_assert(it->is_wrlock());
leonid-s-usov marked this conversation as resolved.
Show resolved Hide resolved

in->pre_cow_old_inode(); // avoid cow mayhem

Expand Down Expand Up @@ -5529,7 +5532,8 @@ bool Locker::local_xlock_start(LocalLockC *lock, const MDRequestRef& mut)
}

lock->get_xlock(mut, mut->get_client());
mut->emplace_lock(lock, MutationImpl::LockOp::XLOCK);
auto it = mut->emplace_lock(lock, MutationImpl::LockOp::XLOCK);
ceph_assert(it->is_xlock());
return true;
}

Expand Down