6:37 a.m.
One way to reproduce the bugs is to set admin_max_workers=0 in
libvirtd.conf, restart libvirtd, and then call:
$ virt-admin server-threadpool-info admin
Changelog:
v1->v2:
- Worked in Daniel Berrangé's comments that:
1. max_workers=0 should not be allowed for the libvirtd (patch 3)
2. it shouldn't be allowed to switch between zero and non-zero
max_workers (patch 4)
- Changed the deadlock fix (patch 1)
- Added two small fixes (patch 5 and 6)
Marc Hartmayer (6):
rpc: Fix deadlock if there is no worker pool available
rpc: Initialize a worker pool for max_workers=0 as well
virThreadPool: Prevent switching between zero and non-zero maxWorkers
daemon: Raise an error if 'max_workers' < 1 in libvirtd.conf
virt-admin: Fix two error messages
rpc: Fix name of include guard
src/libvirt_private.syms | 4 ++
src/remote/remote_daemon_config.c | 5 +++
src/rpc/virnetserver.c | 28 ++++++++------
src/rpc/virnetserverprogram.h | 4 +-
src/util/virthreadpool.c | 81 ++++++++++++++++++++++++++++++---------
src/util/virthreadpool.h | 8 ++++
tools/virt-admin.c | 5 ++-
7 files changed, 101 insertions(+), 34 deletions(-)
--
2.13.4
6:37 a.m.
New subject: [libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available
@srv must be unlocked for the call virNetServerProcessMsg otherwise a
deadlock can occur.
Since the pointer 'srv->workers' will never be changed after
initialization and the thread pool has it's own locking we can release
the lock of 'srv' earlier. This also fixes the deadlock.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5c7f7dd08fe1..091e3ef23406 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -51,6 +51,7 @@ struct _virNetServer {
char *name;
+ /* Immutable pointer, self-locking APIs */
virThreadPoolPtr workers;
char *mdnsGroupName;
@@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
VIR_FREE(job);
}
-static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
- virNetMessagePtr msg,
- void *opaque)
+
+static void
+virNetServerDispatchNewMessage(virNetServerClientPtr client,
+ virNetMessagePtr msg,
+ void *opaque)
{
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
@@ -196,6 +199,9 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
break;
}
}
+ /* we can unlock @srv since @prog can only become invalid in case
+ * of disposing @srv */
+ virObjectUnlock(srv);
if (srv->workers) {
virNetServerJobPtr job;
@@ -223,15 +229,14 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
goto error;
}
- virObjectUnlock(srv);
return;
error:
virNetMessageFree(msg);
virNetServerClientClose(client);
- virObjectUnlock(srv);
}
+
/**
* virNetServerCheckLimits:
* @srv: server to check limits on
--
2.13.4
9:51 a.m.
New subject: [libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
@srv must be unlocked for the call virNetServerProcessMsg otherwise
a
deadlock can occur.
Since the pointer 'srv->workers' will never be changed after
initialization and the thread pool has it's own locking we can release
the lock of 'srv' earlier. This also fixes the deadlock.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/rpc/virnetserver.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5c7f7dd08fe1..091e3ef23406 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -51,6 +51,7 @@ struct _virNetServer {
char *name;
+ /* Immutable pointer, self-locking APIs */
virThreadPoolPtr workers;
char *mdnsGroupName;
@@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
VIR_FREE(job);
}
-static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
- virNetMessagePtr msg,
- void *opaque)
+
+static void
+virNetServerDispatchNewMessage(virNetServerClientPtr client,
+ virNetMessagePtr msg,
+ void *opaque)
{
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
@@ -196,6 +199,9 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
break;
}
}
Should we grab an object reference then to avoid the chance that
something disposes srv right after it's unlocked? And then Unref instead
of Unlock later on?
Following virThreadPoolSendJob finds that it'll grab the srv->workers
pool mutex and check the quit flag before adding a job to and of course
as you point out virNetServerProcessMsg would eventually assume that the
server is unlocked in virNetServerProgramDispatchCall before calling
dispatcher->func.
With a Ref/Unref, I'd feel better and thus consider this,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ /* we can unlock @srv since @prog can only become invalid in
case
+ * of disposing @srv */
+ virObjectUnlock(srv);
if (srv->workers) {
virNetServerJobPtr job;
@@ -223,15 +229,14 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
goto error;
}
- virObjectUnlock(srv);
return;
error:
virNetMessageFree(msg);
virNetServerClientClose(client);
- virObjectUnlock(srv);
}
+
/**
* virNetServerCheckLimits:
* @srv: server to check limits on
2:32 a.m.
New subject: [libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available
On Thu, Jul 19, 2018 at 04:51 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> @srv must be unlocked for the call virNetServerProcessMsg otherwise a
> deadlock can occur.
>
> Since the pointer 'srv->workers' will never be changed after
> initialization and the thread pool has it's own locking we can release
> the lock of 'srv' earlier. This also fixes the deadlock.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> ---
> src/rpc/virnetserver.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 5c7f7dd08fe1..091e3ef23406 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -51,6 +51,7 @@ struct _virNetServer {
>
> char *name;
>
> + /* Immutable pointer, self-locking APIs */
> virThreadPoolPtr workers;
>
> char *mdnsGroupName;
> @@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void
*opaque)
> VIR_FREE(job);
> }
>
> -static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
> - virNetMessagePtr msg,
> - void *opaque)
> +
> +static void
> +virNetServerDispatchNewMessage(virNetServerClientPtr client,
> + virNetMessagePtr msg,
> + void *opaque)
> {
> virNetServerPtr srv = opaque;
> virNetServerProgramPtr prog = NULL;
> @@ -196,6 +199,9 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr
client,
> break;
> }
> }
Should we grab an object reference then to avoid the chance that
something disposes srv right after it's unlocked? And then Unref instead
of Unlock later on?
Following virThreadPoolSendJob finds that it'll grab the srv->workers
pool mutex and check the quit flag before adding a job to and of course
as you point out virNetServerProcessMsg would eventually assume that the
server is unlocked in virNetServerProgramDispatchCall before calling
dispatcher->func.
With a Ref/Unref, I'd feel better and thus consider this,
Yep, I’m fine with that.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks!
John
> + /* we can unlock @srv since @prog can only become invalid in case
> + * of disposing @srv */
> + virObjectUnlock(srv);
>
> if (srv->workers) {
> virNetServerJobPtr job;
> @@ -223,15 +229,14 @@ static void
virNetServerDispatchNewMessage(virNetServerClientPtr client,
> goto error;
> }
>
> - virObjectUnlock(srv);
> return;
>
> error:
> virNetMessageFree(msg);
> virNetServerClientClose(client);
> - virObjectUnlock(srv);
> }
>
> +
> /**
> * virNetServerCheckLimits:
> * @srv: server to check limits on
>
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:37 a.m.
New subject: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
Semantically, there is no difference between an uninitialized worker
pool and an initialized worker pool with zero workers. Let's allow the
worker pool to be initialized for max_workers=0 as well then which
makes the API more symmetric and simplifies code. Validity of the
worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
This patch fixes segmentation faults in
virNetServerGetThreadPoolParameters and
virNetServerSetThreadPoolParameters for the case when no worker pool
is actually initialized (max_workers=0).
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/libvirt_private.syms | 4 +++
src/rpc/virnetserver.c | 13 ++++-----
src/util/virthreadpool.c | 73 ++++++++++++++++++++++++++++++++++++------------
src/util/virthreadpool.h | 8 ++++++
4 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ffe5dfd19b11..aa496ddf8012 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3005,11 +3005,15 @@ virThreadPoolGetCurrentWorkers;
virThreadPoolGetFreeWorkers;
virThreadPoolGetJobQueueDepth;
virThreadPoolGetMaxWorkers;
+virThreadPoolGetMaxWorkersLocked;
virThreadPoolGetMinWorkers;
virThreadPoolGetPriorityWorkers;
+virThreadPoolLock;
virThreadPoolNewFull;
virThreadPoolSendJob;
+virThreadPoolSendJobLocked;
virThreadPoolSetParameters;
+virThreadPoolUnlock;
# util/virtime.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 091e3ef23406..fdee08fee7cd 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -203,7 +203,8 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
* of disposing @srv */
virObjectUnlock(srv);
- if (srv->workers) {
+ virThreadPoolLock(srv->workers);
+ if (virThreadPoolGetMaxWorkersLocked(srv->workers) > 0) {
virNetServerJobPtr job;
if (VIR_ALLOC(job) < 0)
@@ -218,7 +219,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
}
virObjectRef(client);
- if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
+ if (virThreadPoolSendJobLocked(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
virObjectUnref(prog);
@@ -228,10 +229,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
goto error;
}
+ virThreadPoolUnlock(srv->workers);
return;
error:
+ virThreadPoolUnlock(srv->workers);
virNetMessageFree(msg);
virNetServerClientClose(client);
}
@@ -363,8 +366,7 @@ virNetServerPtr virNetServerNew(const char *name,
if (!(srv = virObjectLockableNew(virNetServerClass)))
return NULL;
- if (max_workers &&
- !(srv->workers = virThreadPoolNew(min_workers, max_workers,
+ if (!(srv->workers = virThreadPoolNew(min_workers, max_workers,
priority_workers,
virNetServerHandleJob,
srv)))
@@ -575,21 +577,18 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
goto error;
if (virJSONValueObjectAppendNumberUint(object, "min_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetMinWorkers(srv->workers))
< 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set min_workers data in JSON document"));
goto error;
}
if (virJSONValueObjectAppendNumberUint(object, "max_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetMaxWorkers(srv->workers))
< 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set max_workers data in JSON document"));
goto error;
}
if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set priority_workers data in JSON
document"));
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 10f2bd2c3a03..f18eafb35deb 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -318,17 +318,27 @@ size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool)
return ret;
}
-size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
+
+size_t
+virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool)
+{
+ return pool->maxWorkers;
+}
+
+
+size_t
+virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
{
size_t ret;
virMutexLock(&pool->mutex);
- ret = pool->maxWorkers;
+ ret = virThreadPoolGetMaxWorkersLocked(pool);
virMutexUnlock(&pool->mutex);
return ret;
}
+
size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool)
{
size_t ret;
@@ -373,27 +383,38 @@ size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool)
return ret;
}
-/*
- * @priority - job priority
- * Return: 0 on success, -1 otherwise
- */
-int virThreadPoolSendJob(virThreadPoolPtr pool,
- unsigned int priority,
- void *jobData)
+
+void
+virThreadPoolLock(virThreadPoolPtr pool)
+{
+ virMutexLock(&pool->mutex);
+}
+
+
+void
+virThreadPoolUnlock(virThreadPoolPtr pool)
+{
+ virMutexUnlock(&pool->mutex);
+}
+
+
+int
+virThreadPoolSendJobLocked(virThreadPoolPtr pool,
+ unsigned int priority,
+ void *jobData)
{
virThreadPoolJobPtr job;
- virMutexLock(&pool->mutex);
if (pool->quit)
- goto error;
+ return -1;
if (pool->freeWorkers - pool->jobQueueDepth <= 0 &&
pool->nWorkers < pool->maxWorkers &&
virThreadPoolExpand(pool, 1, false) < 0)
- goto error;
+ return -1;
if (VIR_ALLOC(job) < 0)
- goto error;
+ return -1;
job->data = jobData;
job->priority = priority;
@@ -415,14 +436,30 @@ int virThreadPoolSendJob(virThreadPoolPtr pool,
if (priority)
virCondSignal(&pool->prioCond);
- virMutexUnlock(&pool->mutex);
return 0;
-
- error:
- virMutexUnlock(&pool->mutex);
- return -1;
}
+
+/*
+ * @priority - job priority
+ * Return: 0 on success, -1 otherwise
+ */
+int
+virThreadPoolSendJob(virThreadPoolPtr pool,
+ unsigned int priority,
+ void *jobData)
+{
+ int ret;
+
+ virMutexLock(&pool->mutex);
+ ret = virThreadPoolSendJobLocked(pool,
+ priority,
+ jobData);
+ virMutexUnlock(&pool->mutex);
+ return ret;
+}
+
+
int
virThreadPoolSetParameters(virThreadPoolPtr pool,
long long int minWorkers,
diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h
index e1f362f5bb60..2d65c2bc4ac3 100644
--- a/src/util/virthreadpool.h
+++ b/src/util/virthreadpool.h
@@ -45,6 +45,7 @@ virThreadPoolPtr virThreadPoolNewFull(size_t minWorkers,
size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool);
size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool);
+size_t virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool);
size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool);
size_t virThreadPoolGetCurrentWorkers(virThreadPoolPtr pool);
size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool);
@@ -52,10 +53,17 @@ size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool);
void virThreadPoolFree(virThreadPoolPtr pool);
+void virThreadPoolLock(virThreadPoolPtr pool);
+void virThreadPoolUnlock(virThreadPoolPtr pool);
+
int virThreadPoolSendJob(virThreadPoolPtr pool,
unsigned int priority,
void *jobdata) ATTRIBUTE_NONNULL(1)
ATTRIBUTE_RETURN_CHECK;
+int virThreadPoolSendJobLocked(virThreadPoolPtr pool,
+ unsigned int priority,
+ void *jobData) ATTRIBUTE_NONNULL(1)
+ ATTRIBUTE_RETURN_CHECK;
int virThreadPoolSetParameters(virThreadPoolPtr pool,
long long int minWorkers,
--
2.13.4
9:52 a.m.
New subject: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
Semantically, there is no difference between an uninitialized worker
pool and an initialized worker pool with zero workers. Let's allow the
worker pool to be initialized for max_workers=0 as well then which
makes the API more symmetric and simplifies code. Validity of the
worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
This patch fixes segmentation faults in
virNetServerGetThreadPoolParameters and
virNetServerSetThreadPoolParameters for the case when no worker pool
is actually initialized (max_workers=0).
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/libvirt_private.syms | 4 +++
src/rpc/virnetserver.c | 13 ++++-----
src/util/virthreadpool.c | 73 ++++++++++++++++++++++++++++++++++++------------
src/util/virthreadpool.h | 8 ++++++
4 files changed, 73 insertions(+), 25 deletions(-)
So it seems there's multiple things going on in this patch - maybe
they're semantically tied and I'm missing it ;-)...
The virNetServerNew to not inhibit the call to virThreadPoolNew if
max_workers == 0 would seem to be easily separable with the other places
that would check if srv->workers == NULL before continuing.
I don't understand why virNetServerDispatchNewMessage needs anything
more than if (virThreadPoolGetMaxWorkers(srv->workers)
If I think back to the previous patch, then why not:
size_t workers = 0;
...
if (srv->workers)
workers = virThreadPoolGetMaxWorkers(srv->workers);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* else disposes of it before we use it. */
virObjectRef(srv);
virObjectUnlock(srv);
if (workers > 0) {
...
In the long run, I don't think it's been the desire to expose so many
virthreadpool.c API's - especially the locks.
John
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ffe5dfd19b11..aa496ddf8012 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3005,11 +3005,15 @@ virThreadPoolGetCurrentWorkers;
virThreadPoolGetFreeWorkers;
virThreadPoolGetJobQueueDepth;
virThreadPoolGetMaxWorkers;
+virThreadPoolGetMaxWorkersLocked;
virThreadPoolGetMinWorkers;
virThreadPoolGetPriorityWorkers;
+virThreadPoolLock;
virThreadPoolNewFull;
virThreadPoolSendJob;
+virThreadPoolSendJobLocked;
virThreadPoolSetParameters;
+virThreadPoolUnlock;
# util/virtime.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 091e3ef23406..fdee08fee7cd 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -203,7 +203,8 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
* of disposing @srv */
virObjectUnlock(srv);
- if (srv->workers) {
+ virThreadPoolLock(srv->workers);
+ if (virThreadPoolGetMaxWorkersLocked(srv->workers) > 0) {
virNetServerJobPtr job;
if (VIR_ALLOC(job) < 0)
@@ -218,7 +219,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
}
virObjectRef(client);
- if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
+ if (virThreadPoolSendJobLocked(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
virObjectUnref(prog);
@@ -228,10 +229,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
goto error;
}
+ virThreadPoolUnlock(srv->workers);
return;
error:
+ virThreadPoolUnlock(srv->workers);
virNetMessageFree(msg);
virNetServerClientClose(client);
}
@@ -363,8 +366,7 @@ virNetServerPtr virNetServerNew(const char *name,
if (!(srv = virObjectLockableNew(virNetServerClass)))
return NULL;
- if (max_workers &&
- !(srv->workers = virThreadPoolNew(min_workers, max_workers,
+ if (!(srv->workers = virThreadPoolNew(min_workers, max_workers,
priority_workers,
virNetServerHandleJob,
srv)))
@@ -575,21 +577,18 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv)
goto error;
if (virJSONValueObjectAppendNumberUint(object, "min_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetMinWorkers(srv->workers))
< 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set min_workers data in JSON document"));
goto error;
}
if (virJSONValueObjectAppendNumberUint(object, "max_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetMaxWorkers(srv->workers))
< 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set max_workers data in JSON document"));
goto error;
}
if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
- srv->workers == NULL ? 0 :
virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set priority_workers data in JSON
document"));
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 10f2bd2c3a03..f18eafb35deb 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -318,17 +318,27 @@ size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool)
return ret;
}
-size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
+
+size_t
+virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool)
+{
+ return pool->maxWorkers;
+}
+
+
+size_t
+virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
{
size_t ret;
virMutexLock(&pool->mutex);
- ret = pool->maxWorkers;
+ ret = virThreadPoolGetMaxWorkersLocked(pool);
virMutexUnlock(&pool->mutex);
return ret;
}
+
size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool)
{
size_t ret;
@@ -373,27 +383,38 @@ size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool)
return ret;
}
-/*
- * @priority - job priority
- * Return: 0 on success, -1 otherwise
- */
-int virThreadPoolSendJob(virThreadPoolPtr pool,
- unsigned int priority,
- void *jobData)
+
+void
+virThreadPoolLock(virThreadPoolPtr pool)
+{
+ virMutexLock(&pool->mutex);
+}
+
+
+void
+virThreadPoolUnlock(virThreadPoolPtr pool)
+{
+ virMutexUnlock(&pool->mutex);
+}
+
+
+int
+virThreadPoolSendJobLocked(virThreadPoolPtr pool,
+ unsigned int priority,
+ void *jobData)
{
virThreadPoolJobPtr job;
- virMutexLock(&pool->mutex);
if (pool->quit)
- goto error;
+ return -1;
if (pool->freeWorkers - pool->jobQueueDepth <= 0 &&
pool->nWorkers < pool->maxWorkers &&
virThreadPoolExpand(pool, 1, false) < 0)
- goto error;
+ return -1;
if (VIR_ALLOC(job) < 0)
- goto error;
+ return -1;
job->data = jobData;
job->priority = priority;
@@ -415,14 +436,30 @@ int virThreadPoolSendJob(virThreadPoolPtr pool,
if (priority)
virCondSignal(&pool->prioCond);
- virMutexUnlock(&pool->mutex);
return 0;
-
- error:
- virMutexUnlock(&pool->mutex);
- return -1;
}
+
+/*
+ * @priority - job priority
+ * Return: 0 on success, -1 otherwise
+ */
+int
+virThreadPoolSendJob(virThreadPoolPtr pool,
+ unsigned int priority,
+ void *jobData)
+{
+ int ret;
+
+ virMutexLock(&pool->mutex);
+ ret = virThreadPoolSendJobLocked(pool,
+ priority,
+ jobData);
+ virMutexUnlock(&pool->mutex);
+ return ret;
+}
+
+
int
virThreadPoolSetParameters(virThreadPoolPtr pool,
long long int minWorkers,
diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h
index e1f362f5bb60..2d65c2bc4ac3 100644
--- a/src/util/virthreadpool.h
+++ b/src/util/virthreadpool.h
@@ -45,6 +45,7 @@ virThreadPoolPtr virThreadPoolNewFull(size_t minWorkers,
size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool);
size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool);
+size_t virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool);
size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool);
size_t virThreadPoolGetCurrentWorkers(virThreadPoolPtr pool);
size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool);
@@ -52,10 +53,17 @@ size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool);
void virThreadPoolFree(virThreadPoolPtr pool);
+void virThreadPoolLock(virThreadPoolPtr pool);
+void virThreadPoolUnlock(virThreadPoolPtr pool);
+
int virThreadPoolSendJob(virThreadPoolPtr pool,
unsigned int priority,
void *jobdata) ATTRIBUTE_NONNULL(1)
ATTRIBUTE_RETURN_CHECK;
+int virThreadPoolSendJobLocked(virThreadPoolPtr pool,
+ unsigned int priority,
+ void *jobData) ATTRIBUTE_NONNULL(1)
+ ATTRIBUTE_RETURN_CHECK;
int virThreadPoolSetParameters(virThreadPoolPtr pool,
long long int minWorkers,
2:47 a.m.
New subject: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
On Thu, Jul 19, 2018 at 04:52 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> Semantically, there is no difference between an uninitialized worker
> pool and an initialized worker pool with zero workers. Let's allow the
> worker pool to be initialized for max_workers=0 as well then which
> makes the API more symmetric and simplifies code. Validity of the
> worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
>
> This patch fixes segmentation faults in
> virNetServerGetThreadPoolParameters and
> virNetServerSetThreadPoolParameters for the case when no worker pool
> is actually initialized (max_workers=0).
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> ---
> src/libvirt_private.syms | 4 +++
> src/rpc/virnetserver.c | 13 ++++-----
> src/util/virthreadpool.c | 73 ++++++++++++++++++++++++++++++++++++------------
> src/util/virthreadpool.h | 8 ++++++
> 4 files changed, 73 insertions(+), 25 deletions(-)
>
So it seems there's multiple things going on in this patch - maybe
they're semantically tied and I'm missing it ;-)...
The virNetServerNew to not inhibit the call to virThreadPoolNew if
max_workers == 0 would seem to be easily separable with the other places
that would check if srv->workers == NULL before continuing.
Is the code still safe if the worker count changes after getting the
worker count?
If yes, then I can split the patch if you want to.
I don't understand why virNetServerDispatchNewMessage needs anything
more than if (virThreadPoolGetMaxWorkers(srv->workers)
If I think back to the previous patch, then why not:
size_t workers = 0;
...
if (srv->workers)
workers = virThreadPoolGetMaxWorkers(srv->workers);
The reason is/was that my original code assumed it’s allowed to switch
between zero and non-zero worker counts. My intention was to hold the
lock for srv->workers (and therefore forbid any changes on the worker
count) as long as needed.
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* else disposes of it before we use it. */
virObjectRef(srv);
virObjectUnlock(srv);
if (workers > 0) {
...
In the long run, I don't think it's been the desire to expose so many
virthreadpool.c API's - especially the locks.
Yes, I don’t like it either.
John
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ffe5dfd19b11..aa496ddf8012 100644
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
7:04 a.m.
New subject: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well
On 07/20/2018 03:47 AM, Marc Hartmayer wrote:
On Thu, Jul 19, 2018 at 04:52 PM +0200, John Ferlan
<jferlan(a)redhat.com> wrote:
> On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
>> Semantically, there is no difference between an uninitialized worker
>> pool and an initialized worker pool with zero workers. Let's allow the
>> worker pool to be initialized for max_workers=0 as well then which
>> makes the API more symmetric and simplifies code. Validity of the
>> worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.
>>
>> This patch fixes segmentation faults in
>> virNetServerGetThreadPoolParameters and
>> virNetServerSetThreadPoolParameters for the case when no worker pool
>> is actually initialized (max_workers=0).
>>
>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
>> ---
>> src/libvirt_private.syms | 4 +++
>> src/rpc/virnetserver.c | 13 ++++-----
>> src/util/virthreadpool.c | 73 ++++++++++++++++++++++++++++++++++++------------
>> src/util/virthreadpool.h | 8 ++++++
>> 4 files changed, 73 insertions(+), 25 deletions(-)
>>
>
> So it seems there's multiple things going on in this patch - maybe
> they're semantically tied and I'm missing it ;-)...
>
> The virNetServerNew to not inhibit the call to virThreadPoolNew if
> max_workers == 0 would seem to be easily separable with the other places
> that would check if srv->workers == NULL before continuing.
Is the code still safe if the worker count changes after getting the
worker count?
If yes, then I can split the patch if you want to.
I have pushed patch 1, 3-6 - so I think "order wise" we're perhaps safer
at least since there's checking w/r/t various setting when max is = 0 or
an attempt to set max = 0.
Beyond that, if you grab max workers before unlocking @srv, then I would
think you're as safe as you would be if you kept the log throughout the
top half of the if statement. We know we cannot dispose of srv and that
later on if something has called it quits nothing is going to be added
anyway.
John
>
> I don't understand why virNetServerDispatchNewMessage needs anything
> more than if (virThreadPoolGetMaxWorkers(srv->workers)
>
> If I think back to the previous patch, then why not:
>
> size_t workers = 0;
> ...
>
> if (srv->workers)
> workers = virThreadPoolGetMaxWorkers(srv->workers);
The reason is/was that my original code assumed it’s allowed to switch
between zero and non-zero worker counts. My intention was to hold the
lock for srv->workers (and therefore forbid any changes on the worker
count) as long as needed.
>
> /* we can unlock @srv since @prog can only become invalid in case
> * of disposing @srv, but let's grab a ref first to ensure nothing
> * else disposes of it before we use it. */
> virObjectRef(srv);
> virObjectUnlock(srv);
>
> if (workers > 0) {
> ...
>
> In the long run, I don't think it's been the desire to expose so many
> virthreadpool.c API's - especially the locks.
Yes, I don’t like it either.
>
> John
>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ffe5dfd19b11..aa496ddf8012 100644
[…snip]
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:37 a.m.
New subject: [libvirt] [PATCH v2 3/6] virThreadPool: Prevent switching between zero and non-zero maxWorkers
...since maxWorkers=0 is only intended for virtlockd or virlogd which
must not be multithreaded.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/util/virthreadpool.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index f18eafb35deb..e615c8c07c82 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -479,6 +479,14 @@ virThreadPoolSetParameters(virThreadPoolPtr pool,
goto error;
}
+ if ((maxWorkers == 0 && pool->maxWorkers > 0) ||
+ (maxWorkers > 0 && pool->maxWorkers == 0)) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("maxWorkers must not be switched from zero to
non-zero"
+ " and vice versa"));
+ goto error;
+ }
+
if (minWorkers >= 0) {
if ((size_t) minWorkers > pool->nWorkers &&
virThreadPoolExpand(pool, minWorkers - pool->nWorkers,
--
2.13.4
9:53 a.m.
New subject: [libvirt] [PATCH v2 3/6] virThreadPool: Prevent switching between zero and non-zero maxWorkers
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
...since maxWorkers=0 is only intended for virtlockd or virlogd
which
must not be multithreaded.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/util/virthreadpool.c | 8 ++++++++
1 file changed, 8 insertions(+)
I think this is separable - this could be pushed regardless of the
outcome of patch2, right? Let me know - I can reduce the load the next
update based on patch2 comments.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:48 a.m.
New subject: [libvirt] [PATCH v2 3/6] virThreadPool: Prevent switching between zero and non-zero maxWorkers
On Thu, Jul 19, 2018 at 04:53 PM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> ...since maxWorkers=0 is only intended for virtlockd or virlogd which
> must not be multithreaded.
>
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
> ---
> src/util/virthreadpool.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
I think this is separable - this could be pushed regardless of the
outcome of patch2, right? Let me know - I can reduce the load the next
update based on patch2 comments.
Yep. Thanks for reviewing.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
6:37 a.m.
New subject: [libvirt] [PATCH v2 4/6] daemon: Raise an error if 'max_workers' < 1 in libvirtd.conf
Hypervisor drivers (e.g. QEMU) assume that they run in a separate
thread from the main event loop thread otherwise deadlocks can
occur. Therefore let's report an error if max_workers < 1 is set in
the libvirtd configuration file.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/remote/remote_daemon_config.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c
index b1516befb4a6..27e0c635f1be 100644
--- a/src/remote/remote_daemon_config.c
+++ b/src/remote/remote_daemon_config.c
@@ -337,6 +337,11 @@ daemonConfigLoadOptions(struct daemonConfig *data,
goto error;
if (virConfGetValueUInt(conf, "max_workers", &data->max_workers)
< 0)
goto error;
+ if (data->max_workers < 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("'max_workers' must be greater than 0"));
+ goto error;
+ }
if (virConfGetValueUInt(conf, "max_clients", &data->max_clients)
< 0)
goto error;
if (virConfGetValueUInt(conf, "max_queued_clients",
&data->max_queued_clients) < 0)
--
2.13.4
9:53 a.m.
New subject: [libvirt] [PATCH v2 4/6] daemon: Raise an error if 'max_workers' < 1 in libvirtd.conf
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
Hypervisor drivers (e.g. QEMU) assume that they run in a separate
thread from the main event loop thread otherwise deadlocks can
occur. Therefore let's report an error if max_workers < 1 is set in
the libvirtd configuration file.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/remote/remote_daemon_config.c | 5 +++++
1 file changed, 5 insertions(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
6:37 a.m.
New subject: [libvirt] [PATCH v2 5/6] virt-admin: Fix two error messages
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
tools/virt-admin.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index c86b5763a73c..107a1d870df5 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -552,7 +552,8 @@ cmdSrvThreadpoolSet(vshControl *ctl, const vshCmd *cmd)
VIR_THREADPOOL_WORKERS_MAX, &max) &&
virTypedParamsGetUInt(params, nparams,
VIR_THREADPOOL_WORKERS_MIN, &min) && min >
max) {
- vshError(ctl, "%s", _("--min-workers must be less than
--max-workers"));
+ vshError(ctl, "%s", _("--min-workers must be less than or equal to
"
+ "--max-workers"));
goto cleanup;
}
@@ -952,7 +953,7 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd)
virTypedParamsGetUInt(params, nparams,
VIR_SERVER_CLIENTS_UNAUTH_MAX, &unauth_max) &&
unauth_max > max) {
- vshError(ctl, "%s", _("--max-unauth-clients must be less than
"
+ vshError(ctl, "%s", _("--max-unauth-clients must be less than or
equal to "
"--max-clients"));
goto cleanup;
}
--
2.13.4
9:53 a.m.
New subject: [libvirt] [PATCH v2 5/6] virt-admin: Fix two error messages
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
tools/virt-admin.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
6:37 a.m.
New subject: [libvirt] [PATCH v2 6/6] rpc: Fix name of include guard
The include guard should match the file name and comment.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserverprogram.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserverprogram.h b/src/rpc/virnetserverprogram.h
index c2a5635c959b..03784478de0b 100644
--- a/src/rpc/virnetserverprogram.h
+++ b/src/rpc/virnetserverprogram.h
@@ -21,8 +21,8 @@
* Author: Daniel P. Berrange <berrange(a)redhat.com>
*/
-#ifndef __VIR_NET_PROGRAM_H__
-# define __VIR_NET_PROGRAM_H__
+#ifndef __VIR_NET_SERVER_PROGRAM_H__
+# define __VIR_NET_SERVER_PROGRAM_H__
# include "virnetmessage.h"
# include "virnetserverclient.h"
--
2.13.4
9:53 a.m.
New subject: [libvirt] [PATCH v2 6/6] rpc: Fix name of include guard
On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
The include guard should match the file name and comment.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/rpc/virnetserverprogram.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
9:30 a.m.
On Tue, Jul 03, 2018 at 01:37 PM +0200, Marc Hartmayer <mhartmay(a)linux.ibm.com>
wrote:
One way to reproduce the bugs is to set admin_max_workers=0 in
libvirtd.conf, restart libvirtd, and then call:
$ virt-admin server-threadpool-info admin
Changelog:
v1->v2:
- Worked in Daniel Berrangé's comments that:
1. max_workers=0 should not be allowed for the libvirtd (patch 3)
2. it shouldn't be allowed to switch between zero and non-zero
max_workers (patch 4)
- Changed the deadlock fix (patch 1)
- Added two small fixes (patch 5 and 6)
Marc Hartmayer (6):
rpc: Fix deadlock if there is no worker pool available
rpc: Initialize a worker pool for max_workers=0 as well
virThreadPool: Prevent switching between zero and non-zero maxWorkers
daemon: Raise an error if 'max_workers' < 1 in libvirtd.conf
virt-admin: Fix two error messages
rpc: Fix name of include guard
src/libvirt_private.syms | 4 ++
src/remote/remote_daemon_config.c | 5 +++
src/rpc/virnetserver.c | 28 ++++++++------
src/rpc/virnetserverprogram.h | 4 +-
src/util/virthreadpool.c | 81 ++++++++++++++++++++++++++++++---------
src/util/virthreadpool.h | 8 ++++
tools/virt-admin.c | 5 ++-
7 files changed, 101 insertions(+), 34 deletions(-)
--
2.13.4
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Polite ping (and added Daniel on CC since he had comments on v1).
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2316
days inactive
2334
days old
17 comments
2 participants
participants (2)
-
John Ferlan -
Marc Hartmayer