rados: Make "operation" code comply to pointer passing rules #597

ansiwen · 2021-10-11T14:22:14Z

In #581 we discovered, that we are actually violating the pointer passing rules in some places. For the write buffer of the writeSteps this change uses a PtrGuard, while the return parameters of GetOmapSteps are allocated in C memory.

Related: #584

phlogistonjohn

My only real specific request is to choose a name that's clear for the current "Poke."

I also wonder if we're going to make a direct call to pointer guard from library code if we should have a bit of "soak time" by merging it after we release rather than immediately before release.

Most everything else seems ok to me.

phlogistonjohn · 2021-10-11T14:48:58Z

internal/cutil/ptrguard.go

+// Poke stores the pinned Go pointer in C memory at cPtr
+func (v *PtrGuard) Poke(cPtr CPtr) *PtrGuard {


Suggested change

// Poke stores the pinned Go pointer in C memory at cPtr

func (v *PtrGuard) Poke(cPtr CPtr) *PtrGuard {

// Store the pinned Go pointer into the C memory located at cPtr.

func (v *PtrGuard) Store(cPtr CPtr) *PtrGuard {

I think the above naming would be clearer. I'm not sure if Poke is meant to be a reference to Peek/Poke - but even that seems kind of obscure to me.

Yes, that was the idea (Peek/Poke). But Store is also fine for me.

phlogistonjohn · 2021-10-11T14:52:17Z

rados/write_step.go

 	return &writeStep{
 		b:         b,
-		cBuffer:   (*C.char)(unsafe.Pointer(&b[0])),
+		pg:        cutil.NewPtrGuard(bufPtr),


Is this our first hard dependency on ptrgourd? I think that all previous cases were wrapped so that they either copied data or used ptrguard, right?

If so, I'm ok with it in theory but I'm just a tad hesitant to merge it right before we release. Thoughts?

Yes you are right, I forgot about our ptrguard tag. I mean, we test it, but it's true, that it's not used in the "field" (that is, CSI) yet. Let's not merge then for now.

I just noticed this comment. Does that mean PRs that would depend on this fix (e.g. #581) are blocked until after December's release?

No, the PRs can be merged prior to release. We simply need to discuss our options, some of the work can possibly be split up, for example. In fact I'd prefer to merge as much as possible sooner rather than later Unfortunately, things have been slow due to illnesses and time off. Please continue to ping us on these items so that we don't forget to work on them!

phlogistonjohn · 2021-10-18T18:08:45Z

Following up on this: do you prefer to:
a) take this as-is - adding one new hard dependency on prtguard
b) add some wrapping around this change to make prtguard a build-time toggle, like it's other uses
c) enable prtguard generally, including a new hard dependency on prtguard
d) enable prtguard generally, including a build-time toggle for the new usage

Note: I don't think these all need to be done in this pr, just looking for a general idea where you'd like to go with this. I have a preference but I'm not going to tell you (yet). :-)

ansiwen · 2021-10-19T15:49:49Z

@phlogistonjohn Ok, how about this: we switch to ptrguard by default and wait one release cycle, if someone complains. This PR I will split up so that without ptrguard tag the old ("insecure") code is used. I'm not in the mood to build another "safe" shim layer that we will remove after one release again. ;-)

phlogistonjohn · 2021-10-19T17:49:36Z

That sounds OK to me, but I'm a bit unclear on "wait one release cycle, if someone complains". If we enable prtguard by default (soonish) we'll have the remaining october/november period of pre-release "soak time" before the next release in Dec.
Then unless we find an issue it would be enabled by default in the released version. At that point it'd be the default in all released versions unless we get reports [1]. But I don't quite get the "one release cycle" aspect from your comment.

[1] - what we do if we get reports is contingent on the severity of the report and how many releases it's been enabled IMO

ansiwen · 2021-11-09T02:16:55Z

Sorry, I wasn't clear enough. What I meant was: for the next release we will switch to ptrguard by default, but keep the other code. If until the following release no one complains, we can remove the non-ptrguard code.

ansiwen · 2021-11-09T02:59:08Z

Actually, now that I know a lot more about that whole topic, and given that the pinning API has been accepted, I would address the whole thing differently now: we can assume, that the pinning API will land before the GC becomes "moving". So before the API is there, ptrguard doesn't have to do anything. And as soon as the API is there, it can use it. The only use-case for the current implementation would be, that older versions of go-ceph would still work with some future version of go that might have a moving GC.

phlogistonjohn · 2021-11-09T13:37:47Z

Actually, now that I know a lot more about that whole topic, and given that the pinning API has been accepted, I would address the whole thing differently now: we can assume, that the pinning API will land before the GC becomes "moving". So before the API is there, ptrguard doesn't have to do anything. And as soon as the API is there, it can use it. The only use-case for the current implementation would be, that older versions of go-ceph would still work with some future version of go that might have a moving GC.

Sure, I agree that it wouldn't make a lot of sense to worry about moving gc a lot given the above. To make things more "actionable" what should we do over the next couple of weeks? Break this PR up into parts, one for the ptrguard stuff and one for the other changes? I also want to unblock PR #581.

ansiwen · 2021-11-15T15:35:26Z

I removed the PtrGuard related changes and just added a TODO comment for later reference.

Signed-off-by: Sven Anderson <[email protected]>

phlogistonjohn

LGTM Thanks.

For a little extra context: we discussed this PR earlier today and decided that it would not be the place and time to create a hard dependency on ptrguard given that Go's GC is not a moving GC today.

ansiwen added the no-API This PR does not include any changes to the public API of a go-ceph package label Oct 11, 2021

ansiwen requested a review from phlogistonjohn October 11, 2021 14:22

ansiwen force-pushed the pr/ansiwen/fix-pointer-passing branch from 1973cc7 to 141421d Compare October 11, 2021 14:39

phlogistonjohn requested changes Oct 11, 2021

View reviewed changes

ansiwen force-pushed the pr/ansiwen/fix-pointer-passing branch from 141421d to 4162b0a Compare October 11, 2021 15:27

ansiwen force-pushed the pr/ansiwen/fix-pointer-passing branch from 4162b0a to ab60ced Compare November 15, 2021 15:29

ansiwen added 2 commits November 16, 2021 00:35

rados: C-allocate return parameters in GetOmapStep

872e50a

Signed-off-by: Sven Anderson <[email protected]>

rados: add comment about missing pinning in writeStep

8d9865e

Signed-off-by: Sven Anderson <[email protected]>

ansiwen force-pushed the pr/ansiwen/fix-pointer-passing branch from ab60ced to 8d9865e Compare November 15, 2021 15:36

ansiwen requested a review from phlogistonjohn November 15, 2021 15:36

phlogistonjohn approved these changes Nov 15, 2021

View reviewed changes

mergify bot merged commit b5c4989 into master Nov 15, 2021

mergify bot deleted the pr/ansiwen/fix-pointer-passing branch November 15, 2021 21:47

phlogistonjohn mentioned this pull request Nov 15, 2021

rados: implement binding for rados_write_op_cmpext #581

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rados: Make "operation" code comply to pointer passing rules #597

rados: Make "operation" code comply to pointer passing rules #597

ansiwen commented Oct 11, 2021 •

edited

Loading

phlogistonjohn left a comment

phlogistonjohn Oct 11, 2021

ansiwen Oct 11, 2021

phlogistonjohn Oct 11, 2021

ansiwen Oct 11, 2021

gman0 Nov 8, 2021

phlogistonjohn Nov 8, 2021

phlogistonjohn commented Oct 18, 2021

ansiwen commented Oct 19, 2021

phlogistonjohn commented Oct 19, 2021

ansiwen commented Nov 9, 2021

ansiwen commented Nov 9, 2021

phlogistonjohn commented Nov 9, 2021

ansiwen commented Nov 15, 2021

phlogistonjohn left a comment

		// Poke stores the pinned Go pointer in C memory at cPtr
		func (v PtrGuard) Poke(cPtr CPtr) PtrGuard {

rados: Make "operation" code comply to pointer passing rules #597

rados: Make "operation" code comply to pointer passing rules #597

Conversation

ansiwen commented Oct 11, 2021 • edited Loading

phlogistonjohn left a comment

Choose a reason for hiding this comment

phlogistonjohn Oct 11, 2021

Choose a reason for hiding this comment

ansiwen Oct 11, 2021

Choose a reason for hiding this comment

phlogistonjohn Oct 11, 2021

Choose a reason for hiding this comment

ansiwen Oct 11, 2021

Choose a reason for hiding this comment

gman0 Nov 8, 2021

Choose a reason for hiding this comment

phlogistonjohn Nov 8, 2021

Choose a reason for hiding this comment

phlogistonjohn commented Oct 18, 2021

ansiwen commented Oct 19, 2021

phlogistonjohn commented Oct 19, 2021

ansiwen commented Nov 9, 2021

ansiwen commented Nov 9, 2021

phlogistonjohn commented Nov 9, 2021

ansiwen commented Nov 15, 2021

phlogistonjohn left a comment

Choose a reason for hiding this comment

ansiwen commented Oct 11, 2021 •

edited

Loading