On 08/14/2018 07:19 AM, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8b7cccc521..7c8f79520a 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -56,6 +56,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0), /* The resource is assigned in shared, writable mode */ VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1), + /* The resource is locked for metadata change */ + VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2),

Does this work or make sense for lease type? Of course this adds something that would "conceivably" be available such that we may want to document it on https://libvirt.org/internals/locking.html, but then again that's so sparse it would take a bit of spelunking in the code to figure it out. Anyway, since the name itself seems reasonable to me, Reviewed-by: John Ferlan <jferlan@redhat.com> if you add a few more details around it related to disk vs. lease that'd be nice. If you update the docs eventually with all that you've learned, then that'd be great too! John

} virLockManagerResourceFlags;

typedef enum {

On Mon, Aug 20, 2018 at 09:25:18AM +0200, Michal Prívozník wrote:

On 08/17/2018 12:24 AM, John Ferlan wrote:

...

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver.h | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8b7cccc521..7c8f79520a 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -56,6 +56,8 @@ typedef enum { VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0), /* The resource is assigned in shared, writable mode */ VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1), + /* The resource is locked for metadata change */ + VIR_LOCK_MANAGER_RESOURCE_METADATA = (1 << 2),

Does this work or make sense for lease type?

That's the thing. You're right, it doesn't make sense for lease type. But on the level of RPC of virtlockd both leases and disks kind of blend together. I mean, virtlockd is merely told to lock this or that file.

That's ok. The way I look at it the domain XML describes various resources, disks, leases, etc. The virtlockd daemon provides a generic mechanism for acquiring locks. We just happen to map leases onto the default lock type. Here we're adding a 2nd lock type and do not require any mapping from leases. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

Signed-off-by: Michal Privoznik <mprivozn@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? 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__ */

On Mon, Aug 20, 2018 at 09:25:13AM +0200, Michal Prívozník wrote:

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@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.

Agreed, NFSv3 is already doomed, and this barely makes it worse because of the short lock time. NFSv4 is the fix for anyone who really cares. Better to have stale NFSv3 locks due to dead clients not releasing locks than to loose all your VM disk due to corruption. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

No real support implemented here. But hey, at least we will not fail.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..c1996fb937 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerSanlockPrivatePtr priv = lock->privateData;

virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);

if (priv->res_count == SANLK_MAX_RESOURCES) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0;

+ /* No metadata locking support for now. + * TODO: implement it. */ + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + return 0; +

Doesn't this give someone the false impression that their resource is locked if they choose METADATA? Something doesn't feel right about that - giving the impression that it's supported and the consumer is protected, but when push comes to shove they aren't. I'd be inclined to believe that we may want to do nothing with/for sanlock allowing the virCheckFlags above take care of the consumer.

