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

[Michal Privoznik]

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

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