[v3] Introduce metadata locking

[libvirt] [PATCH v3 00/28] Introduce metadata locking

Posted by Michal Privoznik 5 years, 7 months ago

v3 of:

https://www.redhat.com/archives/libvir-list/2018-August/msg00814.html

What has changed since v2? A lot.
- The lock manager was moved into security manager (which requires a lot
  of preparation which is done in first 8 or so patches).

- The VIR_LOCK_SPACE_ACQUIRE_WAIT flag (2/7 in v2) is dropped as it
  turned out to be harmful. virlockd can't block under any
  circumstances. And we can not introduce a thread pool for it.

- While going through the code I've found couple of bugs which I'm
  fixing in first few patches.

As usual, you can find all the patches at:

https://github.com/zippy2/libvirt/tree/disk_metadata_lock_v3

Michal Prívozník (28):
  virSecurityManagerNewDriver: Fix code pattern
  virSecurityManagerNewStack: Don't ignore virSecurityStackAddNested
    retval
  lock_daemon: Fix some memleaks
  lock_driver_lockd: Don't leak lockspace dirs
  virLockManagerLockDaemonAcquire: Drop useless check
  virLockManagerSanlockAddResource: Do not ignore unknown resource types
  locking: Don't leak private data in virLockManagerLockDaemonNew
  virLockManagerLockDaemonAddResource: Switch to cleanup label rather
    than error
  virlockspace: Allow caller to specify start and length offset in
    virLockSpaceAcquireResource
  lock_driver_lockd: Introduce
    VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA flag
  lock_driver: Introduce new VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON
  _virLockManagerLockDaemonPrivate: Move @hasRWDisks into dom union
  lock_driver: Introduce VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA
  lock_daemon_dispatch: Check for ownerPid rather than ownerId
  locking: Introduce virLockManagerClearResources
  lock_driver: Introduce KEEP_OPEN flags
  lock_manager: Introduce virLockManagerCloseConn
  lock_manager: Allow disabling configFile for virLockManagerPluginNew
  qemu_conf: Introduce metadata_lock_manager
  security_manager: Load lock plugin on init
  security_manager: Introduce virSecurityManagerLockCloseConn
  security_manager: Introduce metadata locking APIs
  security_dac: Pass virSecurityManagerPtr to virSecurityDACSetOwnership
  security_dac: Pass virSecurityManagerPtr to
    virSecurityDACRestoreFileLabelInternal
  security_dac: Fix info messages when chown()-ing
  security_dac: Fix const correctness
  security_dac: Move transaction handling up one level
  security_dac: Lock domain metadata

 cfg.mk                             |   4 +-
 src/libvirt_private.syms           |   2 +
 src/locking/lock_daemon.c          |   3 +
 src/locking/lock_daemon_dispatch.c |  25 +-
 src/locking/lock_driver.h          |  38 +++
 src/locking/lock_driver_lockd.c    | 520 ++++++++++++++++++++++++++-----------
 src/locking/lock_driver_lockd.h    |   1 +
 src/locking/lock_driver_nop.c      |  14 +
 src/locking/lock_driver_sanlock.c  |  50 ++--
 src/locking/lock_manager.c         |  31 ++-
 src/locking/lock_manager.h         |   7 +
 src/qemu/libvirtd_qemu.aug         |   1 +
 src/qemu/qemu.conf                 |   6 +
 src/qemu/qemu_conf.c               |  13 +
 src/qemu/qemu_conf.h               |   1 +
 src/qemu/qemu_driver.c             |  12 +-
 src/qemu/test_libvirtd_qemu.aug.in |   1 +
 src/security/security_dac.c        | 213 +++++++++------
 src/security/security_manager.c    | 366 +++++++++++++++++++++++++-
 src/security/security_manager.h    |  17 +-
 src/util/virlockspace.c            |  15 +-
 src/util/virlockspace.h            |   4 +
 tests/testutilsqemu.c              |   2 +-
 tests/virlockspacetest.c           |  29 ++-
 24 files changed, 1096 insertions(+), 279 deletions(-)

-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 00/28] Introduce metadata locking

Posted by Daniel P. Berrangé 5 years, 7 months ago

On Mon, Aug 27, 2018 at 10:08:13AM +0200, Michal Privoznik wrote:
> v3 of:
> 
> https://www.redhat.com/archives/libvir-list/2018-August/msg00814.html
> 
> What has changed since v2? A lot.
> - The lock manager was moved into security manager (which requires a lot
>   of preparation which is done in first 8 or so patches).
> 
> - The VIR_LOCK_SPACE_ACQUIRE_WAIT flag (2/7 in v2) is dropped as it
>   turned out to be harmful. virlockd can't block under any
>   circumstances. And we can not introduce a thread pool for it.
> 
> - While going through the code I've found couple of bugs which I'm
>   fixing in first few patches.

