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

mds: miscellaneous snap fixes #16778

Merged
merged 5 commits into from Sep 7, 2017
Merged

Conversation

ukernel
Copy link
Contributor

@ukernel ukernel commented Aug 3, 2017

No description provided.

@ukernel ukernel added cephfs Ceph File System bug-fix labels Aug 3, 2017
@ukernel ukernel changed the title mds: miscellaneous snap fixes [DNM] mds: miscellaneous snap fixes Aug 4, 2017
@ukernel ukernel force-pushed the wip-mds-snap-misc branch 2 times, most recently from 35670ee to d6d000b Compare August 11, 2017 11:19
@ceph-jenkins
Copy link
Collaborator

submodules for project are unmodified

Copy link

@amitkumar50 amitkumar50 left a comment

Choose a reason for hiding this comment

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

Please see comments inline

CInode *in = head_in;
if (follows > 0) {
in = mdcache->pick_inode_snap(head_in, follows);
// intermediate snap inodes

Choose a reason for hiding this comment

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

@ukernel Thanks for PR
I believe It would be even better if we start comment with capital letter something as:
// Intermediate snap inodes

Also I believe there is no such use of leaving space between // and Intermediate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you check the code, almost all comments start with space

<< " wrlock lock " << *lock << " on " << *oldin << dendl;
if (!in->client_caps.empty()) {
const set<snapid_t>& snaps = in->find_snaprealm()->get_snaps();
// clone caps?

Choose a reason for hiding this comment

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

@ukernel Same as above comments

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
Current mds track both head inodes and snap inodes through unsorted
map. The unsorted map makes finding snap inode that follows a given
snapid difficult. Currnt MDCache::pick_inode_snap() use snap set to
guess snap inode's last. The method isn't reliable because snap set
may change after creating the snap inode. For example:

MDS cows inode[2,head] with snap set[5,6], which results inode[2,6]
and inode[7,head].

Later mds wants to find snap inode that follows snapid 2. But the
snap set become [5], mds can't find snap inode [2,5].

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
previous commit 4b95fbe "mds: properly do null snapflush part2"
isn't complete. It doesn't properly handle case that snap get deleted
before receiving corresponding snapflush.

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
@ukernel ukernel changed the title [DNM] mds: miscellaneous snap fixes mds: miscellaneous snap fixes Aug 16, 2017
These flags tell mds if there is pending capsnap explictly.
Without this explict notification, mds can only conclude if
client has pending capsnap. The method mds use is inefficient
and error-prone.

Fixes: http://tracker.ceph.com/issues/20752
Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
previous commit "mds: track snap inodes through sorted map" makes
MDCache::dump_cache return 1 on success.

Signed-off-by: "Yan, Zheng" <zyan@redhat.com>
@batrick batrick merged commit f519fca into ceph:master Sep 7, 2017
batrick added a commit that referenced this pull request Sep 7, 2017
* refs/remotes/upstream/pull/16778/head:
	mds: fix return value of MDCache::dump_cache
	mds: new cap message flags indicate if there is pending capsnap
	mds: properly do null snapflush part2
	mds: track snap inodes through sorted map
	mds: properly drop wrlock when finishing snapflush

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants