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

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

[Manos Pitsidianakis]

This series makes implementing some of the bdrv_* callbacks easier for block
filters 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

v4:
  forward only for block filters
  new patch: remove bdrv_media_changed
  dropped commit `block: Use defaults of bdrv_* callbacks in raw`, since raw is
  not a filter driver and is incompatible with the changes.

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 in block filters
  block: remove bdrv_media_changed
  block: remove bdrv_truncate callback in blkdebug
  block: add default implementations for bdrv_co_get_block_status()

 block.c                   | 35 +++++++++++++++++++----------------
 block/blkdebug.c          | 19 ++-----------------
 block/commit.c            | 12 +-----------
 block/io.c                | 26 ++++++++++++++++++++++++++
 block/mirror.c            | 12 +-----------
 block/raw-format.c        |  6 ------
 include/block/block.h     |  1 -
 include/block/block_int.h | 25 +++++++++++++++++++++++--
 8 files changed, 72 insertions(+), 64 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v4 1/4] block: pass bdrv_* methods to bs->file by default in block filters

[Manos Pitsidianakis]

The following functions fail if bs->drv is a filter and does not
implement them:

bdrv_probe_blocksizes
bdrv_probe_geometry
bdrv_truncate
bdrv_has_zero_init
bdrv_get_info

Instead, the call should be passed to bs->file if it exists, to allow
filter drivers to support those methods without implementing them. This
commit makes `drv->is_filter = true` imply that these callbacks will be
forwarded to bs->file by default, so disabling support for these
functions must be done explicitly.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c                   | 21 +++++++++++++++++++--
 include/block/block_int.h |  6 +++++-
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 69439628..7f7530b6 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 && drv->is_filter && bs->file) {
+        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 && drv->is_filter && bs->file) {
+        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 && drv->is_filter) {
+            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->drv->is_filter) {
+        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 && drv->is_filter) {
+            return bdrv_get_info(bs->file->bs, bdi);
+        }
         return -ENOTSUP;
