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@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; }
[...]