Re: [Qemu-block] [Qemu-devel] [PATCH for 2.6 2/3] block: Hide HBitmap in block dirty bitmap interface

[Fam Zheng]

On Tue, 11/24 12:12, Vladimir Sementsov-Ogievskiy wrote:
> On 24.11.2015 05:28, Fam Zheng wrote:
> >On Mon, 11/23 16:34, John Snow wrote:
> >>Hmm, what's the idea, here?
> >>
> >>This patch does a lot more than just hide hbitmap details from callers
> >>of block_dirty_bitmap functions.
> >>
> >>So we're changing the backing hbitmap to always be one where g=0 and the
> >>number of physical bits directly is (now) the same as the number of
> >>'virtual' bits, pre-patch. Then, to compensate, we handle the shift math
> >>to convert the bitmap granularity to sector size and vice-versa in the
> >>Block Dirty Bitmap layer instead of in the hbitmap layer.
> >>
> >>What's the benefit? It looks like we just pull all the implementation
> >>details up from hbitmap and into BdrvDirtyBitmap, which I am not
> >>immediately convinced of as being a benefit.
> >It feels counter intuitive to me with hbitmap handling granularity, it makes 
> >it
> >more like a HGranularityBitmap rather than HBitmap, and is unnecessarily
> >complex to work on.
> >
> >Now it's simplified in that only one BdrvDirtyBitmap needs to care about the
> >granularity, and which I think is a big benefit when we are going to extend 
> >the
> >dirty bitmap interface, for example to serialize and deserialize for
> >persistence.
> >
> >Fam
> 
> But what is the relation from this to adding iterator interface?
> This seems like this patch do two different things:
> 1) granularity changes, described in quotation above
> 2) adding iterator related things, described in commit message
> 

OK, let's try to split this so we still have the BdrvDirtyBitmapIter change
even if the granularity movement is rejected.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.6 2/3] block: Hide HBitmap in block dirty bitmap interface

[John Snow]

On 11/23/2015 09:28 PM, Fam Zheng wrote:
> On Mon, 11/23 16:34, John Snow wrote:
>> Hmm, what's the idea, here?
>>
>> This patch does a lot more than just hide hbitmap details from callers
>> of block_dirty_bitmap functions.
>>
>> So we're changing the backing hbitmap to always be one where g=0 and the
>> number of physical bits directly is (now) the same as the number of
>> 'virtual' bits, pre-patch. Then, to compensate, we handle the shift math
>> to convert the bitmap granularity to sector size and vice-versa in the
>> Block Dirty Bitmap layer instead of in the hbitmap layer.
>>
>> What's the benefit? It looks like we just pull all the implementation
>> details up from hbitmap and into BdrvDirtyBitmap, which I am not
>> immediately convinced of as being a benefit.
> 
> It feels counter intuitive to me with hbitmap handling granularity, it makes 
> it
> more like a HGranularityBitmap rather than HBitmap, and is unnecessarily
> complex to work on.
> 

I guess it's a matter of personal taste on where to try to hide the
complexity. Since hbitmap can and already does manage it for us, inertia
leaves me satisfied with this option.

I don't know if pulling the granularity out of hbitmap and into
BdrvDirtyBitmap (so now we have granularity management code in two
objects) is a significant gain, but I wouldn't NACK this over that
specifically ... I'll let Vladimir et al decide if this does make e.g.
his migration/persistence patches easier to write or not.

I agree that the new iterator object for BdrvDirtyBitmap is good,
though, and wouldn't mind seeing this split up into its two parts:

(1) Hiding the hbitmap implementation detail entirely from users of
BdrvDirtyBitmap, by adding new BdrvDirtyBitmap iterators

(2) Moving the granularity logic up into BdrvDirtyBitmap

> Now it's simplified in that only one BdrvDirtyBitmap needs to care about the
> granularity, and which I think is a big benefit when we are going to extend 
> the
> dirty bitmap interface, for example to serialize and deserialize for
> persistence.
> 
> Fam
> 

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.6 2/3] block: Hide HBitmap in block dirty bitmap interface

[Vladimir Sementsov-Ogievskiy]

On 24.11.2015 05:28, Fam Zheng wrote:

On Mon, 11/23 16:34, John Snow wrote:

Hmm, what's the idea, here?

This patch does a lot more than just hide hbitmap details from callers
of block_dirty_bitmap functions.

So we're changing the backing hbitmap to always be one where g=0 and the
number of physical bits directly is (now) the same as the number of
'virtual' bits, pre-patch. Then, to compensate, we handle the shift math
to convert the bitmap granularity to sector size and vice-versa in the
Block Dirty Bitmap layer instead of in the hbitmap layer.

What's the benefit? It looks like we just pull all the implementation
details up from hbitmap and into BdrvDirtyBitmap, which I am not
immediately convinced of as being a benefit.

It feels counter intuitive to me with hbitmap handling granularity, it makes it
more like a HGranularityBitmap rather than HBitmap, and is unnecessarily
complex to work on.

Now it's simplified in that only one BdrvDirtyBitmap needs to care about the
granularity, and which I think is a big benefit when we are going to extend the
dirty bitmap interface, for example to serialize and deserialize for
persistence.

Fam


But what is the relation from this to adding iterator interface? This 
seems like this patch do two different things:

1) granularity changes, described in quotation above
2) adding iterator related things, described in commit message

--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.6 2/3] block: Hide HBitmap in block dirty bitmap interface

[Fam Zheng]