+    }
     memset(bdi, 0, sizeof(*bdi));
     return drv->bdrv_get_info(bs, bdi);
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 15fa6021..75e93f72 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -87,7 +87,11 @@ struct BlockDriver {
     const char *format_name;
     int instance_size;
 
-    /* set to true if the BlockDriver is a block filter */
+    /* set to true if the BlockDriver is a block filter. Block filters pass
+     * certain callbacks that refer to data (see block.c) to their bs->file if
+     * the driver doesn't implement them. Drivers that do not wish to forward
+     * must implement them and return -ENOTSUP.
+     */
     bool is_filter;
     /* for snapshots block filter like Quorum can implement the
      * following recursive callback.
-- 
2.11.0

[Qemu-devel] [PATCH v4 2/4] block: remove bdrv_media_changed

[Manos Pitsidianakis]

This function is not used anywhere, so remove it.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c                   | 14 --------------
 block/raw-format.c        |  6 ------
 include/block/block.h     |  1 -
 include/block/block_int.h |  1 -
 4 files changed, 22 deletions(-)

diff --git a/block.c b/block.c
index 7f7530b6..5ca60f97 100644
--- a/block.c
+++ b/block.c
@@ -4213,20 +4213,6 @@ bool bdrv_is_inserted(BlockDriverState *bs)
 }
 
 /**
- * Return whether the media changed since the last call to this
- * function, or -ENOTSUP if we don't know.  Most drivers don't know.
- */
-int bdrv_media_changed(BlockDriverState *bs)
-{
-    BlockDriver *drv = bs->drv;
-
-    if (drv && drv->bdrv_media_changed) {
-        return drv->bdrv_media_changed(bs);
-    }
-    return -ENOTSUP;
-}
-
-/**
  * If eject_flag is TRUE, eject the media. Otherwise, close the tray
  */
 void bdrv_eject(BlockDriverState *bs, bool eject_flag)
diff --git a/block/raw-format.c b/block/raw-format.c
index 1ea8c2d7..df3bc5d5 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -346,11 +346,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);
@@ -483,7 +478,6 @@ BlockDriver bdrv_raw = {
     .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,
diff --git a/include/block/block.h b/include/block/block.h
index afe1b615..3cb4cae7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,7 +438,6 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
-int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 75e93f72..3d9a8164 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -246,7 +246,6 @@ struct BlockDriver {
 
     /* removable device specific */
     bool (*bdrv_is_inserted)(BlockDriverState *bs);
-    int (*bdrv_media_changed)(BlockDriverState *bs);
     void (*bdrv_eject)(BlockDriverState *bs, bool eject_flag);
     void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
 
-- 
2.11.0

[Qemu-devel] [PATCH v4 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 and set is_filter to true.

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b25856c4..91ffd1fe 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;
@@ -908,6 +903,7 @@ static BlockDriver bdrv_blkdebug = {
     .format_name            = "blkdebug",
     .protocol_name          = "blkdebug",
     .instance_size          = sizeof(BDRVBlkdebugState),
+    .is_filter              = true,
 
     .bdrv_parse_filename    = blkdebug_parse_filename,
     .bdrv_file_open         = blkdebug_open,
@@ -916,7 +912,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 v4 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>
Reviewed-by: Kevin Wolf <kwolf@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 91ffd1fe..775c464b 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;
@@ -920,7 +910,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 5c146b5a..1a5bb823 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 3d9a8164..3bf8bb33 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -948,6 +948,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 v4 1/4] block: pass bdrv_* methods to bs->file by default in block filters

[Eric Blake]

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

On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
> The following functions fail if bs->drv is a filter and does not
> implement them:
> 
> bdrv_probe_blocksizes
> bdrv_probe_geometry
> bdrv_truncate
> bdrv_has_zero_init
> bdrv_get_info
> 
> Instead, the call should be passed to bs->file if it exists, to allow
> filter drivers to support those methods without implementing them. This
> commit makes `drv->is_filter = true` imply that these callbacks will be
> forwarded to bs->file by default, so disabling support for these
> functions must be done explicitly.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                   | 21 +++++++++++++++++++--
>  include/block/block_int.h |  6 +++++-
>  2 files changed, 24 insertions(+), 3 deletions(-)
> @@ -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->drv->is_filter) {
> +        return bdrv_has_zero_init(bs->file->bs);
> +    }
>  
>      /* safe default */
>      return 0;

Someday, we should probably clean this function (and all callback
implementations) to return bool rather than int.  But not this patch.

Certainly more conservative than the earlier versions.  I'd still trust
Kevin's review over mine, but looks okay to me.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v4 2/4] block: remove bdrv_media_changed

[Eric Blake]

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

On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
> This function is not used anywhere, so remove it.
> 

Might be interesting to figure out when it WAS last used.  If I grepped
correctly, it was commit 21fcf360 back in May 2012?

> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                   | 14 --------------
>  block/raw-format.c        |  6 ------
>  include/block/block.h     |  1 -
>  include/block/block_int.h |  1 -
>  4 files changed, 22 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v4 3/4] block: remove bdrv_truncate callback in blkdebug

[Eric Blake]

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

On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
> Now that bdrv_truncate is passed to bs->file by default, remove the
> callback from block/blkdebug.c and set is_filter to true.

This also automatically gives blkdebug access to

bdrv_probe_blocksizes
bdrv_probe_geometry
bdrv_has_zero_init
bdrv_get_info

all of which make sense to me.

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

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v4 2/4] block: remove bdrv_media_changed

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
>> This function is not used anywhere, so remove it.
>> 
>
> Might be interesting to figure out when it WAS last used.

Yes.  When I see "remove X because it's unused" during patch review, I
immediately ask "why is it unused now, and what was it used for
previously?"  Ideally, the commit message answers these questions
preemptively.

>                                                            If I grepped
> correctly, it was commit 21fcf360 back in May 2012?

Yes.  "fdc: simplify media change handling".  I suspect that commit
broke media change for passed-through host floppy.

Its only implementation went away in commit f709623 "block: Remove host
floppy support".

Suggest

    block: bdrv_media_changed() is unused, remove

    The i82078 floppy device model used to call bdrv_media_changed() to
    implement its media change bit when backed by a host floppy.  This
    went away in 21fcf36 "fdc: simplify media change handling".
    Probably broke host floppy media change.  Host floppy pass-through
    was dropped in commit f709623.  bdrv_media_changed() has never been
    used for anything else.  Remove it.

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

[Markus Armbruster]

Manos Pitsidianakis <el13635@mail.ntua.gr> writes:

> This series makes implementing some of the bdrv_* callbacks easier for block
> filters 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

Haven't seen BlockDriver member is_filter before.  Interesting.  It's
documentation

    /* set to true if the BlockDriver is a block filter */
    bool is_filter;

is seriously lacking.  What does it *mean* to be a block filter?  Which
block layer facilities are affected, and how?

Observation: driver "raw" is filter-like in the sense that all it does
is pass along method arguments and results.  Can't say whether that
makes it a filter in the sense of is_filter, because "the sense of
is_filter" is nebulous to me :)

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

[Manos Pitsidianakis]

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

On Wed, Jul 12, 2017 at 09:49:20AM +0200, Markus Armbruster wrote:
>Manos Pitsidianakis <el13635@mail.ntua.gr> writes:
>
>> This series makes implementing some of the bdrv_* callbacks easier for block
>> filters 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
>
>Haven't seen BlockDriver member is_filter before.  Interesting.  It's
>documentation
>
>    /* set to true if the BlockDriver is a block filter */
>    bool is_filter;
>
>is seriously lacking.  What does it *mean* to be a block filter?  Which
>block layer facilities are affected, and how?

