[Qemu-devel] [PATCH 00/12] make dirty-bitmap byte-based

[Qemu-devel] [PATCH 00/12] make dirty-bitmap byte-based

[Eric Blake]

There are patches floating around to add NBD_CMD_BLOCK_STATUS,
but NBD wants to report status on byte granularity (even if the
reporting will probably be naturally aligned to sectors or even
much higher levels).  I've therefore started the task of
converting our block status code to report at a byte granularity
rather than sectors.

This is part two of that conversion: dirty-bitmap. Other parts
include bdrv_is_allocated (previously submitted) and replacing
bdrv_get_block_status with a byte based callback in all the
drivers (still being written).

Available as a tag at:
git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-dirty-v1

It requires the following (v1 of bdrv_is_allocated, v9 of
blkdebug, and Max's block-next tree):
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01995.html
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01723.html
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01298.html

Eric Blake (12):
  dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented
  migration: Don't lose errno across aio context changes
  dirty-bitmap: Drop unused functions
  dirty-bitmap: Track size in bytes
  dirty-bitmap: Set iterator start by offset, not sector
  dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset
  dirty-bitmap: Change bdrv_get_dirty_count() to report bytes
  dirty-bitmap: Change bdrv_get_dirty() to take bytes
  dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes
  mirror: Switch mirror_dirty_init() to byte-based iteration
  dirty-bitmap: Switch bdrv_set_dirty() to bytes
  dirty-bitmap: Convert internal hbitmap size/granularity

 include/block/block_int.h    |  2 +-
 include/block/dirty-bitmap.h | 21 ++++-------
 block/backup.c               |  7 ++--
 block/dirty-bitmap.c         | 83 ++++++++++++--------------------------------
 block/io.c                   |  6 ++--
 block/mirror.c               | 73 +++++++++++++++++---------------------
 migration/block.c            | 14 ++++----
 7 files changed, 74 insertions(+), 132 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 01/12] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[Eric Blake]

We've been documenting the value in bytes since its introduction
in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.

Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
preparation for a rewrite to a list of dirty sectors in the next
commit 21b5683 in block.c, but the new code mistakenly started
reporting in sectors.

Fixes: https://bugzilla.redhat.com/1441460

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>

---
Too late for 2.9, since the regression has been unnoticed for
nine releases. But worth putting in 2.9.1.
---
 block/dirty-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 519737c..6d8ce5f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -345,7 +345,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
     QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
         BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
         BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-        info->count = bdrv_get_dirty_count(bm);
+        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
         info->granularity = bdrv_dirty_bitmap_granularity(bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
-- 
2.9.3

Re: [Qemu-devel] [PATCH 01/12] dirty-bitmap: Report BlockDirtyInfo.count in bytes, as documented

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> We've been documenting the value in bytes since its introduction
> in commit b9a9b3a4 (v1.3), where it was actually reported in bytes.
> 
> Commit e4654d2 (v2.0) then removed things from block/qapi.c, in
> preparation for a rewrite to a list of dirty sectors in the next
> commit 21b5683 in block.c, but the new code mistakenly started
> reporting in sectors.
> 
> Fixes: https://bugzilla.redhat.com/1441460
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> Too late for 2.9, since the regression has been unnoticed for
> nine releases. But worth putting in 2.9.1.
> ---

Since before I started working here :)

I even documented the wrong thing in my two talks on the matter, but I
suppose I never committed it to bitmaps.md, so...

I guess this is technically correct?

http://i3.kym-cdn.com/photos/images/facebook/000/909/991/48c.jpg

>  block/dirty-bitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 519737c..6d8ce5f 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -345,7 +345,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>      QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
>          BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
>          BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
> -        info->count = bdrv_get_dirty_count(bm);
> +        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
>          info->granularity = bdrv_dirty_bitmap_granularity(bm);
>          info->has_name = !!bm->name;
>          info->name = g_strdup(bm->name);
> 

This is strictly more useful than sectors anyway, so ...

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 02/12] migration: Don't lose errno across aio context changes

[Eric Blake]

set_drity_tracking() was assuming that the errno value set by
bdrv_create_dirty_bitmap() would not be corrupted by either
blk_get_aio_context() or aio_context_release().  Rather than
audit whether this assumption is safe, rewrite the code to just
grab the value of errno sooner.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 migration/block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/block.c b/migration/block.c
index 18d50ff..9a9c214 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -350,9 +350,9 @@ static int set_dirty_tracking(void)
         aio_context_acquire(blk_get_aio_context(bmds->blk));
         bmds->dirty_bitmap = bdrv_create_dirty_bitmap(blk_bs(bmds->blk),
                                                       BLOCK_SIZE, NULL, NULL);
+        ret = -errno;
         aio_context_release(blk_get_aio_context(bmds->blk));
         if (!bmds->dirty_bitmap) {
-            ret = -errno;
             goto fail;
         }
     }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 02/12] migration: Don't lose errno across aio context changes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> set_drity_tracking() was assuming that the errno value set by

*cough*

> bdrv_create_dirty_bitmap() would not be corrupted by either
> blk_get_aio_context() or aio_context_release().  Rather than
> audit whether this assumption is safe, rewrite the code to just
> grab the value of errno sooner.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  migration/block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index 18d50ff..9a9c214 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -350,9 +350,9 @@ static int set_dirty_tracking(void)
>          aio_context_acquire(blk_get_aio_context(bmds->blk));
>          bmds->dirty_bitmap = bdrv_create_dirty_bitmap(blk_bs(bmds->blk),
>                                                        BLOCK_SIZE, NULL, NULL);
> +        ret = -errno;
>          aio_context_release(blk_get_aio_context(bmds->blk));
>          if (!bmds->dirty_bitmap) {
> -            ret = -errno;
>              goto fail;
>          }
>      }
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 02/12] migration: Don't lose errno across aio context changes

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> set_drity_tracking() was assuming that the errno value set by
> bdrv_create_dirty_bitmap() would not be corrupted by either
> blk_get_aio_context() or aio_context_release().  Rather than
> audit whether this assumption is safe, rewrite the code to just
> grab the value of errno sooner.
>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 03/12] dirty-bitmap: Drop unused functions

[Eric Blake]

We had several functions that no one was using, and which used
sector-based interfaces.  I'm trying to convert towards byte-based
interfaces, so it's easier to just drop the unused functions:

bdrv_dirty_bitmap_size
bdrv_dirty_bitmap_get_meta
bdrv_dirty_bitmap_reset_meta
bdrv_dirty_bitmap_meta_granularity

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/dirty-bitmap.h |  8 --------
 block/dirty-bitmap.c         | 34 ----------------------------------
 2 files changed, 42 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 9dea14b..a83979d 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -30,11 +30,9 @@ void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
 uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
 const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
-int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                    int64_t sector);
@@ -42,12 +40,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int64_t nr_sectors);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t cur_sector, int64_t nr_sectors);
-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
-                               BdrvDirtyBitmap *bitmap, int64_t sector,
-                               int nb_sectors);
-void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
-                                  BdrvDirtyBitmap *bitmap, int64_t sector,
-                                  int nb_sectors);
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
                                          uint64_t first_sector);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6d8ce5f..32698d5 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -130,35 +130,6 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
     bitmap->meta = NULL;
 }

-int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
-                               BdrvDirtyBitmap *bitmap, int64_t sector,
-                               int nb_sectors)
-{
-    uint64_t i;
-    int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta);
-
-    /* To optimize: we can make hbitmap to internally check the range in a
-     * coarse level, or at least do it word by word. */
-    for (i = sector; i < sector + nb_sectors; i += sectors_per_bit) {
-        if (hbitmap_get(bitmap->meta, i)) {
-            return true;
-        }
-    }
-    return false;
-}
-
-void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
-                                  BdrvDirtyBitmap *bitmap, int64_t sector,
-                                  int nb_sectors)
-{
-    hbitmap_reset(bitmap->meta, sector, nb_sectors);
-}
-
-int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
-{
-    return bitmap->size;
-}
-
 const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
 {
     return bitmap->name;
@@ -393,11 +364,6 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
     return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }

-uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
-{
-    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
-}
-
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
                                          uint64_t first_sector)
 {
-- 
2.9.3

Re: [Qemu-devel] [PATCH 03/12] dirty-bitmap: Drop unused functions

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> We had several functions that no one was using, and which used
> sector-based interfaces.  I'm trying to convert towards byte-based
> interfaces, so it's easier to just drop the unused functions:
> 
> bdrv_dirty_bitmap_size
> bdrv_dirty_bitmap_get_meta
> bdrv_dirty_bitmap_reset_meta
> bdrv_dirty_bitmap_meta_granularity
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/dirty-bitmap.h |  8 --------
>  block/dirty-bitmap.c         | 34 ----------------------------------
>  2 files changed, 42 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 9dea14b..a83979d 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -30,11 +30,9 @@ void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>  BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
>  uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>  uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
> -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
> -int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
>  int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>                     int64_t sector);
> @@ -42,12 +40,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                             int64_t cur_sector, int64_t nr_sectors);
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                               int64_t cur_sector, int64_t nr_sectors);
> -int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> -                               BdrvDirtyBitmap *bitmap, int64_t sector,
> -                               int nb_sectors);
> -void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
> -                                  BdrvDirtyBitmap *bitmap, int64_t sector,
> -                                  int nb_sectors);
>  BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
>  BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
>                                           uint64_t first_sector);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 6d8ce5f..32698d5 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -130,35 +130,6 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>      bitmap->meta = NULL;
>  }
> 
> -int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> -                               BdrvDirtyBitmap *bitmap, int64_t sector,
> -                               int nb_sectors)
> -{
> -    uint64_t i;
> -    int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta);
> -
> -    /* To optimize: we can make hbitmap to internally check the range in a
> -     * coarse level, or at least do it word by word. */
> -    for (i = sector; i < sector + nb_sectors; i += sectors_per_bit) {
> -        if (hbitmap_get(bitmap->meta, i)) {
> -            return true;
> -        }
> -    }
> -    return false;
> -}
> -
> -void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
> -                                  BdrvDirtyBitmap *bitmap, int64_t sector,
> -                                  int nb_sectors)
> -{
> -    hbitmap_reset(bitmap->meta, sector, nb_sectors);
> -}
> -
> -int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
> -{
> -    return bitmap->size;
> -}
> -
>  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
>  {
>      return bitmap->name;
> @@ -393,11 +364,6 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
>      return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>  }
> 
> -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap)
> -{
> -    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta);
> -}
> -
>  BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
>                                           uint64_t first_sector)
>  {
> 

I think it's likely Vladimir is or at least was relying on some of these
for his migration and persistence series.

Might be nice to let him chime in to see how much of a hassle this is.

Re: [Qemu-devel] [PATCH 03/12] dirty-bitmap: Drop unused functions

[Eric Blake]

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

On 04/12/2017 05:47 PM, John Snow wrote:
> 
> 
> On 04/12/2017 01:49 PM, Eric Blake wrote:
>> We had several functions that no one was using, and which used
>> sector-based interfaces.  I'm trying to convert towards byte-based
>> interfaces, so it's easier to just drop the unused functions:
>>
>> bdrv_dirty_bitmap_size
>> bdrv_dirty_bitmap_get_meta
>> bdrv_dirty_bitmap_reset_meta
>> bdrv_dirty_bitmap_meta_granularity
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  include/block/dirty-bitmap.h |  8 --------
>>  block/dirty-bitmap.c         | 34 ----------------------------------
>>  2 files changed, 42 deletions(-)
>>

> 
> I think it's likely Vladimir is or at least was relying on some of these
> for his migration and persistence series.
> 
> Might be nice to let him chime in to see how much of a hassle this is.

Then let's add him in cc ;)

I'm okay if these functions stay because they have a user, but it would
also be nice if they were properly byte-based (like everything else in
dirty-bitmap at the end of my series).  So even if we remove them here,
we can revert the removal, and re-add them but with a sane interface.

-- 
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 03/12] dirty-bitmap: Drop unused functions

[John Snow]

On 04/12/2017 07:36 PM, Eric Blake wrote:
> On 04/12/2017 05:47 PM, John Snow wrote:
>>
>>
>> On 04/12/2017 01:49 PM, Eric Blake wrote:
>>> We had several functions that no one was using, and which used
>>> sector-based interfaces.  I'm trying to convert towards byte-based
>>> interfaces, so it's easier to just drop the unused functions:
>>>
>>> bdrv_dirty_bitmap_size
>>> bdrv_dirty_bitmap_get_meta
>>> bdrv_dirty_bitmap_reset_meta
>>> bdrv_dirty_bitmap_meta_granularity
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  include/block/dirty-bitmap.h |  8 --------
>>>  block/dirty-bitmap.c         | 34 ----------------------------------
>>>  2 files changed, 42 deletions(-)
>>>
> 
>>
>> I think it's likely Vladimir is or at least was relying on some of these
>> for his migration and persistence series.
>>
>> Might be nice to let him chime in to see how much of a hassle this is.
> 
> Then let's add him in cc ;)
> 

Err... I can't just summon people by mentioning them?

> I'm okay if these functions stay because they have a user, but it would
> also be nice if they were properly byte-based (like everything else in
> dirty-bitmap at the end of my series).  So even if we remove them here,
> we can revert the removal, and re-add them but with a sane interface.
> 

OK, but I will offer to do the work in the interest of not slowing
things down any further.

Do you use any of these, Vladimir?

--js

Re: [Qemu-devel] [PATCH 03/12] dirty-bitmap: Drop unused functions

[Vladimir Sementsov-Ogievskiy]

13.04.2017 02:40, John Snow wrote:
>
> On 04/12/2017 07:36 PM, Eric Blake wrote:
>> On 04/12/2017 05:47 PM, John Snow wrote:
>>>
>>> On 04/12/2017 01:49 PM, Eric Blake wrote:
>>>> We had several functions that no one was using, and which used
>>>> sector-based interfaces.  I'm trying to convert towards byte-based
>>>> interfaces, so it's easier to just drop the unused functions:
>>>>
>>>> bdrv_dirty_bitmap_size
>>>> bdrv_dirty_bitmap_get_meta
>>>> bdrv_dirty_bitmap_reset_meta
>>>> bdrv_dirty_bitmap_meta_granularity
>>>>
>>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>   include/block/dirty-bitmap.h |  8 --------
>>>>   block/dirty-bitmap.c         | 34 ----------------------------------
>>>>   2 files changed, 42 deletions(-)
>>>>
>>> I think it's likely Vladimir is or at least was relying on some of these
>>> for his migration and persistence series.
>>>
>>> Might be nice to let him chime in to see how much of a hassle this is.
>> Then let's add him in cc ;)
>>
> Err... I can't just summon people by mentioning them?
>
>> I'm okay if these functions stay because they have a user, but it would
>> also be nice if they were properly byte-based (like everything else in
>> dirty-bitmap at the end of my series).  So even if we remove them here,
>> we can revert the removal, and re-add them but with a sane interface.
>>
> OK, but I will offer to do the work in the interest of not slowing
> things down any further.
>
> Do you use any of these, Vladimir?
>
> --js
HI all!

only bdrv_dirty_bitmap_size() is used. No problem to add it back with my 
series next update


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 03/12] dirty-bitmap: Drop unused functions

[John Snow]

On 04/13/2017 05:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.04.2017 02:40, John Snow wrote:
>>
>> On 04/12/2017 07:36 PM, Eric Blake wrote:
>>> On 04/12/2017 05:47 PM, John Snow wrote:
>>>>
>>>> On 04/12/2017 01:49 PM, Eric Blake wrote:
>>>>> We had several functions that no one was using, and which used
>>>>> sector-based interfaces.  I'm trying to convert towards byte-based
>>>>> interfaces, so it's easier to just drop the unused functions:
>>>>>
>>>>> bdrv_dirty_bitmap_size
>>>>> bdrv_dirty_bitmap_get_meta
>>>>> bdrv_dirty_bitmap_reset_meta
>>>>> bdrv_dirty_bitmap_meta_granularity
>>>>>
>>>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>>   include/block/dirty-bitmap.h |  8 --------
>>>>>   block/dirty-bitmap.c         | 34 ----------------------------------
>>>>>   2 files changed, 42 deletions(-)
>>>>>
>>>> I think it's likely Vladimir is or at least was relying on some of
>>>> these
>>>> for his migration and persistence series.
>>>>
>>>> Might be nice to let him chime in to see how much of a hassle this is.
>>> Then let's add him in cc ;)
>>>
>> Err... I can't just summon people by mentioning them?
>>
>>> I'm okay if these functions stay because they have a user, but it would
>>> also be nice if they were properly byte-based (like everything else in
>>> dirty-bitmap at the end of my series).  So even if we remove them here,
>>> we can revert the removal, and re-add them but with a sane interface.
>>>
>> OK, but I will offer to do the work in the interest of not slowing
>> things down any further.
>>
>> Do you use any of these, Vladimir?
>>
>> --js
> HI all!
> 
> only bdrv_dirty_bitmap_size() is used. No problem to add it back with my
> series next update
> 
> 

Excellent, thanks!

In that case:

Reviewed-by: John Snow <jsnow@redhat.com>

Should be for the whole series, IIRC.

--js

[Qemu-devel] [PATCH 04/12] dirty-bitmap: Track size in bytes

[Eric Blake]

We are still using an internal hbitmap that tracks a size in sectors,
with the granularity scaled down accordingly, because it lets us
use a shortcut for our iterators which are currently sector-based.
But there's no reason we can't track the dirty bitmap size in bytes,
since it is an internal-only variable.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/dirty-bitmap.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 32698d5..a413df1 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -41,7 +41,7 @@ struct BdrvDirtyBitmap {
     HBitmap *meta;              /* Meta dirty bitmap */
     BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
     char *name;                 /* Optional non-empty unique ID */
-    int64_t size;               /* Size of the bitmap (Number of sectors) */
+    int64_t size;               /* Size of the bitmap, in bytes */
     bool disabled;              /* Bitmap is read-only */
     int active_iterators;       /* How many iterators are active */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -79,24 +79,26 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
 {
     int64_t bitmap_size;
     BdrvDirtyBitmap *bitmap;
-    uint32_t sector_granularity;

-    assert((granularity & (granularity - 1)) == 0);
+    assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);

     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 = bdrv_getlength(bs);
     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));
+    /*
+     * TODO - let hbitmap track full granularity. For now, it is tracking
+     * only sector granularity, as a shortcut for our iterators.
+     */
+    bitmap->bitmap = hbitmap_alloc(bitmap_size >> BDRV_SECTOR_BITS,
+                                   ctz32(granularity) - BDRV_SECTOR_BITS);
     bitmap->size = bitmap_size;
     bitmap->name = g_strdup(name);
     bitmap->disabled = false;
