Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 0/8] Enhance the vector library
- Add vector benchmarks - Optimize vector append - Refine vector fields names - Clean up vector usage in crypto module I'm fine with relicensing these changes for for nbdkit. Changes in v2: - Benchmarks do not run by default (Richard) - Add "make bench" target to run the benchmarks v1: https://listman.redhat.com/archives/libguestfs/2021-October/msg00286.html Nir Soffer (8): common/utils: Add vector benchmarks common/utils: Do not run benchmarks by default common/utils/vector.c: Optimize vector append common/utils/vector: Rename `alloc` to `cap` common/utils/vector: Rename `size` to `len` lib/crypto.c: Simplify vector reserve lib/crypto.c: Don't use empty vector lib/crypto.c: Remove unneeded else Makefile.am | 5 ++ README | 4 ++ common/utils/Makefile.am | 5 +- common/utils/bench.h | 72 ++++++++++++++++++++ common/utils/test-vector.c | 58 +++++++++++++++- common/utils/vector.c | 14 +++- common/utils/vector.h | 40 +++++------ copy/file-ops.c | 14 ++-- copy/main.c | 2 +- copy/multi-thread-copying.c | 2 +- copy/nbd-ops.c | 20 +++--- copy/synch-copying.c | 2 +- fuse/nbdfuse.c | 4 +- fuse/operations.c | 16 ++--- generator/states-connect-socket-activation.c | 2 +- generator/states-newstyle-opt-meta-context.c | 8 +-- info/list.c | 8 +-- info/map.c | 12 ++-- info/show.c | 6 +- lib/crypto.c | 18 ++--- lib/handle.c | 4 +- lib/uri.c | 8 +-- 22 files changed, 236 insertions(+), 88 deletions(-) create mode 100644 common/utils/bench.h -- 2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 1/8] common/utils: Add vector benchmarks
The generic vector reallocs on every append. Add benchmarks to measure
the cost with uint32 vector (used for copying extents) and the effect of
reserving space upfront.
The tests show that realloc is pretty efficient, but calling reserve
before the appends speeds the appends up significantly.
$ ./test-vector | grep bench
bench_reserve: 1000000 appends in 0.004159 s
bench_append: 1000000 appends in 0.014897 s
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
common/utils/Makefile.am | 2 +-
common/utils/bench.h | 72 ++++++++++++++++++++++++++++++++++++++
common/utils/test-vector.c | 49 ++++++++++++++++++++++++++
3 files changed, 122 insertions(+), 1 deletion(-)
create mode 100644 common/utils/bench.h
diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am
index b273ada..4730cea 100644
--- a/common/utils/Makefile.am
+++ b/common/utils/Makefile.am
@@ -59,6 +59,6 @@ test_human_size_SOURCES = test-human-size.c human-size.c
human-size.h
test_human_size_CPPFLAGS = -I$(srcdir)
test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
-test_vector_SOURCES = test-vector.c vector.c vector.h
+test_vector_SOURCES = test-vector.c vector.c vector.h bench.h
test_vector_CPPFLAGS = -I$(srcdir)
test_vector_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/utils/bench.h b/common/utils/bench.h
new file mode 100644
index 0000000..496a361
--- /dev/null
+++ b/common/utils/bench.h
@@ -0,0 +1,72 @@
+/* libnbd
+ * Copyright (C) 2021 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#ifndef LIBNBD_BENCH_H
+#define LIBNBD_BENCH_H
+
+#include <sys/time.h>
+
+#define MICROSECONDS 1000000
+
+struct bench {
+ struct timeval start, stop;
+};
+
+static inline void
+bench_start(struct bench *b)
+{
+ gettimeofday (&b->start, NULL);
+}
+
+static inline void
+bench_stop(struct bench *b)
+{
+ gettimeofday (&b->stop, NULL);
+}
+
+static inline double
+bench_sec(struct bench *b)
+{
+ struct timeval dt;
+
+ dt.tv_sec = b->stop.tv_sec - b->start.tv_sec;
+ dt.tv_usec = b->stop.tv_usec - b->start.tv_usec;
+
+ if (dt.tv_usec < 0) {
+ dt.tv_sec -= 1;
+ dt.tv_usec += MICROSECONDS;
+ }
+
+ return ((double)dt.tv_sec * MICROSECONDS + dt.tv_usec) / MICROSECONDS;
+}
+
+#endif /* LIBNBD_BENCH_H */
diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c
index 94b2aeb..28e882f 100644
--- a/common/utils/test-vector.c
+++ b/common/utils/test-vector.c
@@ -38,9 +38,13 @@
#undef NDEBUG /* Keep test strong even for nbdkit built without assertions */
#include <assert.h>
+#include "bench.h"
#include "vector.h"
+#define APPENDS 1000000
+
DEFINE_VECTOR_TYPE(int64_vector, int64_t);
+DEFINE_VECTOR_TYPE(uint32_vector, uint32_t);
DEFINE_VECTOR_TYPE(string_vector, char *);
static int
@@ -113,10 +117,55 @@ test_string_vector (void)
free (v.ptr);
}
+static void
+bench_reserve (void)
+{
+ uint32_vector v = empty_vector;
+ struct bench b;
+ int64_t usec;
+
+ bench_start(&b);
+
+ uint32_vector_reserve(&v, APPENDS);
+
+ for (uint32_t i = 0; i < APPENDS; i++) {
+ uint32_vector_append (&v, i);
+ }
+
+ bench_stop(&b);
+
+ assert (v.ptr[APPENDS - 1] == APPENDS - 1);
+ free (v.ptr);
+
+ printf ("bench_reserve: %d appends in %.6f s\n", APPENDS, bench_sec
(&b));
+}
+
+static void
+bench_append (void)
+{
+ uint32_vector v = empty_vector;
+ struct bench b;
+
+ bench_start(&b);
+
+ for (uint32_t i = 0; i < APPENDS; i++) {
+ uint32_vector_append (&v, i);
+ }
+
+ bench_stop(&b);
+
+ assert (v.ptr[APPENDS - 1] == APPENDS - 1);
+ free (v.ptr);
+
+ printf ("bench_append: %d appends in %.6f s\n", APPENDS, bench_sec
(&b));
+}
+
int
main (int argc, char *argv[])
{
test_int64_vector ();
test_string_vector ();
+ bench_reserve ();
+ bench_append ();
return 0;
}
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 2/8] common/utils: Do not run benchmarks by default
The new benchmarks are very quick on modern machine, but in overload CI,
under valgrind, or running in qemu emulation they can be too slow to
run by default.
Run the benchmarks only when LIBNBD_BENCH environment variable is
defined.
To run all benchmarks in the project:
make bench
To run single test program with benchmarks:
$ LIBNBD_BENCH=1 ./test-vector
bench_reserve: 1000000 appends in 0.004282 s
bench_append: 1000000 appends in 0.015586 s
CI jobs that run on reliable environment (x86_64?) can add the `bench`
target to the job, to ensure that the benchmarks do not rot.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
Makefile.am | 5 +++++
README | 4 ++++
common/utils/Makefile.am | 3 +++
common/utils/test-vector.c | 11 +++++++----
4 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 0f0427a..87ceec0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -81,6 +81,11 @@ check-valgrind: all
$(MAKE) -C $$d check-valgrind || exit 1; \
done
+bench: all
+ @for d in common/utils; do \
+ $(MAKE) -C $$d bench || exit 1; \
+ done
+
check-root:
@for d in copy; do \
$(MAKE) -C $$d check-root || exit 1; \
diff --git a/README b/README
index a0f2456..6502e7f 100644
--- a/README
+++ b/README
@@ -59,6 +59,10 @@ To run the tests under valgrind:
make check-valgrind
+To run benchmarks:
+
+ make bench
+
Some tests require root permissions (and are therefore risky). If you
want to run these tests, do:
diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am
index 4730cea..0a8629a 100644
--- a/common/utils/Makefile.am
+++ b/common/utils/Makefile.am
@@ -62,3 +62,6 @@ test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
test_vector_SOURCES = test-vector.c vector.c vector.h bench.h
test_vector_CPPFLAGS = -I$(srcdir)
test_vector_CFLAGS = $(WARNINGS_CFLAGS)
+
+bench: test-vector
+ LIBNBD_BENCH=1 ./test-vector
diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c
index 28e882f..d55de07 100644
--- a/common/utils/test-vector.c
+++ b/common/utils/test-vector.c
@@ -163,9 +163,12 @@ bench_append (void)
int
main (int argc, char *argv[])
{
- test_int64_vector ();
- test_string_vector ();
- bench_reserve ();
- bench_append ();
+ if (getenv("LIBNBD_BENCH")) {
+ bench_reserve ();
+ bench_append ();
+ } else {
+ test_int64_vector ();
+ test_string_vector ();
+ }
return 0;
}
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 3/8] common/utils/vector.c: Optimize vector append
Minimize reallocs by growing the backing array by factor of 1.5.
Testing show that now append() is fast without calling reserve()
upfront, simplifying code using vector.
$ ./test-vector | grep bench
bench_reserve: 1000000 appends in 0.003997 s
bench_append: 1000000 appends in 0.003869 s
In practice, this can make "nbdinfo --map" 10 milliseconds faster when
running with image that have 500,000 extents.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
common/utils/vector.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/common/utils/vector.c b/common/utils/vector.c
index 00cd254..7df17e1 100644
--- a/common/utils/vector.c
+++ b/common/utils/vector.c
@@ -41,11 +41,21 @@ int
generic_vector_reserve (struct generic_vector *v, size_t n, size_t itemsize)
{
void *newptr;
+ size_t reqalloc, newalloc;
- newptr = realloc (v->ptr, (n + v->alloc) * itemsize);
+ reqalloc = v->alloc + n;
+ if (reqalloc < v->alloc)
+ return -1; /* overflow */
+
+ newalloc = (v->alloc * 3 + 1) / 2;
+
+ if (newalloc < reqalloc)
+ newalloc = reqalloc;
+
+ newptr = realloc (v->ptr, newalloc * itemsize);
if (newptr == NULL)
return -1;
v->ptr = newptr;
- v->alloc += n;
+ v->alloc = newalloc;
return 0;
}
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 4/8] common/utils/vector: Rename `alloc` to `cap`
The `alloc` field is the maximum number of items you can append to a
vector before it need to be resized. This may confuse users with the
size of the `ptr` array which is `alloc * itemsize`. Rename to "cap",
common term for this property in many languages (e.g C++, Rust, Go).
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
common/utils/vector.c | 16 ++++++++--------
common/utils/vector.h | 10 +++++-----
lib/crypto.c | 4 ++--
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/common/utils/vector.c b/common/utils/vector.c
index 7df17e1..a4b43ce 100644
--- a/common/utils/vector.c
+++ b/common/utils/vector.c
@@ -41,21 +41,21 @@ int
generic_vector_reserve (struct generic_vector *v, size_t n, size_t itemsize)
{
void *newptr;
- size_t reqalloc, newalloc;
+ size_t reqcap, newcap;
- reqalloc = v->alloc + n;
- if (reqalloc < v->alloc)
+ reqcap = v->cap + n;
+ if (reqcap < v->cap)
return -1; /* overflow */
- newalloc = (v->alloc * 3 + 1) / 2;
+ newcap = (v->cap * 3 + 1) / 2;
- if (newalloc < reqalloc)
- newalloc = reqalloc;
+ if (newcap < reqcap)
+ newcap = reqcap;
- newptr = realloc (v->ptr, newalloc * itemsize);
+ newptr = realloc (v->ptr, newcap * itemsize);
if (newptr == NULL)
return -1;
v->ptr = newptr;
- v->alloc = newalloc;
+ v->cap = newcap;
return 0;
}
diff --git a/common/utils/vector.h b/common/utils/vector.h
index 69aad8d..3fc8b68 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -86,7 +86,7 @@
struct name { \
type *ptr; /* Pointer to array of items. */ \
size_t size; /* Number of valid items in the array. */ \
- size_t alloc; /* Number of items allocated. */ \
+ size_t cap; /* Maximum number of items. */ \
}; \
typedef struct name name; \
\
@@ -106,7 +106,7 @@
name##_insert (name *v, type elem, size_t i) \
{ \
assert (i <= v->size); \
- if (v->size >= v->alloc) {
\
+ if (v->size >= v->cap) {
\
if (name##_reserve (v, 1) == -1) return -1; \
} \
memmove (&v->ptr[i+1], &v->ptr[i], (v->size-i) * sizeof
(elem)); \
@@ -137,7 +137,7 @@
{ \
free (v->ptr); \
v->ptr = NULL; \
- v->size = v->alloc = 0; \
+ v->size = v->cap = 0; \
} \
\
/* Iterate over the vector, calling f() on each element. */ \
@@ -168,12 +168,12 @@
(void *) compare); \
}
-#define empty_vector { .ptr = NULL, .size = 0, .alloc = 0 }
+#define empty_vector { .ptr = NULL, .size = 0, .cap = 0 }
struct generic_vector {
void *ptr;
size_t size;
- size_t alloc;
+ size_t cap;
};
extern int generic_vector_reserve (struct generic_vector *v,
diff --git a/lib/crypto.c b/lib/crypto.c
index a9b3789..f570db3 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -141,7 +141,7 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h)
}
for (;;) {
- if (getlogin_r (str.ptr, str.alloc) == 0) {
+ if (getlogin_r (str.ptr, str.cap) == 0) {
return str.ptr;
}
else if (errno != ERANGE) {
@@ -150,7 +150,7 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h)
return NULL;
}
/* Try again with a larger buffer. */
- if (string_reserve (&str, str.alloc == 0 ? 16 : str.alloc * 2) == -1) {
+ if (string_reserve (&str, str.cap == 0 ? 16 : str.cap * 2) == -1) {
set_error (errno, "realloc");
free (str.ptr);
return NULL;
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 5/8] common/utils/vector: Rename `size` to `len`
The field `size` may be confusing with the size of the underlying array.
Rename to `len`, a common term for this concept.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
common/utils/test-vector.c | 2 +-
common/utils/vector.h | 38 ++++++++++----------
copy/file-ops.c | 14 ++++----
copy/main.c | 2 +-
copy/multi-thread-copying.c | 2 +-
copy/nbd-ops.c | 20 +++++------
copy/synch-copying.c | 2 +-
fuse/nbdfuse.c | 4 +--
fuse/operations.c | 16 ++++-----
generator/states-connect-socket-activation.c | 2 +-
generator/states-newstyle-opt-meta-context.c | 8 ++---
info/list.c | 8 ++---
info/map.c | 12 +++----
info/show.c | 6 ++--
lib/handle.c | 4 +--
lib/uri.c | 8 ++---
16 files changed, 74 insertions(+), 74 deletions(-)
diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c
index d55de07..c3339f4 100644
--- a/common/utils/test-vector.c
+++ b/common/utils/test-vector.c
@@ -73,7 +73,7 @@ test_int64_vector (void)
assert (v.ptr[i] == i);
int64_vector_remove (&v, 1);
- assert (v.size == 9);
+ assert (v.len == 9);
assert (v.ptr[1] == 2);
tmp = 10;
diff --git a/common/utils/vector.h b/common/utils/vector.h
index 3fc8b68..9ac0867 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -84,9 +84,9 @@
*/
#define DEFINE_VECTOR_TYPE(name, type) \
struct name { \
- type *ptr; /* Pointer to array of items. */ \
- size_t size; /* Number of valid items in the array. */ \
- size_t cap; /* Maximum number of items. */ \
+ type *ptr; /* Pointer to array of items. */ \
+ size_t len; /* Number of valid items in the array. */ \
+ size_t cap; /* Maximum number of items. */ \
}; \
typedef struct name name; \
\
@@ -101,17 +101,17 @@
sizeof (type)); \
} \
\
- /* Insert at i'th element. i=0 => beginning i=size => append */
\
+ /* Insert at i'th element. i=0 => beginning i=len => append */
\
static inline int \
name##_insert (name *v, type elem, size_t i) \
{ \
- assert (i <= v->size); \
- if (v->size >= v->cap) {
\
+ assert (i <= v->len); \
+ if (v->len >= v->cap) {
\
if (name##_reserve (v, 1) == -1) return -1; \
} \
- memmove (&v->ptr[i+1], &v->ptr[i], (v->size-i) * sizeof
(elem)); \
+ memmove (&v->ptr[i+1], &v->ptr[i], (v->len-i) * sizeof
(elem)); \
v->ptr[i] = elem; \
- v->size++; \
+ v->len++; \
return 0; \
} \
\
@@ -119,16 +119,16 @@
static inline int \
name##_append (name *v, type elem) \
{ \
- return name##_insert (v, elem, v->size); \
+ return name##_insert (v, elem, v->len); \
} \
\
- /* Remove i'th element. i=0 => beginning i=size-1 => end */
\
+ /* Remove i'th element. i=0 => beginning i=len-1 => end */
\
static inline void \
name##_remove (name *v, size_t i) \
{ \
- assert (i < v->size); \
- memmove (&v->ptr[i], &v->ptr[i+1], (v->size-i-1) * sizeof
(type)); \
- v->size--; \
+ assert (i < v->len); \
+ memmove (&v->ptr[i], &v->ptr[i+1], (v->len-i-1) * sizeof
(type)); \
+ v->len--; \
} \
\
/* Remove all elements and deallocate the vector. */ \
@@ -137,7 +137,7 @@
{ \
free (v->ptr); \
v->ptr = NULL; \
- v->size = v->cap = 0; \
+ v->len = v->cap = 0; \
} \
\
/* Iterate over the vector, calling f() on each element. */ \
@@ -145,7 +145,7 @@
name##_iter (name *v, void (*f) (type elem)) \
{ \
size_t i; \
- for (i = 0; i < v->size; ++i) \
+ for (i = 0; i < v->len; ++i) \
f (v->ptr[i]); \
} \
\
@@ -154,7 +154,7 @@
name##_sort (name *v, \
int (*compare) (const type *p1, const type *p2)) \
{ \
- qsort (v->ptr, v->size, sizeof (type), (void *) compare); \
+ qsort (v->ptr, v->len, sizeof (type), (void *) compare); \
} \
\
/* Search for an exactly matching element in the vector using a \
@@ -164,15 +164,15 @@
name##_search (const name *v, const void *key, \
int (*compare) (const void *key, const type *v)) \
{ \
- return bsearch (key, v->ptr, v->size, sizeof (type), \
+ return bsearch (key, v->ptr, v->len, sizeof (type), \
(void *) compare); \
}
-#define empty_vector { .ptr = NULL, .size = 0, .cap = 0 }
+#define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
struct generic_vector {
void *ptr;
- size_t size;
+ size_t len;
size_t cap;
};
diff --git a/copy/file-ops.c b/copy/file-ops.c
index cb787ad..8470434 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -101,7 +101,7 @@ page_size_init (void)
/* Load the page cache map for a particular file into
* rwf->cached_pages. Only used when reading files. This doesn't
- * fail: if a system call fails then rwf->cached_pages.size will be
+ * fail: if a system call fails then rwf->cached_pages.len will be
* zero which is handled in page_cache_evict.
*/
static inline void
@@ -121,7 +121,7 @@ page_cache_map (struct rw_file *rwf)
if (mincore (ptr, rwf->rw.size, rwf->cached_pages.ptr) == -1)
goto out;
- rwf->cached_pages.size = veclen;
+ rwf->cached_pages.len = veclen;
out:
munmap (ptr, rwf->rw.size);
}
@@ -133,7 +133,7 @@ static inline bool
page_was_cached (struct rw_file *rwf, uint64_t offset)
{
uint64_t page = offset / page_size;
- assert (page < rwf->cached_pages.size);
+ assert (page < rwf->cached_pages.len);
return (rwf->cached_pages.ptr[page] & 1) != 0;
}
@@ -151,7 +151,7 @@ page_cache_evict (struct rw_file *rwf, uint64_t orig_offset,
size_t orig_len)
* that pages were mapped so we will not evict them: essentially fall
* back to doing nothing.
*/
- if (rwf->cached_pages.size == 0) return;
+ if (rwf->cached_pages.len == 0) return;
/* Only bother with whole pages. */
offset = ROUND_UP (orig_offset, page_size);
@@ -635,7 +635,7 @@ file_get_extents (struct rw *rw, uintptr_t index,
uint64_t offset, uint64_t count,
extent_list *ret)
{
- ret->size = 0;
+ ret->len = 0;
#ifdef SEEK_HOLE
struct rw_file *rwf = (struct rw_file *)rw;
@@ -704,8 +704,8 @@ file_get_extents (struct rw *rw, uintptr_t index,
/* The last extent may extend beyond the request bounds. We must
* truncate it.
*/
- assert (ret->size > 0);
- last = ret->size - 1;
+ assert (ret->len > 0);
+ last = ret->len - 1;
assert (ret->ptr[last].offset <= end);
if (ret->ptr[last].offset + ret->ptr[last].length > end) {
uint64_t d = ret->ptr[last].offset + ret->ptr[last].length - end;
diff --git a/copy/main.c b/copy/main.c
index 15a6454..67788b5 100644
--- a/copy/main.c
+++ b/copy/main.c
@@ -526,7 +526,7 @@ default_get_extents (struct rw *rw, uintptr_t index,
{
struct extent e;
- ret->size = 0;
+ ret->len = 0;
e.offset = offset;
e.length = count;
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index 4603d8f..b17ca59 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -154,7 +154,7 @@ worker_thread (void *indexp)
else
default_get_extents (src, index, offset, count, &exts);
- for (i = 0; i < exts.size; ++i) {
+ for (i = 0; i < exts.len; ++i) {
struct command *command;
struct buffer *buffer;
char *data;
diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
index cb11e64..dfc62f2 100644
--- a/copy/nbd-ops.c
+++ b/copy/nbd-ops.c
@@ -90,7 +90,7 @@ open_one_nbd_handle (struct rw_nbd *rwn)
/* Cache these. We assume with multi-conn that each handle will act
* the same way.
*/
- if (rwn->handles.size == 0) {
+ if (rwn->handles.len == 0) {
rwn->can_zero = nbd_can_zero (nbd) > 0;
rwn->rw.size = nbd_get_size (nbd);
if (rwn->rw.size == -1) {
@@ -157,7 +157,7 @@ nbd_ops_close (struct rw *rw)
struct rw_nbd *rwn = (struct rw_nbd *) rw;
size_t i;
- for (i = 0; i < rwn->handles.size; ++i) {
+ for (i = 0; i < rwn->handles.len; ++i) {
if (nbd_shutdown (rwn->handles.ptr[i], 0) == -1) {
fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
exit (EXIT_FAILURE);
@@ -176,7 +176,7 @@ nbd_ops_flush (struct rw *rw)
struct rw_nbd *rwn = (struct rw_nbd *) rw;
size_t i;
- for (i = 0; i < rwn->handles.size; ++i) {
+ for (i = 0; i < rwn->handles.len; ++i) {
if (nbd_flush (rwn->handles.ptr[i], 0) == -1) {
fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
exit (EXIT_FAILURE);
@@ -189,7 +189,7 @@ nbd_ops_is_read_only (struct rw *rw)
{
struct rw_nbd *rwn = (struct rw_nbd *) rw;
- if (rwn->handles.size > 0)
+ if (rwn->handles.len > 0)
return nbd_is_read_only (rwn->handles.ptr[0]);
else
return false;
@@ -200,7 +200,7 @@ nbd_ops_can_extents (struct rw *rw)
{
struct rw_nbd *rwn = (struct rw_nbd *) rw;
- if (rwn->handles.size > 0)
+ if (rwn->handles.len > 0)
return nbd_can_meta_context (rwn->handles.ptr[0],
"base:allocation");
else
return false;
@@ -211,7 +211,7 @@ nbd_ops_can_multi_conn (struct rw *rw)
{
struct rw_nbd *rwn = (struct rw_nbd *) rw;
- if (rwn->handles.size > 0)
+ if (rwn->handles.len > 0)
return nbd_can_multi_conn (rwn->handles.ptr[0]);
else
return false;
@@ -226,7 +226,7 @@ nbd_ops_start_multi_conn (struct rw *rw)
for (i = 1; i < connections; ++i)
open_one_nbd_handle (rwn);
- assert (rwn->handles.size == connections);
+ assert (rwn->handles.len == connections);
}
static size_t
@@ -436,13 +436,13 @@ nbd_ops_get_extents (struct rw *rw, uintptr_t index,
nbd = rwn->handles.ptr[index];
- ret->size = 0;
+ ret->len = 0;
while (count > 0) {
const uint64_t old_offset = offset;
size_t i;
- exts.size = 0;
+ exts.len = 0;
if (nbd_block_status (nbd, count, offset,
(nbd_extent_callback) {
.user_data = &exts,
@@ -456,7 +456,7 @@ nbd_ops_get_extents (struct rw *rw, uintptr_t index,
}
/* Copy the extents returned into the final list (ret). */
- for (i = 0; i < exts.size; ++i) {
+ for (i = 0; i < exts.len; ++i) {
uint64_t d;
assert (exts.ptr[i].offset == offset);
diff --git a/copy/synch-copying.c b/copy/synch-copying.c
index c63bd2d..c9899c3 100644
--- a/copy/synch-copying.c
+++ b/copy/synch-copying.c
@@ -70,7 +70,7 @@ synch_copying (void)
else
default_get_extents (src, 0, offset, count, &exts);
- for (i = 0; i < exts.size; ++i) {
+ for (i = 0; i < exts.len; ++i) {
assert (exts.ptr[i].length <= count);
if (exts.ptr[i].zero) {
diff --git a/fuse/nbdfuse.c b/fuse/nbdfuse.c
index b82ce3e..b9d54ab 100644
--- a/fuse/nbdfuse.c
+++ b/fuse/nbdfuse.c
@@ -415,7 +415,7 @@ main (int argc, char *argv[])
handles_append (&nbd, h); /* reserved above, so can't fail */
}
}
- connections = (unsigned) nbd.size;
+ connections = (unsigned) nbd.len;
if (verbose)
fprintf (stderr, "nbdfuse: connections=%u\n", connections);
@@ -511,7 +511,7 @@ main (int argc, char *argv[])
fuse_destroy (fuse);
/* Close NBD handle(s). */
- for (i = 0; i < nbd.size; ++i)
+ for (i = 0; i < nbd.len; ++i)
nbd_close (nbd.ptr[i]);
free (mountpoint);
diff --git a/fuse/operations.c b/fuse/operations.c
index 4c14f04..264acb5 100644
--- a/fuse/operations.c
+++ b/fuse/operations.c
@@ -115,20 +115,20 @@ start_operations_threads (void)
/* This barrier is used to ensure all operations threads have
* started up before we leave this function.
*/
- err = pthread_barrier_init (&barrier, NULL, nbd.size + 1);
+ err = pthread_barrier_init (&barrier, NULL, nbd.len + 1);
if (err != 0) {
errno = err;
perror ("nbdfuse: pthread_barrier_init");
exit (EXIT_FAILURE);
}
- threads = calloc (nbd.size, sizeof (struct thread));
+ threads = calloc (nbd.len, sizeof (struct thread));
if (threads == NULL) {
perror ("calloc");
exit (EXIT_FAILURE);
}
- for (i = 0; i < nbd.size; ++i) {
+ for (i = 0; i < nbd.len; ++i) {
threads[i].n = i;
threads[i].in_flight = 0;
if ((err = pthread_mutex_init (&threads[i].in_flight_mutex, NULL)) != 0
||
@@ -234,11 +234,11 @@ next_thread (void)
{
static _Atomic size_t n = 0;
- if (nbd.size == 1)
+ if (nbd.len == 1)
return 0;
else {
size_t i = n++;
- return i % (nbd.size - 1);
+ return i % (nbd.len - 1);
}
}
@@ -509,15 +509,15 @@ nbdfuse_destroy (void *data)
*/
time (&st);
while (time (NULL) - st <= RELEASE_TIMEOUT) {
- for (i = 0; i < nbd.size; ++i) {
+ for (i = 0; i < nbd.len; ++i) {
if (threads[i].in_flight > 0)
break;
}
- if (i == nbd.size) /* no commands in flight */
+ if (i == nbd.len) /* no commands in flight */
break;
/* Signal to the operations thread to work. */
- for (i = 0; i < nbd.size; ++i) {
+ for (i = 0; i < nbd.len; ++i) {
pthread_mutex_lock (&threads[i].in_flight_mutex);
pthread_cond_signal (&threads[i].in_flight_cond);
pthread_mutex_unlock (&threads[i].in_flight_mutex);
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 8a2add3..800d963 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -51,7 +51,7 @@ prepare_socket_activation_environment (string_vector *env)
char *p;
size_t i;
- assert (env->size == 0);
+ assert (env->len == 0);
/* Reserve slots env[0] and env[1]. */
p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
diff --git a/generator/states-newstyle-opt-meta-context.c
b/generator/states-newstyle-opt-meta-context.c
index bbf155e..30b9617 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -38,7 +38,7 @@ STATE_MACHINE {
else {
assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
opt = NBD_OPT_SET_META_CONTEXT;
- if (!h->structured_replies || h->request_meta_contexts.size == 0) {
+ if (!h->structured_replies || h->request_meta_contexts.len == 0) {
SET_NEXT_STATE (%^OPT_GO.START);
return 0;
}
@@ -48,7 +48,7 @@ STATE_MACHINE {
/* Calculate the length of the option request data. */
len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries
*/;
- for (i = 0; i < h->request_meta_contexts.size; ++i)
+ for (i = 0; i < h->request_meta_contexts.len; ++i)
len += 4 /* length of query */ + strlen
(h->request_meta_contexts.ptr[i]);
h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
@@ -87,7 +87,7 @@ STATE_MACHINE {
switch (send_from_wbuf (h)) {
case -1: SET_NEXT_STATE (%.DEAD); return 0;
case 0:
- h->sbuf.nrqueries = htobe32 (h->request_meta_contexts.size);
+ h->sbuf.nrqueries = htobe32 (h->request_meta_contexts.len);
h->wbuf = &h->sbuf;
h->wlen = sizeof h->sbuf.nrqueries;
h->wflags = MSG_MORE;
@@ -105,7 +105,7 @@ STATE_MACHINE {
return 0;
NEWSTYLE.OPT_META_CONTEXT.PREPARE_NEXT_QUERY:
- if (h->querynum >= h->request_meta_contexts.size) {
+ if (h->querynum >= h->request_meta_contexts.len) {
/* end of list of requested meta contexts */
SET_NEXT_STATE (%PREPARE_FOR_REPLY);
return 0;
diff --git a/info/list.c b/info/list.c
index 71a2acd..daef55d 100644
--- a/info/list.c
+++ b/info/list.c
@@ -75,7 +75,7 @@ free_exports (void)
{
size_t i;
- for (i = 0; i < export_list.size; ++i) {
+ for (i = 0; i < export_list.len; ++i) {
free (export_list.ptr[i].name);
free (export_list.ptr[i].desc);
}
@@ -88,10 +88,10 @@ list_all_exports (const char *uri)
size_t i;
bool list_okay = true;
- if (export_list.size == 0 && json_output)
+ if (export_list.len == 0 && json_output)
fprintf (fp, "\"exports\": []\n");
- for (i = 0; i < export_list.size; ++i) {
+ for (i = 0; i < export_list.len; ++i) {
const char *name = export_list.ptr[i].name;
struct nbd_handle *nbd2;
@@ -122,7 +122,7 @@ list_all_exports (const char *uri)
/* List the metadata of this export. */
if (!show_one_export (nbd2, export_list.ptr[i].desc, i == 0,
- i + 1 == export_list.size))
+ i + 1 == export_list.len))
list_okay = false;
if (probe_content) {
diff --git a/info/map.c b/info/map.c
index de7b7ab..39c5933 100644
--- a/info/map.c
+++ b/info/map.c
@@ -70,7 +70,7 @@ do_map (void)
}
for (offset = 0; offset < size;) {
- prev_entries_size = entries.size;
+ prev_entries_size = entries.len;
if (nbd_block_status (nbd, MIN (size - offset, max_len), offset,
(nbd_extent_callback) {
.callback = extent_callback,
@@ -80,13 +80,13 @@ do_map (void)
exit (EXIT_FAILURE);
}
/* We expect extent_callback to add at least one extent to entries. */
- if (prev_entries_size == entries.size) {
+ if (prev_entries_size == entries.len) {
fprintf (stderr, "%s: --map: server did not return any
extents\n",
progname);
exit (EXIT_FAILURE);
}
- assert ((entries.size & 1) == 0);
- for (i = prev_entries_size; i < entries.size; i += 2)
+ assert ((entries.len & 1) == 0);
+ for (i = prev_entries_size; i < entries.len; i += 2)
offset += entries.ptr[i];
}
@@ -134,7 +134,7 @@ print_extents (uint32_vector *entries)
if (json_output) fprintf (fp, "[\n");
- for (i = 0; i < entries->size; i += 2) {
+ for (i = 0; i < entries->len; i += 2) {
uint32_t type = entries->ptr[last+1];
/* If we're coalescing and the current type is different from the
@@ -227,7 +227,7 @@ print_totals (uint32_vector *entries, int64_t size)
uint64_t c = 0;
size_t i;
- for (i = 0; i < entries->size; i += 2) {
+ for (i = 0; i < entries->len; i += 2) {
uint32_t t = entries->ptr[i+1];
if (t == type)
diff --git a/info/show.c b/info/show.c
index ff241a8..389dece 100644
--- a/info/show.c
+++ b/info/show.c
@@ -131,7 +131,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
fprintf (fp, "\turi: %s\n", uri);
if (show_context) {
fprintf (fp, "\tcontexts:\n");
- for (i = 0; i < contexts.size; ++i)
+ for (i = 0; i < contexts.len; ++i)
fprintf (fp, "\t\t%s\n", contexts.ptr[i]);
}
if (is_rotational >= 0)
@@ -195,10 +195,10 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
if (show_context) {
fprintf (fp, "\t\"contexts\": [\n");
- for (i = 0; i < contexts.size; ++i) {
+ for (i = 0; i < contexts.len; ++i) {
fprintf (fp, "\t\t");
print_json_string (contexts.ptr[i]);
- if (i+1 != contexts.size)
+ if (i+1 != contexts.len)
fputc (',', fp);
fputc ('\n', fp);
}
diff --git a/lib/handle.c b/lib/handle.c
index 67aa875..cbb37e8 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -328,7 +328,7 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const
char *name)
ssize_t
nbd_unlocked_get_nr_meta_contexts (struct nbd_handle *h)
{
- return h->request_meta_contexts.size;
+ return h->request_meta_contexts.len;
}
char *
@@ -336,7 +336,7 @@ nbd_unlocked_get_meta_context (struct nbd_handle *h, size_t
i)
{
char *ret;
- if (i >= h->request_meta_contexts.size) {
+ if (i >= h->request_meta_contexts.len) {
set_error (EINVAL, "meta context request out of range");
return NULL;
}
diff --git a/lib/uri.c b/lib/uri.c
index 5e8dc06..a24dfe5 100644
--- a/lib/uri.c
+++ b/lib/uri.c
@@ -142,7 +142,7 @@ parse_uri_queries (const char *query_raw, uri_query_list
*list)
return 0;
error:
- for (i = 0; i < list->size; ++list) {
+ for (i = 0; i < list->len; ++list) {
free (list->ptr[i].name);
free (list->ptr[i].value);
}
@@ -254,7 +254,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
}
/* Parse the socket parameter. */
- for (i = 0; i < queries.size; i++) {
+ for (i = 0; i < queries.len; i++) {
if (strcmp (queries.ptr[i].name, "socket") == 0)
unixsocket = queries.ptr[i].value;
}
@@ -277,7 +277,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
goto cleanup;
/* Look for some tls-* parameters. */
- for (i = 0; i < queries.size; i++) {
+ for (i = 0; i < queries.len; i++) {
if (strcmp (queries.ptr[i].name, "tls-certificates") == 0) {
if (! h->uri_allow_local_file) {
set_error (EPERM,
@@ -387,7 +387,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const
char *raw_uri)
ret = 0;
cleanup:
- for (i = 0; i < queries.size; ++i) {
+ for (i = 0; i < queries.len; ++i) {
free (queries.ptr[i].name);
free (queries.ptr[i].value);
}
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 6/8] lib/crypto.c: Simplify vector reserve
We don't need to worry now about efficient growing.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
lib/crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/crypto.c b/lib/crypto.c
index f570db3..2ce4d4d 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -150,7 +150,7 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h)
return NULL;
}
/* Try again with a larger buffer. */
- if (string_reserve (&str, str.cap == 0 ? 16 : str.cap * 2) == -1) {
+ if (string_reserve (&str, 16) == -1) {
set_error (errno, "realloc");
free (str.ptr);
return NULL;
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 7/8] lib/crypto.c: Don't use empty vector
getlogin_r() with empty buffer will always fail since the underlying
array is NULL. Reserve space before using the vector.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
lib/crypto.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/lib/crypto.c b/lib/crypto.c
index 2ce4d4d..09d98fd 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -141,6 +141,12 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h)
}
for (;;) {
+ /* Increase capacity (str.cap starts at 0) */
+ if (string_reserve (&str, 16) == -1) {
+ set_error (errno, "realloc");
+ free (str.ptr);
+ return NULL;
+ }
if (getlogin_r (str.ptr, str.cap) == 0) {
return str.ptr;
}
@@ -149,12 +155,6 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h)
free (str.ptr);
return NULL;
}
- /* Try again with a larger buffer. */
- if (string_reserve (&str, 16) == -1) {
- set_error (errno, "realloc");
- free (str.ptr);
- return NULL;
- }
}
}
--
2.31.1
Nir Soffer
2021-Oct-31 15:38 UTC
[Libguestfs] [PATCH libnbd v2 8/8] lib/crypto.c: Remove unneeded else
We return on successful if, so the else is not needed. This makes the
flow more clear.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
lib/crypto.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/crypto.c b/lib/crypto.c
index 09d98fd..340a6a0 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -147,10 +147,12 @@ nbd_unlocked_get_tls_username (struct nbd_handle *h)
free (str.ptr);
return NULL;
}
+
if (getlogin_r (str.ptr, str.cap) == 0) {
return str.ptr;
}
- else if (errno != ERANGE) {
+
+ if (errno != ERANGE) {
set_error (errno, "getlogin_r");
free (str.ptr);
return NULL;
--
2.31.1
Eric Blake
2021-Nov-08 19:14 UTC
[Libguestfs] [PATCH libnbd v2 3/8] common/utils/vector.c: Optimize vector append
On Sun, Oct 31, 2021 at 05:38:52PM +0200, Nir Soffer wrote:> Minimize reallocs by growing the backing array by factor of 1.5. > > Testing show that now append() is fast without calling reserve() > upfront, simplifying code using vector. > > $ ./test-vector | grep bench > bench_reserve: 1000000 appends in 0.003997 s > bench_append: 1000000 appends in 0.003869 s > > In practice, this can make "nbdinfo --map" 10 milliseconds faster when > running with image that have 500,000 extents. > > Signed-off-by: Nir Soffer <nsoffer at redhat.com> > --- > common/utils/vector.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/common/utils/vector.c b/common/utils/vector.c > index 00cd254..7df17e1 100644 > --- a/common/utils/vector.c > +++ b/common/utils/vector.c > @@ -41,11 +41,21 @@ int > generic_vector_reserve (struct generic_vector *v, size_t n, size_t itemsize) > { > void *newptr; > + size_t reqalloc, newalloc; > > - newptr = realloc (v->ptr, (n + v->alloc) * itemsize); > + reqalloc = v->alloc + n; > + if (reqalloc < v->alloc) > + return -1; /* overflow */The value of errno is indeterminate here,...> + > + newalloc = (v->alloc * 3 + 1) / 2;On a 32-bit machine, are we at risk of v->alloc*3 overflowing size_t? If so, should that be another place where we do an early return -1?> + > + if (newalloc < reqalloc) > + newalloc = reqalloc; > + > + newptr = realloc (v->ptr, newalloc * itemsize); > if (newptr == NULL) > return -1;but ENOMEM here. We probably want to guarantee it is ENOMEM on all error paths, rather than having to audit whether callers care.> v->ptr = newptr; > - v->alloc += n; > + v->alloc = newalloc; > return 0; > } > -- > 2.31.1 >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org