[libvirt] [PATCH v2] doc: Correct the default werror policy

Philipp Hahn posted 1 patch 170 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/c14d5994579d9212189be0f3c511c59d99375950.1487849026.git.hahn@univention.de
docs/formatdomain.html.in | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

[libvirt] [PATCH v2] doc: Correct the default werror policy

Posted by Philipp Hahn 170 weeks ago
The documentation is plain wrong about the default write_error policy,
as its only implemented by QEMU (src/vz/vz_utils.c is the only other
case, which simply explodes, is anything except other then
VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT is used).

And QEMUs default is VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE.

Signed-off-by: Philipp Hahn <hahn@univention.de>
---
 docs/formatdomain.html.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
---
Osier Yang proposed v1 on 2011-10-24, which never got applied due to
discussions of "enospace" vs. "enospc".

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 02ce792..3e1a193 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2745,7 +2745,8 @@
             how the hypervisor will behave on a disk read or write
             error, possible values are "stop", "report", "ignore", and
             "enospace".<span class="since">Since 0.8.0, "report" since
-            0.9.7</span> The default setting of error_policy is "report".
+            0.9.7</span> The default is left to the discretion of the
+            hypervisor: QEMU defaults to "enospace".<br/>
             There is also an
             optional <code>rerror_policy</code> that controls behavior
             for read errors only. <span class="since">Since
@@ -2755,8 +2756,7 @@
             read errors. Also note that "enospace" is not a valid
             policy for read errors, so if <code>error_policy</code> is
             set to "enospace" and no <code>rerror_policy</code> is
-            given, the read error policy will be left at its default,
-            which is "report".
+            given, the read error policy will be left at its default.
           </li>
           <li>
             The optional <code>io</code> attribute controls specific
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] doc: Correct the default werror policy

Posted by Martin Kletzander 170 weeks ago
On Thu, Feb 23, 2017 at 12:30:02PM +0100, Philipp Hahn wrote:
>The documentation is plain wrong about the default write_error policy,
>as its only implemented by QEMU (src/vz/vz_utils.c is the only other
>case, which simply explodes, is anything except other then
>VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT is used).
>
>And QEMUs default is VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE.
>
>Signed-off-by: Philipp Hahn <hahn@univention.de>
>---
> docs/formatdomain.html.in | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>---
>Osier Yang proposed v1 on 2011-10-24, which never got applied due to
>discussions of "enospace" vs. "enospc".
>
>diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>index 02ce792..3e1a193 100644
>--- a/docs/formatdomain.html.in
>+++ b/docs/formatdomain.html.in
>@@ -2745,7 +2745,8 @@
>             how the hypervisor will behave on a disk read or write
>             error, possible values are "stop", "report", "ignore", and
>             "enospace".<span class="since">Since 0.8.0, "report" since
>-            0.9.7</span> The default setting of error_policy is "report".
>+            0.9.7</span> The default is left to the discretion of the
>+            hypervisor: QEMU defaults to "enospace".<br/>

Did really all the versions do that?  I'm not sure.  Anyway, I don't
really think we should guarantee something that we have no power over,
especially since they can change the default and nobody will fix it in
our docs.  Only defaults that QEMU *driver* selects shoud be mentioned
in here (as that will also be visible in the XML), but when talking
about hypervisor defaults, I would just leave it to "hypervisor
default".

>             There is also an
>             optional <code>rerror_policy</code> that controls behavior
>             for read errors only. <span class="since">Since
>@@ -2755,8 +2756,7 @@
>             read errors. Also note that "enospace" is not a valid
>             policy for read errors, so if <code>error_policy</code> is
>             set to "enospace" and no <code>rerror_policy</code> is
>-            given, the read error policy will be left at its default,
>-            which is "report".
>+            given, the read error policy will be left at its default.
>           </li>
>           <li>
>             The optional <code>io</code> attribute controls specific
>--
>2.1.4
>
>--
>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

[libvirt] [PATCH v3] doc: Correct the default werror policy

Posted by Philipp Hahn 169 weeks ago
The documentation is plain wrong about the default write_error policy,
as its only implemented by QEMU (src/vz/vz_utils.c is the only other
case, which simply explodes, is anything except other then
VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT is used).

And QEMUs default is VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE.

Signed-off-by: Philipp Hahn <hahn@univention.de>
---
Osier Yang proposed v1 on 2011-10-24, which never got applied due to
discussions of "enospace" vs. "enospc". 

v2: Remove internal QEMU default

 docs/formatdomain.html.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 02ce792..dc44a55 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2745,7 +2745,8 @@
             how the hypervisor will behave on a disk read or write
             error, possible values are "stop", "report", "ignore", and
             "enospace".<span class="since">Since 0.8.0, "report" since
