8:59 a.m.
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
8:59 a.m.
New subject: [libvirt] [PATCH v2 1/8] iscsi_direct: Make virStorageBackendISCSIDirectGetLun report error properly
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(a)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
8:31 a.m.
New subject: [libvirt] [PATCH v2 1/8] iscsi_direct: Make virStorageBackendISCSIDirectGetLun report error properly
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(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
8:59 a.m.
New subject: [libvirt] [PATCH v2 2/8] 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.
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(a)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
8:53 a.m.
New subject: [libvirt] [PATCH v2 2/8] storage_backend_iscsi_direct: Simplify vol zeroing
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:12 a.m.
New subject: [libvirt] [PATCH v2 2/8] storage_backend_iscsi_direct: Simplify vol zeroing
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
11:18 a.m.
New subject: [libvirt] [PATCH v2 2/8] storage_backend_iscsi_direct: Simplify vol zeroing
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
1:17 a.m.
New subject: [libvirt] [PATCH v2 2/8] storage_backend_iscsi_direct: Simplify vol zeroing
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
8:59 a.m.
New subject: [libvirt] [PATCH v2 3/8] iscsi_direct: Don't overwrite error in virStorageBackenISCSIDirectWipeVol()
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(a)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
8:54 a.m.
New subject: [libvirt] [PATCH v2 3/8] iscsi_direct: Don't overwrite error in virStorageBackenISCSIDirectWipeVol()
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(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
8:59 a.m.
New subject: [libvirt] [PATCH v2 4/8] 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 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
8:58 a.m.
New subject: [libvirt] [PATCH v2 4/8] virISCSIDirectReportLuns: Drop ClearVols
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(a)redhat.com>
---
src/storage/storage_backend_iscsi_direct.c | 1 -
1 file changed, 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
8:59 a.m.
New subject: [libvirt] [PATCH v2 5/8] 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>
---
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
9:14 a.m.
New subject: [libvirt] [PATCH v2 5/8] storageVolWipePattern: Don't take shortcut to refreshPool()
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:58 a.m.
New subject: [libvirt] [PATCH v2 5/8] storageVolWipePattern: Don't take shortcut to refreshPool()
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(a)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
8:59 a.m.
New subject: [libvirt] [PATCH v2 6/8] 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 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
9:19 a.m.
New subject: [libvirt] [PATCH v2 6/8] storage_driver: Introduce storagePoolRefreshImpl()
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(a)redhat.com>
---
src/storage/storage_driver.c | 61 +++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 29 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
8:59 a.m.
New subject: [libvirt] [PATCH v2 7/8] 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 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
9:23 a.m.
New subject: [libvirt] [PATCH v2 7/8] storagePoolRefreshFailCleanup: Clear volumes on failed refresh
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(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(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
8:59 a.m.
New subject: [libvirt] [PATCH v2 8/8] 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
9:24 a.m.
New subject: [libvirt] [PATCH v2 8/8] virsh-pool: Offer only active pool for pool-refresh completer
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(a)redhat.com>
---
tools/virsh-pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Pavel Hrdina <phrdina(a)redhat.com>
2116
days inactive
2126
days old
20 comments
3 participants
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Pavel Hrdina