@@ -246,12 +248,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bitmap;
-    uint64_t size = bdrv_nb_sectors(bs);
+    int64_t size = bdrv_getlength(bs);

+    assert(size >= 0);
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         assert(!bdrv_dirty_bitmap_frozen(bitmap));
         assert(!bitmap->active_iterators);
-        hbitmap_truncate(bitmap->bitmap, size);
+        hbitmap_truncate(bitmap->bitmap, size >> BDRV_SECTOR_BITS);
         bitmap->size = size;
     }
 }
@@ -419,7 +422,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
         hbitmap_reset_all(bitmap->bitmap);
     } else {
         HBitmap *backup = bitmap->bitmap;
-        bitmap->bitmap = hbitmap_alloc(bitmap->size,
+        bitmap->bitmap = hbitmap_alloc(bitmap->size >> BDRV_SECTOR_BITS,
                                        hbitmap_granularity(backup));
         *out = backup;
     }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 04/12] dirty-bitmap: Track size in bytes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> We are still using an internal hbitmap that tracks a size in sectors,
> with the granularity scaled down accordingly, because it lets us
> use a shortcut for our iterators which are currently sector-based.
> But there's no reason we can't track the dirty bitmap size in bytes,
> since it is an internal-only variable.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/dirty-bitmap.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 32698d5..a413df1 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -41,7 +41,7 @@ struct BdrvDirtyBitmap {
>      HBitmap *meta;              /* Meta dirty bitmap */
>      BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>      char *name;                 /* Optional non-empty unique ID */
> -    int64_t size;               /* Size of the bitmap (Number of sectors) */
> +    int64_t size;               /* Size of the bitmap, in bytes */
>      bool disabled;              /* Bitmap is read-only */
>      int active_iterators;       /* How many iterators are active */
>      QLIST_ENTRY(BdrvDirtyBitmap) list;
> @@ -79,24 +79,26 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>  {
>      int64_t bitmap_size;
>      BdrvDirtyBitmap *bitmap;
> -    uint32_t sector_granularity;
> 
> -    assert((granularity & (granularity - 1)) == 0);
> +    assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);
> 
>      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 = bdrv_getlength(bs);
>      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));
> +    /*
> +     * TODO - let hbitmap track full granularity. For now, it is tracking
> +     * only sector granularity, as a shortcut for our iterators.
> +     */
> +    bitmap->bitmap = hbitmap_alloc(bitmap_size >> BDRV_SECTOR_BITS,
> +                                   ctz32(granularity) - BDRV_SECTOR_BITS);
>      bitmap->size = bitmap_size;
>      bitmap->name = g_strdup(name);
>      bitmap->disabled = false;
> @@ -246,12 +248,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>  {
>      BdrvDirtyBitmap *bitmap;
> -    uint64_t size = bdrv_nb_sectors(bs);
> +    int64_t size = bdrv_getlength(bs);
> 
> +    assert(size >= 0);
>      QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>          assert(!bdrv_dirty_bitmap_frozen(bitmap));
>          assert(!bitmap->active_iterators);
> -        hbitmap_truncate(bitmap->bitmap, size);
> +        hbitmap_truncate(bitmap->bitmap, size >> BDRV_SECTOR_BITS);
>          bitmap->size = size;
>      }
>  }
> @@ -419,7 +422,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>          hbitmap_reset_all(bitmap->bitmap);
>      } else {
>          HBitmap *backup = bitmap->bitmap;
> -        bitmap->bitmap = hbitmap_alloc(bitmap->size,
> +        bitmap->bitmap = hbitmap_alloc(bitmap->size >> BDRV_SECTOR_BITS,
>                                         hbitmap_granularity(backup));
>          *out = backup;
>      }
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 05/12] dirty-bitmap: Set iterator start by offset, not sector

[Eric Blake]

All callers to bdrv_dirty_iter_new() passed 0 for their initial
starting point, drop that parameter.

All callers to bdrv_set_dirty_iter() were scaling an offset to
a sector number; move the scaling to occur internally to dirty
bitmap code instead.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/dirty-bitmap.h | 5 ++---
 block/backup.c               | 5 ++---
 block/dirty-bitmap.c         | 9 ++++-----
 block/mirror.c               | 4 ++--
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a83979d..efcec60 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -41,11 +41,10 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t cur_sector, int64_t nr_sectors);
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
-BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
-                                         uint64_t first_sector);
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
diff --git a/block/backup.c b/block/backup.c
index 63ca208..efa4896 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -372,7 +372,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)

     granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
     clusters_per_iter = MAX((granularity / job->cluster_size), 1);
-    dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
+    dbi = bdrv_dirty_iter_new(job->sync_bitmap);

     /* Find the next dirty sector(s) */
     while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
@@ -403,8 +403,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
         /* If the bitmap granularity is smaller than the backup granularity,
          * we need to advance the iterator pointer to the next cluster. */
         if (granularity < job->cluster_size) {
-            bdrv_set_dirty_iter(dbi,
-                                cluster * job->cluster_size / BDRV_SECTOR_SIZE);
+            bdrv_set_dirty_iter(dbi, cluster * job->cluster_size);
         }

         last_cluster = cluster - 1;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index a413df1..3fb4871 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -367,11 +367,10 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
     return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
 }

-BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
-                                         uint64_t first_sector)
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
 {
     BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
+    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, 0);
     iter->bitmap = bitmap;
     bitmap->active_iterators++;
     return iter;
@@ -488,9 +487,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
 /**
  * Advance a BdrvDirtyBitmapIter to an arbitrary offset.
  */
-void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
 {
-    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num);
+    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
 }

 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index c92335a..7c1d6bf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -370,7 +370,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
         if (next_dirty > next_offset || next_dirty < 0) {
             /* The bitmap iterator's cache is stale, refresh it */
-            bdrv_set_dirty_iter(s->dbi, next_offset >> BDRV_SECTOR_BITS);
+            bdrv_set_dirty_iter(s->dbi, next_offset);
             next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
         }
         assert(next_dirty == next_offset);
@@ -779,7 +779,7 @@ static void coroutine_fn mirror_run(void *opaque)
     }

     assert(!s->dbi);
-    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
+    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
     for (;;) {
         uint64_t delay_ns = 0;
         int64_t cnt, delta;
-- 
2.9.3

Re: [Qemu-devel] [PATCH 05/12] dirty-bitmap: Set iterator start by offset, not sector

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> All callers to bdrv_dirty_iter_new() passed 0 for their initial
> starting point, drop that parameter.
> 
> All callers to bdrv_set_dirty_iter() were scaling an offset to
> a sector number; move the scaling to occur internally to dirty
> bitmap code instead.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/dirty-bitmap.h | 5 ++---
>  block/backup.c               | 5 ++---
>  block/dirty-bitmap.c         | 9 ++++-----
>  block/mirror.c               | 4 ++--
>  4 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index a83979d..efcec60 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -41,11 +41,10 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                               int64_t cur_sector, int64_t nr_sectors);
>  BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
> -BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> -                                         uint64_t first_sector);
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
> -void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> diff --git a/block/backup.c b/block/backup.c
> index 63ca208..efa4896 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -372,7 +372,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> 
>      granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
>      clusters_per_iter = MAX((granularity / job->cluster_size), 1);
> -    dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
> +    dbi = bdrv_dirty_iter_new(job->sync_bitmap);
> 
>      /* Find the next dirty sector(s) */
>      while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
> @@ -403,8 +403,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>          /* If the bitmap granularity is smaller than the backup granularity,
>           * we need to advance the iterator pointer to the next cluster. */
>          if (granularity < job->cluster_size) {
> -            bdrv_set_dirty_iter(dbi,
> -                                cluster * job->cluster_size / BDRV_SECTOR_SIZE);
> +            bdrv_set_dirty_iter(dbi, cluster * job->cluster_size);
>          }
> 
>          last_cluster = cluster - 1;
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index a413df1..3fb4871 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -367,11 +367,10 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
>      return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>  }
> 
> -BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> -                                         uint64_t first_sector)
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
>  {
>      BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> -    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
> +    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, 0);
>      iter->bitmap = bitmap;
>      bitmap->active_iterators++;
>      return iter;
> @@ -488,9 +487,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>  /**
>   * Advance a BdrvDirtyBitmapIter to an arbitrary offset.
>   */
> -void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
>  {
> -    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num);
> +    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
>  }
> 
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index c92335a..7c1d6bf 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -370,7 +370,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
>          if (next_dirty > next_offset || next_dirty < 0) {
>              /* The bitmap iterator's cache is stale, refresh it */
> -            bdrv_set_dirty_iter(s->dbi, next_offset >> BDRV_SECTOR_BITS);
> +            bdrv_set_dirty_iter(s->dbi, next_offset);
>              next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
>          }
>          assert(next_dirty == next_offset);
> @@ -779,7 +779,7 @@ static void coroutine_fn mirror_run(void *opaque)
>      }
> 
>      assert(!s->dbi);
> -    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> +    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
>      for (;;) {
>          uint64_t delay_ns = 0;
>          int64_t cnt, delta;
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 06/12] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[Eric Blake]

