[Qemu-devel] [PATCH v3 0/4] block: Block driver callbacks fixes

[Qemu-devel] [PATCH v3 0/4] block: Block driver callbacks fixes

[Manos Pitsidianakis]

This series makes implementing some of the bdrv_* callbacks easier for block
drivers by passing requests to bs->file if bs->drv doesn't implement it instead
of failing, and adding default bdrv_co_get_block_status() implementations.

This is based against Kevin Wolf's block branch, commit
da4bd74d2450ab72a7c26bbabb10c6a287dd043e

v3:
  minor changes by Eric Blake's suggestion
  new patch: remove bdrv_truncate method from blkdebug

v2:
  do not pass to bs->file if bs->drv is NULL
  move bs->file check outside of bdrv_inc_in_flight() area in bdrv_co_ioctl()
  new patch: remove duplicate code from block/raw-format.c

Manos Pitsidianakis (4):
  block: Pass bdrv_* methods to bs->file by default
  block: Use defaults of bdrv_* callbacks in raw
  block: Remove bdrv_truncate callback in blkdebug
  block: Add default implementations for bdrv_co_get_block_status()

 block.c                   | 27 +++++++++++++++++++++++++--
 block/blkdebug.c          | 18 +-----------------
 block/commit.c            | 12 +-----------
 block/io.c                | 31 +++++++++++++++++++++++++++++++
 block/mirror.c            | 12 +-----------
 block/raw-format.c        | 30 ------------------------------
 include/block/block_int.h | 18 ++++++++++++++++++
 7 files changed, 77 insertions(+), 71 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v3 1/4] block: Pass bdrv_* methods to bs->file by default

[Manos Pitsidianakis]

The following functions fail if bs->drv does not implement them:

bdrv_probe_blocksizes
bdrv_probe_geometry
bdrv_truncate
bdrv_has_zero_init
bdrv_get_info
bdrv_media_changed
bdrv_eject
bdrv_lock_medium
bdrv_co_ioctl

Instead, the call should be passed to bs->file if it exists, to allow
filter drivers to support those methods without implementing them.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c    | 27 +++++++++++++++++++++++++--
 block/io.c |  5 +++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 69439628..8a122142 100644
--- a/block.c
+++ b/block.c
@@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 
     if (drv && drv->bdrv_probe_blocksizes) {
         return drv->bdrv_probe_blocksizes(bs, bsz);
+    } else if (drv && bs->file && bs->file->bs) {
+        return bdrv_probe_blocksizes(bs->file->bs, bsz);
     }
 
     return -ENOTSUP;
@@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 
     if (drv && drv->bdrv_probe_geometry) {
         return drv->bdrv_probe_geometry(bs, geo);
+    } else if (drv && bs->file && bs->file->bs) {
+        return bdrv_probe_geometry(bs->file->bs, geo);
     }
 
     return -ENOTSUP;
@@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
 
     assert(child->perm & BLK_PERM_RESIZE);
 
+    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
     if (!drv) {
         error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
     }
     if (!drv->bdrv_truncate) {
+        if (bs->file && bs->file->bs) {
+            return bdrv_truncate(bs->file, offset, errp);
+        }
         error_setg(errp, "Image format driver does not support resize");
         return -ENOTSUP;
     }
@@ -3778,6 +3786,9 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     if (bs->drv->bdrv_has_zero_init) {
         return bs->drv->bdrv_has_zero_init(bs);
     }
+    if (bs->file && bs->file->bs) {
+        return bdrv_has_zero_init(bs->file->bs);
+    }
 
     /* safe default */
     return 0;
@@ -3832,10 +3843,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
+    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
+    if (!drv) {
         return -ENOMEDIUM;
-    if (!drv->bdrv_get_info)
+    }
+    if (!drv->bdrv_get_info) {
+        if (bs->file && bs->file->bs) {
+            return bdrv_get_info(bs->file->bs, bdi);
+        }
         return -ENOTSUP;
+    }
     memset(bdi, 0, sizeof(*bdi));
     return drv->bdrv_get_info(bs, bdi);
 }
@@ -4205,6 +4222,8 @@ int bdrv_media_changed(BlockDriverState *bs)
 
     if (drv && drv->bdrv_media_changed) {
         return drv->bdrv_media_changed(bs);
+    } else if (drv && bs->file && bs->file->bs) {
+        return bdrv_media_changed(bs->file->bs);
     }
     return -ENOTSUP;
 }