switch (type) { case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: if (driver->autoDiskLease) { @@ -953,12 +959,17 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_RESTRICT | VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY, -1);

- if (priv->res_count == 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->res_count == 0) { + if (priv->hasRWDisks && + driver->requireLeaseForDisks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Read/write, exclusive access, disks were present, but no leases specified")); + return -1; + } + + /* We are not handling METADATA flag yet. So no resources + * case is no-op for now. */ + return 0;

Similar comment to patch4 w/r/t resource type (e.g. disk or lease), but now at least it's more obvious to me that hasRWDisks means lock for disk vs. not. Still it's odd to think that returning 0, but not actually getting the lock is the "right" thing to do. "Theoretically", if AddResource failed, then would Acquire ever be called w/ this flag?

}

/* We only initialize 'sock' if we are in the real

BTW: Now that I think about them together... - lock_driver_lockd.h - extract from patch4 and merge to patch3. I suppose it could be separate too to keep mgmt vs. space separate, but they're related enough to keep them together. Perhaps this is where a few more "words" about usage expectations as part of the commit message. - lock_daemon_dispatch.c - extract from patch4 and whether it goes before or after is one of those chicken/egg type quandaries. Keeping it with the fcntl/lockd as opposed to the sanlock implementation doesn't feel right (even though sanlock doesn't use any LOCK_SPACE symbols). I can be swayed otherwise... maybe someone else will pipe in. John

On 08/20/2018 09:25 AM, Michal Prívozník wrote:

On 08/17/2018 04:49 PM, John Ferlan wrote:

...

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

...

No real support implemented here. But hey, at least we will not fail.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..c1996fb937 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerSanlockPrivatePtr priv = lock->privateData;

virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);

if (priv->res_count == SANLK_MAX_RESOURCES) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0;

+ /* No metadata locking support for now. + * TODO: implement it. */ + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + return 0; +

Doesn't this give someone the false impression that their resource is locked if they choose METADATA?

Okay, I'll provide some implementation. Even though sanlock is not really my cup of coffee :-)

Actually, sanlock is doomed. It allows us to set the initial offset (with posing some weird restrictions on it) and it doesn't allow us to specify length we want to lock (it looks like it locks whole disk sector). Therefore we can't lock offsets 0 and 1 independently. D'oh! Maybe we could lock offsets 0 and 1MB but I'm unsure how sanlock behaves when offset is outside of the file (we can't safely assume that every file we will try to lock is at least 1MB big, can we? No we can't! OVMF_VARS is only 128KB long). https://linux.die.net/man/8/sanlock and scroll down to offsets. Therefore I rather stick with my dummy implementation and have somebody else look at this. Somebody who understands how sanlock works. Michal

On Mon, Aug 20, 2018 at 03:59:23PM +0100, Daniel P. Berrangé wrote:

On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote:

...

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

...

No real support implemented here. But hey, at least we will not fail.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..c1996fb937 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerSanlockPrivatePtr priv = lock->privateData;

virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);

if (priv->res_count == SANLK_MAX_RESOURCES) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0;

+ /* No metadata locking support for now. + * TODO: implement it. */ + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + return 0; +

Doesn't this give someone the false impression that their resource is locked if they choose METADATA?

Something doesn't feel right about that - giving the impression that it's supported and the consumer is protected, but when push comes to shove they aren't.

I'd be inclined to believe that we may want to do nothing with/for sanlock allowing the virCheckFlags above take care of the consumer.

Yeah, this doesn't feel right to me. I think we need to treat the metadata locking as completely independant of the content locking. This implies we should have a separate configuration for metadata locking, where the only valid options are "lockd" or "nop".

eg our available matrix looks like

METADATA | nop lockd sanlock ---------+-------------------- nop | Y Y N CONTENT lockd | Y Y N sanlock | Y Y N

Oh and even for virtlockd we need to consider the config separately for content vs metdata. For content we have the possibility to lock out of band files as a proxy for the main file. This is primarily done for protecting shared blocks devices as locking the device node doesn't apply across hosts. For metadata locking, we only ever care about the local device node as changes to labelling/ownership don't propagate across hosts. IOW, we should always directly lock the file in question for metadata locking, and never use an out of band proxy for locking. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Aug 20, 2018 at 07:29:30PM +0200, Michal Prívozník wrote:

On 08/20/2018 05:04 PM, Daniel P. Berrangé wrote:

...

On Mon, Aug 20, 2018 at 03:59:23PM +0100, Daniel P. Berrangé wrote:

...

On Fri, Aug 17, 2018 at 10:49:12AM -0400, John Ferlan wrote:

...

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

...

No real support implemented here. But hey, at least we will not fail.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/locking/lock_driver_sanlock.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 3e5f0e37b0..c1996fb937 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -791,7 +791,8 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, virLockManagerSanlockPrivatePtr priv = lock->privateData;

virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | - VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); + VIR_LOCK_MANAGER_RESOURCE_SHARED | + VIR_LOCK_MANAGER_RESOURCE_METADATA, -1);

if (priv->res_count == SANLK_MAX_RESOURCES) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -804,6 +805,11 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) return 0;

+ /* No metadata locking support for now. + * TODO: implement it. */ + if (flags & VIR_LOCK_MANAGER_RESOURCE_METADATA) + return 0; +

Doesn't this give someone the false impression that their resource is locked if they choose METADATA?

Something doesn't feel right about that - giving the impression that it's supported and the consumer is protected, but when push comes to shove they aren't.

I'd be inclined to believe that we may want to do nothing with/for sanlock allowing the virCheckFlags above take care of the consumer.

Yeah, this doesn't feel right to me. I think we need to treat the metadata locking as completely independant of the content locking. This implies we should have a separate configuration for metadata locking, where the only valid options are "lockd" or "nop".

eg our available matrix looks like

METADATA | nop lockd sanlock ---------+-------------------- nop | Y Y N CONTENT lockd | Y Y N sanlock | Y Y N