Thanks to recent cleanups, all callers were scaling a return value
of sectors into bytes; do the scaling internally instead.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/backup.c       | 2 +-
 block/dirty-bitmap.c | 2 +-
 block/mirror.c       | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index efa4896..6efd864 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -375,7 +375,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
     dbi = bdrv_dirty_iter_new(job->sync_bitmap);

     /* Find the next dirty sector(s) */
-    while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
+    while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
         cluster = offset / job->cluster_size;

         /* Fake progress updates for any clusters we skipped */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 3fb4871..2f9f554 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -397,7 +397,7 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)

 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 {
-    return hbitmap_iter_next(&iter->hbi);
+    return hbitmap_iter_next(&iter->hbi) * BDRV_SECTOR_SIZE;
 }

 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
diff --git a/block/mirror.c b/block/mirror.c
index 7c1d6bf..f404ff3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -335,10 +335,10 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
     int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);

-    offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+    offset = bdrv_dirty_iter_next(s->dbi);
     if (offset < 0) {
         bdrv_set_dirty_iter(s->dbi, 0);
-        offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+        offset = bdrv_dirty_iter_next(s->dbi);
         trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
                                   BDRV_SECTOR_SIZE);
         assert(offset >= 0);
@@ -367,11 +367,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
             break;
         }

-        next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+        next_dirty = bdrv_dirty_iter_next(s->dbi);
         if (next_dirty > next_offset || next_dirty < 0) {
             /* The bitmap iterator's cache is stale, refresh it */
             bdrv_set_dirty_iter(s->dbi, next_offset);
-            next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+            next_dirty = bdrv_dirty_iter_next(s->dbi);
         }
         assert(next_dirty == next_offset);
         nb_chunks++;
-- 
2.9.3

Re: [Qemu-devel] [PATCH 06/12] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Thanks to recent cleanups, all callers were scaling a return value
> of sectors into bytes; do the scaling internally instead.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/backup.c       | 2 +-
>  block/dirty-bitmap.c | 2 +-
>  block/mirror.c       | 8 ++++----
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index efa4896..6efd864 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -375,7 +375,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>      dbi = bdrv_dirty_iter_new(job->sync_bitmap);
> 
>      /* Find the next dirty sector(s) */
> -    while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
> +    while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
>          cluster = offset / job->cluster_size;
> 
>          /* Fake progress updates for any clusters we skipped */
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 3fb4871..2f9f554 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -397,7 +397,7 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
> 
>  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
>  {
> -    return hbitmap_iter_next(&iter->hbi);
> +    return hbitmap_iter_next(&iter->hbi) * BDRV_SECTOR_SIZE;
>  }
> 
>  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> diff --git a/block/mirror.c b/block/mirror.c
> index 7c1d6bf..f404ff3 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -335,10 +335,10 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>      bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
>      int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);
> 
> -    offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +    offset = bdrv_dirty_iter_next(s->dbi);
>      if (offset < 0) {
>          bdrv_set_dirty_iter(s->dbi, 0);
> -        offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +        offset = bdrv_dirty_iter_next(s->dbi);
>          trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
>                                    BDRV_SECTOR_SIZE);
>          assert(offset >= 0);
> @@ -367,11 +367,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>              break;
>          }
> 
> -        next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +        next_dirty = bdrv_dirty_iter_next(s->dbi);
>          if (next_dirty > next_offset || next_dirty < 0) {
>              /* The bitmap iterator's cache is stale, refresh it */
>              bdrv_set_dirty_iter(s->dbi, next_offset);
> -            next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
> +            next_dirty = bdrv_dirty_iter_next(s->dbi);
>          }
>          assert(next_dirty == next_offset);
>          nb_chunks++;
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 07/12] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes

[Eric Blake]

Thanks to recent cleanups, all callers were scaling a return value
of sectors into bytes; do the scaling internally instead.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/dirty-bitmap.c |  4 ++--
 block/mirror.c       | 13 +++++--------
 migration/block.c    |  2 +-
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 2f9f554..e3c2e34 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -319,7 +319,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
     QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
         BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
         BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
-        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
+        info->count = bdrv_get_dirty_count(bm);
         info->granularity = bdrv_dirty_bitmap_granularity(bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
@@ -494,7 +494,7 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)

 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
-    return hbitmap_count(bitmap->bitmap);
+    return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
 }

 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index f404ff3..1b98a77 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -794,11 +794,10 @@ static void coroutine_fn mirror_run(void *opaque)

         cnt = bdrv_get_dirty_count(s->dirty_bitmap);
         /* s->common.offset contains the number of bytes already processed so
-         * far, cnt is the number of dirty sectors remaining and
+         * far, cnt is the number of dirty bytes remaining and
          * s->bytes_in_flight is the number of bytes currently being
          * processed; together those are the current total operation length */
-        s->common.len = s->common.offset + s->bytes_in_flight +
-            cnt * BDRV_SECTOR_SIZE;
+        s->common.len = s->common.offset + s->bytes_in_flight + cnt;

         /* Note that even when no rate limit is applied we need to yield
          * periodically with no pending I/O so that bdrv_drain_all() returns.
@@ -810,8 +809,7 @@ static void coroutine_fn mirror_run(void *opaque)
             s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
             if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
                 (cnt == 0 && s->in_flight > 0)) {
-                trace_mirror_yield(s, cnt * BDRV_SECTOR_SIZE,
-                                   s->buf_free_count, s->in_flight);
+                trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
                 mirror_wait_for_io(s);
                 continue;
             } else if (cnt != 0) {
@@ -852,7 +850,7 @@ static void coroutine_fn mirror_run(void *opaque)
              * whether to switch to target check one last time if I/O has
              * come in the meanwhile, and if not flush the data to disk.
              */
-            trace_mirror_before_drain(s, cnt * BDRV_SECTOR_SIZE);
+            trace_mirror_before_drain(s, cnt);

             bdrv_drained_begin(bs);
             cnt = bdrv_get_dirty_count(s->dirty_bitmap);
@@ -871,8 +869,7 @@ static void coroutine_fn mirror_run(void *opaque)
         }

         ret = 0;
-        trace_mirror_before_sleep(s, cnt * BDRV_SECTOR_SIZE,
-                                  s->synced, delay_ns);
+        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
         if (!s->synced) {
             block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
             if (block_job_is_cancelled(&s->common)) {
diff --git a/migration/block.c b/migration/block.c
index 9a9c214..3daa5c7 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -672,7 +672,7 @@ static int64_t get_remaining_dirty(void)
         aio_context_release(blk_get_aio_context(bmds->blk));
     }

-    return dirty << BDRV_SECTOR_BITS;
+    return dirty;
 }

 /* Called with iothread lock taken.  */
-- 
2.9.3

