Patchwork block: for HMP commit() operations on 'all', skip non-COW drives

login
register
mail settings
Submitter Jeff Cody
Date Feb. 26, 2013, 2:55 p.m.
Message ID <340453d16312b750540b462e80742aeb4e3a4625.1361890319.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/223233/
State New
Headers show

Comments

Jeff Cody - Feb. 26, 2013, 2:55 p.m.
During a commit of 'all' using the HMP non-live commit, the operation
is aborted and returns error on the first error enountered.  When
non-COW drives are in use (e.g. ejected floppy, cdrom, or drives without
a backing parent), that means a commit all will return an error of either
-ENOMEDIUM or -ENOTSUP.  This is not desirable, so for the 'all' commit
case, only attempt the commit if both bs->drv and bs->backing_hd are
present.

More succinctly: 'commit all' now means a commit on all COW drives.

This means an individual commit to a specific non-COW drive will still
return the appropriate error (-ENOMEDIUM if eject / not present, -ENOTSUP
if no backing file).

Reported-by: Jan Kiszka <jan.kiszka@web.de>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Paolo Bonzini - Feb. 26, 2013, 3:03 p.m.
Il 26/02/2013 15:55, Jeff Cody ha scritto:
> During a commit of 'all' using the HMP non-live commit, the operation
> is aborted and returns error on the first error enountered.  When
> non-COW drives are in use (e.g. ejected floppy, cdrom, or drives without
> a backing parent), that means a commit all will return an error of either
> -ENOMEDIUM or -ENOTSUP.  This is not desirable, so for the 'all' commit
> case, only attempt the commit if both bs->drv and bs->backing_hd are
> present.
> 
> More succinctly: 'commit all' now means a commit on all COW drives.
> 
> This means an individual commit to a specific non-COW drive will still
> return the appropriate error (-ENOMEDIUM if eject / not present, -ENOTSUP
> if no backing file).
> 
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 50dab8e..7a105dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -1620,9 +1620,11 @@ int bdrv_commit_all(void)
>      BlockDriverState *bs;
>  
>      QTAILQ_FOREACH(bs, &bdrv_states, list) {
> -        int ret = bdrv_commit(bs);
> -        if (ret < 0) {
> -            return ret;
> +        if (bs->drv && bs->backing_hd) {
> +            int ret = bdrv_commit(bs);
> +            if (ret < 0) {
> +                return ret;
> +            }
>          }
>      }
>      return 0;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Kevin Wolf - Feb. 26, 2013, 3:11 p.m.
Am 26.02.2013 um 15:55 hat Jeff Cody geschrieben:
> During a commit of 'all' using the HMP non-live commit, the operation
> is aborted and returns error on the first error enountered.  When
> non-COW drives are in use (e.g. ejected floppy, cdrom, or drives without
> a backing parent), that means a commit all will return an error of either
> -ENOMEDIUM or -ENOTSUP.  This is not desirable, so for the 'all' commit
> case, only attempt the commit if both bs->drv and bs->backing_hd are
> present.
> 
> More succinctly: 'commit all' now means a commit on all COW drives.
> 
> This means an individual commit to a specific non-COW drive will still
> return the appropriate error (-ENOMEDIUM if eject / not present, -ENOTSUP
> if no backing file).
> 
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Stefan Hajnoczi - Feb. 26, 2013, 4:14 p.m.
On Tue, Feb 26, 2013 at 09:55:48AM -0500, Jeff Cody wrote:
> During a commit of 'all' using the HMP non-live commit, the operation
> is aborted and returns error on the first error enountered.  When
> non-COW drives are in use (e.g. ejected floppy, cdrom, or drives without
> a backing parent), that means a commit all will return an error of either
> -ENOMEDIUM or -ENOTSUP.  This is not desirable, so for the 'all' commit
> case, only attempt the commit if both bs->drv and bs->backing_hd are
> present.
> 
> More succinctly: 'commit all' now means a commit on all COW drives.
> 
> This means an individual commit to a specific non-COW drive will still
> return the appropriate error (-ENOMEDIUM if eject / not present, -ENOTSUP
> if no backing file).
> 
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan
Markus Armbruster - Feb. 27, 2013, 8:20 a.m.
Jeff Cody <jcody@redhat.com> writes:

> During a commit of 'all' using the HMP non-live commit, the operation
> is aborted and returns error on the first error enountered.  When
> non-COW drives are in use (e.g. ejected floppy, cdrom, or drives without
> a backing parent), that means a commit all will return an error of either
> -ENOMEDIUM or -ENOTSUP.  This is not desirable, so for the 'all' commit
> case, only attempt the commit if both bs->drv and bs->backing_hd are
> present.
>
> More succinctly: 'commit all' now means a commit on all COW drives.
>
> This means an individual commit to a specific non-COW drive will still
> return the appropriate error (-ENOMEDIUM if eject / not present, -ENOTSUP
> if no backing file).
>
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Pointing to the commit that caused the regression would have been nice,
but it's not worth a respin.

Patch

diff --git a/block.c b/block.c
index 50dab8e..7a105dc 100644
--- a/block.c
+++ b/block.c
@@ -1620,9 +1620,11 @@  int bdrv_commit_all(void)
     BlockDriverState *bs;
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
-        int ret = bdrv_commit(bs);
-        if (ret < 0) {
-            return ret;
+        if (bs->drv && bs->backing_hd) {
+            int ret = bdrv_commit(bs);
+            if (ret < 0) {
+                return ret;
+            }
         }
     }
     return 0;