[libvirt] [PATCH] storageVolCreateXMLFrom: Allow multiple accesses to origvol
When creating a new volume, it is possible to copy data into it from another already existing volume (referred to as @origvol). Obviously, the read-only access to @origvol is required, which is thread safe (probably not performance-wise though). However, with current code both @newvol and @origvol are marked as building for the time of copying data from the @origvol to @newvol. The rationale behind is to disallow some operations on both @origvol and @newvol, e.g. vol-wipe, vol-delete, vol-download. While it makes sense to not allow such operations on partly copied mirror, but it doesn't make sense to disallow the operations on the source (@origvol). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2cb8347..a3f398f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1912,7 +1912,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; - origvol->building = 1; newvol->building = 1; virStoragePoolObjUnlock(pool); @@ -1929,7 +1928,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjLock(origpool); storageDriverUnlock(driver); - origvol->building = 0; newvol->building = 0; allocation = newvol->target.allocation; pool->asyncjobs--; -- 1.9.0
On Wed, Apr 16, 2014 at 04:05:38PM +0200, Michal Privoznik wrote:
When creating a new volume, it is possible to copy data into it from another already existing volume (referred to as @origvol). Obviously, the read-only access to @origvol is required, which is thread safe (probably not performance-wise though). However, with current code both @newvol and @origvol are marked as building for the time of copying data from the @origvol to @newvol. The rationale behind is to disallow some operations on both @origvol and @newvol, e.g. vol-wipe, vol-delete, vol-download. While it makes sense to not allow such operations on partly copied mirror, but it doesn't make sense to disallow the operations on the source (@origvol).
What if someone tries to delete or resize or otherwise change the source volume while the copy is still in progress ? We could just say that's user error I guess. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 16.04.2014 16:09, Daniel P. Berrange wrote:
On Wed, Apr 16, 2014 at 04:05:38PM +0200, Michal Privoznik wrote:
When creating a new volume, it is possible to copy data into it from another already existing volume (referred to as @origvol). Obviously, the read-only access to @origvol is required, which is thread safe (probably not performance-wise though). However, with current code both @newvol and @origvol are marked as building for the time of copying data from the @origvol to @newvol. The rationale behind is to disallow some operations on both @origvol and @newvol, e.g. vol-wipe, vol-delete, vol-download. While it makes sense to not allow such operations on partly copied mirror, but it doesn't make sense to disallow the operations on the source (@origvol).
What if someone tries to delete or resize or otherwise change the source volume while the copy is still in progress ? We could just say that's user error I guess.
Yes, I'd call it user error. Moreover, deletion is a special case - Linux (and other POSIX compliant OSes) are perfectly comfortable with deleting a file while copying it. In other cases, users should get an sys error once libvirt tries to unlink() the file. But then again, even reaching this point is user error. However, if we want to keep status quo, I'm okay with squashing this in: diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 9ad38e1..3bbff9a 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -64,6 +64,7 @@ struct _virStorageVolDef { int type; /* enum virStorageVolType */ unsigned int building; + bool in_use; virStorageVolSource source; virStorageSource target; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a3f398f..546992a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1640,7 +1640,7 @@ storageVolDelete(virStorageVolPtr obj, if (virStorageVolDeleteEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -1879,7 +1879,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, goto cleanup; } - if (origvol->building) { + if (origvol->building || origvol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), origvol->name); @@ -1913,6 +1913,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, /* Drop the pool lock during volume allocation */ pool->asyncjobs++; newvol->building = 1; + origvol->in_use = true; virStoragePoolObjUnlock(pool); if (origpool) { @@ -1928,6 +1929,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjLock(origpool); storageDriverUnlock(driver); + origvol->in_use = false; newvol->building = 0; allocation = newvol->target.allocation; pool->asyncjobs--; @@ -2008,7 +2010,7 @@ storageVolDownload(virStorageVolPtr obj, if (virStorageVolDownloadEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -2074,7 +2076,7 @@ storageVolUpload(virStorageVolPtr obj, if (virStorageVolUploadEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -2165,7 +2167,7 @@ storageVolResize(virStorageVolPtr obj, if (virStorageVolResizeEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); @@ -2472,7 +2474,7 @@ storageVolWipePattern(virStorageVolPtr obj, if (virStorageVolWipePatternEnsureACL(obj->conn, pool->def, vol) < 0) goto cleanup; - if (vol->building) { + if (vol->building || vol->in_use) { virReportError(VIR_ERR_OPERATION_INVALID, _("volume '%s' is still being allocated."), vol->name); Michal
participants (2)
-
Daniel P. Berrange -
Michal Privoznik