Re: [Qemu-devel] [PATCH 07/12] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Thanks to recent cleanups, all callers were scaling a return value
> of sectors into bytes; do the scaling internally instead.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/dirty-bitmap.c |  4 ++--
>  block/mirror.c       | 13 +++++--------
>  migration/block.c    |  2 +-
>  3 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 2f9f554..e3c2e34 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -319,7 +319,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>      QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
>          BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
>          BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
> -        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
> +        info->count = bdrv_get_dirty_count(bm);
>          info->granularity = bdrv_dirty_bitmap_granularity(bm);
>          info->has_name = !!bm->name;
>          info->name = g_strdup(bm->name);
> @@ -494,7 +494,7 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
> 
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
> -    return hbitmap_count(bitmap->bitmap);
> +    return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
>  }
> 
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index f404ff3..1b98a77 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -794,11 +794,10 @@ static void coroutine_fn mirror_run(void *opaque)
> 
>          cnt = bdrv_get_dirty_count(s->dirty_bitmap);
>          /* s->common.offset contains the number of bytes already processed so
> -         * far, cnt is the number of dirty sectors remaining and
> +         * far, cnt is the number of dirty bytes remaining and
>           * s->bytes_in_flight is the number of bytes currently being
>           * processed; together those are the current total operation length */
> -        s->common.len = s->common.offset + s->bytes_in_flight +
> -            cnt * BDRV_SECTOR_SIZE;
> +        s->common.len = s->common.offset + s->bytes_in_flight + cnt;
> 
>          /* Note that even when no rate limit is applied we need to yield
>           * periodically with no pending I/O so that bdrv_drain_all() returns.
> @@ -810,8 +809,7 @@ static void coroutine_fn mirror_run(void *opaque)
>              s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>              if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>                  (cnt == 0 && s->in_flight > 0)) {
> -                trace_mirror_yield(s, cnt * BDRV_SECTOR_SIZE,
> -                                   s->buf_free_count, s->in_flight);
> +                trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
>                  mirror_wait_for_io(s);
>                  continue;
>              } else if (cnt != 0) {
> @@ -852,7 +850,7 @@ static void coroutine_fn mirror_run(void *opaque)
>               * whether to switch to target check one last time if I/O has
>               * come in the meanwhile, and if not flush the data to disk.
>               */
> -            trace_mirror_before_drain(s, cnt * BDRV_SECTOR_SIZE);
> +            trace_mirror_before_drain(s, cnt);
> 
>              bdrv_drained_begin(bs);
>              cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> @@ -871,8 +869,7 @@ static void coroutine_fn mirror_run(void *opaque)
>          }
> 
>          ret = 0;
> -        trace_mirror_before_sleep(s, cnt * BDRV_SECTOR_SIZE,
> -                                  s->synced, delay_ns);
> +        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>          if (!s->synced) {
>              block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
>              if (block_job_is_cancelled(&s->common)) {
> diff --git a/migration/block.c b/migration/block.c
> index 9a9c214..3daa5c7 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -672,7 +672,7 @@ static int64_t get_remaining_dirty(void)
>          aio_context_release(blk_get_aio_context(bmds->blk));
>      }
> 
> -    return dirty << BDRV_SECTOR_BITS;
> +    return dirty;
>  }
> 
>  /* Called with iothread lock taken.  */
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 07/12] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Thanks to recent cleanups, all callers were scaling a return value
> of sectors into bytes; do the scaling internally instead.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/dirty-bitmap.c |  4 ++--
>  block/mirror.c       | 13 +++++--------
>  migration/block.c    |  2 +-
>  3 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 2f9f554..e3c2e34 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -319,7 +319,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>      QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
>          BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
>          BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
> -        info->count = bdrv_get_dirty_count(bm) << BDRV_SECTOR_BITS;
> +        info->count = bdrv_get_dirty_count(bm);
>          info->granularity = bdrv_dirty_bitmap_granularity(bm);
>          info->has_name = !!bm->name;
>          info->name = g_strdup(bm->name);
> @@ -494,7 +494,7 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
> 
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
> -    return hbitmap_count(bitmap->bitmap);
> +    return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
>  }
> 
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index f404ff3..1b98a77 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -794,11 +794,10 @@ static void coroutine_fn mirror_run(void *opaque)
> 
>          cnt = bdrv_get_dirty_count(s->dirty_bitmap);
>          /* s->common.offset contains the number of bytes already processed so
> -         * far, cnt is the number of dirty sectors remaining and
> +         * far, cnt is the number of dirty bytes remaining and
>           * s->bytes_in_flight is the number of bytes currently being
>           * processed; together those are the current total operation length */
> -        s->common.len = s->common.offset + s->bytes_in_flight +
> -            cnt * BDRV_SECTOR_SIZE;
> +        s->common.len = s->common.offset + s->bytes_in_flight + cnt;
> 
>          /* Note that even when no rate limit is applied we need to yield
>           * periodically with no pending I/O so that bdrv_drain_all() returns.
> @@ -810,8 +809,7 @@ static void coroutine_fn mirror_run(void *opaque)
>              s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>              if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>                  (cnt == 0 && s->in_flight > 0)) {
> -                trace_mirror_yield(s, cnt * BDRV_SECTOR_SIZE,
> -                                   s->buf_free_count, s->in_flight);
> +                trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
>                  mirror_wait_for_io(s);
>                  continue;
>              } else if (cnt != 0) {
> @@ -852,7 +850,7 @@ static void coroutine_fn mirror_run(void *opaque)
>               * whether to switch to target check one last time if I/O has
>               * come in the meanwhile, and if not flush the data to disk.
>               */
> -            trace_mirror_before_drain(s, cnt * BDRV_SECTOR_SIZE);
> +            trace_mirror_before_drain(s, cnt);
> 
>              bdrv_drained_begin(bs);
>              cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> @@ -871,8 +869,7 @@ static void coroutine_fn mirror_run(void *opaque)
>          }
> 
>          ret = 0;
> -        trace_mirror_before_sleep(s, cnt * BDRV_SECTOR_SIZE,
> -                                  s->synced, delay_ns);
> +        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>          if (!s->synced) {
>              block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
>              if (block_job_is_cancelled(&s->common)) {
> diff --git a/migration/block.c b/migration/block.c
> index 9a9c214..3daa5c7 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -672,7 +672,7 @@ static int64_t get_remaining_dirty(void)
>          aio_context_release(blk_get_aio_context(bmds->blk));
>      }
> 
> -    return dirty << BDRV_SECTOR_BITS;
> +    return dirty;
>  }
> 
>  /* Called with iothread lock taken.  */
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 08/12] dirty-bitmap: Change bdrv_get_dirty() to take bytes

[Eric Blake]

Half the callers were already scaling bytes to sectors; the other
half can eventually be simplified to use byte iteration.  Both
callers were already using the result as a bool, so make that
explicit.  Making the change also makes it easier for a future
dirty-bitmap patch to offload scaling over to the internal hbitmap.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/dirty-bitmap.h | 4 ++--
 block/dirty-bitmap.c         | 8 ++++----
 block/mirror.c               | 3 +--
 migration/block.c            | 3 +--
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index efcec60..b8434e5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,8 +34,8 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
 const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
-int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
-                   int64_t sector);
+bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                    int64_t offset);
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int64_t nr_sectors);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index e3c2e34..c8100d2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -332,13 +332,13 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
     return list;
 }

-int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
-                   int64_t sector)
+bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                    int64_t offset)
 {
     if (bitmap) {
-        return hbitmap_get(bitmap->bitmap, sector);
+        return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
     } else {
-        return 0;
+        return false;
     }
 }

diff --git a/block/mirror.c b/block/mirror.c
index 1b98a77..1e2e655 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -359,8 +359,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         int64_t next_offset = offset + nb_chunks * s->granularity;
         int64_t next_chunk = next_offset / s->granularity;
         if (next_offset >= s->bdev_length ||
-            !bdrv_get_dirty(source, s->dirty_bitmap,
-                            next_offset >> BDRV_SECTOR_BITS)) {
+            !bdrv_get_dirty(source, s->dirty_bitmap, next_offset)) {
             break;
         }
         if (test_bit(next_chunk, s->in_flight_bitmap)) {
diff --git a/migration/block.c b/migration/block.c
index 3daa5c7..9e21aeb 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -537,8 +537,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
         } else {
             blk_mig_unlock();
         }
-        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector)) {
-
+        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector * BDRV_SECTOR_SIZE)) {
             if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
                 nr_sectors = total_sectors - sector;
             } else {
-- 
2.9.3

Re: [Qemu-devel] [PATCH 08/12] dirty-bitmap: Change bdrv_get_dirty() to take bytes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Half the callers were already scaling bytes to sectors; the other
> half can eventually be simplified to use byte iteration.  Both
> callers were already using the result as a bool, so make that
> explicit.  Making the change also makes it easier for a future
> dirty-bitmap patch to offload scaling over to the internal hbitmap.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/dirty-bitmap.h | 4 ++--
>  block/dirty-bitmap.c         | 8 ++++----
>  block/mirror.c               | 3 +--
>  migration/block.c            | 3 +--
>  4 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index efcec60..b8434e5 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -34,8 +34,8 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> -                   int64_t sector);
> +bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                    int64_t offset);
>  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                             int64_t cur_sector, int64_t nr_sectors);
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index e3c2e34..c8100d2 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -332,13 +332,13 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>      return list;
>  }
> 
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> -                   int64_t sector)
> +bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                    int64_t offset)
>  {
>      if (bitmap) {
> -        return hbitmap_get(bitmap->bitmap, sector);
> +        return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
>      } else {
> -        return 0;
> +        return false;
>      }
>  }
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 1b98a77..1e2e655 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -359,8 +359,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          int64_t next_offset = offset + nb_chunks * s->granularity;
>          int64_t next_chunk = next_offset / s->granularity;
>          if (next_offset >= s->bdev_length ||
> -            !bdrv_get_dirty(source, s->dirty_bitmap,
> -                            next_offset >> BDRV_SECTOR_BITS)) {
> +            !bdrv_get_dirty(source, s->dirty_bitmap, next_offset)) {
>              break;
>          }
>          if (test_bit(next_chunk, s->in_flight_bitmap)) {
> diff --git a/migration/block.c b/migration/block.c
> index 3daa5c7..9e21aeb 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -537,8 +537,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>          } else {
>              blk_mig_unlock();
>          }
> -        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector)) {
> -
> +        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector * BDRV_SECTOR_SIZE)) {

This one is a little weirder now though, isn't it? We're asking for the
dirty status of a single byte, technically. In practice, the scaling
factor will always cover the entire sector, but it reads a lot jankier now.

>              if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
>                  nr_sectors = total_sectors - sector;
>              } else {
> 

Oh well, it was always janky...

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 08/12] dirty-bitmap: Change bdrv_get_dirty() to take bytes

[Eric Blake]

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

On 04/12/2017 07:25 PM, John Snow wrote:

>> +++ b/migration/block.c
>> @@ -537,8 +537,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>>          } else {
>>              blk_mig_unlock();
>>          }
>> -        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector)) {
>> -
>> +        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector * BDRV_SECTOR_SIZE)) {
> 
> This one is a little weirder now though, isn't it? We're asking for the
> dirty status of a single byte, technically. In practice, the scaling
> factor will always cover the entire sector, but it reads a lot jankier now.
> 
>>              if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
>>                  nr_sectors = total_sectors - sector;
>>              } else {
>>
> 
> Oh well, it was always janky...

True. Think of it as "is the granularity (which may be a sector, a
cluster, or even some other size) that contains 'offset' dirty?".  I
really think migration/block.c will be easier to read once converted to
byte operations everywhere, but have not yet tackled it (it was hard
enough tackling mirror, backup, commit, and stream in parallel for the
previous series).

> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 

Thanks for the reviews, by the way, even if the prerequisite patches
still haven't been fully reviewed.

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

[Qemu-devel] [PATCH 09/12] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes

[Eric Blake]

