[libvirt] [PATCH 0/6] Couple of storage driver improvements
*** BLURB HERE *** Michal Prívozník (6): storage_backend_iscsi_direct: Simplify vol zeroing virISCSIDirectReportLuns: Drop ClearVols storageVolWipePattern: Don't take shortcut to refreshPool() storage_driver: Introduce storagePoolRefreshImpl() storagePoolRefreshFailCleanup: Clear volumes on failed refresh virsh-pool: Offer only active pool for pool-refresh completer src/storage/storage_backend_gluster.c | 2 - src/storage/storage_backend_iscsi_direct.c | 21 ++---- src/storage/storage_backend_logical.c | 12 +--- src/storage/storage_backend_rbd.c | 4 +- src/storage/storage_driver.c | 77 ++++++++++++---------- src/storage/storage_util.c | 2 - tools/virsh-pool.c | 2 +- 7 files changed, 55 insertions(+), 65 deletions(-) -- 2.19.2
So far we have two branches: either we zero BLOCK_PER_PACKET (currently 128) block at one, or if we're close to the last block then we zero out one block at the time. This is very suboptimal. We know how many block are there left. Might as well just write them all at once. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 786663534d..574a65449d 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -645,21 +645,15 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, return ret; while (lba < nb_block) { - if (nb_block - lba > block_size * BLOCK_PER_PACKET) { + const uint64_t to_write = MIN(nb_block - lba + 1, BLOCK_PER_PACKET); - if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, - block_size * BLOCK_PER_PACKET, - block_size, 0, 0, 0, 0, 0))) - return -1; - scsi_free_scsi_task(task); - lba += BLOCK_PER_PACKET; - } else { - if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, block_size, + if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, + to_write * block_size, block_size, 0, 0, 0, 0, 0))) - return -1; - scsi_free_scsi_task(task); - lba++; - } + return -1; + scsi_free_scsi_task(task); + + lba += to_write; } return 0; -- 2.19.2
On 3/1/19 11:42 AM, Michal Privoznik wrote:
So far we have two branches: either we zero BLOCK_PER_PACKET (currently 128) block at one, or if we're close to the last block then we zero out one block at the time. This is very suboptimal. We know how many block are there left. Might as well just write them all at once.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 786663534d..574a65449d 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -645,21 +645,15 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, return ret;
while (lba < nb_block) { - if (nb_block - lba > block_size * BLOCK_PER_PACKET) { + const uint64_t to_write = MIN(nb_block - lba + 1, BLOCK_PER_PACKET);
- if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, - block_size * BLOCK_PER_PACKET, - block_size, 0, 0, 0, 0, 0))) - return -1; - scsi_free_scsi_task(task); - lba += BLOCK_PER_PACKET; - } else { - if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, block_size, + if (!(task = iscsi_write16_sync(iscsi, lun, lba, data, + to_write * block_size, block_size, 0, 0, 0, 0, 0))) - return -1;
Self-NACK. I've experimented a bit and realized we can use writesame16_sync() which allows us to allocate smaller buffer and also it is a bit faster. I'll post v2 for this one. Michal
In bf5cf610f206d5d54 I've fixed a problem where iscsi-direct backend was reporting only the last LUN. The fix consisted of moving virStoragePoolObjClearVols() one level up. However, as it turns out, storage driver already calls it before calling refreshPool callback (which is virStorageBackendISCSIDirectRefreshPool() in this case). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 574a65449d..a5ab9dabbc 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -375,7 +375,6 @@ virISCSIDirectReportLuns(virStoragePoolObjPtr pool, def->capacity = 0; def->allocation = 0; - virStoragePoolObjClearVols(pool); for (i = 0; i < list->num; i++) { if (virISCSIDirectRefreshVol(pool, iscsi, list->luns[i], portal) < 0) goto cleanup; -- 2.19.2
In d16f803d780 we've tried to solve an issue that after wiping an image its format might have changed (e.g. from qcow2 to raw) but libvirt wasn't probing the image format. We fixed this by calling virStorageBackendRefreshVolTargetUpdate() which is what refreshPool() would end up calling. But this shortcut is not good enough because the function is called only for local types of volumes (like dir, fs, netfs). But now that more backends support volume wiping we have to call the function with more caution. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Truth to be told, I don't like this approach very much. I'd rather replace this with storagePoolRefreshImpl() call which is introduced in next patch. src/storage/storage_driver.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 34634e97d9..72a39b36b1 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2539,11 +2539,15 @@ storageVolWipePattern(virStorageVolPtr vol, if (rc < 0) goto cleanup; - /* Instead of using the refreshVol, since much changes on the target - * volume, let's update using the same function as refreshPool would - * use when it discovers a volume. The only failure to capture is -1, - * we can ignore -2. */ - if (virStorageBackendRefreshVolTargetUpdate(voldef) == -1) + /* For local volumes, Instead of using the refreshVol, since + * much changes on the target volume, let's update using the + * same function as refreshPool would use when it discovers a + * volume. The only failure to capture is -1, we can ignore + * -2. */ + if ((backend->type == VIR_STORAGE_POOL_DIR || + backend->type == VIR_STORAGE_POOL_FS || + backend->type == VIR_STORAGE_POOL_NETFS) && + virStorageBackendRefreshVolTargetUpdate(voldef) == -1) goto cleanup; ret = 0; -- 2.19.2
This is a wrapper over refreshPool() call as at all places we are doing basically the same. Might as well have a single function to call. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 61 +++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 72a39b36b1..029e8d326a 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -92,6 +92,21 @@ storagePoolRefreshFailCleanup(virStorageBackendPtr backend, } +static int +storagePoolRefreshImpl(virStorageBackendPtr backend, + virStoragePoolObjPtr obj, + const char *stateFile) +{ + virStoragePoolObjClearVols(obj); + if (backend->refreshPool(obj) < 0) { + storagePoolRefreshFailCleanup(backend, obj, stateFile); + return -1; + } + + return 0; +} + + /** * virStoragePoolUpdateInactive: * @poolptr: pointer to a variable holding the pool object pointer @@ -148,15 +163,12 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, * it anyway, but if they do and fail, we want to log error and * continue with other pools. */ - if (active) { - virStoragePoolObjClearVols(obj); - if (backend->refreshPool(obj) < 0) { - storagePoolRefreshFailCleanup(backend, obj, stateFile); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to restart storage pool '%s': %s"), - def->name, virGetLastErrorMessage()); - active = false; - } + if (active && + storagePoolRefreshImpl(backend, obj, stateFile) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restart storage pool '%s': %s"), + def->name, virGetLastErrorMessage()); + active = false; } virStoragePoolObjSetActive(obj, active); @@ -203,12 +215,10 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, if (started) { VIR_AUTOFREE(char *) stateFile = NULL; - virStoragePoolObjClearVols(obj); stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 || - backend->refreshPool(obj) < 0) { - storagePoolRefreshFailCleanup(backend, obj, stateFile); + storagePoolRefreshImpl(backend, obj, stateFile) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); @@ -735,10 +745,9 @@ storagePoolCreateXML(virConnectPtr conn, stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - virStoragePoolObjClearVols(obj); - if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 || - backend->refreshPool(obj) < 0) { - storagePoolRefreshFailCleanup(backend, obj, stateFile); + if (!stateFile || + virStoragePoolSaveState(stateFile, def) < 0 || + storagePoolRefreshImpl(backend, obj, stateFile) < 0) { goto error; } @@ -930,10 +939,9 @@ storagePoolCreate(virStoragePoolPtr pool, stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - virStoragePoolObjClearVols(obj); - if (!stateFile || virStoragePoolSaveState(stateFile, def) < 0 || - backend->refreshPool(obj) < 0) { - storagePoolRefreshFailCleanup(backend, obj, stateFile); + if (!stateFile || + virStoragePoolSaveState(stateFile, def) < 0 || + storagePoolRefreshImpl(backend, obj, stateFile) < 0) { goto cleanup; } @@ -1130,6 +1138,7 @@ storagePoolRefresh(virStoragePoolPtr pool, virStoragePoolObjPtr obj; virStoragePoolDefPtr def; virStorageBackendPtr backend; + VIR_AUTOFREE(char *) stateFile = NULL; int ret = -1; virObjectEventPtr event = NULL; @@ -1158,13 +1167,8 @@ storagePoolRefresh(virStoragePoolPtr pool, goto cleanup; } - virStoragePoolObjClearVols(obj); - if (backend->refreshPool(obj) < 0) { - VIR_AUTOFREE(char *) stateFile = NULL; - - stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); - storagePoolRefreshFailCleanup(backend, obj, stateFile); - + stateFile = virFileBuildPath(driver->stateDir, def->name, ".xml"); + if (storagePoolRefreshImpl(backend, obj, stateFile) < 0) { event = virStoragePoolEventLifecycleNew(def->name, def->uuid, VIR_STORAGE_POOL_EVENT_STOPPED, @@ -2254,8 +2258,7 @@ virStorageVolPoolRefreshThread(void *opaque) if (!(backend = virStorageBackendForType(def->type))) goto cleanup; - virStoragePoolObjClearVols(obj); - if (backend->refreshPool(obj) < 0) + if (storagePoolRefreshImpl(backend, obj, NULL) < 0) VIR_DEBUG("Failed to refresh storage pool"); event = virStoragePoolEventRefreshNew(def->name, def->uuid); -- 2.19.2
If pool refresh failed, then the internal table of volumes is probably left in inconsistent or incomplete state anyways. Clear it out then. This has an advantage that we can move the virStoragePoolObjClearVols() from those very few backends that do call it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_gluster.c | 2 -- src/storage/storage_backend_logical.c | 12 +++--------- src/storage/storage_backend_rbd.c | 4 +--- src/storage/storage_driver.c | 2 ++ src/storage/storage_util.c | 2 -- 5 files changed, 6 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 819993439a..5955d834d9 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -402,8 +402,6 @@ virStorageBackendGlusterRefreshPool(virStoragePoolObjPtr pool) if (dir) glfs_closedir(dir); virStorageBackendGlusterClose(state); - if (ret < 0) - virStoragePoolObjClearVols(pool); return ret; } diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 77e4dfb8b1..83b5f27151 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -760,14 +760,13 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool) 2 }; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - int ret = -1; VIR_AUTOPTR(virCommand) cmd = NULL; virWaitForDevices(); /* Get list of all logical volumes */ if (virStorageBackendLogicalFindLVs(pool, NULL) < 0) - goto cleanup; + return -1; cmd = virCommandNewArgList(VGS, "--separator", ":", @@ -788,14 +787,9 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool) pool, "vgs", NULL) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - if (ret < 0) - virStoragePoolObjClearVols(pool); - return ret; + return 0; } /* diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 2b7af1db23..3eae780c44 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -648,10 +648,8 @@ virStorageBackendRBDRefreshPool(virStoragePoolObjPtr pool) goto cleanup; } - if (virStoragePoolObjAddVol(pool, vol) < 0) { - virStoragePoolObjClearVols(pool); + if (virStoragePoolObjAddVol(pool, vol) < 0) goto cleanup; - } vol = NULL; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 029e8d326a..c1d519934b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -81,6 +81,8 @@ storagePoolRefreshFailCleanup(virStorageBackendPtr backend, { virErrorPtr orig_err = virSaveLastError(); + virStoragePoolObjClearVols(obj); + if (stateFile) unlink(stateFile); if (backend->stopPool) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 7a879b0f46..62f857f9ea 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3620,8 +3620,6 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) ret = 0; cleanup: VIR_DIR_CLOSE(dir); - if (ret < 0) - virStoragePoolObjClearVols(pool); return ret; } -- 2.19.2
Only active pools can be refreshed. But our completer offers just all pool, even inactive ones. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index d98fd80330..f641f5776c 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -735,7 +735,7 @@ static const vshCmdInfo info_pool_refresh[] = { }; static const vshCmdOptDef opts_pool_refresh[] = { - VIRSH_COMMON_OPT_POOL_FULL(0), + VIRSH_COMMON_OPT_POOL_FULL(VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE), {.name = NULL} }; -- 2.19.2
participants (1)
-
Michal Privoznik