| Message ID | 20231228101232.372142-7-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Add full dirty bitmap support | expand |
On 12/28/23 11:12, Alexander Ivanov wrote: > For parallels images extensions we need to allocate host clusters > without any connection to BAT. Move host clusters allocation code to > parallels_allocate_host_clusters(). > > This function can be called not only from coroutines so all the > *_co_* functions were replaced by corresponding wrappers. > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> > --- > block/parallels.c | 128 ++++++++++++++++++++++++---------------------- > block/parallels.h | 3 ++ > 2 files changed, 71 insertions(+), 60 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index 13726fb3d5..658902ae51 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -268,58 +268,31 @@ static void parallels_free_used_bitmap(BlockDriverState *bs) > s->used_bmap = NULL; > } > > -static int64_t coroutine_fn GRAPH_RDLOCK > -allocate_clusters(BlockDriverState *bs, int64_t sector_num, > - int nb_sectors, int *pnum) > +int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs, > + int64_t *clusters) > { > - int ret = 0; > BDRVParallelsState *s = bs->opaque; > - int64_t i, pos, idx, to_allocate, first_free, host_off; > - > - pos = block_status(s, sector_num, nb_sectors, pnum); > - if (pos > 0) { > - return pos; > - } > - > - idx = sector_num / s->tracks; > - to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; > - > - /* > - * This function is called only by parallels_co_writev(), which will never > - * pass a sector_num at or beyond the end of the image (because the block > - * layer never passes such a sector_num to that function). Therefore, idx > - * is always below s->bat_size. > - * block_status() will limit *pnum so that sector_num + *pnum will not > - * exceed the image end. Therefore, idx + to_allocate cannot exceed > - * s->bat_size. > - * Note that s->bat_size is an unsigned int, therefore idx + to_allocate > - * will always fit into a uint32_t. > - */ > - assert(idx < s->bat_size && idx + to_allocate <= s->bat_size); > + int64_t first_free, next_used, host_off, prealloc_clusters; > + int64_t bytes, prealloc_bytes; > + uint32_t new_usedsize; > + int ret = 0; > > first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size); > if (first_free == s->used_bmap_size) { > - uint32_t new_usedsize; > - int64_t bytes = to_allocate * s->cluster_size; > - bytes += s->prealloc_size * BDRV_SECTOR_SIZE; > - > host_off = s->data_end * BDRV_SECTOR_SIZE; > + prealloc_clusters = *clusters + s->prealloc_size / s->tracks; > + bytes = *clusters * s->cluster_size; > + prealloc_bytes = prealloc_clusters * s->cluster_size; > > - /* > - * We require the expanded size to read back as zero. If the > - * user permitted truncation, we try that; but if it fails, we > - * force the safer-but-slower fallocate. > - */ This comment seems useful. I'd better keep it as is. For now (if we will not have a re-submission, I'll have the comment back in the code). If the code will be resubmitted, please add it back. > if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) { > - ret = bdrv_co_truncate(bs->file, host_off + bytes, > - false, PREALLOC_MODE_OFF, > - BDRV_REQ_ZERO_WRITE, NULL); > + ret = bdrv_truncate(bs->file, host_off + prealloc_bytes, false, > + PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL); > if (ret == -ENOTSUP) { > s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; > } > } > if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) { > - ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0); > + ret = bdrv_pwrite_zeroes(bs->file, host_off, prealloc_bytes, 0); > } > if (ret < 0) { > return ret; > @@ -329,15 +302,15 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num, > s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size, > new_usedsize); > s->used_bmap_size = new_usedsize; > + if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) { > + s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE; > + } > } else { > - int64_t next_used; > next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free); > > /* Not enough continuous clusters in the middle, adjust the size */ > - if (next_used - first_free < to_allocate) { > - to_allocate = next_used - first_free; > - *pnum = (idx + to_allocate) * s->tracks - sector_num; > - } > + *clusters = MIN(*clusters, next_used - first_free); > + bytes = *clusters * s->cluster_size; > > host_off = s->data_start * BDRV_SECTOR_SIZE; > host_off += first_free * s->cluster_size; > @@ -349,14 +322,59 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num, > */ > if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE && > host_off < s->data_end * BDRV_SECTOR_SIZE) { > - ret = bdrv_co_pwrite_zeroes(bs->file, host_off, > - s->cluster_size * to_allocate, 0); > + ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0); > if (ret < 0) { > return ret; > } > } > } > > + ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size, > + host_off, *clusters); > + if (ret < 0) { > + /* Image consistency is broken. Alarm! */ > + return ret; > + } > + > + return host_off; > +} > + > +static int64_t coroutine_fn GRAPH_RDLOCK > +allocate_clusters(BlockDriverState *bs, int64_t sector_num, > + int nb_sectors, int *pnum) > +{ > + int ret = 0; > + BDRVParallelsState *s = bs->opaque; > + int64_t i, pos, idx, to_allocate, host_off; > + > + pos = block_status(s, sector_num, nb_sectors, pnum); > + if (pos > 0) { > + return pos; > + } > + > + idx = sector_num / s->tracks; > + to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; > + > + /* > + * This function is called only by parallels_co_writev(), which will never > + * pass a sector_num at or beyond the end of the image (because the block > + * layer never passes such a sector_num to that function). Therefore, idx > + * is always below s->bat_size. > + * block_status() will limit *pnum so that sector_num + *pnum will not > + * exceed the image end. Therefore, idx + to_allocate cannot exceed > + * s->bat_size. > + * Note that s->bat_size is an unsigned int, therefore idx + to_allocate > + * will always fit into a uint32_t. > + */ > + assert(idx < s->bat_size && idx + to_allocate <= s->bat_size); > + > + host_off = parallels_allocate_host_clusters(bs, &to_allocate); > + if (host_off < 0) { > + return host_off; > + } > + > + *pnum = MIN(*pnum, (idx + to_allocate) * s->tracks - sector_num); > + > /* > * Try to read from backing to fill empty clusters > * FIXME: 1. previous write_zeroes may be redundant > @@ -373,33 +391,23 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num, > > ret = bdrv_co_pread(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE, > nb_cow_bytes, buf, 0); > - if (ret < 0) { > - qemu_vfree(buf); > - return ret; > + if (ret == 0) { > + ret = bdrv_co_pwrite(bs->file, host_off, nb_cow_bytes, buf, 0); > } > > - ret = bdrv_co_pwrite(bs->file, s->data_end * BDRV_SECTOR_SIZE, > - nb_cow_bytes, buf, 0); > qemu_vfree(buf); > if (ret < 0) { > + parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size, > + host_off, to_allocate); > return ret; > } > } > > - ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size, > - host_off, to_allocate); > - if (ret < 0) { > - /* Image consistency is broken. Alarm! */ > - return ret; > - } > for (i = 0; i < to_allocate; i++) { > parallels_set_bat_entry(s, idx + i, > host_off / BDRV_SECTOR_SIZE / s->off_multiplier); > host_off += s->cluster_size; > } > - if (host_off > s->data_end * BDRV_SECTOR_SIZE) { > - s->data_end = host_off / BDRV_SECTOR_SIZE; > - } > > return bat2sect(s, idx) + sector_num % s->tracks; > } > diff --git a/block/parallels.h b/block/parallels.h > index 02b857b4a4..493c89e976 100644 > --- a/block/parallels.h > +++ b/block/parallels.h > @@ -95,6 +95,9 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap, > int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap, > uint32_t bitmap_size, int64_t off, uint32_t count); > > +int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs, > + int64_t *clusters); > + > int GRAPH_RDLOCK > parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off, > Error **errp); There is a difference in between what we have had before this patch and after. On a error originally we have had data_end unchanged, now it points to a wrong location. May be this would be mitigated later, but I'd prefer to have data_end updated in mark_unused. That would make a lot of sense. Anyway, with data_end dropped at the end of the series, this would not worth efforts. Thus this is fine. With a note about comment, Reviewed-by: Denis V. Lunev <den@openvz.org>
diff --git a/block/parallels.c b/block/parallels.c index 13726fb3d5..658902ae51 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -268,58 +268,31 @@ static void parallels_free_used_bitmap(BlockDriverState *bs) s->used_bmap = NULL; } -static int64_t coroutine_fn GRAPH_RDLOCK -allocate_clusters(BlockDriverState *bs, int64_t sector_num, - int nb_sectors, int *pnum) +int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs, + int64_t *clusters) { - int ret = 0; BDRVParallelsState *s = bs->opaque; - int64_t i, pos, idx, to_allocate, first_free, host_off; - - pos = block_status(s, sector_num, nb_sectors, pnum); - if (pos > 0) { - return pos; - } - - idx = sector_num / s->tracks; - to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; - - /* - * This function is called only by parallels_co_writev(), which will never - * pass a sector_num at or beyond the end of the image (because the block - * layer never passes such a sector_num to that function). Therefore, idx - * is always below s->bat_size. - * block_status() will limit *pnum so that sector_num + *pnum will not - * exceed the image end. Therefore, idx + to_allocate cannot exceed - * s->bat_size. - * Note that s->bat_size is an unsigned int, therefore idx + to_allocate - * will always fit into a uint32_t. - */ - assert(idx < s->bat_size && idx + to_allocate <= s->bat_size); + int64_t first_free, next_used, host_off, prealloc_clusters; + int64_t bytes, prealloc_bytes; + uint32_t new_usedsize; + int ret = 0; first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size); if (first_free == s->used_bmap_size) { - uint32_t new_usedsize; - int64_t bytes = to_allocate * s->cluster_size; - bytes += s->prealloc_size * BDRV_SECTOR_SIZE; - host_off = s->data_end * BDRV_SECTOR_SIZE; + prealloc_clusters = *clusters + s->prealloc_size / s->tracks; + bytes = *clusters * s->cluster_size; + prealloc_bytes = prealloc_clusters * s->cluster_size; - /* - * We require the expanded size to read back as zero. If the - * user permitted truncation, we try that; but if it fails, we - * force the safer-but-slower fallocate. - */ if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) { - ret = bdrv_co_truncate(bs->file, host_off + bytes, - false, PREALLOC_MODE_OFF, - BDRV_REQ_ZERO_WRITE, NULL); + ret = bdrv_truncate(bs->file, host_off + prealloc_bytes, false, + PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL); if (ret == -ENOTSUP) { s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; } } if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) { - ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0); + ret = bdrv_pwrite_zeroes(bs->file, host_off, prealloc_bytes, 0); } if (ret < 0) { return ret; @@ -329,15 +302,15 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num, s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size, new_usedsize); s->used_bmap_size = new_usedsize; + if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) { + s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE; + } } else { - int64_t next_used; next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free); /* Not enough continuous clusters in the middle, adjust the size */ - if (next_used - first_free < to_allocate) { - to_allocate = next_used - first_free; - *pnum = (idx + to_allocate) * s->tracks - sector_num; - } + *clusters = MIN(*clusters, next_used - first_free); + bytes = *clusters * s->cluster_size; host_off = s->data_start * BDRV_SECTOR_SIZE; host_off += first_free * s->cluster_size; @@ -349,14 +322,59 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num, */ if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE && host_off < s->data_end * BDRV_SECTOR_SIZE) { - ret = bdrv_co_pwrite_zeroes(bs->file, host_off, - s->cluster_size * to_allocate, 0); + ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0); if (ret < 0) { return ret; } } } + ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size, + host_off, *clusters); + if (ret < 0) { + /* Image consistency is broken. Alarm! */ + return ret; + } + + return host_off; +} + +static int64_t coroutine_fn GRAPH_RDLOCK +allocate_clusters(BlockDriverState *bs, int64_t sector_num, + int nb_sectors, int *pnum) +{ + int ret = 0; + BDRVParallelsState *s = bs->opaque; + int64_t i, pos, idx, to_allocate, host_off; + + pos = block_status(s, sector_num, nb_sectors, pnum); + if (pos > 0) { + return pos; + } + + idx = sector_num / s->tracks; + to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx; + + /* + * This function is called only by parallels_co_writev(), which will never + * pass a sector_num at or beyond the end of the image (because the block + * layer never passes such a sector_num to that function). Therefore, idx + * is always below s->bat_size. + * block_status() will limit *pnum so that sector_num + *pnum will not + * exceed the image end. Therefore, idx + to_allocate cannot exceed + * s->bat_size. + * Note that s->bat_size is an unsigned int, therefore idx + to_allocate + * will always fit into a uint32_t. + */ + assert(idx < s->bat_size && idx + to_allocate <= s->bat_size); + + host_off = parallels_allocate_host_clusters(bs, &to_allocate); + if (host_off < 0) { + return host_off; + } + + *pnum = MIN(*pnum, (idx + to_allocate) * s->tracks - sector_num); + /* * Try to read from backing to fill empty clusters * FIXME: 1. previous write_zeroes may be redundant @@ -373,33 +391,23 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num, ret = bdrv_co_pread(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE, nb_cow_bytes, buf, 0); - if (ret < 0) { - qemu_vfree(buf); - return ret; + if (ret == 0) { + ret = bdrv_co_pwrite(bs->file, host_off, nb_cow_bytes, buf, 0); } - ret = bdrv_co_pwrite(bs->file, s->data_end * BDRV_SECTOR_SIZE, - nb_cow_bytes, buf, 0); qemu_vfree(buf); if (ret < 0) { + parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size, + host_off, to_allocate); return ret; } } - ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size, - host_off, to_allocate); - if (ret < 0) { - /* Image consistency is broken. Alarm! */ - return ret; - } for (i = 0; i < to_allocate; i++) { parallels_set_bat_entry(s, idx + i, host_off / BDRV_SECTOR_SIZE / s->off_multiplier); host_off += s->cluster_size; } - if (host_off > s->data_end * BDRV_SECTOR_SIZE) { - s->data_end = host_off / BDRV_SECTOR_SIZE; - } return bat2sect(s, idx) + sector_num % s->tracks; } diff --git a/block/parallels.h b/block/parallels.h index 02b857b4a4..493c89e976 100644 --- a/block/parallels.h +++ b/block/parallels.h @@ -95,6 +95,9 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap, int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap, uint32_t bitmap_size, int64_t off, uint32_t count); +int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs, + int64_t *clusters); + int GRAPH_RDLOCK parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off, Error **errp);
For parallels images extensions we need to allocate host clusters without any connection to BAT. Move host clusters allocation code to parallels_allocate_host_clusters(). This function can be called not only from coroutines so all the *_co_* functions were replaced by corresponding wrappers. Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com> --- block/parallels.c | 128 ++++++++++++++++++++++++---------------------- block/parallels.h | 3 ++ 2 files changed, 71 insertions(+), 60 deletions(-)