[libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe

John Ferlan posted 2 patches 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170824222829.22398-1-jferlan@redhat.com
src/storage/storage_driver.c |  7 ++--
src/storage/storage_util.c   | 82 +++++++++++++++++++++++++++++---------------
src/storage/storage_util.h   |  3 ++
3 files changed, 62 insertions(+), 30 deletions(-)
[libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Posted by John Ferlan 6 years, 7 months ago
Alter wipeVol to do same refresh operation as pool refresh would do.

John Ferlan (2):
  storage: Introduce virStorageBackendRefreshVolTargetUpdate
  storage: Use virStorageBackendRefreshVolTargetUpdate after wipeVol

 src/storage/storage_driver.c |  7 ++--
 src/storage/storage_util.c   | 82 +++++++++++++++++++++++++++++---------------
 src/storage/storage_util.h   |  3 ++
 3 files changed, 62 insertions(+), 30 deletions(-)

-- 
2.9.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Posted by Martin Kletzander 6 years, 7 months ago
On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
>Alter wipeVol to do same refresh operation as pool refresh would do.
>

I think we should rather keep the format as it is.  Did I miss something
here or isn't the source of the problem just the fact that we wipe the
volume without keeping the format?

>John Ferlan (2):
>  storage: Introduce virStorageBackendRefreshVolTargetUpdate
>  storage: Use virStorageBackendRefreshVolTargetUpdate after wipeVol
>
> src/storage/storage_driver.c |  7 ++--
> src/storage/storage_util.c   | 82 +++++++++++++++++++++++++++++---------------
> src/storage/storage_util.h   |  3 ++
> 3 files changed, 62 insertions(+), 30 deletions(-)
>
>--
>2.9.5
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Posted by John Ferlan 6 years, 7 months ago

On 08/25/2017 05:44 AM, Martin Kletzander wrote:
> On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
>> Alter wipeVol to do same refresh operation as pool refresh would do.
>>
> 
> I think we should rather keep the format as it is.  Did I miss something
> here or isn't the source of the problem just the fact that we wipe the
> volume without keeping the format?
> 

Once you "wipe" the file/image the format that was there (such as qcow2
as noted in the bz from patch2) is no longer there.

Consider the following sequence:

virsh vol-create-as default bz 10M --format qcow2

virsh vol-dumpxml bz default | grep format
    <format type='qcow2'/>

qemu-img info /home/vm-images/bz
image: /home/vm-images/bz
file format: qcow2
virtual size: 10M (10485760 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
    compat: 0.10
    refcount bits: 16

virsh vol-wipe bz default

qemu-img info /home/vm-images/bz
image: /home/vm-images/bz
file format: raw
virtual size: 196K (200704 bytes)
disk size: 196K

virsh vol-dumpxml bz default | grep format
    <format type='qcow2'/>

(without the patch in 2/2 of course)

BTW: I did consider just changing the format to RAW regardless, but
figured that was just too simple and may not be the right answer for
every case.

John

>> John Ferlan (2):
>>  storage: Introduce virStorageBackendRefreshVolTargetUpdate
>>  storage: Use virStorageBackendRefreshVolTargetUpdate after wipeVol
>>
>> src/storage/storage_driver.c |  7 ++--
>> src/storage/storage_util.c   | 82
>> +++++++++++++++++++++++++++++---------------
>> src/storage/storage_util.h   |  3 ++
>> 3 files changed, 62 insertions(+), 30 deletions(-)
>>
>> -- 
>> 2.9.5
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Posted by Martin Kletzander 6 years, 7 months ago
On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote:
>
>
>On 08/25/2017 05:44 AM, Martin Kletzander wrote:
>> On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
>>> Alter wipeVol to do same refresh operation as pool refresh would do.
>>>
>>
>> I think we should rather keep the format as it is.  Did I miss something
>> here or isn't the source of the problem just the fact that we wipe the
>> volume without keeping the format?
>>
>
>Once you "wipe" the file/image the format that was there (such as qcow2
>as noted in the bz from patch2) is no longer there.
>
>Consider the following sequence:
>
>virsh vol-create-as default bz 10M --format qcow2
>
>virsh vol-dumpxml bz default | grep format
>    <format type='qcow2'/>
>
>qemu-img info /home/vm-images/bz
>image: /home/vm-images/bz
>file format: qcow2
>virtual size: 10M (10485760 bytes)
>disk size: 196K
>cluster_size: 65536
>Format specific information:
>    compat: 0.10
>    refcount bits: 16
>
>virsh vol-wipe bz default
>
>qemu-img info /home/vm-images/bz
>image: /home/vm-images/bz
>file format: raw
>virtual size: 196K (200704 bytes)
>disk size: 196K
>
>virsh vol-dumpxml bz default | grep format
>    <format type='qcow2'/>
>
>(without the patch in 2/2 of course)
>
>BTW: I did consider just changing the format to RAW regardless, but
>figured that was just too simple and may not be the right answer for
>every case.
>

Sure, but that's not my point.  My point is that instead of rewriting
the data with zeros, we could re-initialize it.  Anyway, from what I see
in the docs, we "fixed" it by documenting this behaviour already, so my
point is moo:

https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe

(I guess I should know that when I pushed that documentation into the tree)

So ACK series.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Posted by John Ferlan 6 years, 7 months ago

On 08/25/2017 08:28 AM, Martin Kletzander wrote:
> On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote:
>>
>>
>> On 08/25/2017 05:44 AM, Martin Kletzander wrote:
>>> On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
>>>> Alter wipeVol to do same refresh operation as pool refresh would do.
>>>>
>>>
>>> I think we should rather keep the format as it is.  Did I miss something
>>> here or isn't the source of the problem just the fact that we wipe the
>>> volume without keeping the format?
>>>
>>
>> Once you "wipe" the file/image the format that was there (such as qcow2
>> as noted in the bz from patch2) is no longer there.
>>
>> Consider the following sequence:
>>
>> virsh vol-create-as default bz 10M --format qcow2
>>
>> virsh vol-dumpxml bz default | grep format
>>    <format type='qcow2'/>
>>
>> qemu-img info /home/vm-images/bz
>> image: /home/vm-images/bz
>> file format: qcow2
>> virtual size: 10M (10485760 bytes)
>> disk size: 196K
>> cluster_size: 65536
>> Format specific information:
>>    compat: 0.10
>>    refcount bits: 16
>>
>> virsh vol-wipe bz default
>>
>> qemu-img info /home/vm-images/bz
>> image: /home/vm-images/bz
>> file format: raw
>> virtual size: 196K (200704 bytes)
>> disk size: 196K
>>
>> virsh vol-dumpxml bz default | grep format
>>    <format type='qcow2'/>
>>
>> (without the patch in 2/2 of course)
>>
>> BTW: I did consider just changing the format to RAW regardless, but
>> figured that was just too simple and may not be the right answer for
>> every case.
>>
> 
> Sure, but that's not my point.  My point is that instead of rewriting
> the data with zeros, we could re-initialize it.  Anyway, from what I see
> in the docs, we "fixed" it by documenting this behaviour already, so my
> point is moo:
> 
> https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe
> 

Doh! I should have looked there too...  Interesting though... So should
we instead close this bug as documented to work that way?  Or should the
text there be modified as well since the bz will cause the alteration to
at least RAW for "most" storage backends - I think it's only the disk
backend w/ an extended partition and a sparse logical volume that cannot
be wiped, but those fail the wipe - so we'd never get to the change the
target.format code.

John


> (I guess I should know that when I pushed that documentation into the tree)
> 
> So ACK series.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Alter refresh algorithm for volWipe
Posted by Martin Kletzander 6 years, 7 months ago
On Fri, Aug 25, 2017 at 06:11:49PM -0400, John Ferlan wrote:
>
>
>On 08/25/2017 08:28 AM, Martin Kletzander wrote:
>> On Fri, Aug 25, 2017 at 07:52:25AM -0400, John Ferlan wrote:
>>>
>>>
>>> On 08/25/2017 05:44 AM, Martin Kletzander wrote:
>>>> On Thu, Aug 24, 2017 at 06:28:27PM -0400, John Ferlan wrote:
>>>>> Alter wipeVol to do same refresh operation as pool refresh would do.
>>>>>
>>>>
>>>> I think we should rather keep the format as it is.  Did I miss something
>>>> here or isn't the source of the problem just the fact that we wipe the
>>>> volume without keeping the format?
>>>>
>>>
>>> Once you "wipe" the file/image the format that was there (such as qcow2
>>> as noted in the bz from patch2) is no longer there.
>>>
>>> Consider the following sequence:
>>>
>>> virsh vol-create-as default bz 10M --format qcow2
>>>
>>> virsh vol-dumpxml bz default | grep format
>>>    <format type='qcow2'/>
>>>
>>> qemu-img info /home/vm-images/bz
>>> image: /home/vm-images/bz
>>> file format: qcow2
>>> virtual size: 10M (10485760 bytes)
>>> disk size: 196K
>>> cluster_size: 65536
>>> Format specific information:
>>>    compat: 0.10
>>>    refcount bits: 16
>>>
>>> virsh vol-wipe bz default
>>>
>>> qemu-img info /home/vm-images/bz
>>> image: /home/vm-images/bz
>>> file format: raw
>>> virtual size: 196K (200704 bytes)
>>> disk size: 196K
>>>
>>> virsh vol-dumpxml bz default | grep format
>>>    <format type='qcow2'/>
>>>
>>> (without the patch in 2/2 of course)
>>>
>>> BTW: I did consider just changing the format to RAW regardless, but
>>> figured that was just too simple and may not be the right answer for
>>> every case.
>>>
>>
>> Sure, but that's not my point.  My point is that instead of rewriting
>> the data with zeros, we could re-initialize it.  Anyway, from what I see
>> in the docs, we "fixed" it by documenting this behaviour already, so my
>> point is moo:
>>
>> https://libvirt.org/html/libvirt-libvirt-storage.html#virStorageVolWipe
>>
>
>Doh! I should have looked there too...  Interesting though... So should
>we instead close this bug as documented to work that way?  Or should the
>text there be modified as well since the bz will cause the alteration to
>at least RAW for "most" storage backends - I think it's only the disk
>backend w/ an extended partition and a sparse logical volume that cannot
>be wiped, but those fail the wipe - so we'd never get to the change the
>target.format code.
>

I think that all the problems are essentially caused by people
misunderstanding the API's purpose.  That's why I ACKed it, as we at
least report the right information.  I think your solution is precisely
the midpoint between not doing anything and fixing all corner cases.

>John
>
>
>> (I guess I should know that when I pushed that documentation into the tree)
>>
>> So ACK series.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list