[libvirt] [PATCH v2 0/8] Couple of storage driver improvements
v2 of: https://www.redhat.com/archives/libvir-list/2019-March/msg00001.html diff to v1: - use WRITESAME instead of WRITE in 2/8 - Patches 1/8 and 3/8 are new. I've noticed we are overwriting an error when something goes wrong while volume wiping. Michal Prívozník (8): iscsi_direct: Make virStorageBackendISCSIDirectGetLun report error properly storage_backend_iscsi_direct: Simplify vol zeroing iscsi_direct: Don't overwrite error in virStorageBackenISCSIDirectWipeVol() 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 | 80 +++++++++++++--------- 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, 96 insertions(+), 83 deletions(-) -- 2.19.2
This function reports error for one of the two error paths. This is unfortunate as a caller see this function failing but doesn't know right away if an error was reported. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 786663534d..fc3b6550f7 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -605,22 +605,16 @@ static int virStorageBackendISCSIDirectGetLun(virStorageVolDefPtr vol, int *lun) { - const char *name = vol->name; - int ret = -1; + const char *name; - if (!STRPREFIX(name, VOL_NAME_PREFIX)) { + if (!(name = STRSKIP(vol->name, VOL_NAME_PREFIX)) || + virStrToLong_i(name, NULL, 10, lun) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid volume name %s"), name); - goto cleanup; + _("Invalid volume name %s"), vol->name); + return -1; } - name += strlen(VOL_NAME_PREFIX); - if (virStrToLong_i(name, NULL, 10, lun) < 0) - goto cleanup; - - ret = 0; - cleanup: - return ret; + return 0; } static int -- 2.19.2
On Wed, Mar 06, 2019 at 03:59:11PM +0100, Michal Privoznik wrote:
This function reports error for one of the two error paths. This is unfortunate as a caller see this function failing but doesn't know right away if an error was reported.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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. Also, SCSI protocol has WRITESAME command which allows us to write a single buffer multiple times. This means, that we have to transfer the buffer far less frequently and have the destination write it over and over again. Ideally, we would allocate a buffer with size equivalent to block size of the LUN and then write it as many times as there are blocks on the LUN. Sadly, this works well in theory but in practise I've found out that it's possible to write only 4096 blocks at once. Any higher value leads to a failure. But still better than Twili^Wwhat we have now. Since 4096 seems like an arbitrary number, I'm also implementing a mechanism that exponentially decreases the number of blocks we're trying to write at once. It starts at 4096 blocks and if that's too much the number is halved, and so on. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 55 ++++++++++++++++------ 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index fc3b6550f7..0af7adf32c 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -39,9 +39,11 @@ #define ISCSI_DEFAULT_TARGET_PORT 3260 #define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000 -#define BLOCK_PER_PACKET 128 #define VOL_NAME_PREFIX "unit:0:0:" +/* Empirically tested to be the highest value to work without any error. */ +#define WIPE_BLOCKS_AT_ONCE 4096 + VIR_LOG_INIT("storage.storage_backend_iscsi_direct"); static struct iscsi_context * @@ -624,6 +626,7 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, uint64_t lba = 0; uint32_t block_size; uint64_t nb_block; + uint64_t wipe_at_once = WIPE_BLOCKS_AT_ONCE; struct scsi_task *task = NULL; int lun = 0; int ret = -1; @@ -635,25 +638,49 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, return ret; if (virISCSIDirectGetVolumeCapacity(iscsi, lun, &block_size, &nb_block)) return ret; - if (VIR_ALLOC_N(data, block_size * BLOCK_PER_PACKET)) + if (VIR_ALLOC_N(data, block_size)) return ret; + VIR_DEBUG("Starting zeroing of lun=%d block_size=%ju nb_block=%ju wipe_at_once=%ju", + lun, (uintmax_t) block_size, (uintmax_t) nb_block, (uintmax_t) wipe_at_once); + while (lba < nb_block) { - if (nb_block - lba > block_size * BLOCK_PER_PACKET) { + const uint64_t to_write = MIN(nb_block - lba + 1, wipe_at_once); + bool fail = false; - 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, - block_size, 0, 0, 0, 0, 0))) - return -1; + if (!(task = iscsi_writesame16_sync(iscsi, lun, lba, + data, block_size, + to_write, + 0, 0, 0, 0))) + fail = true; + + if (task && + task->status == SCSI_STATUS_CHECK_CONDITION && + task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST && + task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB) { + /* This means that we tried to write too much blocks at once. + * Halve it (if it's not small enough already) and retry. */ + if (wipe_at_once > 1) { + wipe_at_once /= 2; + VIR_DEBUG("Halving wipe_at_once to %ju", (uintmax_t) wipe_at_once); + scsi_free_scsi_task(task); + continue; + } + + fail = true; + } + + if (fail) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to run writesame: %s"), + iscsi_get_error(iscsi)); scsi_free_scsi_task(task); - lba++; + return -1; } + + scsi_free_scsi_task(task); + + lba += to_write; } return 0; -- 2.19.2
On Wed, Mar 06, 2019 at 03:59:12PM +0100, 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.
Also, SCSI protocol has WRITESAME command which allows us to write a single buffer multiple times. This means, that we have to transfer the buffer far less frequently and have the destination write it over and over again. Ideally, we would allocate a buffer with size equivalent to block size of the LUN and then write it as many times as there are blocks on the LUN. Sadly, this works well in theory but in practise I've found out that it's possible to write only 4096 blocks at once. Any higher value leads to a failure. But still better than Twili^Wwhat we have now.
s/Twili^W//
Since 4096 seems like an arbitrary number, I'm also implementing a mechanism that exponentially decreases the number of blocks we're trying to write at once. It starts at 4096 blocks and if that's too much the number is halved, and so on.
According to documentation if you set 0 as the number of blocks it should write the same block starting at the LBA to the end of the device. Would be nice to try it out and improve the algorithm to use only single transfer. Otherwise the patch looks good. Pavel
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 55 ++++++++++++++++------ 1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index fc3b6550f7..0af7adf32c 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -39,9 +39,11 @@
#define ISCSI_DEFAULT_TARGET_PORT 3260 #define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000 -#define BLOCK_PER_PACKET 128 #define VOL_NAME_PREFIX "unit:0:0:"
+/* Empirically tested to be the highest value to work without any error. */ +#define WIPE_BLOCKS_AT_ONCE 4096 + VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
static struct iscsi_context * @@ -624,6 +626,7 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, uint64_t lba = 0; uint32_t block_size; uint64_t nb_block; + uint64_t wipe_at_once = WIPE_BLOCKS_AT_ONCE; struct scsi_task *task = NULL; int lun = 0; int ret = -1; @@ -635,25 +638,49 @@ virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, return ret; if (virISCSIDirectGetVolumeCapacity(iscsi, lun, &block_size, &nb_block)) return ret; - if (VIR_ALLOC_N(data, block_size * BLOCK_PER_PACKET)) + if (VIR_ALLOC_N(data, block_size)) return ret;
+ VIR_DEBUG("Starting zeroing of lun=%d block_size=%ju nb_block=%ju wipe_at_once=%ju", + lun, (uintmax_t) block_size, (uintmax_t) nb_block, (uintmax_t) wipe_at_once); + while (lba < nb_block) { - if (nb_block - lba > block_size * BLOCK_PER_PACKET) { + const uint64_t to_write = MIN(nb_block - lba + 1, wipe_at_once); + bool fail = false;
- 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, - block_size, 0, 0, 0, 0, 0))) - return -1; + if (!(task = iscsi_writesame16_sync(iscsi, lun, lba, + data, block_size, + to_write, + 0, 0, 0, 0))) + fail = true; + + if (task && + task->status == SCSI_STATUS_CHECK_CONDITION && + task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST && + task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB) { + /* This means that we tried to write too much blocks at once. + * Halve it (if it's not small enough already) and retry. */ + if (wipe_at_once > 1) { + wipe_at_once /= 2; + VIR_DEBUG("Halving wipe_at_once to %ju", (uintmax_t) wipe_at_once); + scsi_free_scsi_task(task); + continue; + } + + fail = true; + } + + if (fail) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to run writesame: %s"), + iscsi_get_error(iscsi)); scsi_free_scsi_task(task); - lba++; + return -1; } + + scsi_free_scsi_task(task); + + lba += to_write; }
return 0; -- 2.19.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 3/15/19 2:53 PM, Pavel Hrdina wrote:
On Wed, Mar 06, 2019 at 03:59:12PM +0100, 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.
Also, SCSI protocol has WRITESAME command which allows us to write a single buffer multiple times. This means, that we have to transfer the buffer far less frequently and have the destination write it over and over again. Ideally, we would allocate a buffer with size equivalent to block size of the LUN and then write it as many times as there are blocks on the LUN. Sadly, this works well in theory but in practise I've found out that it's possible to write only 4096 blocks at once. Any higher value leads to a failure. But still better than Twili^Wwhat we have now.
s/Twili^W//
Since 4096 seems like an arbitrary number, I'm also implementing a mechanism that exponentially decreases the number of blocks we're trying to write at once. It starts at 4096 blocks and if that's too much the number is halved, and so on.
According to documentation if you set 0 as the number of blocks it should write the same block starting at the LBA to the end of the device. Would be nice to try it out and improve the algorithm to use only single transfer.
I've tried several values and found there are some differencies between docs and actual IMPL. What you suggest would be ideal of course, but the Linux Kernel I've tested doesn't allow me to specify anything else but [1, 4096]. Maybe it has something to do with the fact that LUN is virtualized and in fact a file on a filesystem. At any rate, we should be able to zero that too, shouldn't we? Michal
On Fri, Mar 15, 2019 at 04:12:17PM +0100, Michal Privoznik wrote:
On 3/15/19 2:53 PM, Pavel Hrdina wrote:
On Wed, Mar 06, 2019 at 03:59:12PM +0100, 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.
Also, SCSI protocol has WRITESAME command which allows us to write a single buffer multiple times. This means, that we have to transfer the buffer far less frequently and have the destination write it over and over again. Ideally, we would allocate a buffer with size equivalent to block size of the LUN and then write it as many times as there are blocks on the LUN. Sadly, this works well in theory but in practise I've found out that it's possible to write only 4096 blocks at once. Any higher value leads to a failure. But still better than Twili^Wwhat we have now.
s/Twili^W//
Since 4096 seems like an arbitrary number, I'm also implementing a mechanism that exponentially decreases the number of blocks we're trying to write at once. It starts at 4096 blocks and if that's too much the number is halved, and so on.
According to documentation if you set 0 as the number of blocks it should write the same block starting at the LBA to the end of the device. Would be nice to try it out and improve the algorithm to use only single transfer.
I've tried several values and found there are some differencies between docs and actual IMPL. What you suggest would be ideal of course, but the Linux Kernel I've tested doesn't allow me to specify anything else but [1, 4096]. Maybe it has something to do with the fact that LUN is virtualized and in fact a file on a filesystem. At any rate, we should be able to zero that too, shouldn't we?
So I was able to find the limit in linux source code and the limit is 4096 for file-based block stores. It looks like that the WRITESAME command is not supported by ramdisk backstore so we probably cannot use it to wipe the iscsi volumes. Pavel
Michal
On 3/15/19 5:18 PM, Pavel Hrdina wrote:
On Fri, Mar 15, 2019 at 04:12:17PM +0100, Michal Privoznik wrote:
On 3/15/19 2:53 PM, Pavel Hrdina wrote:
On Wed, Mar 06, 2019 at 03:59:12PM +0100, 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.
Also, SCSI protocol has WRITESAME command which allows us to write a single buffer multiple times. This means, that we have to transfer the buffer far less frequently and have the destination write it over and over again. Ideally, we would allocate a buffer with size equivalent to block size of the LUN and then write it as many times as there are blocks on the LUN. Sadly, this works well in theory but in practise I've found out that it's possible to write only 4096 blocks at once. Any higher value leads to a failure. But still better than Twili^Wwhat we have now.
s/Twili^W//
Since 4096 seems like an arbitrary number, I'm also implementing a mechanism that exponentially decreases the number of blocks we're trying to write at once. It starts at 4096 blocks and if that's too much the number is halved, and so on.
According to documentation if you set 0 as the number of blocks it should write the same block starting at the LBA to the end of the device. Would be nice to try it out and improve the algorithm to use only single transfer.
I've tried several values and found there are some differencies between docs and actual IMPL. What you suggest would be ideal of course, but the Linux Kernel I've tested doesn't allow me to specify anything else but [1, 4096]. Maybe it has something to do with the fact that LUN is virtualized and in fact a file on a filesystem. At any rate, we should be able to zero that too, shouldn't we?
So I was able to find the limit in linux source code and the limit is 4096 for file-based block stores. It looks like that the WRITESAME command is not supported by ramdisk backstore so we probably cannot use it to wipe the iscsi volumes.
Ah, thank you for such in depth analysis. Yeah, we can't use it. So let me respin my original patch then. Michal
If virStorageBackendISCSIDirectVolWipeZero() fails, it has already reported an error which is probably specific enough. Do not overwrite it with some generic one. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 0af7adf32c..613c38e225 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -706,12 +706,8 @@ virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr pool, switch ((virStorageVolWipeAlgorithm) algorithm) { case VIR_STORAGE_VOL_WIPE_ALG_ZERO: - if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to wipe volume %s"), - vol->name); + if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) goto cleanup; - } break; case VIR_STORAGE_VOL_WIPE_ALG_TRIM: case VIR_STORAGE_VOL_WIPE_ALG_NNSA: -- 2.19.2
On Wed, Mar 06, 2019 at 03:59:13PM +0100, Michal Privoznik wrote:
If virStorageBackendISCSIDirectVolWipeZero() fails, it has already reported an error which is probably specific enough. Do not overwrite it with some generic one.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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 613c38e225..1f33fadad9 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -377,7 +377,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
On Wed, Mar 06, 2019 at 03:59:14PM +0100, Michal Privoznik wrote:
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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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> --- 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 98be434005..cefffdfd64 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2525,11 +2525,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
On Wed, Mar 06, 2019 at 03:59:15PM +0100, Michal Privoznik wrote:
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> --- 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 98be434005..cefffdfd64 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2525,11 +2525,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)
virStorageBackendRefreshLocal() is used for vstorage as well and vstorage supports wiping so this should probably include vstorage. I don't like this ugly hack, but I guess there is no better and easy way how to refactor it. Pavel
goto cleanup;
ret = 0; -- 2.19.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 3/15/19 3:14 PM, Pavel Hrdina wrote:
On Wed, Mar 06, 2019 at 03:59:15PM +0100, Michal Privoznik wrote:
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> --- 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 98be434005..cefffdfd64 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2525,11 +2525,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)
virStorageBackendRefreshLocal() is used for vstorage as well and vstorage supports wiping so this should probably include vstorage.
Ah, good point. I'm adding it here on the list.
I don't like this ugly hack, but I guess there is no better and easy way how to refactor it.
Right. I don't see any neither. Michal
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 cefffdfd64..8cb3c40e51 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()); @@ -721,10 +731,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; } @@ -916,10 +925,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; } @@ -1116,6 +1124,7 @@ storagePoolRefresh(virStoragePoolPtr pool, virStoragePoolObjPtr obj; virStoragePoolDefPtr def; virStorageBackendPtr backend; + VIR_AUTOFREE(char *) stateFile = NULL; int ret = -1; virObjectEventPtr event = NULL; @@ -1144,13 +1153,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, @@ -2240,8 +2244,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
On Wed, Mar 06, 2019 at 03:59:16PM +0100, Michal Privoznik wrote:
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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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 8cb3c40e51..fee1220b53 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
On Wed, Mar 06, 2019 at 03:59:17PM +0100, Michal Privoznik wrote:
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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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
On Wed, Mar 06, 2019 at 03:59:18PM +0100, Michal Privoznik wrote:
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(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Pavel Hrdina