@@ -4218,6 +4237,8 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag)
 
     if (drv && drv->bdrv_eject) {
         drv->bdrv_eject(bs, eject_flag);
+    } else if (drv && bs->file && bs->file->bs) {
+        bdrv_eject(bs->file->bs, eject_flag);
     }
 }
 
@@ -4233,6 +4254,8 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked)
 
     if (drv && drv->bdrv_lock_medium) {
         drv->bdrv_lock_medium(bs, locked);
+    } else if (drv && bs->file && bs->file->bs) {
+        bdrv_lock_medium(bs->file->bs, locked);
     }
 }
 
diff --git a/block/io.c b/block/io.c
index 5c146b5a..c22a9bf2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2425,6 +2425,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
     };
     BlockAIOCB *acb;
 
+    if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl &&
+        bs->file && bs->file->bs) {
+        return bdrv_co_ioctl(bs->file->bs, req, buf);
+    }
+
     bdrv_inc_in_flight(bs);
     if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
         co.ret = -ENOTSUP;
-- 
2.11.0

[Qemu-devel] [PATCH v3 2/4] block: Use defaults of bdrv_* callbacks in raw

[Manos Pitsidianakis]

Now that passing the call to bs->file is the default for some bdrv_*
callbacks, remove the duplicate implementations in block/raw-format.c

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/raw-format.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index 1ea8c2d7..ccd63411 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -312,11 +312,6 @@ static int64_t raw_getlength(BlockDriverState *bs)
     return s->size;
 }
 
-static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
-{
-    return bdrv_get_info(bs->file->bs, bdi);
-}
-
 static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     if (bs->probed) {
@@ -346,21 +341,6 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
     return bdrv_truncate(bs->file, offset, errp);
 }
 
-static int raw_media_changed(BlockDriverState *bs)
-{
-    return bdrv_media_changed(bs->file->bs);
-}
-
-static void raw_eject(BlockDriverState *bs, bool eject_flag)
-{
-    bdrv_eject(bs->file->bs, eject_flag);
-}
-
-static void raw_lock_medium(BlockDriverState *bs, bool locked)
-{
-    bdrv_lock_medium(bs->file->bs, locked);
-}
-
 static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
     BDRVRawState *s = bs->opaque;
@@ -370,11 +350,6 @@ static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
     return bdrv_co_ioctl(bs->file->bs, req, buf);
 }
 
-static int raw_has_zero_init(BlockDriverState *bs)
-{
-    return bdrv_has_zero_init(bs->file->bs);
-}
-
 static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
 {
     return bdrv_create_file(filename, opts, errp);
@@ -479,16 +454,11 @@ BlockDriver bdrv_raw = {
     .bdrv_truncate        = &raw_truncate,
     .bdrv_getlength       = &raw_getlength,
     .has_variable_length  = true,
-    .bdrv_get_info        = &raw_get_info,
     .bdrv_refresh_limits  = &raw_refresh_limits,
     .bdrv_probe_blocksizes = &raw_probe_blocksizes,
     .bdrv_probe_geometry  = &raw_probe_geometry,
-    .bdrv_media_changed   = &raw_media_changed,
-    .bdrv_eject           = &raw_eject,
-    .bdrv_lock_medium     = &raw_lock_medium,
     .bdrv_co_ioctl        = &raw_co_ioctl,
     .create_opts          = &raw_create_opts,
-    .bdrv_has_zero_init   = &raw_has_zero_init
 };
 
 static void bdrv_raw_init(void)
-- 
2.11.0

[Qemu-devel] [PATCH v3 3/4] block: Remove bdrv_truncate callback in blkdebug

[Manos Pitsidianakis]

Now that bdrv_truncate is passed to bs->file by default, remove the
callback from block/blkdebug.c

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/blkdebug.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b25856c4..5e118e10 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -821,11 +821,6 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
     return bdrv_getlength(bs->file->bs);
 }
 
-static int blkdebug_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
-{
-    return bdrv_truncate(bs->file, offset, errp);
-}
-
 static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -916,7 +911,6 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_child_perm        = bdrv_filter_default_perms,
 
     .bdrv_getlength         = blkdebug_getlength,
-    .bdrv_truncate          = blkdebug_truncate,
     .bdrv_refresh_filename  = blkdebug_refresh_filename,
     .bdrv_refresh_limits    = blkdebug_refresh_limits,
 
-- 
2.11.0

[Qemu-devel] [PATCH v3 4/4] block: Add default implementations for bdrv_co_get_block_status()

[Manos Pitsidianakis]

