Bug #21419
closedclient: is ceph_caps_for_mode correct for r/o opens?
0%
Description
Greg was reviewing my delegation patches and noticed that we really aren't getting enough caps for read delegations. The initial version of that code uses ceph_caps_for_mode, which does this:
if (mode & CEPH_FILE_MODE_RD)
caps |= CEPH_CAP_FILE_SHARED |
CEPH_CAP_FILE_RD | CEPH_CAP_FILE_CACHE;
if (mode & CEPH_FILE_MODE_WR)
caps |= CEPH_CAP_FILE_EXCL |
CEPH_CAP_FILE_WR | CEPH_CAP_FILE_BUFFER |
CEPH_CAP_AUTH_SHARED | CEPH_CAP_AUTH_EXCL |
CEPH_CAP_XATTR_SHARED | CEPH_CAP_XATTR_EXCL;
_open will allow the open to be satisfied from the cache if you have all of the caps for the mode.
Now, suppose we open a file r/o (maybe as one user), someone changes the mode on another host (such that a subsequent r/o open should fail), and then we issue another open against it. I think that will be broken in the current code.
Is seems like r/o opens should request As (and probably Xs), and we shouldn't satisfy opens from the cache unless we have As caps (+Xs if there are ACLs).
I'll see if I can come up with a testcase.
Updated by Patrick Donnelly over 6 years ago
- Project changed from Ceph to CephFS
- Category deleted (
129) - Source set to Development
- Component(FS) Client added
Updated by Greg Farnum over 6 years ago
- Priority changed from Low to High
Okay, so I think this is okay in many instances because Client::open() uses path_walk(), and that correctly invokes the may_lookup() and other permission-checking code. And that code does seem to use the correct _getattr stuff to refresh caps as needed.
But that doesn't directly help any of our code which goes in sideways (like all the libraries). And it's definitely a problem if that's not working right!
Updated by Zheng Yan over 6 years ago
which sideway? client::open() call the may_open() too.
Updated by Jeff Layton over 6 years ago
- Priority changed from High to Normal
With some git archaeology, I was able to dig up this:
commit ff2c0c58640bf9bbc21a2289ea1fade410844a61
Author: Sage Weil <sage@newdream.net>
Date: Fri Apr 3 21:03:32 2009 -0700
mds: do not want LINK caps for any open file; no non-FILE caps when readonly
When readonly, we don't want anything other than FILE caps. (The mds
will of course give us more when it can.)
For WR and RDWR, we still want AUTH and XATTR caps, for now, since there
is some possibility that the client will want to change them.
Prior to that patch, the caps requested were more or less what I was suggesting.before. The justification is not 100% clear to me in this patch description though.
Updated by Jeff Layton over 6 years ago
Zheng Yan wrote:
which sideway? client::open() call the may_open() too.
Yes, I think you're right. We'll call may_open in most of these cases, unless someone has changed fuse_default_permissions and/or client_permissions to non-default values [1]. Still, I'd like to get a better understanding of why we request the caps we do in these cases.
ceph_caps_for_mode has more or less been the same since 2009. That may be because it's correct and there is no need to change it, but I think this is something we need to have clearly documented. The caps it chooses seem a bit arbitrary at first glance.
[1]: btw, maybe we need to deprecate one of those options? Don't they more or less do the same thing?
Updated by Greg Farnum over 6 years ago
Well, these are just the sets of caps the client requests when it's opening a file, right?
So I'm pretty sure it was Sage's guess about a good heuristic for what clients would want without slowing down others too much. That may be wrong, and the measure of goodness may have changed now that we're doing client-side permission checks, though (and other than that, there's not any reason I can think of that the requested caps would change in the last 8 years. They are of design and necessity pretty stable.)
As for why these caps...well, they're the caps you need to do stuff efficiently. Fscr lets you do readahead (although you don't get the dates and stuff); AxsXsxFxwb lets you write a file, write xattrs, and change mode etc without going through the mds. They offer control for the operations you are likely to perform when opening a file for read or write.
Updated by Patrick Donnelly over 6 years ago
- Subject changed from is ceph_caps_for_mode correct for r/o opens? to client: is ceph_caps_for_mode correct for r/o opens?
- Status changed from New to Need More Info
Jeff, any update on this?
Updated by Jeff Layton over 6 years ago
- Priority changed from Normal to Low
No, I've not had time to look at it. For now, I'll just mark this as low priority until I can revisit ir.
Updated by Jeff Layton over 6 years ago
- Status changed from Need More Info to Rejected
Ok, I think you're right. may_open happens at a higher level and we will simply request the caps at that point. False alarm.