qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitm


From: Eric Blake
Subject: Re: [PATCH v3 3/9] block: Make it easier to learn which BDS support bitmaps
Date: Mon, 11 May 2020 13:16:16 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0

On 5/11/20 4:21 AM, Max Reitz wrote:
On 08.05.20 20:03, Eric Blake wrote:
Upcoming patches will enhance bitmap support in qemu-img, but in doing
so, it turns out to be nice to suppress output when bitmaps make no
sense (such as on a qcow2 v2 image).  Add a hook to make this easier
to query.

In the future, when we improve the ability to look up bitmaps through
a filter, we will probably also want to teach the block layer to
automatically let filters pass this request on through.

Signed-off-by: Eric Blake <address@hidden>
---
  block/qcow2.h                | 1 +
  include/block/block_int.h    | 1 +
  include/block/dirty-bitmap.h | 1 +
  block/dirty-bitmap.c         | 9 +++++++++
  block/qcow2-bitmap.c         | 7 +++++++
  block/qcow2.c                | 1 +
  6 files changed, 20 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index f4de0a27d5c3..fb2b2b5a7b4d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -764,6 +764,7 @@ bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState 
*bs,
  int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                              const char *name,
                                              Error **errp);
+bool qcow2_dirty_bitmap_supported(BlockDriverState *bs);

  ssize_t coroutine_fn
  qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index df6d0273d679..cb1082da4c43 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -560,6 +560,7 @@ struct BlockDriver {
                               uint64_t parent_perm, uint64_t parent_shared,
                               uint64_t *nperm, uint64_t *nshared);

+    bool (*bdrv_dirty_bitmap_supported)(BlockDriverState *bs);

All BDSs support bitmaps, but only some support persistent dirty
bitmaps, so I think the name should reflect that.

How about .bdrv_dirty_bitmap_supports_persistent?


Conceptually, this looks reasonable.  This information might indeed be
nice to have, and I’m not sure whether we should extend any existing
interface to return it.

(The interfaces that come to my mind are:
(1) bdrv_can_store_new_dirty_bitmap() below, which we could make accept
a NULL @name to return basically the same information.  But it’s still a
bit different, because I’d expect that function to return whether any
bitmap can be stored then, not whether the node supports bitmaps at all.
  So e.g. if there are already too many bitmaps, it should return false,
even though the node itself does support bitmaps.

[which reminds me - a while ago, we had patches for qcow2 handling with 64k bitmaps, or whatever insane number it took to overflow data structures, and I don't know if those ever got applied...]


(2) bdrv_get_info()/BlockDriverInfo: This information would fit in very
nicely here, but do we have to put it here just because it does?  I
don’t think so.  This patch adds 20 lines of code, that shows that it’s
very simple to add a dedicated method, and it’s certainly a bit easier
to use than to invoke bdrv_get_info() and throw away all the other
information.  Perhaps this patch only shows that BlockDriverInfo doesn’t
make much sense in the first place, and most of its fields should have
been scalar return values from dedicated functions.)

Indeed, you (re-)discovered some of the very reasons why I chose to make a new interface. I could tweak the commit message to mention alternatives, if that would help.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]