Re: [Qemu-block] [PULL 5/7] nbd/server: implement dirty bitmap export

[Eric Blake]

On 06/21/2018 05:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.06.2018 06:19, Eric Blake wrote:
> > From: Vladimir Sementsov-Ogievskiy <address@hidden>
> >
> > Handle a new NBD meta namespace: "qemu", and corresponding queries:
> > "qemu:dirty-bitmap:<export bitmap name>".
> >
> > With the new metadata context negotiated, BLOCK_STATUS query will reply
> > with dirty-bitmap data, converted to extents. The new public function
> > nbd_export_bitmap selects which bitmap to export. For now, only one 
> > bitmap
> > may be exported.

> > +/*
> > + * Populate @extents from a dirty bitmap. Unless @dont_fragment, the
> > + * final extent may exceed the original @length. Store in @length the
> > + * byte length encoded (which may be smaller or larger than the
> > + * original), and return the number of extents used.
> > + */
> > +static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, 
> > uint64_t offset,
> > +                                      uint64_t *length, NBDExtent 
> > *extents,
> > +                                      unsigned int nb_extents,
> > +                                      bool dont_fragment)
> > +{
> > +    uint64_t begin = offset, end;
> > +    uint64_t overall_end = offset + *length;
> > +    unsigned int i = 0;
> > +    BdrvDirtyBitmapIter *it;
> > +    bool dirty;
> > +
> > +    bdrv_dirty_bitmap_lock(bitmap);
> > +
> > +    it = bdrv_dirty_iter_new(bitmap);
> > +    dirty = bdrv_get_dirty_locked(NULL, bitmap, offset);
> > +
> > +    while (begin < overall_end && i < nb_extents) {
> > +        if (dirty) {
> > +            end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
> > +        } else {
> > +            bdrv_set_dirty_iter(it, begin);
> > +            end = bdrv_dirty_iter_next(it);
> > +        }
>
> after tjos begin may be zero, and end near to the bitmap end,
> so end != -1 and end-begin > 4G (and dont_fragment may be false)

Oh, good catch. Such a situation will still need to be clamped to under 
4G (it's only possible on the last extent reported to the user, since in 
that case, end > overall_end given that overall_end is at most a 32-bit 
request beyond the original begin), so...

> > +        if (end == -1) {

...we just need to tweak this to 'if (end == -1 || end - begin > UINT32_MAX)

> > +            /* Cap to an aligned value < 4G beyond begin. */
> > +            end = MIN(bdrv_dirty_bitmap_size(bitmap),
> > +                      begin + 0x100000000ULL -
> > +                      bdrv_dirty_bitmap_granularity(bitmap));

Also, it might be more legible if I spell this as UINT32_MAX + 1 instead 
of open-coding the 33-bit constant.

> > +        }
> > +        if (dont_fragment && end > overall_end) {
> > +            end = overall_end;
> > +        }


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