[PATCH] libxl: Add lock process indicator to libxlDomainObjPrivate object

[Michal Privoznik]

On 2/23/21 5:27 AM, Jim Fehlig wrote:
> The libvirt libxl driver has no access to FDs associated with VM disks.
> The disks are opened by libxl.so and any related FDs are not exposed to
> applications. The prevents using virtlockd's auto-release feature to
> release locks when the FD is closed. Acquiring and releasing locks is
> explicitly handled by the libxl driver.
> 
> The current logic is structured such that locks are acquired in
> libxlDomainStart and released in libxlDomainCleanup. This works well
> except for migration, where the locks must be released on the source
> host before the domain can be started on the destination host, but the
> domain cannot be cleaned up until the migration confirmation stage.
> When libxlDomainCleanup if finally called in the confirm stage, locks
> are again released resulting in confusing errors from virtlockd and
> libvirtd
> 
> virtlockd[8095]: resource busy: Lockspace resource 'xxxxxx' is not locked
> libvirtd[8050]: resource busy: Lockspace resource 'xxxxxx' is not locked
> libvirtd[8050]: Unable to release lease on testvm
> 
> The error is also encountered in some error cases, e.g. when
> libxlDomainStart fails before acquiring locks and libxlDomainCleanup
> is still used for cleanup.
> 
> In lieu of a mechanism to check if a lock has been acquired, this patch
> takes an easy approach to fixing the unnecessary lock releases by adding
> an indicator to the libxlDomainPrivate object that can be set when the
> lock is acquired and cleared when the lock is released. libxlDomainCleanup
> can then skip releasing the lock in cases where it was previously released
> or never acquired in the first place.
> 
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
> 
> danpb and I discussed this issue on IRC and after pursuing several options
> I've decided to send this simple approach to the list for comment. I reworked
> the code in several ways but in the end cant escape the fact that there are
> occasions where domain cleanup needs to be performed but stopping the lock
> process is not needed. Another simple approach I pursued was creating a
> cleanup function that skipped virDomainLockProcessPause, but this approach
> was a bit cleaner IMO. Other suggestions welcomed.
> 
> All is not lost in the effort as it resulted in several cleanup and
> improvement patches that I'll post after release. It includes decomposing
> libxlDomainStart and making it idempotent on error so libxlDomainCleanup
> only needs to be called after a successful call to libxlDomainStart.
> 
>   src/libxl/libxl_domain.c    | 13 +++++++++----
>   src/libxl/libxl_domain.h    |  1 +
>   src/libxl/libxl_migration.c |  8 ++++++--
>   3 files changed, 16 insertions(+), 6 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

and safe for freeze.

Michal