bdrv_co_get_block_status_from_file() and
bdrv_co_get_block_status_from_backing() set *file to bs->file and
bs->backing respectively, so that bdrv_co_get_block_status() can recurse
to them. Future block drivers won't have to duplicate code to implement
this.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/blkdebug.c          | 12 +-----------
 block/commit.c            | 12 +-----------
 block/io.c                | 26 ++++++++++++++++++++++++++
 block/mirror.c            | 12 +-----------
 include/block/block_int.h | 18 ++++++++++++++++++
 5 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5e118e10..5947e115 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -641,16 +641,6 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs,
     return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
 }
 
-static int64_t coroutine_fn blkdebug_co_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
-{
-    *pnum = nb_sectors;
-    *file = bs->file->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-        (sector_num << BDRV_SECTOR_BITS);
-}
-
 static void blkdebug_close(BlockDriverState *bs)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -919,7 +909,7 @@ static BlockDriver bdrv_blkdebug = {
     .bdrv_co_flush_to_disk  = blkdebug_co_flush,
     .bdrv_co_pwrite_zeroes  = blkdebug_co_pwrite_zeroes,
     .bdrv_co_pdiscard       = blkdebug_co_pdiscard,
-    .bdrv_co_get_block_status = blkdebug_co_get_block_status,
+    .bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
 
     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
diff --git a/block/commit.c b/block/commit.c
index 524bd549..5b04f832 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -247,16 +247,6 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs,
     return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
 }
 
-static int64_t coroutine_fn bdrv_commit_top_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
-{
-    *pnum = nb_sectors;
-    *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-           (sector_num << BDRV_SECTOR_BITS);
-}
-
 static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts)
 {
     bdrv_refresh_filename(bs->backing->bs);
@@ -282,7 +272,7 @@ static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c,
 static BlockDriver bdrv_commit_top = {
     .format_name                = "commit_top",
     .bdrv_co_preadv             = bdrv_commit_top_preadv,
-    .bdrv_co_get_block_status   = bdrv_commit_top_get_block_status,
+    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_commit_top_refresh_filename,
     .bdrv_close                 = bdrv_commit_top_close,
     .bdrv_child_perm            = bdrv_commit_top_child_perm,
diff --git a/block/io.c b/block/io.c
index c22a9bf2..d28c6864 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1706,6 +1706,32 @@ typedef struct BdrvCoGetBlockStatusData {
     bool done;
 } BdrvCoGetBlockStatusData;
 
+int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
+                                                        int64_t sector_num,
+                                                        int nb_sectors,
+                                                        int *pnum,
+                                                        BlockDriverState **file)
+{
+    assert(bs->file && bs->file->bs);
+    *pnum = nb_sectors;
+    *file = bs->file->bs;
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+           (sector_num << BDRV_SECTOR_BITS);
+}
+
+int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
+                                                           int64_t sector_num,
+                                                           int nb_sectors,
+                                                           int *pnum,
+                                                           BlockDriverState **file)
+{
+    assert(bs->backing && bs->backing->bs);
+    *pnum = nb_sectors;
+    *file = bs->backing->bs;
+    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
+           (sector_num << BDRV_SECTOR_BITS);
+}
+
 /*
  * Returns the allocation status of the specified sectors.
  * Drivers not implementing the functionality are assumed to not support
diff --git a/block/mirror.c b/block/mirror.c
index 61a862dc..e8bf5f40 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1052,16 +1052,6 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
     return bdrv_co_flush(bs->backing->bs);
 }
 
-static int64_t coroutine_fn bdrv_mirror_top_get_block_status(
-    BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
-    BlockDriverState **file)
-{
-    *pnum = nb_sectors;
-    *file = bs->backing->bs;
-    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-           (sector_num << BDRV_SECTOR_BITS);
-}
-
 static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags)
 {
@@ -1108,7 +1098,7 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
     .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
     .bdrv_co_flush              = bdrv_mirror_top_flush,
-    .bdrv_co_get_block_status   = bdrv_mirror_top_get_block_status,
+    .bdrv_co_get_block_status   = bdrv_co_get_block_status_from_backing,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_close                 = bdrv_mirror_top_close,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 15fa6021..ab851073 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -945,6 +945,24 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
                                uint64_t perm, uint64_t shared,
                                uint64_t *nperm, uint64_t *nshared);
 
+/*
+ * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * their file.
+ */
+int64_t coroutine_fn bdrv_co_get_block_status_from_file(BlockDriverState *bs,
+                                                        int64_t sector_num,
+                                                        int nb_sectors,
+                                                        int *pnum,
+                                                        BlockDriverState **file);
+/*
+ * Default implementation for drivers to pass bdrv_co_get_block_status() to
+ * their backing file.
+ */
+int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,
+                                                           int64_t sector_num,
+                                                           int nb_sectors,
+                                                           int *pnum,
+                                                           BlockDriverState **file);
 const char *bdrv_get_parent_name(const BlockDriverState *bs);
 void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
 bool blk_dev_has_removable_media(BlockBackend *blk);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v3 1/4] block: Pass bdrv_* methods to bs->file by default