-            0.9.7</span> The default setting of error_policy is "report".
+            0.9.7</span> The default is left to the discretion of the
+            hypervisor.<br/>
             There is also an
             optional <code>rerror_policy</code> that controls behavior
             for read errors only. <span class="since">Since
@@ -2755,8 +2756,7 @@
             read errors. Also note that "enospace" is not a valid
             policy for read errors, so if <code>error_policy</code> is
             set to "enospace" and no <code>rerror_policy</code> is
-            given, the read error policy will be left at its default,
-            which is "report".
+            given, the read error policy will be left at its default.
           </li>
           <li>
             The optional <code>io</code> attribute controls specific
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] doc: Correct the default werror policy

Posted by Martin Kletzander 168 weeks ago
On Wed, Mar 01, 2017 at 08:44:54PM +0100, Philipp Hahn wrote:
>The documentation is plain wrong about the default write_error policy,
>as its only implemented by QEMU (src/vz/vz_utils.c is the only other
>case, which simply explodes, is anything except other then
>VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT is used).
>

Sounds like bashing about some problem.  Is there any bug somewhere?  It
does not add any useful information to the commit message, so I'll strip
it off before pushing.

So my question is; is there anything in that driver that needs fixing
WRT this patch?

>And QEMUs default is VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE.
>
>Signed-off-by: Philipp Hahn <hahn@univention.de>
>---
>Osier Yang proposed v1 on 2011-10-24, which never got applied due to
>discussions of "enospace" vs. "enospc".
>
>v2: Remove internal QEMU default
>
> docs/formatdomain.html.in | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>index 02ce792..dc44a55 100644
>--- a/docs/formatdomain.html.in
>+++ b/docs/formatdomain.html.in
>@@ -2745,7 +2745,8 @@
>             how the hypervisor will behave on a disk read or write
>             error, possible values are "stop", "report", "ignore", and
>             "enospace".<span class="since">Since 0.8.0, "report" since
>-            0.9.7</span> The default setting of error_policy is "report".
>+            0.9.7</span> The default is left to the discretion of the
>+            hypervisor.<br/>
>             There is also an
>             optional <code>rerror_policy</code> that controls behavior
>             for read errors only. <span class="since">Since
>@@ -2755,8 +2756,7 @@
>             read errors. Also note that "enospace" is not a valid
>             policy for read errors, so if <code>error_policy</code> is
>             set to "enospace" and no <code>rerror_policy</code> is
>-            given, the read error policy will be left at its default,
>-            which is "report".
>+            given, the read error policy will be left at its default.
>           </li>
>           <li>
>             The optional <code>io</code> attribute controls specific
>--
>2.1.4
>
>--
>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 v3] doc: Correct the default werror policy

Posted by Philipp Hahn 168 weeks ago
Hello,

Am 08.03.2017 um 17:28 schrieb Martin Kletzander:
> On Wed, Mar 01, 2017 at 08:44:54PM +0100, Philipp Hahn wrote:
>> The documentation is plain wrong about the default write_error policy,
>> as its only implemented by QEMU (src/vz/vz_utils.c is the only other
>> case, which simply explodes, is anything except other then
>> VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT is used).
>>
> 
> Sounds like bashing about some problem.  Is there any bug somewhere?  It
> does not add any useful information to the commit message, so I'll strip
> it off before pushing.

My problem is, that the documentation does not match the implementation
and from reading that documentation you get a wrong impression:

The documentation claims, that the default is "report", while the QEMU
internal default is "ENOSPC".
This has a sever difference, namely that your VM gets suspended when
your host systems file systems get full. This is a good default, as you
don't loose any data, but on the other hand your VM disappears from the
network until you free some space and resume the VM.

I have seen this happen multiple times and I know the behaviour by now,
but others have reported this "strange" behaviour to me several times:
they keep wondering why their VM suddenly gets paused, while the libvirt
documentation tells them that the VM would see all errors (report).

So lets please tell those people to look at the QEMU code and not the
libvirt code.

> So my question is; is there anything in that driver that needs fixing
> WRT this patch?

The default is okay and the most save regarding data loss, but might not
be okay if service availability is more important to you.

