Repository: ceph/go-ceph
Pull request #441 (2021-01-18 23:26:13)

cutil: use SyncBuffer for Iovec
In order to be able to switch on and off `PtrGuard` usage, `SyncBuffer`
implements a typical usage pattern of `PtrGuard`, that is a data buffer
referenced from inside a C allocated struct, in two ways:

1. using a `PtrGuard` to directly store a pointer to the Go buffer in C memory, or
2. allocating a C buffer of same size and syncing data back with `C.memcpy()`.  The implementation can be chosen with the `ptrguard` build tag.

The main motivation for `PtrGuard` was read and write buffers as they
are used in `Iovec`.  This change uses `SyncBuffer` for the `Iovec`
implementation, so that the no-copy PtrGuard implementation can be
enabled with the `ptrguard` build tag.

The change also adds a test run with `PtrGuard` enabled, so we make sure both implementations stay operable.
    
Spin-Off from #419 
Depends on: #435

## Checklist
- [X] Added tests for features and functional changes
- [X] Public functions and types are documented
- [X] Standard formatting is applied to Go code


ansiwen: I want to add, that while implementing this, the CBytes version that is copying data back and forth and is supposed to be "safer", was clearly more fragile and needed more care.
ansiwen: Setting do-not-merge label, until dependencies got merged.
mergify[bot]: This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏
ansiwen: @phlogistonjohn re your comment in [#431](https://github.com/ceph/go-ceph/pull/431#discussion_r559606696) to change to an `(v Iovec) Sync() [][]byte` signature: first I liked that idea and implemented it, but while doing that I realized: it's against the idea of a synchronized buffer and the in-place-fill semantics of `cephfs.Preadv()`, and would rather move it to a buffer converter. To fullfil the semantic of `Preadv`, `sync()` would have to return the identical slices, and `Preadv` would depend on that behavior. In this case I think it's saver to make that "return-parameter" semantic explicit, and not return a identical data value. Also remember that this is just a shim layer for a zero-copy implementation like PtrGuard, and that the goal is to remove the "Sync" entirely, because it's a NOP in the zero-copy case. Incorporating the data as a return value here would would be counter productive because we can't remove the call later. 
mergify[bot]: This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏
mergify[bot]: This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏
ansiwen: I guess we should try to merge this too, together with PtrGuards, so that we have the memcpy version, and can finally enable the "gcodebug=2" flag.
dpulls[bot]: :tada: All dependencies have been resolved !
phlogistonjohn: It seems like the normal CI jobs didn't run (yet)?
@ansiwen can we rebase and see if that kicks off the CI test jobs?
ansiwen: > It seems like the normal CI jobs didn't run (yet)?
> @ansiwen can we rebase and see if that kicks off the CI test jobs?

No need to rebase, just deleting the ansiwen-pr-ptrguard branch rebased this on master automatically.

Patch:
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 8da1bc6..227708a 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -37,6 +37,8 @@ jobs:
     - uses: actions/checkout@v2
     - name: Run test container
       run: make test-container "CEPH_VERSION=${{ matrix.ceph_version }}" "RESULTS_DIR=$PWD/_results"
+    - name: Run ptrguard test container
+      run: make test-container "USE_PTRGUARD=true" "CEPH_VERSION=${{ matrix.ceph_version }}" "RESULTS_DIR=$PWD/_results"
     - name: Archive coverage results
       uses: actions/upload-artifact@v2
       with:
diff --git a/Makefile b/Makefile
index 9f0affa..6932633 100644
--- a/Makefile
+++ b/Makefile
@@ -7,6 +7,7 @@ CEPH_VERSION := nautilus
 RESULTS_DIR :=
 CHECK_GOFMT_FLAGS := -e -s -l
 IMPLEMENTS_OPTS :=
+BUILD_TAGS := $(CEPH_VERSION)
 
 ifeq ($(CONTAINER_CMD),)
 	CONTAINER_CMD:=$(shell docker version >/dev/null 2>&1 && echo docker)
@@ -21,6 +22,11 @@ BUILDFILE=.build.$(CEPH_VERSION)
 # the name of the image plus ceph version as tag
 CI_IMAGE_TAG=$(CI_IMAGE_NAME):$(CEPH_VERSION)
 
+ifneq ($(USE_PTRGUARD),)
+	CONTAINER_OPTS += -e USE_PTRGUARD=true
+	BUILD_TAGS := $(BUILD_TAGS),ptrguard
+endif
+
 SELINUX := $(shell getenforce 2>/dev/null)
 ifeq ($(SELINUX),Enforcing)
 	VOLUME_FLAGS = :z
@@ -31,11 +37,11 @@ ifdef RESULTS_DIR
 endif
 
 build:
-	go build -v -tags $(CEPH_VERSION) $(shell go list ./... | grep -v /contrib)
+	go build -v -tags $(BUILD_TAGS) $(shell go list ./... | grep -v /contrib)
 fmt:
 	go fmt ./...
 test:
-	go test -v -tags $(CEPH_VERSION) ./...
+	go test -v -tags $(BUILD_TAGS) ./...
 
 .PHONY: test-docker test-container
 test-docker: test-container
@@ -78,7 +84,7 @@ test-binaries: \
 test-bins: test-binaries
 
 %.test: % force_go_build
-	go test -c -tags $(CEPH_VERSION) ./$<
+	go test -c -tags $(BUILD_TAGS) ./$<
 
 implements:
 	go build -o implements ./contrib/implements
diff --git a/cephfs/file.go b/cephfs/file.go
index 83c49b8..2b5c74e 100644
--- a/cephfs/file.go
+++ b/cephfs/file.go
@@ -158,6 +158,7 @@ func (f *File) Preadv(data [][]byte, offset int64) (int, error) {
 	case ret == 0:
 		return 0, io.EOF
 	}
+	iov.Sync()
 	return int(ret), nil
 }
 
