Small interface cleanups for struct ceph_statx #11093
Conversation
jtlayton
changed the title
jtlayton
force-pushed
the
wip-jlayton-statx
branch
from
c02df6d
to
1715907
Compare
|
Added another patch to remove some currently-unused fields. We can readd those later if we have need for them. |
| ASSERT_EQ(stx.stx_btime, stx.stx_ctime); | ||
| ASSERT_EQ(stx.stx_btime_ns, stx.stx_ctime_ns); | ||
| ASSERT_TRUE(stx.stx_ctime.tv_sec == stx.stx_btime.tv_sec && | ||
| stx.stx_ctime.tv_nsec == stx.stx_btime.tv_nsec); | ||
| ceph_close(cmount, fd); |
There was a problem hiding this comment.
Do timespecs not have have equality operator you can use instead of calling out the pieces?
There was a problem hiding this comment.
You would think there would be, but no -- there isn't a standard comparison helper function you can call here. That's a long standing gripe with timespecs, fwiw. Certainly we could roll our own, but I don't think it's worth it.
That said, some 3rd party libs will provide routines for handling them. Maybe boost or something does? I'll look into it...
| @@ -11605,6 +11605,9 @@ int Client::ll_create(Inode *parent, const char *name, mode_t mode, | |||
| int flags, struct stat *attr, Inode **outp, Fh **fhp, | |||
| int uid, int gid) | |||
| { | |||
| if (fhp) | |||
| *fhp = NULL; | |||
There was a problem hiding this comment.
If it's necessary we can do this unconditionally and clean up comments about "may be" NULL?
There was a problem hiding this comment.
Yeah, now that you mention it, I don't see any callers that pass a NULL pointer in here. I'll change it to initialize it unconditionally.
|
Hopefully this is at least a little neater. Removing the NULL fhp checks also allows us to remove a bit of cruftiness at the bottom of ll_create. Oh my, but the patch description was bogus before I pushed. I'll fix that up when I squash the patches. Basically what I did was add a timespec_eq helper and use that in all of the tests. |
|
Also, one more patch here. The proposed upstream interface always sets the file type bits in the stx_mode. Let's do the same, which should allow ganesha (and other callers) to skip requesting CEPH_STATX_MODE (and hence the As cap) if all they want is the file type. |
| @@ -6852,6 +6852,7 @@ void Client::fill_statx(Inode *in, unsigned int mask, struct ceph_statx *stx) | |||
| /* These are always considered to be available */ | |||
| stx->stx_dev = in->snapid; | |||
| stx->stx_blksize = MAX(in->layout.stripe_unit, 4096); | |||
| stx->stx_mode = S_IFMT & in->mode; | |||
There was a problem hiding this comment.
Is this supposed to be a logical OR? I don't think we actually want to restrict users to only accessing file type data but I'm not very well-versed in these interfaces.
There was a problem hiding this comment.
No, we want logical AND. David H.'s original statx set says:
(2) st_mode.
The part of this that identifies the file type will always be
available, irrespective of the setting of STATX_MODE. The access
flags and sticky bit are as for class (1).
In our case, the caller needs to check for (or request) CEPH_STATX_MODE if he wants to use the permission bits. If he doesn't have that, then he can still access the type bits, but nothing else in the mode.
In principle, we could just set the full mode unconditionally there and only set CEPH_STATX_MODE flag in stx_mask if we have As caps, but I think it's a little cleaner to avoid setting any of the permission bits if we don't hold As.
There was a problem hiding this comment.
Oh, I misunderstood the context here; looks good.
…= NULL The logic in ll_create relies on *fhp being zeroed out on entry into the function, but that's no longer being done since commit e08210d. Fix this by reinstating that initialization. Luckily, it looks like the two main callers (FUSE and nfs-ganesha) already handle this safely, but the SyntheticClient does not. Move the zeroing of the pointer into ll_create, and allow callers to pass in a pointer to an uninitialized value. With this, we can remove the initialization in the fuse client code as well. Also, we can get rid of some unneeded NULL pointer checks if we don't allow callers to pass in a NULL fhp pointer. As best I can tell, no one does that currently and no comments over ceph_ll_create seem to claim that this should be allowed. Signed-off-by: Jeff Layton <jlayton@redhat.com>
Holdover from the proposed kernel API that doesn't really help anything here. If we ever want to extend this struct, we'll just append to the end and add new want mask bits. Signed-off-by: Jeff Layton <jlayton@redhat.com>
This is just much more convenient for applications that want to use the API. We're not talking to the kernel here, so there's no real reason we need to use fixed length fields. Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
This reverts commit 2115de0. This is unnecessary now that we're using dev_t's in ceph_statx. Signed-off-by: Jeff Layton <jlayton@redhat.com>
We don't provide stx_version and stx_information is currently unused. Signed-off-by: Jeff Layton <jlayton@redhat.com>
The proposed statx interface does this, and this would allow ganesha to get away without requesting CEPH_STATX_MODE when all it wants is enough to instantiate a filehandle. Signed-off-by: Jeff Layton <jlayton@redhat.com>
jtlayton
force-pushed
the
wip-jlayton-statx
branch
from
a893bb4
to
b332eed
Compare
When I originally copied the proposed statx interface for ceph, I think I took it a bit too far and left a lot of the fields as fixed-length when that wasn't really necessary. So, as I mentioned in standup yesterday, this is a set of changes to struct ceph_statx to give the interface a bit more of a userland flavor. The main changes are to keep a dev_t in the structure for the stx_dev and stx_rdev fields, and to convert all of the timestamps to use struct timespec instead of separate fields for the secs and nsecs.
There's also a small bugfix for ll_create, which had a broken caller in the (unused) SyntheticClient code.