On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/locking/lock_daemon_dispatch.c | 12 ++++++++++--
> src/locking/lock_driver_lockd.c | 31 +++++++++++++++++++++----------
> src/locking/lock_driver_lockd.h | 1 +
> 3 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c
> index 10248ec0b5..c24571ceac 100644
> --- a/src/locking/lock_daemon_dispatch.c
> +++ b/src/locking/lock_daemon_dispatch.c
> @@ -37,6 +37,9 @@ VIR_LOG_INIT("locking.lock_daemon_dispatch");
>
> #include "lock_daemon_dispatch_stubs.h"
>
> +#define DEFAULT_OFFSET 0
> +#define METADATA_OFFSET 1
> +
Curious, does this lead to the prospect of being able to acquire/use
offset==0 length==1 and offset==1 length==1 at least conceptually and
granularity wise?
Related or not, there's a strange NFSv3 collision between QEMU and NFS
related to some sort of fcntl locking granularity. More details that you
possibly want to read at:
https://bugzilla.redhat.com/show_bug.cgi?id=1592582
https://bugzilla.redhat.com/show_bug.cgi?id=1547095
I only note it because well it was on my 'short term history' radar and
suffice to say it's an ugly and not fun issue dealing with these locks.
> static int
> virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNUSED,
> virNetServerClientPtr client,
> @@ -50,13 +53,14 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU
> virNetServerClientGetPrivateData(client);
> virLockSpacePtr lockspace;
> unsigned int newFlags;
> - off_t start = 0;
> + off_t start = DEFAULT_OFFSET;
> off_t len = 1;
>
> virMutexLock(&priv->lock);
>
> virCheckFlagsGoto(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
> - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE, cleanup);
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA, cleanup);
>
> if (priv->restricted) {
> virReportError(VIR_ERR_OPERATION_DENIED, "%s",
> @@ -82,6 +86,10 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU
> newFlags |= VIR_LOCK_SPACE_ACQUIRE_SHARED;
> if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE)
> newFlags |= VIR_LOCK_SPACE_ACQUIRE_AUTOCREATE;
> + if (flags & VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA) {
> + start = METADATA_OFFSET;
> + newFlags |= VIR_LOCK_SPACE_ACQUIRE_WAIT;
> + }
If this is documented in more detail, then it should be noted that a
METADATA lock forces usage of the wait API's. I can only imagine
someone, some day will ask for a wait w/ timeout to avoid waiting too long.
>
> if (virLockSpaceAcquireResource(lockspace,
> args->name,
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 957a963a7b..bd14ed8930 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -475,9 +475,11 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
> bool autoCreate = false;
>
> virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
> - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1);
> + VIR_LOCK_MANAGER_RESOURCE_SHARED |
> + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);
>
> - if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
> + if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY &&
> + !(flags & VIR_LOCK_MANAGER_RESOURCE_METADATA))
> return 0;
Could someone pass READONLY & METADATA and not return 0 here? Yes,
doesn't make sense, but combining them leads to the logic matrix question.
>
> switch (type) {
> @@ -489,7 +491,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
> }
> if (!driver->autoDiskLease) {
> if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
> - VIR_LOCK_MANAGER_RESOURCE_READONLY)))
> + VIR_LOCK_MANAGER_RESOURCE_READONLY |
> + VIR_LOCK_MANAGER_RESOURCE_METADATA)))
> priv->hasRWDisks = true;
> return 0;
> }
> @@ -602,6 +605,10 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
> priv->resources[priv->nresources-1].flags |=
> VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
>
> + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA)
> + priv->resources[priv->nresources-1].flags |=
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA;
> +
> return 0;
>
> error:
> @@ -626,12 +633,15 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
> virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY |
> VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1);
>
> - if (priv->nresources == 0 &&
> - priv->hasRWDisks &&
> - driver->requireLeaseForDisks) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("Read/write, exclusive access, disks were present, but no leases specified"));
> - return -1;
> + if (priv->nresources == 0) {
I'm assuming there's a relationship between metadata and nresources == 0
which really isn't apparent especially since the subsequent error
message is about leases. Do we need to check for resource type?
Or in the top level API do we need to only allow METADATA for DISK type?
> + if (priv->hasRWDisks &&
> + driver->requireLeaseForDisks) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Read/write, exclusive access, disks were present, but no leases specified"));
> + return -1;
> + }
> +
> + return 0;
> }
>
> if (!(client = virLockManagerLockDaemonConnect(lock, &program, &counter)))
> @@ -711,7 +721,8 @@ static int virLockManagerLockDaemonRelease(virLockManagerPtr lock,
>
> args.flags &=
> ~(VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED |
> - VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE);
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE |
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA);
>
> if (virNetClientProgramCall(program,
> client,
> diff --git a/src/locking/lock_driver_lockd.h b/src/locking/lock_driver_lockd.h
> index 6931fe7425..9882793260 100644
> --- a/src/locking/lock_driver_lockd.h
> +++ b/src/locking/lock_driver_lockd.h
> @@ -25,6 +25,7 @@
> enum virLockSpaceProtocolAcquireResourceFlags {
> VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED = (1 << 0),
> VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE = (1 << 1),
> + VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_METADATA = (1 << 2),
Use of RESOURCE_METADATA would be more consistent wouldn't it?
John
I'll continue looking tomorrow...
> };
>
> #endif /* __VIR_LOCK_DRIVER_LOCKD_H__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
