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

client: Fix lookup of "/.." in jewel #12766

Merged
merged 1 commit into from Jan 20, 2017
Merged

Conversation

jtlayton
Copy link
Contributor

@jtlayton jtlayton commented Jan 3, 2017

This should fix a problem for nfs-ganesha when built against libcephfs. Currently with the jewel client, doing a lookup of ".." at the root directory ends up returning -ENOENT. This converts it to consider the parent of the root to be the root itself (similarly to how /.. works in Linux).

Only lightly tested so far but it is building now and I'll run the whole fs suite over it once it's done.

The CEPH_INO_DOTDOT thing is quite strange. Under most OS (Linux
included), the parent of the root is itself. IOW, at the root, '.' and
'..' refer to the same inode.

Change the ceph client to do the same, as this allows users to get
valid stat info for '..', as well as elimnating some special-casing.

Also in several places, we're checking dn_set.empty as an indicator
of being the root. While that is true for the root, it's also true
for unlinked directories.

This patch has treats them the same. An unlinked directory will
be reparented to itself, effectively acting as a root of its own.

Fixes: http://tracker.ceph.com/issues/18408
Signed-off-by: Jeff Layton <jlayton@redhat.com>
(cherry picked from commit 30d4ca0)
@jtlayton jtlayton changed the base branch from master to jewel January 3, 2017 18:07
@jtlayton jtlayton added the cephfs Ceph File System label Jan 3, 2017
@jtlayton
Copy link
Contributor Author

jtlayton commented Jan 3, 2017

Test run here mostly passed with a few failures that look unrelated to this:

http://pulpito.ceph.com/jlayton-2017-01-03_20:28:20-fs-wip-18408-jewel---basic-smithi/

@jtlayton
Copy link
Contributor Author

jtlayton commented Jan 4, 2017

Not sure why the build failed there. Looks like something fell over in jenkins? The patch itself doesn't break anything in my testing.

@alfredodeza
Copy link
Contributor

Jenkins test this please

@jtlayton
Copy link
Contributor Author

jtlayton commented Jan 4, 2017

Assigning @gregsfortytwo and @jcsp to review. This should be harmless in jewel, but let me know if you have any objections.

@ukernel ukernel self-requested a review January 9, 2017 02:09
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This looks fine to me; I don't think we had any other changes this depends on.

@gregsfortytwo gregsfortytwo added this to the jewel milestone Jan 12, 2017
@liewegas liewegas changed the title Fix lookup of "/.." in jewel client: Fix lookup of "/.." in jewel Jan 20, 2017
@smithfarm
Copy link
Contributor

@ukernel This passed a cephfs run here [1] with a few failures that appear unrelated to the PR. OK to merge?

[1] #12766 (comment)

@smithfarm
Copy link
Contributor

oh, I see @ukernel already approved

@smithfarm smithfarm merged commit 72b24f0 into ceph:jewel Jan 20, 2017
@jtlayton jtlayton deleted the wip-18408-jewel branch January 20, 2017 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cephfs Ceph File System
Projects
None yet
5 participants