Having some troubles parsing the table. Do you mean:

| Content | Metadata ---------+------------------- nop | Y | Y lockd | Y | Y sanlock | Y | N

Where Y says the respective type (content/metadata) can/cannot be locked by lock driver in question? I.e. we would have 'metadata_lock_manager' config value in qemu.conf and it'd accept only 'nop' and 'lockd'.

Heh, ok, yes, that is a simpler table :-)

Then we can have 'metadata_lockspace_dir' in /etc/libvirt/qemu-lockd.conf where the lockspace would be created.

No need for a metadata_lockspace_dir, since we should always be locking the resource itself, never an out of band proxy file.

...

Oh and even for virtlockd we need to consider the config separately for content vs metdata.

Do you mean like completely new config file? qemu-lockd-metadata.conf? Okay. So far we only need one config option (metadata_lockspace_dir) but it might turn out we need more and it would make sens to keep options separated from content locking options.

I don't think we need a config file for now. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

In order for our drivers to lock resources for metadata change we need set of new APIs. Fortunately, we don't have to care about every possible device a domain can have. We care only about those which can live on a network filesystem and hence can be accessed by multiple daemons at the same time. These devices are covered in virDomainLockMetadataLock() and only a small fraction of those can be hotplugged (covered in the rest of the introduced APIs).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 8 ++ src/locking/domain_lock.c | 304 ++++++++++++++++++++++++++++++++++++++++++---- src/locking/domain_lock.h | 28 +++++ 3 files changed, 317 insertions(+), 23 deletions(-)

There seems to be more than just what's described going on in this patch which could be split out.... 1. Use @def in virDomainLockManagerNew 2. Add @metadataonly to virDomainLockManagerNew and virDomainLockManagerAddImage 3. Introduce virDomainLockManagerAddMemory and virDomainLockManagerAddFile along with changes to virDomainLockManagerNew along with perhaps a few words why those files were chosen and what decisions led to their selection. If someone adds something in the future how would they know to add them to the list? 4. Various virDomainLockMetadata* API's.

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca4a192a4a..720ae12301 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1284,6 +1284,14 @@ virDomainLockImageAttach; virDomainLockImageDetach; virDomainLockLeaseAttach; virDomainLockLeaseDetach; +virDomainLockMetadataDiskLock; +virDomainLockMetadataDiskUnlock; +virDomainLockMetadataImageLock; +virDomainLockMetadataImageUnlock; +virDomainLockMetadataLock; +virDomainLockMetadataMemLock; +virDomainLockMetadataMemUnlock; +virDomainLockMetadataUnlock; virDomainLockProcessInquire; virDomainLockProcessPause; virDomainLockProcessResume; diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 705b132457..19a097fb25 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -69,7 +69,8 @@ static int virDomainLockManagerAddLease(virLockManagerPtr lock,

static int virDomainLockManagerAddImage(virLockManagerPtr lock, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool metadataOnly) { unsigned int diskFlags = 0; int type = virStorageSourceGetActualType(src); @@ -82,10 +83,14 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, type == VIR_STORAGE_TYPE_DIR)) return 0;

- if (src->readonly) - diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; - if (src->shared) - diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; + if (metadataOnly) { + diskFlags = VIR_LOCK_MANAGER_RESOURCE_METADATA; + } else { + if (src->readonly) + diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; + if (src->shared) + diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; + }