Currently it is only used in bdrv_recurse_is_first_non_filter. 

>Observation: driver "raw" is filter-like in the sense that all it does
>is pass along method arguments and results.  Can't say whether that
>makes it a filter in the sense of is_filter, because "the sense of
>is_filter" is nebulous to me :)

I'm not very acquainted with raw, so I can't really comment. But the 
drivers I'm working on, throttle and before write notifier have that 
exact semantic, ie they do something when IO is intercepted and pass 
everything to the BDSes below. There was a mini discussion about raw and 
filters in the previous version's thread.

I might add documentation to block_int.h in the future. When I first 
read it it felt nebulous to me too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v4 1/4] block: pass bdrv_* methods to bs->file by default in block filters

[Stefan Hajnoczi]

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

On Tue, Jul 11, 2017 at 07:37:45PM +0300, Manos Pitsidianakis wrote:
> The following functions fail if bs->drv is a filter and does not
> implement them:
> 
> bdrv_probe_blocksizes
> bdrv_probe_geometry
> bdrv_truncate
> bdrv_has_zero_init
> bdrv_get_info
> 
> Instead, the call should be passed to bs->file if it exists, to allow
> filter drivers to support those methods without implementing them. This
> commit makes `drv->is_filter = true` imply that these callbacks will be
> forwarded to bs->file by default, so disabling support for these
> functions must be done explicitly.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                   | 21 +++++++++++++++++++--
>  include/block/block_int.h |  6 +++++-
>  2 files changed, 24 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 2/4] block: remove bdrv_media_changed

[Stefan Hajnoczi]

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

On Tue, Jul 11, 2017 at 07:37:46PM +0300, Manos Pitsidianakis wrote:
> This function is not used anywhere, so remove it.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                   | 14 --------------
>  block/raw-format.c        |  6 ------
>  include/block/block.h     |  1 -
>  include/block/block_int.h |  1 -
>  4 files changed, 22 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Stefan Hajnoczi]

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

On Tue, Jul 11, 2017 at 07:37:47PM +0300, Manos Pitsidianakis wrote:
> Now that bdrv_truncate is passed to bs->file by default, remove the
> callback from block/blkdebug.c and set is_filter to true.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block/blkdebug.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Stefan Hajnoczi]

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

On Tue, Jul 11, 2017 at 07:37:48PM +0300, Manos Pitsidianakis wrote:
> 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>
> Reviewed-by: Kevin Wolf <kwolf@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(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v4 2/4] block: remove bdrv_media_changed

[Kevin Wolf]

Am 12.07.2017 um 09:41 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
> >> This function is not used anywhere, so remove it.
> >> 
> >
> > Might be interesting to figure out when it WAS last used.
> 
> Yes.  When I see "remove X because it's unused" during patch review, I
> immediately ask "why is it unused now, and what was it used for
> previously?"  Ideally, the commit message answers these questions
> preemptively.
> 
> >                                                            If I grepped
> > correctly, it was commit 21fcf360 back in May 2012?
> 
> Yes.  "fdc: simplify media change handling".  I suspect that commit
> broke media change for passed-through host floppy.
> 
> Its only implementation went away in commit f709623 "block: Remove host
> floppy support".
> 
> Suggest
> 
>     block: bdrv_media_changed() is unused, remove
> 
>     The i82078 floppy device model used to call bdrv_media_changed() to
>     implement its media change bit when backed by a host floppy.  This
>     went away in 21fcf36 "fdc: simplify media change handling".
>     Probably broke host floppy media change.  Host floppy pass-through
>     was dropped in commit f709623.  bdrv_media_changed() has never been
>     used for anything else.  Remove it.

Manos, if you're happy with this, I can update the commit message while
applying the series.

Kevin

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

[Kevin Wolf]

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

Am 12.07.2017 um 10:10 hat Manos Pitsidianakis geschrieben:
> On Wed, Jul 12, 2017 at 09:49:20AM +0200, Markus Armbruster wrote:
> >Manos Pitsidianakis <el13635@mail.ntua.gr> writes:
> >
> >>This series makes implementing some of the bdrv_* callbacks easier for block
> >>filters 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
> >
> >Haven't seen BlockDriver member is_filter before.  Interesting.  It's
> >documentation
> >
> >   /* set to true if the BlockDriver is a block filter */
> >   bool is_filter;
> >
> >is seriously lacking.  What does it *mean* to be a block filter?  Which
> >block layer facilities are affected, and how?
> 
> Currently it is only used in bdrv_recurse_is_first_non_filter.
> 
> >Observation: driver "raw" is filter-like in the sense that all it does
> >is pass along method arguments and results.  Can't say whether that
> >makes it a filter in the sense of is_filter, because "the sense of
> >is_filter" is nebulous to me :)
> 
> I'm not very acquainted with raw, so I can't really comment. But the
> drivers I'm working on, throttle and before write notifier have that
> exact semantic, ie they do something when IO is intercepted and pass
> everything to the BDSes below. There was a mini discussion about raw
> and filters in the previous version's thread.
> 
> I might add documentation to block_int.h in the future. When I first
> read it it felt nebulous to me too.

