Message ID | 20190930151502.7829-10-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | Further bitmaps improvements | expand |
On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote: > Use bdrv_dirty_bitmap_next_dirty_area for bitmap_to_extents. Since > bdrv_dirty_bitmap_next_dirty_area is very accurate in its interface, > we'll never exceed requested region with last chunk. So, we don't need > dont_fragment, and bitmap_to_extents() interface becomes clean enough > to not require any comment. Comments are a useful style, even if functions seem trivial. When req_one is in effect, we have to stop at the requested length. When req_one is not in effect, the NBD spec does not require us to stop until the next change in extent status, but also does not force us to continue past. So this change is fine from the protocol standpoint. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > nbd/server.c | 58 ++++++++++++++++------------------------------------ > 1 file changed, 18 insertions(+), 40 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index cc63d8ad21..edbdb1b6b6 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -2023,57 +2023,35 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle, > return nbd_co_send_extents(client, handle, ea, last, context_id, errp); > } > > -/* > - * Populate @ea from a dirty bitmap. Unless @dont_fragment, the > - * final extent may exceed the original @length. > - */ I would have kept the first sentence, and dropped only the second. > static void bitmap_to_extents(BdrvDirtyBitmap *bitmap, > uint64_t offset, uint64_t length, > - NBDExtentArray *ea, bool dont_fragment) > + NBDExtentArray *es) > { > - uint64_t begin = offset, end = offset; > - uint64_t overall_end = offset + length; > - BdrvDirtyBitmapIter *it; > - bool dirty; > + int64_t start, dirty_start, dirty_count; > + int64_t end = offset + length; > + bool full = false; > > bdrv_dirty_bitmap_lock(bitmap); > + for (start = offset; > + bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX, > + &dirty_start, &dirty_count); > + start = dirty_start + dirty_count) > + { > + if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) || > + (nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0)) As long as bdrv_dirty_bitmap_next_dirty_area works correctly, this should work regardless of whether start is dirty or clean (if dirty, the first call will be a 0-length no-op). > + { > + full = true; > break; > } > - begin = end; > - dirty = next_dirty; > } > > - bdrv_dirty_iter_free(it); > + if (!full) { > + /* last non dirty extent */ > + nbd_extent_array_add(es, end - start, 0); > + } Losing the possibility of reporting beyond the end of the original request (when req_one is not in force) is not fatal (it might make some clients less efficient when walking the entire disk, but qemu as a client isn't currently taking advantage of NBD's permission to return extra length). Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/nbd/server.c b/nbd/server.c index cc63d8ad21..edbdb1b6b6 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2023,57 +2023,35 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle, return nbd_co_send_extents(client, handle, ea, last, context_id, errp); } -/* - * Populate @ea from a dirty bitmap. Unless @dont_fragment, the - * final extent may exceed the original @length. - */ static void bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t length, - NBDExtentArray *ea, bool dont_fragment) + NBDExtentArray *es) { - uint64_t begin = offset, end = offset; - uint64_t overall_end = offset + length; - BdrvDirtyBitmapIter *it; - bool dirty; + int64_t start, dirty_start, dirty_count; + int64_t end = offset + length; + bool full = false; bdrv_dirty_bitmap_lock(bitmap); - it = bdrv_dirty_iter_new(bitmap); - dirty = bdrv_dirty_bitmap_get_locked(bitmap, offset); - - while (begin < overall_end) { - bool next_dirty = !dirty; - - if (dirty) { - end = bdrv_dirty_bitmap_next_zero(bitmap, begin, INT64_MAX); - } else { - bdrv_set_dirty_iter(it, begin); - end = bdrv_dirty_iter_next(it); - } - if (end == -1 || end - begin > UINT32_MAX) { - /* Cap to an aligned value < 4G beyond begin. */ - end = MIN(bdrv_dirty_bitmap_size(bitmap), - begin + UINT32_MAX + 1 - - bdrv_dirty_bitmap_granularity(bitmap)); - next_dirty = dirty; - } - if (dont_fragment && end > overall_end) { - end = overall_end; - } - - if (nbd_extent_array_add(ea, end - begin, - dirty ? NBD_STATE_DIRTY : 0) < 0) { + for (start = offset; + bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX, + &dirty_start, &dirty_count); + start = dirty_start + dirty_count) + { + if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) || + (nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0)) + { + full = true; break; } - begin = end; - dirty = next_dirty; } - bdrv_dirty_iter_free(it); + if (!full) { + /* last non dirty extent */ + nbd_extent_array_add(es, end - start, 0); + } bdrv_dirty_bitmap_unlock(bitmap); - - assert(offset < end); } static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle, @@ -2084,7 +2062,7 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle, unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS; g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents); - bitmap_to_extents(bitmap, offset, length, ea, dont_fragment); + bitmap_to_extents(bitmap, offset, length, ea); return nbd_co_send_extents(client, handle, ea, last, context_id, errp); }
Use bdrv_dirty_bitmap_next_dirty_area for bitmap_to_extents. Since bdrv_dirty_bitmap_next_dirty_area is very accurate in its interface, we'll never exceed requested region with last chunk. So, we don't need dont_fragment, and bitmap_to_extents() interface becomes clean enough to not require any comment. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- nbd/server.c | 58 ++++++++++++++++------------------------------------ 1 file changed, 18 insertions(+), 40 deletions(-)