We can even apply the following change on top, as QEMU is the only
driver implementing a configurable error policy:

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index dc44a55..025007f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2744,13 +2744,14 @@
>              The optional <code>error_policy</code> attribute controls
>              how the hypervisor will behave on a disk read or write
>              error, possible values are "stop", "report", "ignore", and
> -            "enospace".<span class="since">Since 0.8.0, "report" since
> +            "enospace".<span class="since">Since 0.8.0 (QEMU and KVM only), "report" since
>              0.9.7</span> The default is left to the discretion of the
>              hypervisor.<br/>
>              There is also an
>              optional <code>rerror_policy</code> that controls behavior
>              for read errors only. <span class="since">Since
> -            0.9.7</span>. If no rerror_policy is given, error_policy
> +            0.9.7 (QEMU and KVM only)</span>.
> +            If no rerror_policy is given, error_policy
>              is used for both read and write errors. If rerror_policy
>              is given, it overrides the <code>error_policy</code> for
>              read errors. Also note that "enospace" is not a valid

And "report" is the default for QEMU since
release_0_10_0~298 ,which made the policy configurable, but the commit
message claims "report" was even the default before that:

> commit 428c570512c1d9298b52dc9fc1a541b542a5c117
> Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Wed Jan 21 18:59:04 2009 +0000
> 
>     Stop VM on ENOSPC error. (Gleb Natapov)
>     
>     This version of the patch adds new option "werror" to -drive flag.
>     Possible values are:
>     
>     report    - report errors to a guest as IO errors
>     ignore    - continue as if nothing happened
>     stop      - stop VM on any error and retry last command on resume
>     enospc    - stop vm on ENOSPC error and retry last command on resume
>                 all other errors are reported to a guest.
>     
>     Default is "report" to maintain current behaviour.
>     
>     Signed-off-by: Gleb Natapov <gleb@redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>     
>     
>     git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@6388 c046a42c-6fe2-441c-8c8c-71466251a162

Philipp

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] doc: Correct the default werror policy

Posted by Martin Kletzander 168 weeks ago
On Wed, Mar 08, 2017 at 07:26:08PM +0100, Philipp Hahn wrote:
>Hello,
>
>Am 08.03.2017 um 17:28 schrieb Martin Kletzander:
>> On Wed, Mar 01, 2017 at 08:44:54PM +0100, Philipp Hahn wrote:
>>> The documentation is plain wrong about the default write_error policy,
>>> as its only implemented by QEMU (src/vz/vz_utils.c is the only other
>>> case, which simply explodes, is anything except other then
>>> VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT is used).
>>>
>>
>> Sounds like bashing about some problem.  Is there any bug somewhere?  It
>> does not add any useful information to the commit message, so I'll strip
>> it off before pushing.
>
>My problem is, that the documentation does not match the implementation
>and from reading that documentation you get a wrong impression:
>
>The documentation claims, that the default is "report", while the QEMU
>internal default is "ENOSPC".
>This has a sever difference, namely that your VM gets suspended when
>your host systems file systems get full. This is a good default, as you
>don't loose any data, but on the other hand your VM disappears from the
>network until you free some space and resume the VM.
>
>I have seen this happen multiple times and I know the behaviour by now,
>but others have reported this "strange" behaviour to me several times:
>they keep wondering why their VM suddenly gets paused, while the libvirt
>documentation tells them that the VM would see all errors (report).
>
>So lets please tell those people to look at the QEMU code and not the
>libvirt code.
>

That's why we are trying not to promise any hypervisor defaults in the
docs (my review for v2).  Mostly because the hypervisor default might
change.  Every time you need something specific you need to specify it
in the XML.  I'm sorry that disappointed you.

>> So my question is; is there anything in that driver that needs fixing
>> WRT this patch?
>
>The default is okay and the most save regarding data loss, but might not
>be okay if service availability is more important to you.
>
>We can even apply the following change on top, as QEMU is the only
>driver implementing a configurable error policy:
>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index dc44a55..025007f 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -2744,13 +2744,14 @@
>>              The optional <code>error_policy</code> attribute controls
>>              how the hypervisor will behave on a disk read or write
>>              error, possible values are "stop", "report", "ignore", and
>> -            "enospace".<span class="since">Since 0.8.0, "report" since
>> +            "enospace".<span class="since">Since 0.8.0 (QEMU and KVM only), "report" since
>>              0.9.7</span> The default is left to the discretion of the
>>              hypervisor.<br/>
>>              There is also an
>>              optional <code>rerror_policy</code> that controls behavior
>>              for read errors only. <span class="since">Since
>> -            0.9.7</span>. If no rerror_policy is given, error_policy
>> +            0.9.7 (QEMU and KVM only)</span>.
>> +            If no rerror_policy is given, error_policy
>>              is used for both read and write errors. If rerror_policy
>>              is given, it overrides the <code>error_policy</code> for
>>              read errors. Also note that "enospace" is not a valid
>

Yeah, unfortunately this is something we should handle more efficiently.
There's a lot of XML and documentation that's only QEMU-related.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list