On Mon, 11/23 16:34, John Snow wrote:
> Hmm, what's the idea, here?
> 
> This patch does a lot more than just hide hbitmap details from callers
> of block_dirty_bitmap functions.
> 
> So we're changing the backing hbitmap to always be one where g=0 and the
> number of physical bits directly is (now) the same as the number of
> 'virtual' bits, pre-patch. Then, to compensate, we handle the shift math
> to convert the bitmap granularity to sector size and vice-versa in the
> Block Dirty Bitmap layer instead of in the hbitmap layer.
> 
> What's the benefit? It looks like we just pull all the implementation
> details up from hbitmap and into BdrvDirtyBitmap, which I am not
> immediately convinced of as being a benefit.

It feels counter intuitive to me with hbitmap handling granularity, it makes it
more like a HGranularityBitmap rather than HBitmap, and is unnecessarily
complex to work on.

Now it's simplified in that only one BdrvDirtyBitmap needs to care about the
granularity, and which I think is a big benefit when we are going to extend the
dirty bitmap interface, for example to serialize and deserialize for
persistence.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH for 2.6 2/3] block: Hide HBitmap in block dirty bitmap interface

[John Snow]

On 11/20/2015 04:59 AM, Fam Zheng wrote:
> HBitmap is an implementation detail of block dirty bitmap that should be 
> hidden
> from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlying
> HBitmapIter.
> 
> A small difference in the interface is, before, an HBitmapIter is initialized
> in place, now the new BdrvDirtyBitmapIter must be dynamically allocated 
> because
> the structure definition is in block.c.
> 
> Two current users are converted too.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block.c   | 79 
> ++-
>  block/backup.c| 14 +
>  block/mirror.c| 14 +
>  include/block/block.h |  9 --
>  4 files changed, 82 insertions(+), 34 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3a7324b..e225050 100644
> --- a/block.c
> +++ b/block.c
> @@ -63,14 +63,22 @@
>   * or enabled. A frozen bitmap can only abdicate() or reclaim().
>   */
>  struct BdrvDirtyBitmap {
> +int gran_shift; /* Bits to right shift from sector number to
> +   bit index. */
>  HBitmap *bitmap;/* Dirty sector bitmap implementation */
>  BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>  char *name; /* Optional non-empty unique ID */
>  int64_t size;   /* Size of the bitmap (Number of sectors) */
>  bool disabled;  /* Bitmap is read-only */
> +int active_iterators;   /* How many iterators are active */
>  QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
>  
> +struct BdrvDirtyBitmapIter {
> +HBitmapIter hbi;
> +BdrvDirtyBitmap *bitmap;
> +};
> +
>  #define NOT_DONE 0x7fff /* used while emulated sync operation in 
> progress */
>  
>  struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
> @@ -3157,24 +3165,26 @@ BdrvDirtyBitmap 
> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>  {
>  int64_t bitmap_size;
>  BdrvDirtyBitmap *bitmap;
> -uint32_t sector_granularity;
> +int gran_shift;
>  
>  assert((granularity & (granularity - 1)) == 0);
> +/* Caller should check that */
> +assert(granularity >= BDRV_SECTOR_SIZE);
>  
> +gran_shift = ctz32(granularity) - BDRV_SECTOR_BITS;
>  if (name && bdrv_find_dirty_bitmap(bs, name)) {
>  error_setg(errp, "Bitmap already exists: %s", name);
>  return NULL;
>  }
> -sector_granularity = granularity >> BDRV_SECTOR_BITS;
> -assert(sector_granularity);
> -bitmap_size = bdrv_nb_sectors(bs);
> +bitmap_size = DIV_ROUND_UP(bdrv_getlength(bs), granularity);
>  if (bitmap_size < 0) {
>  error_setg_errno(errp, -bitmap_size, "could not get length of 
> device");
>  errno = -bitmap_size;
>  return NULL;
>  }
>  bitmap = g_new0(BdrvDirtyBitmap, 1);
> -bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
> +bitmap->bitmap = hbitmap_alloc(bitmap_size, 0);

Hmm, what's the idea, here?

This patch does a lot more than just hide hbitmap details from callers
of block_dirty_bitmap functions.

So we're changing the backing hbitmap to always be one where g=0 and the
number of physical bits directly is (now) the same as the number of
'virtual' bits, pre-patch. Then, to compensate, we handle the shift math
to convert the bitmap granularity to sector size and vice-versa in the
Block Dirty Bitmap layer instead of in the hbitmap layer.

What's the benefit? It looks like we just pull all the implementation
details up from hbitmap and into BdrvDirtyBitmap, which I am not
immediately convinced of as being a benefit.

> +bitmap->gran_shift = gran_shift;
>  bitmap->size = bitmap_size;
>  bitmap->name = g_strdup(name);
>  bitmap->disabled = false;
> @@ -3293,9 +3303,10 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>  {
>  BdrvDirtyBitmap *bitmap;
> -uint64_t size = bdrv_nb_sectors(bs);
>  
>  QLIST_FOREACH(bitmap, >dirty_bitmaps, list) {
> +int64_t size = bdrv_nb_sectors(bs) >> bitmap->gran_shift;
> +/* TODO: what if size < 0? */
>  assert(!bdrv_dirty_bitmap_frozen(bitmap));
>  hbitmap_truncate(bitmap->bitmap, size);
>  bitmap->size = size;
> @@ -3307,6 +3318,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
> BdrvDirtyBitmap *bitmap)
>  BdrvDirtyBitmap *bm, *next;
>  QLIST_FOREACH_SAFE(bm, >dirty_bitmaps, list, next) {
>  if (bm == bitmap) {
> +assert(!bitmap->active_iterators);
>  assert(!bdrv_dirty_bitmap_frozen(bm));
>  QLIST_REMOVE(bitmap, list);
>  hbitmap_free(bitmap->bitmap);
> @@ -3354,7 +3366,7 @@ BlockDirtyInfoList 
> *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>  int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
> sector)
>  {
>