VIR_DEBUG("Add disk %s", src->path); if (virLockManagerAddResource(lock, @@ -101,13 +106,68 @@ static int virDomainLockManagerAddImage(virLockManagerPtr lock, }

+static int +virDomainLockManagerAddMemory(virLockManagerPtr lock, + const virDomainMemoryDef *mem) +{ + const char *path = NULL; + + switch ((virDomainMemoryModel) mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: + path = mem->nvdimmPath;

Why not flip flop the order of new helpers and just call/return virDomainLockManagerAddFile directly with mem->nvdimmPath

+ break; + + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + case VIR_DOMAIN_MEMORY_MODEL_NONE: + break; + } + + if (!path) + return 0;

And then just return 0 here. or just call it here since it returns 0 if @!file anyway.

+ + VIR_DEBUG("Adding memory %s", path); + if (virLockManagerAddResource(lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + path, + 0, + NULL, + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0) + return -1; + + return 0; +} + + +static int +virDomainLockManagerAddFile(virLockManagerPtr lock, + const char *file) +{ + if (!file) + return 0; + + VIR_DEBUG("Adding file %s", file); + if (virLockManagerAddResource(lock, + VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, + file, + 0, + NULL, + VIR_LOCK_MANAGER_RESOURCE_METADATA) < 0) + return -1; + + return 0; +} + + static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, bool withResources, + bool metadataOnly, unsigned int flags) { virLockManagerPtr lock; + const virDomainDef *def = dom->def; size_t i; virLockManagerParam params[] = { { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UUID, @@ -115,11 +175,11 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_STRING, .key = "name", - .value = { .str = dom->def->name }, + .value = { .str = def->name }, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, .key = "id", - .value = { .iv = dom->def->id }, + .value = { .iv = def->id }, }, { .type = VIR_LOCK_MANAGER_PARAM_TYPE_UINT, .key = "pid", @@ -133,7 +193,7 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p withResources=%d", plugin, dom, withResources);

- memcpy(params[0].value.uuid, dom->def->uuid, VIR_UUID_BUFLEN); + memcpy(params[0].value.uuid, def->uuid, VIR_UUID_BUFLEN);

if (!(lock = virLockManagerNew(virLockManagerPluginGetDriver(plugin), VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN, @@ -144,19 +204,46 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin,

if (withResources) { VIR_DEBUG("Adding leases"); - for (i = 0; i < dom->def->nleases; i++) - if (virDomainLockManagerAddLease(lock, dom->def->leases[i]) < 0) + for (i = 0; i < def->nleases; i++) + if (virDomainLockManagerAddLease(lock, def->leases[i]) < 0) goto error; + }

+ if (withResources || metadataOnly) {

So this would seemingly answer my question long ago - one could get one or both locks (@0 w/ length==1 and @1 w/ length==1)

VIR_DEBUG("Adding disks"); - for (i = 0; i < dom->def->ndisks; i++) { - virDomainDiskDefPtr disk = dom->def->disks[i]; + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i];

- if (virDomainLockManagerAddImage(lock, disk->src) < 0) + if (virDomainLockManagerAddImage(lock, disk->src, metadataOnly) < 0) goto error; } }

+ if (metadataOnly) { + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr mem = def->mems[i]; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto error; + } + + if (def->os.loader && + virDomainLockManagerAddFile(lock, def->os.loader->nvram) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.kernel) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.initrd) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.dtb) < 0) + goto error; + + if (virDomainLockManagerAddFile(lock, def->os.slic_table) < 0) + goto error; + } + return lock;

error: @@ -178,7 +265,7 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", plugin, dom, paused, fd);

- if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, false, VIR_LOCK_MANAGER_NEW_STARTED))) return -1;

@@ -203,7 +290,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state);

- if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, false, 0))) return -1;

ret = virLockManagerRelease(lock, state, 0); @@ -223,7 +310,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%s", plugin, dom, NULLSTR(state));

- if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true, false, 0))) return -1;

ret = virLockManagerAcquire(lock, state, 0, dom->def->onLockFailure, NULL); @@ -242,7 +329,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state);

- if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true, false, 0))) return -1;

ret = virLockManagerInquire(lock, state, 0); @@ -262,10 +349,10 @@ int virDomainLockImageAttach(virLockManagerPluginPtr plugin,

VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src);

- if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, false, 0))) return -1;

- if (virDomainLockManagerAddImage(lock, src) < 0) + if (virDomainLockManagerAddImage(lock, src, false) < 0) goto cleanup;

if (virLockManagerAcquire(lock, NULL, 0, @@ -299,10 +386,10 @@ int virDomainLockImageDetach(virLockManagerPluginPtr plugin,

VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src);

- if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) return -1;

- if (virDomainLockManagerAddImage(lock, src) < 0) + if (virDomainLockManagerAddImage(lock, src, false) < 0) goto cleanup;

if (virLockManagerRelease(lock, NULL, 0) < 0) @@ -336,7 +423,7 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease);

- if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false, false, 0))) return -1;

if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -364,7 +451,7 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease);

- if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, 0))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) return -1;

if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -380,3 +467,174 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin,

