On 08/16/2018 05:22 PM, John Ferlan wrote: > > > 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@xxxxxxxxxx> >> --- >> 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... I'm not sure we can do that. Sure, I though of that but then I've seen virStorageBackendRBDVolWipe() which calls virStorageBackendRBDNewState() -> virStoragePoolObjGetDef() and we certainly do not want to call that with pool unlocked. > > 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. Ah, okay I haven't realized we do have asyncJobs. That way we can unlock the pool object just before calling the backend callback. However, for the RBD and iscsi-direct cases I think we still should guard PoolObjGetDef() calls with pool lock+unlock. > 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 ;-)). Whether a good idea or not we should be prepared for that. But if we set asyncJob then refresh is denied, so we are safe IMO. > > 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. Okay, I'll rework my patch. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
