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: don't request lookup parent if ino is root #12478
Conversation
Signed-off-by: huanwen ren <ren.huanwen@zte.com.cn>
957b8d4
to
c78896d
Compare
lookup_parent exit(1) = -22 when ino is 1 2016-12-13 19:21:34.696287 7f4a20949480 3 client.3835100 lookup_ino exit(1) = 0 |
@renhwztetecs what was the old behaviour? If it was doing something bad (like crashing) could you open a tracker ticket an reference it from the commit please |
@@ -7973,7 +7973,6 @@ int Client::lookup_parent(Inode *ino, const UserPerm& perms, Inode **parent) | |||
MetaRequest *req = new MetaRequest(CEPH_MDS_OP_LOOKUPPARENT); | |||
filepath path(ino->ino); | |||
req->set_filepath(path); | |||
req->set_inode(ino); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a specific reason to not include the inode? In general we need this and I think the reasons still apply to a lookup_parent — the inode whose parent we're looking up might have migrated and need to be searched out, amongst other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregsfortytwo thanks for your reviews
It's cleanup.
I don't see the transfer from the client to the server side of the INO, will be used in the server handle_client_lookup_ino () function, at the same time, the inode whose parent can use "CInode *in = mdcache->get_inode(ino)" get it from the Server::handle_client_lookup_ino().
do not know I understand the right,if not, please correct me,thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me: inode is only used in build_client_request if the path is unset, so setting both was redundant.
@jcsp |
Signed-off-by: huanwen ren <ren.huanwen@zte.com.cn>
Signed-off-by: huanwen ren <ren.huanwen@zte.com.cn>
c78896d
to
51e4129
Compare
@@ -7973,7 +7973,6 @@ int Client::lookup_parent(Inode *ino, const UserPerm& perms, Inode **parent) | |||
MetaRequest *req = new MetaRequest(CEPH_MDS_OP_LOOKUPPARENT); | |||
filepath path(ino->ino); | |||
req->set_filepath(path); | |||
req->set_inode(ino); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me: inode is only used in build_client_request if the path is unset, so setting both was redundant.
Signed-off-by: huanwen ren ren.huanwen@zte.com.cn