return ret; } + + +int +virDomainLockMetadataLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom) +{ + virLockManagerPtr lock; + const unsigned int flags = 0;

We never change this from 0 to something else (not even the next patch)... Not that it matters, just noting it considering that other new API's just pass 0 and not flags which is only ever set to 0.

+ int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p", plugin, dom); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0))) + return -1; + + if (virLockManagerAcquire(lock, NULL, flags, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0)

Strange to see a longer second line, although I understand seeing NULL along leads to why not just put it on the previous line type review comment. FWIW: Seeing how @action is set here led me down the path of seeing how it's used. I wonder if perhaps that could be used in the lockd/sanlock code in order to determine how/whether to fail rather than just returning 0 in sanlock when METADATA isn't supported. If it really matters that is. It's not 100% clear to me what/how the expected action usage should be.

+ goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom) +{ + virLockManagerPtr lock; + const unsigned int flags = 0;

Similar @flags commentary.

+ int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p", plugin, dom); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, true, 0)))

Since MetadataLock/Unlock are the only ones passing true for the @metadataOnly bool in virDomainLockManagerNew, I'm "conflicted" over whether there should a helper for both in lieu of the bool, but then the bool does the trick and it's following @withResources which was there since inception, so I guess it's fine albeit odd w/r/t a *New API doing more than just allocating memory or setting defaults. Not an issue, but an observation, in case you were wondering. Trying to learn a bit about this code while doing this review... John

+ return -1; + + if (virLockManagerRelease(lock, NULL, flags) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataImageLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddImage(lock, src, true) < 0) + goto cleanup; + + if (virLockManagerAcquire(lock, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataImageUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddImage(lock, src, true) < 0) + goto cleanup; + + if (virLockManagerRelease(lock, NULL, 0) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataDiskLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockMetadataImageLock(plugin, dom, disk->src); +} + + +int +virDomainLockMetadataDiskUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockMetadataImageUnlock(plugin, dom, disk->src); +} + + +int +virDomainLockMetadataMemLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p mem=%p", plugin, dom, mem); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto cleanup; + + if (virLockManagerAcquire(lock, NULL, 0, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} + + +int +virDomainLockMetadataMemUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem) +{ + virLockManagerPtr lock; + int ret = -1; + + VIR_DEBUG("plugin=%p dom=%p mem=%p", plugin, dom, mem); + + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false, false, 0))) + return -1; + + if (virDomainLockManagerAddMemory(lock, mem) < 0) + goto cleanup; + + if (virLockManagerRelease(lock, NULL, 0) < 0) + goto cleanup; + + ret = 0; + cleanup: + virLockManagerFree(lock); + return ret; +} diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index fb4910230c..fbd3ee1d4e 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -66,4 +66,32 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, virDomainObjPtr dom, virDomainLeaseDefPtr lease);

+int virDomainLockMetadataLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom); + +int virDomainLockMetadataUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom); + +int virDomainLockMetadataDiskLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk); +int virDomainLockMetadataDiskUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk); + +int virDomainLockMetadataImageLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + +int virDomainLockMetadataImageUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + +int virDomainLockMetadataMemLock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem); +int virDomainLockMetadataMemUnlock(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainMemoryDefPtr mem); + #endif /* __VIR_DOMAIN_LOCK_H__ */

On 08/17/2018 08:38 PM, John Ferlan wrote:

On 08/14/2018 07:19 AM, Michal Privoznik wrote:

...

Fortunately, we have qemu wrappers so it's sufficient to put lock/unlock call only there.

Kind of sparse... Maybe a few words related to what benefit locking the metadata provides. There is a danger in all this too if the unlock does fail since metadata locks are wait type locks any subsequent lock will wait.

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index af3be42854..527563947c 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -26,6 +26,7 @@ #include "qemu_domain.h" #include "qemu_security.h" #include "virlog.h" +#include "locking/domain_lock.h"

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + bool locked = false; + + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) + goto cleanup; + + locked = true;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup;

+ locked = false;

Technically not true yet. Also , I think two such attempts with the second eliciting the VIR_WARN is perfectly reasonable (IOW: on failure of the following goto cleanup with locked == true, try again, and spew that message).

This repeats a few times, but I'll note just once.

I don't think it is safe to unlock something twice if we've locked it just once.

...

+ + if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + VIR_WARN("unable to release metadata lock");

Should we add the @stdin_path and @vm->pid and/or vm->name here? It may help some day. Similar comments for other new VIR_WARN's, but the parameters change...

Ah, good point. Okay.

...

return ret; }

@@ -68,6 +83,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, bool migrated) { qemuDomainObjPrivatePtr priv = vm->privateData; + bool unlock = true; + + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) + unlock = false;