[Kevin Wolf]

Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
> The following functions fail if bs->drv does not implement them:
> 
> bdrv_probe_blocksizes
> bdrv_probe_geometry
> bdrv_truncate
> bdrv_has_zero_init
> bdrv_get_info
> bdrv_media_changed
> bdrv_eject
> bdrv_lock_medium
> bdrv_co_ioctl
> 
> Instead, the call should be passed to bs->file if it exists, to allow
> filter drivers to support those methods without implementing them.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c    | 27 +++++++++++++++++++++++++--
>  block/io.c |  5 +++++
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 69439628..8a122142 100644
> --- a/block.c
> +++ b/block.c
> @@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>  
>      if (drv && drv->bdrv_probe_blocksizes) {
>          return drv->bdrv_probe_blocksizes(bs, bsz);
> +    } else if (drv && bs->file && bs->file->bs) {

The check for bs->file->bs isn't really necessary, we never have a
BdrvChild with a NULL bs.

> +        return bdrv_probe_blocksizes(bs->file->bs, bsz);
>      }
>  
>      return -ENOTSUP;

This change actually changes existing behaviour. Basically, all of your
additions also apply for qcow2 images or other image formats, and we
need to check whether they make sense there.

bdrv_probe_blocksizes() is used for the default values for the physical
and logical block sizes for block devices if no value is explicitly
specified in -device. This makes sense as it will allow the guest to
optimise its requests so that they align with the block size that is
optimal for the storage that contains the image. (I also think that we
probably should implement .bdrv_probe_blocksizes() not only for
host_device, but also for regular files to get optimal performance by
default.)

Changing the defaults can be a problem for cross-version live migration
if the default values are used. Management tools that want to do live
migration are supposed to explicitly provide all options they can, so
this should be okay.

Eric, does libvirt set the physical/logical block size explicitly? If
not, it would be good if it started to do so.

This hunk is non-trivial enough that it's probably worth a patch of its
own with a detailed commit message explaining why we want this change
and why it is safe.

> @@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
>  
>      if (drv && drv->bdrv_probe_geometry) {
>          return drv->bdrv_probe_geometry(bs, geo);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        return bdrv_probe_geometry(bs->file->bs, geo);
>      }
>  
>      return -ENOTSUP;

The probed geometry would refer to the physical image file, not to the
actually presented image. So propagating this to bs->file is wrong for
image formats.

Possibly checking .is_filter in addition would be enough.

If you decide that this is the right field, please add some note to the
comment for the .is_filter declaration that tells about the assumptions
that are made when the field is set. I'm not completely sure what they
are, but maybe something like this: If bs->file != NULL, then all data
exposed by the filter node is mapped at the same offset in bs->file.

> @@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
>  
>      assert(child->perm & BLK_PERM_RESIZE);
>  
> +    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
>      if (!drv) {
>          error_setg(errp, "No medium inserted");
>          return -ENOMEDIUM;
>      }
>      if (!drv->bdrv_truncate) {
> +        if (bs->file && bs->file->bs) {
> +            return bdrv_truncate(bs->file, offset, errp);
> +        }
>          error_setg(errp, "Image format driver does not support resize");
>          return -ENOTSUP;
>      }

This is actively dangerous without an .is_filter check!

All image formats that don't support image resizing would instead just
truncate the image file without considering the format at all.

> @@ -3778,6 +3786,9 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>      if (bs->drv->bdrv_has_zero_init) {
>          return bs->drv->bdrv_has_zero_init(bs);
>      }
> +    if (bs->file && bs->file->bs) {
> +        return bdrv_has_zero_init(bs->file->bs);
> +    }
>  
>      /* safe default */
>      return 0;

This is wrong for image formats.

