4:42 a.m.
*** 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
4:42 a.m.
New subject: [libvirt] [PATCH 1/6] storage_backend_iscsi_direct: Simplify vol zeroing
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(a)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
7:08 a.m.
New subject: [libvirt] [PATCH 1/6] storage_backend_iscsi_direct: Simplify vol zeroing
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(a)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
4:42 a.m.
New subject: [libvirt] [PATCH 2/6] virISCSIDirectReportLuns: Drop ClearVols
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(a)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
4:42 a.m.
New subject: [libvirt] [PATCH 3/6] storageVolWipePattern: Don't take shortcut to refreshPool()
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(a)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
4:42 a.m.
New subject: [libvirt] [PATCH 4/6] storage_driver: Introduce storagePoolRefreshImpl()
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(a)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
4:42 a.m.
New subject: [libvirt] [PATCH 5/6] storagePoolRefreshFailCleanup: Clear volumes on failed refresh
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(a)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
4:42 a.m.
New subject: [libvirt] [PATCH 6/6] virsh-pool: Offer only active pool for pool-refresh completer
Only active pools can be refreshed. But our completer offers just
all pool, even inactive ones.
Signed-off-by: Michal Privoznik <mprivozn(a)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
2124
days inactive
2129
days old
7 comments
1 participants
participants (1)
-
Michal Privoznik