I've not done a detailed per patch code review, but having looked
at the overall design concept across the patches, I think it looks
pretty good. Only one conceptual comment....

>  cfg.mk                             |   4 +-
>  src/libvirt_private.syms           |   2 +
>  src/locking/lock_daemon.c          |   3 +
>  src/locking/lock_daemon_dispatch.c |  25 +-
>  src/locking/lock_driver.h          |  38 +++
>  src/locking/lock_driver_lockd.c    | 520 ++++++++++++++++++++++++++-----------
>  src/locking/lock_driver_lockd.h    |   1 +
>  src/locking/lock_driver_nop.c      |  14 +
>  src/locking/lock_driver_sanlock.c  |  50 ++--
>  src/locking/lock_manager.c         |  31 ++-
>  src/locking/lock_manager.h         |   7 +
>  src/qemu/libvirtd_qemu.aug         |   1 +
>  src/qemu/qemu.conf                 |   6 +
>  src/qemu/qemu_conf.c               |  13 +
>  src/qemu/qemu_conf.h               |   1 +
>  src/qemu/qemu_driver.c             |  12 +-
>  src/qemu/test_libvirtd_qemu.aug.in |   1 +
>  src/security/security_dac.c        | 213 +++++++++------
>  src/security/security_manager.c    | 366 +++++++++++++++++++++++++-
>  src/security/security_manager.h    |  17 +-

Why no integration into the security_selinux.c driver ? The apparmor
driver probably doesn't need it as it doesn't touchthe files to setup
its security profile, but SELinux should need protection.

>  src/util/virlockspace.c            |  15 +-
>  src/util/virlockspace.h            |   4 +
>  tests/testutilsqemu.c              |   2 +-
>  tests/virlockspacetest.c           |  29 ++-
>  24 files changed, 1096 insertions(+), 279 deletions(-)

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 00/28] Introduce metadata locking

Posted by Michal Privoznik 5 years, 7 months ago

On 08/31/2018 03:33 PM, Daniel P. Berrangé wrote:
> On Mon, Aug 27, 2018 at 10:08:13AM +0200, Michal Privoznik wrote:
>> v3 of:
>>
>> https://www.redhat.com/archives/libvir-list/2018-August/msg00814.html
>>
>> What has changed since v2? A lot.
>> - The lock manager was moved into security manager (which requires a lot
>>   of preparation which is done in first 8 or so patches).
>>
>> - The VIR_LOCK_SPACE_ACQUIRE_WAIT flag (2/7 in v2) is dropped as it
>>   turned out to be harmful. virlockd can't block under any
>>   circumstances. And we can not introduce a thread pool for it.
>>
>> - While going through the code I've found couple of bugs which I'm
>>   fixing in first few patches.
> 
> I've not done a detailed per patch code review, but having looked
> at the overall design concept across the patches, I think it looks
> pretty good. Only one conceptual comment....
> 
>>  cfg.mk                             |   4 +-
>>  src/libvirt_private.syms           |   2 +
>>  src/locking/lock_daemon.c          |   3 +
>>  src/locking/lock_daemon_dispatch.c |  25 +-
>>  src/locking/lock_driver.h          |  38 +++
>>  src/locking/lock_driver_lockd.c    | 520 ++++++++++++++++++++++++++-----------
>>  src/locking/lock_driver_lockd.h    |   1 +
>>  src/locking/lock_driver_nop.c      |  14 +
>>  src/locking/lock_driver_sanlock.c  |  50 ++--
>>  src/locking/lock_manager.c         |  31 ++-
>>  src/locking/lock_manager.h         |   7 +
>>  src/qemu/libvirtd_qemu.aug         |   1 +
>>  src/qemu/qemu.conf                 |   6 +
>>  src/qemu/qemu_conf.c               |  13 +
>>  src/qemu/qemu_conf.h               |   1 +
>>  src/qemu/qemu_driver.c             |  12 +-
>>  src/qemu/test_libvirtd_qemu.aug.in |   1 +
>>  src/security/security_dac.c        | 213 +++++++++------
>>  src/security/security_manager.c    | 366 +++++++++++++++++++++++++-
>>  src/security/security_manager.h    |  17 +-
> 
> Why no integration into the security_selinux.c driver ? The apparmor
> driver probably doesn't need it as it doesn't touchthe files to setup
> its security profile, but SELinux should need protection.

Yes it does. I should have noted that selinux driver is WIP. Firstly I
wanted to see if the patches I posted are good and if they were I'll
post patches for selinux.

Sorry for the confusion.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list