2:25 a.m.
On 08/17/2018 01:53 AM, John Ferlan wrote:
On 08/14/2018 07:19 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> 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?
Yes, this is exactly the purpose. I've taken inspiration from qemu. They
lock bytes from 100 to 100 + N, depending on what operations they want
the disk for (checkout raw_apply_lock_bytes() form block/file-posix.c).
And my idea was for virtlockd to do the same. Currently, it locks 0th
byte of given file and with metadata flag it would lock the first one.
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.
Oh yeah, NFSv3 and file locks. Well, I'm not making our situation any
worse here. Qemu is already locking disks, and it is doing so for the
while lifetime of the domain whereas my patches would lock for only a
fraction of a second (well, as long as it takes for chown() and
setfilecon_raw() to run). It is very unlikely that connection to the
NFSv3 server will be lost right when metadata lock is held.
> 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.
That should never happen (TM). The metadata lock is acquired only for
time that seclabels are set on some domain paths (and as we will see in
6/7 we are not going to lock all the paths). As soon as they are set the
lock is released.
>
> 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.
So far there isn't such caller, but sure I can add an explicit check
that errors out if both READONLY and METADATA are set at the same time.
>
> 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?
Well, nresources == 0 can happen regardless of METADATA flag if
autoDiskLease is turned off. In that case no domain disk is added into
@resources and thus nresources can be zero. This is the situation where
we require users to put <lease/> into domain XML which will then protect
them.
By the same token, when locking for METADATA, we can not ignore
autoDiskLease setting (because if user has turned off disk leases they
probably did not set any lockspace dir and certainly did not put it onto
shared NFS, as required by our docs
https://libvirt.org/locking-lockd.html#lockdplugin) and we can't lock
files automatically. We must rely on <lease/>. However, we shouldn't
fail because from METADATA POV we don't care if a disk is RO or RW.
Long story short, if users want to rely on <lease/> solely we should
honour that.
> + 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?
Oh, okay.
Michal