diff --git a/entrypoint.sh b/entrypoint.sh
index b9b42c1..82b5e4a 100755
--- a/entrypoint.sh
+++ b/entrypoint.sh
@@ -94,7 +94,15 @@ while true ; do
 done
 
 if [ -n "${CEPH_VERSION}" ]; then
-    BUILD_TAGS="-tags ${CEPH_VERSION}"
+    BUILD_TAGS="${CEPH_VERSION}"
+fi
+
+if [ -n "${USE_PTRGUARD}" ]; then
+    BUILD_TAGS+=",ptrguard"
+fi
+
+if [ -n "${BUILD_TAGS}" ]; then
+    BUILD_TAGS="-tags ${BUILD_TAGS}"
 fi
 
 show() {
diff --git a/internal/cutil/aliases.go b/internal/cutil/aliases.go
index 7717fd9..a9af61a 100644
--- a/internal/cutil/aliases.go
+++ b/internal/cutil/aliases.go
@@ -2,6 +2,7 @@ package cutil
 
 /*
 #include <stdlib.h>
+#include <string.h>
 typedef void* voidptr;
 */
 import "C"
@@ -10,11 +11,16 @@ import (
 	"unsafe"
 )
 
-// PtrSize is the size of a pointer
-const PtrSize = C.sizeof_voidptr
+const (
+	// MaxIdx is the maximum index on 32 bit systems
+	MaxIdx = 1<<31 - 1 // 2GB, max int32 value, should be safe
 
-// SizeTSize is the size of C.size_t
-const SizeTSize = C.sizeof_size_t
+	// PtrSize is the size of a pointer
+	PtrSize = C.sizeof_voidptr
+
+	// SizeTSize is the size of C.size_t
+	SizeTSize = C.sizeof_size_t
+)
 
 // SizeT wraps size_t from C.
 type SizeT C.size_t
@@ -43,3 +49,14 @@ func Malloc(s SizeT) CPtr { return CPtr(C.malloc(C.size_t(s))) }
 
 // Free is C.free
 func Free(p CPtr) { C.free(unsafe.Pointer(p)) }
+
+// CString is C.CString
+func CString(s string) CharPtr { return CharPtr((C.CString(s))) }
+
+// CBytes is C.CBytes
+func CBytes(b []byte) CPtr { return CPtr(C.CBytes(b)) }
+
+// Memcpy is C.memcpy
+func Memcpy(dst, src CPtr, n SizeT) {
+	C.memcpy(unsafe.Pointer(dst), unsafe.Pointer(src), C.size_t(n))
+}
diff --git a/internal/cutil/iovec.go b/internal/cutil/iovec.go
index d89de06..a711369 100644
--- a/internal/cutil/iovec.go
+++ b/internal/cutil/iovec.go
@@ -5,74 +5,56 @@ package cutil
 #include <sys/uio.h>
 */
 import "C"
-
 import (
 	"unsafe"
 )
 
-var iovecSize uintptr
-
-// StructIovecPtr is an unsafe pointer wrapping C's `*struct iovec`.
-type StructIovecPtr unsafe.Pointer
-
-// Iovec helps manage struct iovec arrays needed by some C functions.
+// Iovec is a slice of iovec structs. Might have allocated C memory, so it must
+// be freed with the Free() method.
 type Iovec struct {
-	// cvec represents an array of struct iovec C memory
-	cvec unsafe.Pointer
-	// length of the array (in elements)
-	length int
+	iovec []C.struct_iovec
+	sbs   []*SyncBuffer
 }
 
-// NewIovec creates an Iovec, and underlying C memory, of the specified size.
-func NewIovec(l int) *Iovec {
-	r := &Iovec{
-		cvec:   C.malloc(C.size_t(l) * C.size_t(iovecSize)),
-		length: l,
+const iovecSize = C.sizeof_struct_iovec
+
+// ByteSlicesToIovec creates an Iovec and links it to Go buffers in data.
+func ByteSlicesToIovec(data [][]byte) (v Iovec) {
+	n := len(data)
+	iovecMem := C.malloc(iovecSize * C.size_t(n))
+	v.iovec = (*[MaxIdx]C.struct_iovec)(iovecMem)[:n:n]
+	for i, b := range data {
+		sb := NewSyncBuffer(CPtr(&v.iovec[i].iov_base), b)
+		v.sbs = append(v.sbs, sb)
+		v.iovec[i].iov_len = C.size_t(len(b))
 	}
-	return r
+	return
 }
 
-// ByteSlicesToIovec takes a slice of byte slices and returns a new iovec that
-// maps the slice data to struct iovec entries.
-func ByteSlicesToIovec(data [][]byte) *Iovec {
-	iov := NewIovec(len(data))
-	for i := range data {
-		iov.Set(i, data[i])
+// Sync makes sure the slices contain the same as the C buffers
+func (v *Iovec) Sync() {
+	for _, sb := range v.sbs {
+		sb.Sync()
 	}
-	return iov
 }
 
-// Pointer returns a StructIovecPtr that represents the C memory of the
-// underlying array.
-func (v *Iovec) Pointer() StructIovecPtr {
-	return StructIovecPtr(unsafe.Pointer(v.cvec))
+// Pointer returns a pointer to the iovec
+func (v *Iovec) Pointer() unsafe.Pointer {
+	return unsafe.Pointer(&v.iovec[0])
 }
 
-// Len returns the number of entries in the Iovec.
+// Len returns a pointer to the iovec
 func (v *Iovec) Len() int {
-	return v.length
+	return len(v.iovec)
 }
 
 // Free the C memory in the Iovec.
 func (v *Iovec) Free() {
-	if v.cvec != nil {
-		C.free(v.cvec)
-		v.cvec = nil
-		v.length = 0
+	for _, sb := range v.sbs {
+		sb.Release()
 	}
-}
-
-// Set will map the memory of the given byte slice to the iovec at the
-// specified position.
-func (v *Iovec) Set(i int, buf []byte) {
-	offset := uintptr(i) * iovecSize
-	iov := (*C.struct_iovec)(unsafe.Pointer(
-		uintptr(unsafe.Pointer(v.cvec)) + offset))
-	iov.iov_base = unsafe.Pointer(&buf[0])
-	iov.iov_len = C.size_t(len(buf))
-}
-
-func init() {
-	var iovec C.struct_iovec
-	iovecSize = unsafe.Sizeof(iovec)
+	if len(v.iovec) != 0 {
+		C.free(unsafe.Pointer(&v.iovec[0]))
+	}
+	v.iovec = nil
 }
diff --git a/internal/cutil/sync_buffer.go b/internal/cutil/sync_buffer.go
new file mode 100644
index 0000000..9fdc38e
--- /dev/null
+++ b/internal/cutil/sync_buffer.go
@@ -0,0 +1,28 @@
+// +build ptrguard
+
+package cutil
+
+import (
+	"unsafe"
+)
+
+// SyncBuffer is a C buffer connected to a data slice
+type SyncBuffer struct {
+	pg *PtrGuard
+}
+
+// NewSyncBuffer creates a C buffer from a data slice and stores it at CPtr
+func NewSyncBuffer(cPtr CPtr, data []byte) *SyncBuffer {
+	var v SyncBuffer
+	v.pg = NewPtrGuard(cPtr, unsafe.Pointer(&data[0]))
+	return &v
+}
+
+// Release releases the C buffer and nulls its stored pointer
+func (v *SyncBuffer) Release() {
+	v.pg.Release()
+}
+
+// Sync asserts that changes in the C buffer are available in the data
+// slice
+func (v *SyncBuffer) Sync() {}
diff --git a/internal/cutil/sync_buffer_memcpy.go b/internal/cutil/sync_buffer_memcpy.go
new file mode 100644
index 0000000..137ae62
--- /dev/null
+++ b/internal/cutil/sync_buffer_memcpy.go
@@ -0,0 +1,37 @@
+// +build !ptrguard
+
+package cutil
+
+// SyncBuffer is a C buffer connected to a data slice
+type SyncBuffer struct {
+	data []byte
+	cPtr *CPtr
+}
+
+// NewSyncBuffer creates a C buffer from a data slice and stores it at CPtr
+func NewSyncBuffer(cPtr CPtr, data []byte) *SyncBuffer {
+	var v SyncBuffer
+	v.data = data
+	v.cPtr = (*CPtr)(cPtr)
+	*v.cPtr = CBytes(data)
+	return &v
+}
+
+// Release releases the C buffer and nulls its stored pointer
+func (v *SyncBuffer) Release() {
+	if v.cPtr != nil {
+		Free(*v.cPtr)
+		*v.cPtr = nil
+		v.cPtr = nil
+	}
+	v.data = nil
+}
+
+// Sync asserts that changes in the C buffer are available in the data
+// slice
+func (v *SyncBuffer) Sync() {
+	if v.cPtr == nil {
+		return
+	}
+	Memcpy(CPtr(&v.data[0]), CPtr(*v.cPtr), SizeT(len(v.data)))
+}


Test patch:
diff --git a/internal/cutil/iovec_test.go b/internal/cutil/iovec_test.go
index 9978877..0534131 100644
--- a/internal/cutil/iovec_test.go
+++ b/internal/cutil/iovec_test.go
@@ -5,53 +5,54 @@ import (
 	"unsafe"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestIovec(t *testing.T) {
-	t.Run("newAndFree", func(t *testing.T) {
-		iov := NewIovec(3)
-		iov.Free()
-	})
-	t.Run("setBufs", func(t *testing.T) {
-		b1 := []byte("foo")
-		b2 := []byte("barbar")
-		b3 := []byte("bazbazbaz")
-		iov := NewIovec(3)
-		iov.Set(0, b1)
-		iov.Set(1, b2)
-		iov.Set(2, b3)
-		iov.Free()
-		// free also unsets internal values
-		assert.Equal(t, unsafe.Pointer(nil), iov.cvec)
-		assert.Equal(t, 0, iov.length)
-	})
-	t.Run("testGetters", func(t *testing.T) {
-		b1 := []byte("foo")
-		b2 := []byte("barbar")
-		b3 := []byte("bazbazbaz")
-		b4 := []byte("zonk")
-		iov := NewIovec(4)
-		defer iov.Free()
-		iov.Set(0, b1)
-		iov.Set(1, b2)
-		iov.Set(2, b3)
-		iov.Set(3, b4)
-
-		assert.NotNil(t, iov.Pointer())
-		assert.Equal(t, 4, iov.Len())
-	})
-}
-
-func TestByteSlicesToIovec(t *testing.T) {
-	d := [][]byte{
-		[]byte("ramekin"),
-		[]byte("shuffleboard"),
-		[]byte("tranche"),
-		[]byte("phycobilisomes"),
+	strs := []string{
+		"foo",
+		"barbar",
+		"bazbazbaz",
 	}
-	iov := ByteSlicesToIovec(d)
-	defer iov.Free()
-
-	assert.NotNil(t, iov.Pointer())
-	assert.Equal(t, 4, iov.Len())
+	var data [][]byte
+	for _, s := range strs {
+		data = append(data, []byte(s))
+	}
+	iovec := ByteSlicesToIovec(data)
+	p := iovec.Pointer()
+	assert.NotNil(t, p)
+	assert.Equal(t, iovec.Len(), len(data))
+	assert.Equal(t, p, unsafe.Pointer(&iovec.iovec[0]))
+	for i, iov := range iovec.iovec {
+		require.NotNil(t, iov.iov_base)
+		assert.Equal(t, int(iov.iov_len), len(data[i]))
+		assert.Equal(t, data[i], (*[MaxIdx]byte)(iov.iov_base)[:iov.iov_len:iov.iov_len])
+	}
+	// data didn't change
+	for i, b := range data {
+		assert.Equal(t, string(b), strs[i])
+	}
+	// clear iovec buffers
+	for _, iov := range iovec.iovec {
+		b := (*[MaxIdx]byte)(iov.iov_base)[:iov.iov_len:iov.iov_len]
+		for i := range b {
+			b[i] = 0
+		}
+	}
+	iovec.Sync()
+	// data must be cleared
+	for _, b := range data {
+		for i := range b {
+			assert.Zero(t, b[i])
+		}
+	}
+	iovec.Free()
+	for _, iov := range iovec.iovec {
+		assert.Equal(t, iov.iov_base, unsafe.Pointer(nil))
+		assert.Zero(t, iov.iov_len)
+	}
+	iovec.Free()
+	iovec.Sync()
+	iovec.Sync()
+	iovec.Free()
 }