client: don't take extra target inode reference in ll_link #11440
Conversation
|
LGTM |
| @@ -522,9 +522,9 @@ static void fuse_ll_link(fuse_req_t req, fuse_ino_t ino, fuse_ino_t newparent, | |||
| fuse_reply_entry(req, &fe); | |||
| } else { | |||
| fuse_reply_err(req, -r); | |||
| cfuse->iput(in); | |||
There was a problem hiding this comment.
Okay, let's add a comment about what's going on here: namely, that we are only putting the reference in the failure case because we otherwise need the reference for the new parent we created and already have in cache.
|
I asked you to pull this out because I wasn't quite sure about the refcount manipulation going on (which I am happy with now), and to address your question. As I understand things, and as you note, we generally take a reference when returning a pointer to the caller, and expect the caller to return that reference to us. If you look at a ceph-fuse debug log, you'll see that we generally get an "ll_forget" line ll_* function invocation — that's fuse invoking its "forget" function. I've never been an expert on the interface but the API seems to be that it expects you to have allocated it a reference on every API call returning the inode, and it will drop them whenever it doesn't need them. I guess it expects the same from ll_link — the FUSE API calls don't really look any different, and in this case it really does have an extra reference now that there's a new dentry pointing at the inode! |
|
Got it. Thanks for the explanation. Just pushed a SQUASH patch that adds a comment to fuse_ll_link. Look ok? |
|
LGTM. Can you squash it down? |
For the life of me, I can't figure out where this reference is ever put. We usually take a reference like this when there is an Inode ** parameter that we'll return to the caller. ll_link doesn't have one of those, so as best I can tell this reference is just leaked. Zheng however says that FUSE will eventually put this reference via ll_forget. I still don't quite get how that works, but the other callers clearly do not handle this correctly. Change the code to only make the fuse code take this extra reference. Signed-off-by: Jeff Layton <jlayton@redhat.com>
jtlayton
force-pushed
the
wip-jlayton-linkref
branch
2 times, most recently
from
9de9110
to
23956de
Compare
|
Done. Let me know if/when I should merge. Thanks! |
|
This commit was already tested when it was on the other PR (http://pulpito.ceph.com/jspray-2016-10-14_08:45:33-fs-wip-jcsp-testing-20161012---basic-mira) |
@gregsfortytwo asked that I break out this patch separately from the other PR I have. He also mentioned that he thought it didn't look quite right. Greg, can you elaborate on what you see as wrong here?
----------------------8<---------------------------------
For the life of me, I can't figure out where this reference is ever put.
We usually take a reference like this when there is an Inode **
parameter that we'll return to the caller. ll_link doesn't have one of
those, so as best I can tell this reference is just leaked.
Zheng however says that FUSE will eventually put this reference via
ll_forget. I still don't quite get how that works, but the other callers
clearly do not handle this correctly. Change the code to only make the
fuse code take this extra reference.
Signed-off-by: Jeff Layton jlayton@redhat.com