> @@ -3832,10 +3843,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
>  int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> +    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
> +    if (!drv) {
>          return -ENOMEDIUM;
> -    if (!drv->bdrv_get_info)
> +    }
> +    if (!drv->bdrv_get_info) {
> +        if (bs->file && bs->file->bs) {
> +            return bdrv_get_info(bs->file->bs, bdi);
> +        }
>          return -ENOTSUP;
> +    }
>      memset(bdi, 0, sizeof(*bdi));
>      return drv->bdrv_get_info(bs, bdi);
>  }

Same thing.

> @@ -4205,6 +4222,8 @@ int bdrv_media_changed(BlockDriverState *bs)
>  
>      if (drv && drv->bdrv_media_changed) {
>          return drv->bdrv_media_changed(bs);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        return bdrv_media_changed(bs->file->bs);
>      }
>      return -ENOTSUP;
>  }

This function appears to be unused, so instead of discussing whether
this actually makes sense, we can simply remove it. :-)

> @@ -4218,6 +4237,8 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag)
>  
>      if (drv && drv->bdrv_eject) {
>          drv->bdrv_eject(bs, eject_flag);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        bdrv_eject(bs->file->bs, eject_flag);
>      }
>  }
>  
> @@ -4233,6 +4254,8 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked)
>  
>      if (drv && drv->bdrv_lock_medium) {
>          drv->bdrv_lock_medium(bs, locked);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        bdrv_lock_medium(bs->file->bs, locked);
>      }
>  }

I'm not sure if media change can be handled transparently without
explicit support in all drivers in the chain. This is probably even true
for filter drivers.

So I think I would just drop these two hunks from the patch.

> diff --git a/block/io.c b/block/io.c
> index 5c146b5a..c22a9bf2 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2425,6 +2425,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
>      };
>      BlockAIOCB *acb;
>  
> +    if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl &&
> +        bs->file && bs->file->bs) {
> +        return bdrv_co_ioctl(bs->file->bs, req, buf);
> +    }
> +
>      bdrv_inc_in_flight(bs);
>      if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
>          co.ret = -ENOTSUP;

This makes me nervous. Basically, SCSI passthrough doesn't work well
with anything that isn't just a file-posix BDS. Bypassing block nodes
for it, whether image formats or filter drivers, might get these drivers
into surprising states and might even break the image.

I'd rather not support bdrv_co_ioctl() for anything that can't
explicitly handle it.

Kevin

Re: [Qemu-devel] [PATCH v3 2/4] block: Use defaults of bdrv_* callbacks in raw

[Kevin Wolf]

Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
> Now that passing the call to bs->file is the default for some bdrv_*
> callbacks, remove the duplicate implementations in block/raw-format.c
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

If you change patch 1 as suggested there, most of these functions will
have to stay.

The exception is .bdrv_has_zero_init, for which I suggested an
.is_filter check. However, raw isn't declared a filter currently. If we
want to declare it a filter, the definition I proposed in the other mail
doesn't work because raw can apply an offset - this why it needs its own
function for probing geometry, so with that changed definition checking
.is_filter might not be enough any more for all of the functions in
patch 1.

Maybe it turns out in the end that raw just is special enough that
having its own functions is okay.

Kevin

Re: [Qemu-devel] [PATCH v3 3/4] block: Remove bdrv_truncate callback in blkdebug

[Kevin Wolf]

Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
> Now that bdrv_truncate is passed to bs->file by default, remove the
> callback from block/blkdebug.c
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

We'll need to set .is_filter = true for blkdebug now. I haven't yet
looked into the existing implications that this has, but intuitively
this should be okay.

When you do so, please explain in the commit message the implications
that this has.

Kevin

Re: [Qemu-devel] [PATCH v3 4/4] block: Add default implementations for bdrv_co_get_block_status()

[Kevin Wolf]

Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
> bdrv_co_get_block_status_from_file() and
> bdrv_co_get_block_status_from_backing() set *file to bs->file and
> bs->backing respectively, so that bdrv_co_get_block_status() can recurse
> to them. Future block drivers won't have to duplicate code to implement
> this.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/4] block: Pass bdrv_* methods to bs->file by default

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

[adding libvirt for block size discussion]

On 07/04/2017 04:44 AM, Kevin Wolf wrote:
> Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
>> The following functions fail if bs->drv does not implement them:
>>
>> bdrv_probe_blocksizes
>> bdrv_probe_geometry
>> bdrv_truncate
>> bdrv_has_zero_init
>> bdrv_get_info
>> bdrv_media_changed
>> bdrv_eject
>> bdrv_lock_medium
>> bdrv_co_ioctl
>>
>> Instead, the call should be passed to bs->file if it exists, to allow
>> filter drivers to support those methods without implementing them.
>>