Some of the callers were already scaling bytes to sectors; others
can be easily converted to pass byte offsets, all in our shift
towards a consistent byte interface everywhere.  Making the change
will also make it easier to write the hold-out callers to use byte
rather than sectors for their iterations; it also makes it easier
for a future dirty-bitmap patch to offload scaling over to the
internal hbitmap.  Although all callers happen to pass
sector-aligned values, make the internal scaling robust to any
sub-sector requests.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/dirty-bitmap.h |  4 ++--
 block/dirty-bitmap.c         | 14 ++++++++++----
 block/mirror.c               | 16 ++++++++--------
 migration/block.c            |  7 +++++--
 4 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index b8434e5..fdff1e2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -37,9 +37,9 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
 bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                     int64_t offset);
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                           int64_t cur_sector, int64_t nr_sectors);
+                           int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                             int64_t cur_sector, int64_t nr_sectors);
+                             int64_t offset, int64_t bytes);
 BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c8100d2..8e7822c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -401,17 +401,23 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 }

 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                           int64_t cur_sector, int64_t nr_sectors)
+                           int64_t offset, int64_t bytes)
 {
+    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+    hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+                end_sector - (offset >> BDRV_SECTOR_BITS));
 }

 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-                             int64_t cur_sector, int64_t nr_sectors)
+                             int64_t offset, int64_t bytes)
 {
+    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+    hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+                  end_sector - (offset >> BDRV_SECTOR_BITS));
 }

 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
diff --git a/block/mirror.c b/block/mirror.c
index 1e2e655..21b4f5d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -141,8 +141,7 @@ static void mirror_write_complete(void *opaque, int ret)
     if (ret < 0) {
         BlockErrorAction action;

-        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >> BDRV_SECTOR_BITS,
-                              op->bytes >> BDRV_SECTOR_BITS);
+        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
         action = mirror_error_action(s, false, -ret);
         if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
             s->ret = ret;
@@ -161,8 +160,7 @@ static void mirror_read_complete(void *opaque, int ret)
     if (ret < 0) {
         BlockErrorAction action;

-        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >> BDRV_SECTOR_BITS,
-                              op->bytes >> BDRV_SECTOR_BITS);
+        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
         action = mirror_error_action(s, true, -ret);
         if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
             s->ret = ret;
@@ -380,8 +378,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
      * calling bdrv_get_block_status_above could yield - if some blocks are
      * marked dirty in this window, we need to know.
      */
-    bdrv_reset_dirty_bitmap(s->dirty_bitmap, offset >> BDRV_SECTOR_BITS,
-                            nb_chunks * sectors_per_chunk);
+    bdrv_reset_dirty_bitmap(s->dirty_bitmap, offset,
+                            nb_chunks * s->granularity);
     bitmap_set(s->in_flight_bitmap, offset / s->granularity, nb_chunks);
     while (nb_chunks > 0 && offset < s->bdev_length) {
         int64_t ret;
@@ -614,7 +612,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)

     if (base == NULL && !bdrv_has_zero_init(target_bs)) {
         if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
-            bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
+            bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
             return 0;
         }

@@ -667,7 +665,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
         assert(n > 0);
         if (ret == 1) {
-            bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
+            bdrv_set_dirty_bitmap(s->dirty_bitmap,
+                                  sector_num * BDRV_SECTOR_SIZE,
+                                  n * BDRV_SECTOR_SIZE);
         }
         sector_num += n;
     }
diff --git a/migration/block.c b/migration/block.c
index 9e21aeb..e875211 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -331,7 +331,8 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
     blk->aiocb = blk_aio_preadv(bb, cur_sector * BDRV_SECTOR_SIZE, &blk->qiov,
                                 0, blk_mig_read_cb, blk);

-    bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
+    bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector * BDRV_SECTOR_SIZE,
+                            nr_sectors * BDRV_SECTOR_SIZE);
     aio_context_release(blk_get_aio_context(bmds->blk));
     qemu_mutex_unlock_iothread();

@@ -575,7 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
                 g_free(blk);
             }

-            bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
+            bdrv_reset_dirty_bitmap(bmds->dirty_bitmap,
+                                    sector * BDRV_SECTOR_SIZE,
+                                    nr_sectors * BDRV_SECTOR_SIZE);
             sector += nr_sectors;
             bmds->cur_dirty = sector;

-- 
2.9.3

Re: [Qemu-devel] [PATCH 09/12] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Some of the callers were already scaling bytes to sectors; others
> can be easily converted to pass byte offsets, all in our shift
> towards a consistent byte interface everywhere.  Making the change
> will also make it easier to write the hold-out callers to use byte
> rather than sectors for their iterations; it also makes it easier
> for a future dirty-bitmap patch to offload scaling over to the
> internal hbitmap.  Although all callers happen to pass
> sector-aligned values, make the internal scaling robust to any
> sub-sector requests.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/dirty-bitmap.h |  4 ++--
>  block/dirty-bitmap.c         | 14 ++++++++++----
>  block/mirror.c               | 16 ++++++++--------
>  migration/block.c            |  7 +++++--
>  4 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index b8434e5..fdff1e2 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -37,9 +37,9 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
>  bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>                      int64_t offset);
>  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> -                           int64_t cur_sector, int64_t nr_sectors);
> +                           int64_t offset, int64_t bytes);
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> -                             int64_t cur_sector, int64_t nr_sectors);
> +                             int64_t offset, int64_t bytes);
>  BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
>  BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index c8100d2..8e7822c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -401,17 +401,23 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
>  }
> 
>  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> -                           int64_t cur_sector, int64_t nr_sectors)
> +                           int64_t offset, int64_t bytes)
>  {
> +    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
> +
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> -    hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +    hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
> +                end_sector - (offset >> BDRV_SECTOR_BITS));
>  }
> 
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> -                             int64_t cur_sector, int64_t nr_sectors)
> +                             int64_t offset, int64_t bytes)
>  {
> +    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
> +
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> -    hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +    hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
> +                  end_sector - (offset >> BDRV_SECTOR_BITS));
>  }
> 
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> diff --git a/block/mirror.c b/block/mirror.c
> index 1e2e655..21b4f5d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -141,8 +141,7 @@ static void mirror_write_complete(void *opaque, int ret)
>      if (ret < 0) {
>          BlockErrorAction action;
> 
> -        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >> BDRV_SECTOR_BITS,
> -                              op->bytes >> BDRV_SECTOR_BITS);
> +        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
>          action = mirror_error_action(s, false, -ret);
>          if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
>              s->ret = ret;
> @@ -161,8 +160,7 @@ static void mirror_read_complete(void *opaque, int ret)
>      if (ret < 0) {
>          BlockErrorAction action;
> 
> -        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset >> BDRV_SECTOR_BITS,
> -                              op->bytes >> BDRV_SECTOR_BITS);
> +        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
>          action = mirror_error_action(s, true, -ret);
>          if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
>              s->ret = ret;
> @@ -380,8 +378,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>       * calling bdrv_get_block_status_above could yield - if some blocks are
>       * marked dirty in this window, we need to know.
>       */
> -    bdrv_reset_dirty_bitmap(s->dirty_bitmap, offset >> BDRV_SECTOR_BITS,
> -                            nb_chunks * sectors_per_chunk);
> +    bdrv_reset_dirty_bitmap(s->dirty_bitmap, offset,
> +                            nb_chunks * s->granularity);
>      bitmap_set(s->in_flight_bitmap, offset / s->granularity, nb_chunks);
>      while (nb_chunks > 0 && offset < s->bdev_length) {
>          int64_t ret;
> @@ -614,7 +612,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
> 
>      if (base == NULL && !bdrv_has_zero_init(target_bs)) {
>          if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
> -            bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
> +            bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
>              return 0;
>          }
> 
> @@ -667,7 +665,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>          n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
>          assert(n > 0);
>          if (ret == 1) {
> -            bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
> +            bdrv_set_dirty_bitmap(s->dirty_bitmap,
> +                                  sector_num * BDRV_SECTOR_SIZE,
> +                                  n * BDRV_SECTOR_SIZE);
>          }
>          sector_num += n;
>      }
> diff --git a/migration/block.c b/migration/block.c
> index 9e21aeb..e875211 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -331,7 +331,8 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>      blk->aiocb = blk_aio_preadv(bb, cur_sector * BDRV_SECTOR_SIZE, &blk->qiov,
>                                  0, blk_mig_read_cb, blk);
> 
> -    bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
> +    bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector * BDRV_SECTOR_SIZE,
> +                            nr_sectors * BDRV_SECTOR_SIZE);
>      aio_context_release(blk_get_aio_context(bmds->blk));
>      qemu_mutex_unlock_iothread();
> 
> @@ -575,7 +576,9 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>                  g_free(blk);
>              }
> 
> -            bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
> +            bdrv_reset_dirty_bitmap(bmds->dirty_bitmap,
> +                                    sector * BDRV_SECTOR_SIZE,
> +                                    nr_sectors * BDRV_SECTOR_SIZE);
>              sector += nr_sectors;
>              bmds->cur_dirty = sector;
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 10/12] mirror: Switch mirror_dirty_init() to byte-based iteration

[Eric Blake]