/* In contrast to qemuSecuritySetAllLabel, do not use * secdriver transactions here. This function is called from @@ -79,6 +98,10 @@ qemuSecurityRestoreAllLabel(virQEMUDriverPtr driver, vm->def, migrated, priv->chardevStdioLogd); + + if (unlock && + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + VIR_WARN("unable to release metadata lock"); }

@@ -88,6 +111,12 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0) + goto cleanup; + + locked = true;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -103,9 +132,17 @@ qemuSecuritySetDiskLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup;

+ locked = false; + + if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + VIR_WARN("unable to release disk metadata lock"); return ret; }

@@ -116,6 +153,12 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataDiskLock(driver->lockManager, vm, disk) < 0) + goto cleanup; + + locked = true;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -131,9 +174,17 @@ qemuSecurityRestoreDiskLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup;

+ locked = false; + + if (virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataDiskUnlock(driver->lockManager, vm, disk) < 0) + VIR_WARN("unable to release disk metadata lock"); return ret; }

@@ -144,6 +195,12 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, virStorageSourcePtr src) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0) + goto cleanup; + + locked = true;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -159,9 +216,17 @@ qemuSecuritySetImageLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup;

+ locked = false; + + if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + VIR_WARN("unable to release image metadata lock"); return ret; }

@@ -172,6 +237,12 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, virStorageSourcePtr src) { int ret = -1; + bool locked = false; + + if (virDomainLockMetadataImageLock(driver->lockManager, vm, src) < 0) + goto cleanup; + + locked = true;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -187,9 +258,17 @@ qemuSecurityRestoreImageLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup;

+ locked = false; + + if (virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataImageUnlock(driver->lockManager, vm, src) < 0) + VIR_WARN("unable to release image metadata lock"); return ret; }

Just below here is HostdevLabel processing... For a SCSI host device that would seem to cover the type of resource described in the cover letter to patch6 - at least for iSCSI LUNs (and of course vHBA LUNs).

Need to look at the hostdev->mode and hostdev->source.subsys.type and dig into the subsys.u.scsi to get to the src->path.

Oh, this was the reason I made VIR_WARN() so sparse. I did not wanted to get into business of getting paths from all the complicated structs we have. I'll just put vm->def->name and vm->pid into the warn message. Michal

On 08/20/2018 05:16 PM, Daniel P. Berrangé wrote:

On Tue, Aug 14, 2018 at 01:19:43PM +0200, Michal Privoznik wrote:

...

Fortunately, we have qemu wrappers so it's sufficient to put lock/unlock call only there.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_security.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c index af3be42854..527563947c 100644 --- a/src/qemu/qemu_security.c +++ b/src/qemu/qemu_security.c @@ -26,6 +26,7 @@ #include "qemu_domain.h" #include "qemu_security.h" #include "virlog.h" +#include "locking/domain_lock.h"

#define VIR_FROM_THIS VIR_FROM_QEMU

@@ -39,6 +40,12 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + bool locked = false; + + if (virDomainLockMetadataLock(driver->lockManager, vm) < 0) + goto cleanup; + + locked = true;

if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) && virSecurityManagerTransactionStart(driver->securityManager) < 0) @@ -55,9 +62,17 @@ qemuSecuritySetAllLabel(virQEMUDriverPtr driver, vm->pid) < 0) goto cleanup;

+ locked = false; + + if (virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + goto cleanup; + ret = 0; cleanup: virSecurityManagerTransactionAbort(driver->securityManager); + if (locked && + virDomainLockMetadataUnlock(driver->lockManager, vm) < 0) + VIR_WARN("unable to release metadata lock"); return ret; }

I'm wondering if this is really the right level to plug in the metadata locking ? What about if we just pass a virLockManagerPtr to the security drivers and let them lock each resource at the time they need to modify its metadata. This will trivially guarantee that we always lock the exact set of files that we'll be changing metadata on.

eg in SELinux driver the virSecuritySELinuxSetFileconHelper method would directly call virLockManagerAcquire & virLockManagerRelease, avoiding the entire virDomainLock abstraction which was really focused around managing the content locks around lifecycle state changes.

Yeah, I vaguely recall writing code like this (when I was trying to solve this some time ago). Okay, let me see if that's feasible. But boy, this is getting hairy. Michal