> 
>> +        return bdrv_probe_blocksizes(bs->file->bs, bsz);
>>      }
>>  
>>      return -ENOTSUP;
> 
> This change actually changes existing behaviour. Basically, all of your
> additions also apply for qcow2 images or other image formats, and we
> need to check whether they make sense there.
> 
> bdrv_probe_blocksizes() is used for the default values for the physical
> and logical block sizes for block devices if no value is explicitly
> specified in -device. This makes sense as it will allow the guest to
> optimise its requests so that they align with the block size that is
> optimal for the storage that contains the image. (I also think that we
> probably should implement .bdrv_probe_blocksizes() not only for
> host_device, but also for regular files to get optimal performance by
> default.)
> 
> Changing the defaults can be a problem for cross-version live migration
> if the default values are used. Management tools that want to do live
> migration are supposed to explicitly provide all options they can, so
> this should be okay.
> 
> Eric, does libvirt set the physical/logical block size explicitly? If
> not, it would be good if it started to do so.

Libvirt supports user control of block size via this XML in <disk>:
    <blockio logical_block_size='512' physical_block_size='4096'/>
which it translates to command-line logical_block_size= and
physical_block_size=.  But I didn't see anywhere that libvirt tries to
do anything with block size in QMP, which makes me wonder if that is a
libvirt bug for not allowing a block size when hot-plugging a drive, or
a weakness in QMP.

Your point that if libvirt isn't always specifying block size, then we
risk the block size changing on migration (that is, libvirt should be
patched to specify block size always, and not just when the XML
specifies it).


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/4] block: Pass bdrv_* methods to bs->file by default

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On 07/04/2017 04:44 AM, Kevin Wolf wrote:
> Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
>> The following functions fail if bs->drv does not implement them:
>>

>> @@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
>>  
>>      if (drv && drv->bdrv_probe_geometry) {
>>          return drv->bdrv_probe_geometry(bs, geo);
>> +    } else if (drv && bs->file && bs->file->bs) {
>> +        return bdrv_probe_geometry(bs->file->bs, geo);
>>      }
>>  
>>      return -ENOTSUP;
> 
> The probed geometry would refer to the physical image file, not to the
> actually presented image. So propagating this to bs->file is wrong for
> image formats.
> 
> Possibly checking .is_filter in addition would be enough.

In fact, this made me think: if we define .is_filter as an enum instead
of a bool, we could implement 0 for no filter, 1 for file filter, and 2
for backing filter.  Then instead of having to have 2 separate default
bdrv_get_block_status_for_*() generic functions in patch 4, we could
instead patch bdrv_co_get_block_status() to check .is_filter, and
automatically fall back to bs->file or bs->backing according to the enum
value stored in .is_filter.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/4] block: Pass bdrv_* methods to bs->file by default

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1740 bytes --]

Am 05.07.2017 um 14:43 hat Eric Blake geschrieben:
> On 07/04/2017 04:44 AM, Kevin Wolf wrote:
> > Am 04.07.2017 um 10:40 hat Manos Pitsidianakis geschrieben:
> >> The following functions fail if bs->drv does not implement them:
> >>
> 
> >> @@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
> >>  
> >>      if (drv && drv->bdrv_probe_geometry) {
> >>          return drv->bdrv_probe_geometry(bs, geo);
> >> +    } else if (drv && bs->file && bs->file->bs) {
> >> +        return bdrv_probe_geometry(bs->file->bs, geo);
> >>      }
> >>  
> >>      return -ENOTSUP;
> > 
> > The probed geometry would refer to the physical image file, not to the
> > actually presented image. So propagating this to bs->file is wrong for
> > image formats.
> > 
> > Possibly checking .is_filter in addition would be enough.
> 
> In fact, this made me think: if we define .is_filter as an enum instead
> of a bool, we could implement 0 for no filter, 1 for file filter, and 2
> for backing filter.  Then instead of having to have 2 separate default
> bdrv_get_block_status_for_*() generic functions in patch 4, we could
> instead patch bdrv_co_get_block_status() to check .is_filter, and
> automatically fall back to bs->file or bs->backing according to the enum
> value stored in .is_filter.

After reviewing the rest of the series, I'm not completely convinced of
using .is_filter any more. If you look at the patch for the raw format
driver, it actually turns out that even though we would intuitively call
it a filter, it has different properties than for example a throttle
filter and passing through function calls to bs->file makes sense for a
different set of functions.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]