Now that we have adjusted the majority of the calls this function
makes to be byte-based, it is easier to read the code if it makes
passes over the image using bytes rather than sectors.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 21b4f5d..846e392 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -601,15 +601,13 @@ static void mirror_throttle(MirrorBlockJob *s)

 static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 {
-    int64_t sector_num, end;
+    int64_t offset;
     BlockDriverState *base = s->base;
     BlockDriverState *bs = s->source;
     BlockDriverState *target_bs = blk_bs(s->target);
-    int ret, n;
+    int ret;
     int64_t count;

-    end = s->bdev_length / BDRV_SECTOR_SIZE;
-
     if (base == NULL && !bdrv_has_zero_init(target_bs)) {
         if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
             bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
@@ -617,9 +615,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         }

         s->initial_zeroing_ongoing = true;
-        for (sector_num = 0; sector_num < end; ) {
-            int nb_sectors = MIN(end - sector_num,
-                QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS);
+        for (offset = 0; offset < s->bdev_length; ) {
+            int bytes = MIN(s->bdev_length - offset,
+                            QEMU_ALIGN_DOWN(INT_MAX, s->granularity));

             mirror_throttle(s);

@@ -635,9 +633,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
                 continue;
             }

-            mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
-                                      nb_sectors * BDRV_SECTOR_SIZE, false);
-            sector_num += nb_sectors;
+            mirror_do_zero_or_discard(s, offset, bytes, false);
+            offset += bytes;
         }

         mirror_wait_for_all_io(s);
@@ -645,10 +642,10 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
     }

     /* First part, loop on the sectors and initialize the dirty bitmap.  */
-    for (sector_num = 0; sector_num < end; ) {
+    for (offset = 0; offset < s->bdev_length; ) {
         /* Just to make sure we are not exceeding int limit. */
-        int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
-                             end - sector_num);
+        int bytes = MIN(s->bdev_length - offset,
+                        QEMU_ALIGN_DOWN(INT_MAX, s->granularity));

         mirror_throttle(s);

@@ -656,20 +653,16 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             return 0;
         }

-        ret = bdrv_is_allocated_above(bs, base, sector_num * BDRV_SECTOR_SIZE,
-                                      nb_sectors * BDRV_SECTOR_SIZE, &count);
+        ret = bdrv_is_allocated_above(bs, base, offset, bytes, &count);
         if (ret < 0) {
             return ret;
         }

-        n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
-        assert(n > 0);
+        count = QEMU_ALIGN_UP(count, BDRV_SECTOR_SIZE);
         if (ret == 1) {
-            bdrv_set_dirty_bitmap(s->dirty_bitmap,
-                                  sector_num * BDRV_SECTOR_SIZE,
-                                  n * BDRV_SECTOR_SIZE);
+            bdrv_set_dirty_bitmap(s->dirty_bitmap, offset, count);
         }
-        sector_num += n;
+        offset += count;
     }
     return 0;
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH 10/12] mirror: Switch mirror_dirty_init() to byte-based iteration

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Now that we have adjusted the majority of the calls this function
> makes to be byte-based, it is easier to read the code if it makes
> passes over the image using bytes rather than sectors.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/mirror.c | 35 ++++++++++++++---------------------
>  1 file changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 21b4f5d..846e392 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -601,15 +601,13 @@ static void mirror_throttle(MirrorBlockJob *s)
> 
>  static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>  {
> -    int64_t sector_num, end;
> +    int64_t offset;
>      BlockDriverState *base = s->base;
>      BlockDriverState *bs = s->source;
>      BlockDriverState *target_bs = blk_bs(s->target);
> -    int ret, n;
> +    int ret;
>      int64_t count;
> 
> -    end = s->bdev_length / BDRV_SECTOR_SIZE;
> -
>      if (base == NULL && !bdrv_has_zero_init(target_bs)) {
>          if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
>              bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
> @@ -617,9 +615,9 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>          }
> 
>          s->initial_zeroing_ongoing = true;
> -        for (sector_num = 0; sector_num < end; ) {
> -            int nb_sectors = MIN(end - sector_num,
> -                QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS);
> +        for (offset = 0; offset < s->bdev_length; ) {
> +            int bytes = MIN(s->bdev_length - offset,
> +                            QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
> 
>              mirror_throttle(s);
> 
> @@ -635,9 +633,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>                  continue;
>              }
> 
> -            mirror_do_zero_or_discard(s, sector_num * BDRV_SECTOR_SIZE,
> -                                      nb_sectors * BDRV_SECTOR_SIZE, false);
> -            sector_num += nb_sectors;
> +            mirror_do_zero_or_discard(s, offset, bytes, false);
> +            offset += bytes;
>          }
> 
>          mirror_wait_for_all_io(s);
> @@ -645,10 +642,10 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>      }
> 
>      /* First part, loop on the sectors and initialize the dirty bitmap.  */
> -    for (sector_num = 0; sector_num < end; ) {
> +    for (offset = 0; offset < s->bdev_length; ) {
>          /* Just to make sure we are not exceeding int limit. */
> -        int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
> -                             end - sector_num);
> +        int bytes = MIN(s->bdev_length - offset,
> +                        QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
> 
>          mirror_throttle(s);
> 
> @@ -656,20 +653,16 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              return 0;
>          }
> 
> -        ret = bdrv_is_allocated_above(bs, base, sector_num * BDRV_SECTOR_SIZE,
> -                                      nb_sectors * BDRV_SECTOR_SIZE, &count);
> +        ret = bdrv_is_allocated_above(bs, base, offset, bytes, &count);
>          if (ret < 0) {
>              return ret;
>          }
> 
> -        n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
> -        assert(n > 0);
> +        count = QEMU_ALIGN_UP(count, BDRV_SECTOR_SIZE);
>          if (ret == 1) {
> -            bdrv_set_dirty_bitmap(s->dirty_bitmap,
> -                                  sector_num * BDRV_SECTOR_SIZE,
> -                                  n * BDRV_SECTOR_SIZE);
> +            bdrv_set_dirty_bitmap(s->dirty_bitmap, offset, count);
>          }
> -        sector_num += n;
> +        offset += count;
>      }
>      return 0;
>  }
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 11/12] dirty-bitmap: Switch bdrv_set_dirty() to bytes

[Eric Blake]

Both callers already had bytes available, but were scaling to
sectors.  Move the scaling to internal code.  In the case of
bdrv_aligned_pwritev(), we are now passing the exact offset
rather than a rounded sector-aligned value, but that's okay
as long as dirty bitmap widens start/bytes to granularity
boundaries.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h | 2 +-
 block/dirty-bitmap.c      | 8 +++++---
 block/io.c                | 6 ++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 08063c1..0b737fd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -917,7 +917,7 @@ void blk_dev_eject_request(BlockBackend *blk, bool force);
 bool blk_dev_is_tray_open(BlockBackend *blk);
 bool blk_dev_is_medium_locked(BlockBackend *blk);

-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int64_t nr_sect);
+void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
 bool bdrv_requests_pending(BlockDriverState *bs);

 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8e7822c..ef165eb 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -478,15 +478,17 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
     hbitmap_deserialize_finish(bitmap->bitmap);
 }

-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
-                    int64_t nr_sectors)
+void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
 {
     BdrvDirtyBitmap *bitmap;
+    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
+
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         if (!bdrv_dirty_bitmap_enabled(bitmap)) {
             continue;
         }
-        hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+        hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
+                    end_sector - (offset >> BDRV_SECTOR_BITS));
     }
 }

diff --git a/block/io.c b/block/io.c
index 9218329..d22d35f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1328,7 +1328,6 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     bool waited;
     int ret;

-    int64_t start_sector = offset >> BDRV_SECTOR_BITS;
     int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
     uint64_t bytes_remaining = bytes;
     int max_transfer;
@@ -1407,7 +1406,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);

     ++bs->write_gen;
-    bdrv_set_dirty(bs, start_sector, end_sector - start_sector);
+    bdrv_set_dirty(bs, offset, bytes);

     if (bs->wr_highest_offset < offset + bytes) {
         bs->wr_highest_offset = offset + bytes;
@@ -2535,8 +2534,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
     ret = 0;
 out:
     ++bs->write_gen;
-    bdrv_set_dirty(bs, req.offset >> BDRV_SECTOR_BITS,
-                   req.bytes >> BDRV_SECTOR_BITS);
+    bdrv_set_dirty(bs, req.offset, req.bytes);
     tracked_request_end(&req);
     bdrv_dec_in_flight(bs);
     return ret;
-- 
2.9.3

Re: [Qemu-devel] [PATCH 11/12] dirty-bitmap: Switch bdrv_set_dirty() to bytes

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Both callers already had bytes available, but were scaling to
> sectors.  Move the scaling to internal code.  In the case of
> bdrv_aligned_pwritev(), we are now passing the exact offset
> rather than a rounded sector-aligned value, but that's okay
> as long as dirty bitmap widens start/bytes to granularity
> boundaries.

Yes, that shouldn't be a problem. Granularity math will make sure this
comes out in the wash.

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block_int.h | 2 +-
>  block/dirty-bitmap.c      | 8 +++++---
>  block/io.c                | 6 ++----
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 08063c1..0b737fd 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -917,7 +917,7 @@ void blk_dev_eject_request(BlockBackend *blk, bool force);
>  bool blk_dev_is_tray_open(BlockBackend *blk);
>  bool blk_dev_is_medium_locked(BlockBackend *blk);
> 
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int64_t nr_sect);
> +void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes);
>  bool bdrv_requests_pending(BlockDriverState *bs);
> 
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 8e7822c..ef165eb 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -478,15 +478,17 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
>      hbitmap_deserialize_finish(bitmap->bitmap);
>  }
> 
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> -                    int64_t nr_sectors)
> +void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
>  {
>      BdrvDirtyBitmap *bitmap;
> +    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
> +
>      QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>          if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>              continue;
>          }
> -        hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +        hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
> +                    end_sector - (offset >> BDRV_SECTOR_BITS));