Not something that should stop this series, but while you're adding some
implications of being a filter to block_int.h, we still don't have a
definition of which block drivers qualify as filters.

"do something when IO is intercepted and pass everything to the BDSes
below" is probably not enough, every driver does this unless it is a
protocol driver.

We could further qualify it such that reads/writes on filter always
result in reads/writes on the same offset in bs->file (this disqualifies
the raw format, which you seem to want) and the block status is always
taken from bs->file.

We could also add that the data read/written must be the same - it's not
quite clear to me yet if we want to require this or can make use of the
property.

I'm open for other suggestion of what makes a filter a filter in the
sense of this field. Ideally it would contain enough (or maybe better:
the right) conditions that you can use it to justify why for each of the
functions that you pass down by default, this is the right behaviour.

For example, passing through bdrv_probe_geometry() makes sense because
all I/O is passed through unmodified and the image size is the same,
etc.

Kevin

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

Re: [Qemu-devel] [PATCH v4 2/4] block: remove bdrv_media_changed

[Manos Pitsidianakis]

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

On Wed, Jul 12, 2017 at 05:15:01PM +0200, Kevin Wolf wrote:
>Am 12.07.2017 um 09:41 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>>
>> > On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
>> >> This function is not used anywhere, so remove it.
>> >>
>> >
>> > Might be interesting to figure out when it WAS last used.
>>
>> Yes.  When I see "remove X because it's unused" during patch review, I
>> immediately ask "why is it unused now, and what was it used for
>> previously?"  Ideally, the commit message answers these questions
>> preemptively.
>>
>> >                                                            If I grepped
>> > correctly, it was commit 21fcf360 back in May 2012?
>>
>> Yes.  "fdc: simplify media change handling".  I suspect that commit
>> broke media change for passed-through host floppy.
>>
>> Its only implementation went away in commit f709623 "block: Remove host
>> floppy support".
>>
>> Suggest
>>
>>     block: bdrv_media_changed() is unused, remove
>>
>>     The i82078 floppy device model used to call bdrv_media_changed() to
>>     implement its media change bit when backed by a host floppy.  This
>>     went away in 21fcf36 "fdc: simplify media change handling".
>>     Probably broke host floppy media change.  Host floppy pass-through
>>     was dropped in commit f709623.  bdrv_media_changed() has never been
>>     used for anything else.  Remove it.
>
>Manos, if you're happy with this, I can update the commit message while
>applying the series.

Of course, no problem.
Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v4 2/4] block: remove bdrv_media_changed

[Eric Blake]

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

On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
> This function is not used anywhere, so remove it.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                   | 14 --------------
>  block/raw-format.c        |  6 ------
>  include/block/block.h     |  1 -
>  include/block/block_int.h |  1 -
>  4 files changed, 22 deletions(-)
> 

> +++ b/block/raw-format.c
> @@ -346,11 +346,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);
> -}
> -

There's a merge conflict here if this lands after Max's pending block
pull request, but should be trivial to resolve.

-- 
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 v4 1/4] block: pass bdrv_* methods to bs->file by default in block filters

[Eric Blake]

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

On 07/11/2017 11:37 AM, Manos Pitsidianakis wrote:
> The following functions fail if bs->drv is a filter and does not
> implement them:
> 
> bdrv_probe_blocksizes
> bdrv_probe_geometry
> bdrv_truncate
> bdrv_has_zero_init
> bdrv_get_info
> 
> Instead, the call should be passed to bs->file if it exists, to allow
> filter drivers to support those methods without implementing them. This
> commit makes `drv->is_filter = true` imply that these callbacks will be
> forwarded to bs->file by default, so disabling support for these
> functions must be done explicitly.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                   | 21 +++++++++++++++++++--
>  include/block/block_int.h |  6 +++++-
>  2 files changed, 24 insertions(+), 3 deletions(-)

> @@ -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 && drv->is_filter) {
> +            return bdrv_truncate(bs->file, offset, errp);
> +        }

This has a semantic (but not merge) conflict with Max's preallocation
work, and it started to be non-trivial for me to adjust locally in my
testing. It may be easiest if you send a v5 on top of
'https://github.com/XanClic/qemu.git block', since Max already has a
pending pull request:

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg02841.html

-- 
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 --]