[libvirt] [PATCH] storage: Don't hold storage pool locked during wipeVol

[John Ferlan]

On 08/13/2018 06:49 AM, Michal Privoznik wrote:
> Currently the way virStorageVolWipe() works is it looks up
> pool containing given volume and hold it locked throughout while
> API execution. This is suboptimal because wiping a volume means
> writing data to it which can take ages. And if the whole pool is
> locked during that operation no other API can be issued (nor
> pool-list).
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/storage/storage_backend_iscsi_direct.c | 5 +++++
>  src/storage/storage_backend_rbd.c          | 7 ++++++-
>  src/storage/storage_util.c                 | 8 +++++++-
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 

Why not be more similar to storageVolCreateXMLFrom? That is handle the
in_use incr/decr at the storage driver level. Seems we could create a
helper that would follow the same steps...

For volume wiping, RBD and iscsi-direct use the @pool (obj), but by
incrementing not only vol->in_use, but the pool asyncjobs we can inhibit
the undefine, destroy, or deletion of the pool that would cause issues
for those uses AFAICT. Inhibiting refresh during these operations is a
matter of perhaps opinion as to whether it's a good idea or not -
although I suppose if a volume is open for write (locked), trying to
open/read to get stat type information probably is going to wait anyway
(although I'll admit I haven't put much time or research into that
thought - just going by gut feel ;-)).

BTW: Wouldn't uploadVol have the same issues?  Seems we have only cared
about build vol from.  Since uploadVol checks vol->in_use it seems
logical using the same argument as above that it too should use some new
helper to change pool asyncjobs and vol in_use. The building setting
could just be another parameter.

John

> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 1624066e9c..58d25557f1 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -691,6 +691,9 @@ virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr pool,

<sigh>

Should be BackendISCSI instead of BackenISCSI

and I see this whole new module used single blank line spacing between
functions instead of 2 blank lines. Both would be trivial patches IMO.

>      if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, NULL)))
>          return -1;
>  
> +    vol->in_use++;
> +    virObjectUnlock(pool);
> +
>      switch ((virStorageVolWipeAlgorithm) algorithm) {
>      case VIR_STORAGE_VOL_WIPE_ALG_ZERO:
>          if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) {
> @@ -719,6 +722,8 @@ virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr pool,
>   cleanup:
>      virISCSIDirectDisconnect(iscsi);
>      iscsi_destroy_context(iscsi);
> +    virObjectLock(pool);
> +    vol->in_use--;
>      return ret;
>  }

[...]