Well, that's worse, but luckily you've got more patches. :)

>      }
>  }
> 
> diff --git a/block/io.c b/block/io.c
> index 9218329..d22d35f 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1328,7 +1328,6 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
>      bool waited;
>      int ret;
> 
> -    int64_t start_sector = offset >> BDRV_SECTOR_BITS;
>      int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
>      uint64_t bytes_remaining = bytes;
>      int max_transfer;
> @@ -1407,7 +1406,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
>      bdrv_debug_event(bs, BLKDBG_PWRITEV_DONE);
> 
>      ++bs->write_gen;
> -    bdrv_set_dirty(bs, start_sector, end_sector - start_sector);
> +    bdrv_set_dirty(bs, offset, bytes);
> 
>      if (bs->wr_highest_offset < offset + bytes) {
>          bs->wr_highest_offset = offset + bytes;
> @@ -2535,8 +2534,7 @@ int coroutine_fn bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset,
>      ret = 0;
>  out:
>      ++bs->write_gen;
> -    bdrv_set_dirty(bs, req.offset >> BDRV_SECTOR_BITS,
> -                   req.bytes >> BDRV_SECTOR_BITS);
> +    bdrv_set_dirty(bs, req.offset, req.bytes);
>      tracked_request_end(&req);
>      bdrv_dec_in_flight(bs);
>      return ret;
> 

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 12/12] dirty-bitmap: Convert internal hbitmap size/granularity

[Eric Blake]

Now that all callers are using byte-based interfaces, there's no
reason for our internal hbitmap to remain with sector-based
granularity.  It also simplifies our internal scaling, since we
already know that hbitmap widens requests out to granularity
boundaries.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/dirty-bitmap.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index ef165eb..26ca084 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -37,7 +37,7 @@
  *     or enabled. A frozen bitmap can only abdicate() or reclaim().
  */
 struct BdrvDirtyBitmap {
-    HBitmap *bitmap;            /* Dirty sector bitmap implementation */
+    HBitmap *bitmap;            /* Dirty bitmap implementation */
     HBitmap *meta;              /* Meta dirty bitmap */
     BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
     char *name;                 /* Optional non-empty unique ID */
@@ -93,12 +93,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
         return NULL;
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
-    /*
-     * TODO - let hbitmap track full granularity. For now, it is tracking
-     * only sector granularity, as a shortcut for our iterators.
-     */
-    bitmap->bitmap = hbitmap_alloc(bitmap_size >> BDRV_SECTOR_BITS,
-                                   ctz32(granularity) - BDRV_SECTOR_BITS);
+    bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
     bitmap->size = bitmap_size;
     bitmap->name = g_strdup(name);
     bitmap->disabled = false;
@@ -254,7 +249,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         assert(!bdrv_dirty_bitmap_frozen(bitmap));
         assert(!bitmap->active_iterators);
-        hbitmap_truncate(bitmap->bitmap, size >> BDRV_SECTOR_BITS);
+        hbitmap_truncate(bitmap->bitmap, size);
         bitmap->size = size;
     }
 }
@@ -336,7 +331,7 @@ bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
                     int64_t offset)
 {
     if (bitmap) {
-        return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
+        return hbitmap_get(bitmap->bitmap, offset);
     } else {
         return false;
     }
@@ -364,7 +359,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)

 uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
 {
-    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+    return 1U << hbitmap_granularity(bitmap->bitmap);
 }

 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
@@ -397,27 +392,21 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)

 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 {
-    return hbitmap_iter_next(&iter->hbi) * BDRV_SECTOR_SIZE;
+    return hbitmap_iter_next(&iter->hbi);
 }

 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t offset, int64_t bytes)
 {
-    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
-
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
-                end_sector - (offset >> BDRV_SECTOR_BITS));
+    hbitmap_set(bitmap->bitmap, offset, bytes);
 }

 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t offset, int64_t bytes)
 {
-    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
-
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
-                  end_sector - (offset >> BDRV_SECTOR_BITS));
+    hbitmap_reset(bitmap->bitmap, offset, bytes);
 }

 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
@@ -427,7 +416,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
         hbitmap_reset_all(bitmap->bitmap);
     } else {
         HBitmap *backup = bitmap->bitmap;
-        bitmap->bitmap = hbitmap_alloc(bitmap->size >> BDRV_SECTOR_BITS,
+        bitmap->bitmap = hbitmap_alloc(bitmap->size,
                                        hbitmap_granularity(backup));
         *out = backup;
     }
@@ -481,14 +470,12 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
 void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
 {
     BdrvDirtyBitmap *bitmap;
-    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);

     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         if (!bdrv_dirty_bitmap_enabled(bitmap)) {
             continue;
         }
-        hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
-                    end_sector - (offset >> BDRV_SECTOR_BITS));
+        hbitmap_set(bitmap->bitmap, offset, bytes);
     }
 }

@@ -497,12 +484,12 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
  */
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
 {
-    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
+    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset);
 }

 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
-    return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
+    return hbitmap_count(bitmap->bitmap);
 }

 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-- 
2.9.3

Re: [Qemu-devel] [PATCH 12/12] dirty-bitmap: Convert internal hbitmap size/granularity

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Now that all callers are using byte-based interfaces, there's no
> reason for our internal hbitmap to remain with sector-based
> granularity.  It also simplifies our internal scaling, since we
> already know that hbitmap widens requests out to granularity
> boundaries.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/dirty-bitmap.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index ef165eb..26ca084 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -37,7 +37,7 @@
>   *     or enabled. A frozen bitmap can only abdicate() or reclaim().
>   */
>  struct BdrvDirtyBitmap {
> -    HBitmap *bitmap;            /* Dirty sector bitmap implementation */
> +    HBitmap *bitmap;            /* Dirty bitmap implementation */
>      HBitmap *meta;              /* Meta dirty bitmap */
>      BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>      char *name;                 /* Optional non-empty unique ID */
> @@ -93,12 +93,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>          return NULL;
>      }
>      bitmap = g_new0(BdrvDirtyBitmap, 1);
> -    /*
> -     * TODO - let hbitmap track full granularity. For now, it is tracking
> -     * only sector granularity, as a shortcut for our iterators.
> -     */
> -    bitmap->bitmap = hbitmap_alloc(bitmap_size >> BDRV_SECTOR_BITS,
> -                                   ctz32(granularity) - BDRV_SECTOR_BITS);
> +    bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
>      bitmap->size = bitmap_size;
>      bitmap->name = g_strdup(name);
>      bitmap->disabled = false;
> @@ -254,7 +249,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>      QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>          assert(!bdrv_dirty_bitmap_frozen(bitmap));
>          assert(!bitmap->active_iterators);
> -        hbitmap_truncate(bitmap->bitmap, size >> BDRV_SECTOR_BITS);
> +        hbitmap_truncate(bitmap->bitmap, size);
>          bitmap->size = size;
>      }
>  }
> @@ -336,7 +331,7 @@ bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>                      int64_t offset)
>  {
>      if (bitmap) {
> -        return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
> +        return hbitmap_get(bitmap->bitmap, offset);
>      } else {
>          return false;
>      }
> @@ -364,7 +359,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> 
>  uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
>  {
> -    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> +    return 1U << hbitmap_granularity(bitmap->bitmap);
>  }
> 
>  BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
> @@ -397,27 +392,21 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
> 
>  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
>  {
> -    return hbitmap_iter_next(&iter->hbi) * BDRV_SECTOR_SIZE;
> +    return hbitmap_iter_next(&iter->hbi);
>  }
> 
>  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                             int64_t offset, int64_t bytes)
>  {
> -    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
> -
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> -    hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
> -                end_sector - (offset >> BDRV_SECTOR_BITS));
> +    hbitmap_set(bitmap->bitmap, offset, bytes);
>  }
> 
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                               int64_t offset, int64_t bytes)
>  {
> -    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
> -
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> -    hbitmap_reset(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
> -                  end_sector - (offset >> BDRV_SECTOR_BITS));
> +    hbitmap_reset(bitmap->bitmap, offset, bytes);
>  }
> 
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> @@ -427,7 +416,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>          hbitmap_reset_all(bitmap->bitmap);
>      } else {
>          HBitmap *backup = bitmap->bitmap;
> -        bitmap->bitmap = hbitmap_alloc(bitmap->size >> BDRV_SECTOR_BITS,
> +        bitmap->bitmap = hbitmap_alloc(bitmap->size,
>                                         hbitmap_granularity(backup));
>          *out = backup;
>      }
> @@ -481,14 +470,12 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
>  {
>      BdrvDirtyBitmap *bitmap;
> -    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
> 
>      QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>          if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>              continue;
>          }
> -        hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
> -                    end_sector - (offset >> BDRV_SECTOR_BITS));
> +        hbitmap_set(bitmap->bitmap, offset, bytes);
>      }
>  }
> 
> @@ -497,12 +484,12 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes)
>   */
>  void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
>  {
> -    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
> +    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset);
>  }
> 
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
> -    return hbitmap_count(bitmap->bitmap) << BDRV_SECTOR_BITS;
> +    return hbitmap_count(bitmap->bitmap);
>  }
> 
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
> 

Reviewed-by: John Snow <jsnow@redhat.com>