-
Notifications
You must be signed in to change notification settings - Fork 257
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
rados: implement binding for rados_write_op_cmpext #581
Conversation
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.
The code generally looks good to me. However, we've recently started using a new api-stability policy were new API calls are put in a "preview" state and protected by a build tag.
Below I've suggested the required doc comment change, but we'll also need the new API and test placed in a file with the build tag "ceph_preview" - I'm open to alternative names but since this is an existing larger functioal area you can default to "write_op_preview.go" and "write_op_preview_test.go" if nothing better comes to mind.
I hope this isn't too much of a hassle. Please do let me know if you have any comments / questions about it.
Sorry, one small thing in addition to the above: please squash the 2nd patch into the first as well as any subsequent patches for the doc comment and file splitting/build tag work. Thanks! |
b0e5562
to
2926a90
Compare
@Mergifyio rebase |
Command
|
2926a90
to
a4c3be6
Compare
@phlogistonjohn regarding the API stability policy you've mentioned: how long does it take for a feature to graduate from preview into GA? |
This isn't really firm yet, but I had previously thought that 2 releases would be sufficient (see: #525 (reply in thread) ) Please do participate in #525 for deeper discussions for the api stability plan too! |
I forgot to mention, if you need to use a preview api in practice it's just a matter of specifying the |
rados/write_op_preview.go
Outdated
// size_t cmp_len, | ||
// uint64_t off, | ||
// int * prval); | ||
func (w *WriteOp) CmpExt(b []byte, offset uint64, prval *int32) { |
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.
Why did you choose an int32
and not just int
?
rados/write_op_preview.go
Outdated
oe.cBuffer, | ||
oe.cDataLen, | ||
oe.cOffset, | ||
(*C.int)(unsafe.Pointer(prval))) |
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.
I think there are several problems luring here:
- a
C.int
is possibly larger than anint32
which will cause the C code to overwrite the memory afterprval
. prval
is not returned immediately, but set, when the step is executed. That means the C code keeps storingprval
after the function returned. Sinceprval
is a Go pointer, the pointer passing rules don't allow that. Soprval
actually must be C allocated.
If I understand the existing code correctly, you are supposed to use &oe.rval
here, which is exactly for this use. @phlogistonjohn can you confirm?
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.
I just saw that writeStep
doesn't have an rval
yet. So your would have to add it, like in the GetOmapStep
. Again, @phlogistonjohn, please confirm.
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.
We have in fact a bug around that in the existing code. I filed it under #584.
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.
Yeah, good catches thanks.
The way I understand it, the rval/prval stuff helps you determine which specific step failed when you have a batch of many steps. Since we use the various *Step types to mediate the actual C batches into Go we should probably take that approach or have a specific step type for CmpExt.
I'll reply to the new issue separately.
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.
Thank you, @ansiwen! I guess you're referring to this?
Go code may pass a Go pointer to C provided the Go memory to which it points does not contain any Go pointers. The C code must preserve this property: it must not store any Go pointers in Go memory, even temporarily.
Please correct me if I'm wrong, but doesn't this restriction (not keeping pointers around after C function returns) only pertain to GC and the possibility of C code accessing memory after it's been free'd? The way I understood it, the struct is stored in r.steps
, which is definitely kept around at least until Operate()
is called (so it won't be hit by GC), after which that memory is never touched by the C code again. Isn't this enough to guarantee that use-after-free won't occur?
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.
Regarding the integer size, is it guaranteed that C's and Go's int
types would have the same size, or that Go's int
is at least as big as C's?
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.
I guess you're referring to this?
Go code may pass a Go pointer to C provided the Go memory to which it points does not contain any Go pointers. The C code must preserve this property: it must not store any Go pointers in Go memory, even temporarily.
No, in this case I'm referring to one paragraph later:
C code may not keep a copy of a Go pointer after the call returns.
and
C code may store Go pointers in C memory, subject to the rule above: it must stop storing the Go pointer when the C function returns.
Please correct me if I'm wrong, but doesn't this restriction (not keeping pointers around after C function returns) only pertain to GC and the possibility of C code accessing memory after it's been free'd? The way I understood it, the struct is stored in
r.steps
, which is definitely kept around at least untilOperate()
is called (so it won't be hit by GC), after which that memory is never touched by the C code again. Isn't this enough to guarantee that use-after-free won't occur?
That is the most obvious case of how pointers can get invalid, but not the only one. The GC is allowed to move objects around, and the current implementation in Go actually does it already in the case of objects on the stack. But also objects on the heap could be moved, although that is not implemented in practice yet. But another implementation of Go or later versions of Go might do that. The problem is, there is no contract that guarantees, that an object will always stay at the same address. The GC keeps track of all pointers in Go memory, so it could adjust them as necessary, but it cannot do so with pointers in C memory, which means they would become dangling.
Regarding the integer size, is it guaranteed that C's and Go's
int
types would have the same size, or that Go'sint
is at least as big as C's?
Actually I don't know, I also would have to look it up. And that's why we should use the type C.int
, which is guaranteed to have the same size as an int in C. IIUC you will remove the prval
from the external API, so it's no problem to use C.int. But in general, if I want to make sure that two types have the same size at compile time, I use this trick:
const _ = -(unsafe.Sizeof(int(0)) - unsafe.Sizeof(C.int(0)))
This always results in a constant ... overflows uintptr
compile error, if the sizes are no equal.
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.
@ansiwen Thank you, that's very insightful.
For checking type sizes, wouldn't it be enough to make sure that sizeof( Go's int ) >= sizeof( C's int )
-- at least in this case?
const _ = unsafe.Sizeof(int(0)) - unsafe.Sizeof(C.int(0))
Because otherwise with hard equality, I'm in fact getting an overflow error on an amd64 CentOS system. Printing these values revealed that on this particular system Go's int
is 8 bytes while C's is 4.
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.
Would something like 4596bd7 work?
4596bd7
to
5a2db33
Compare
In the last commit I've accidentally left in code that was unrelated to this PR. |
Just a friendly reminder that the next go-ceph release is less than a week away (2021-10-12). The following release will be in December. If there's no strong need for this right away that's great, and not a problem. I just want to make sure we're all aware. Thanks! |
@phlogistonjohn thanks for the notice! I have two more PRs in queue (that are hopefully less controversial than this one), but I wasn't aware the deadline is so close. |
86d4ade
to
496dc49
Compare
The CI seems to be failing on this?
|
That's a common "test flake" that is tracked in #574 - I will restart the CI run. |
@Mergifyio rebase |
✅ Branch has been successfully rebased |
496dc49
to
666cdc8
Compare
@Mergifyio rebase |
✅ Branch has been successfully rebased |
666cdc8
to
8329507
Compare
PR #597 has been merged after reducing it's scope a bit. I think you've already incorporated the changes needed to match the fixes in that PR, but I recommend double checking. To pass the CI we need to ensure the new API is documented. I've recently added docs pertaining to that: Please review and feel free to ask any questions or for any assistance as I realize this process is new. |
Hi @gman0 - do you have some time to revisit this in the next couple of weeks? I think it would be disappointing if we couldn't manage to get this PR in shape before the next release. Please let us know if there's any confusion about the process, as there have been recent changes, so we can help! |
Hi @phlogistonjohn thank you for the reminder and apologies for the delay, I'll try to work on this ASAP. I'm just busy with other tasks at the moment. There's actually about 6 more bindings for read and write RADOS ops I'd like to add though... I'm fully aware of December's release, and have doubts all these changes will land in time. |
Totally understood.
File them at whatever pace works for you, we'll try to be responsive and if it doesn't seem like it don't hesitate to "@"-ping us. Since this one has been open a while I just want to give it a fair chance to land in the Dec. release! |
8329507
to
e1d7ac3
Compare
I have two questions:
|
PTAL @phlogistonjohn @ansiwen |
Regarding the CI: I can't see |
You and other contributors like you are expected to run the command and commit the results.
I'll take a look.
I think you hit a test flake / timeout issue. I've restarted the tests. If we see the same problem occur repeatedly on your pr it might be related, but at this time I doubt it, I think you got unlucky. |
WriteOpCmpExtStep seems OK to me. We do want a new step type for each new kind of step. I think your pr is good as-is wrt WriteOpCmpExtStep. If we do find ourselves repeating a lot of code in the future, we can create a private "helper" type that gets embedded in the various AbcXyzStep types. Does that make sense? |
Thanks! Sounds good. I'll run |
@phlogistonjohn sorry but I'm having trouble getting
|
e1d7ac3
to
0b8e223
Compare
By the way, the CI was failing because of gofmt. Apparently I missed adding
|
Lines 243 to 253 in 20da1c6
|
0b8e223
to
0955c0b
Compare
I've run |
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.
LGTM, thanks a bunch for sticking with this.
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.
I have some cosmetic suggestions. Otherwise LGTM.
internal/cutil/aliases.go
Outdated
// IntSize is the size of C.int | ||
IntSize = C.sizeof_int |
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.
You don't need this. The constants in cutil
are for files that don't import C
, but you are only using it in a file that imports C anyway, so you can just use C.sizeof_int
directly there.
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.
Ah ok, I didn't know that.
internal/cutil/aliases.go
Outdated
) | ||
|
||
// Compile-time assertion ensuring that Go's `int` is at least as large as C's. | ||
const _ = unsafe.Sizeof(int(0)) - unsafe.Sizeof(C.int(0)) |
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.
Could also be
const _ = unsafe.Sizeof(int(0)) - unsafe.Sizeof(C.int(0)) | |
const _ = unsafe.Sizeof(int(0)) - C.sizeof_int |
but doesn't matter actuallly.
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.
That's probably nicer. Maybe later you / we could find a better place for these compile-time assertions, or is cutil/aliases.go
ok?
rados/write_op_preview.go
Outdated
} | ||
|
||
func (s *WriteOpCmpExtStep) update() error { | ||
s.Result = (int)(*s.prval) |
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.
s.Result = (int)(*s.prval) | |
s.Result = int(*s.prval) |
0955c0b
to
2b0d7fd
Compare
Pull request has been modified.
This commit implements binding for rados_write_op_cmpext RADOS Write operation. Includes a unit test. Signed-off-by: Robert Vasek <[email protected]>
2b0d7fd
to
9d4b149
Compare
@ansiwen @phlogistonjohn I've addressed the comments, can you please take a look again? |
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.
LGTM, 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.
Thanks a ton for your contribution here.
Thank you both for all the feedback and reviews! |
This PR adds binding for
rados_write_op_cmpext
RADOS Write operation including unit tests.Checklist
Related: ceph/ceph-csi#2148