Message ID | 20171012185916.22776-17-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Series | add byte-based block_status driver callbacks | expand |
12.10.2017 21:59, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. Update the vdi driver accordingly. Note that the > TODO is already covered (the block layer guarantees bounds of its > requests), and that we can remove the now-unused s->block_sectors. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v4: rebase to interface tweak > v3: no change > v2: rebase to mapping flag > --- > block/vdi.c | 33 +++++++++++++-------------------- > 1 file changed, 13 insertions(+), 20 deletions(-) > > diff --git a/block/vdi.c b/block/vdi.c > index 6f83221ddc..b30886823d 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -171,8 +171,6 @@ typedef struct { > uint32_t *bmap; > /* Size of block (bytes). */ > uint32_t block_size; > - /* Size of block (sectors). */ > - uint32_t block_sectors; > /* First sector of block map. */ > uint32_t bmap_sector; > /* VDI header (converted to host endianness). */ > @@ -462,7 +460,6 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > bs->total_sectors = header.disk_size / SECTOR_SIZE; > > s->block_size = header.block_size; > - s->block_sectors = header.block_size / SECTOR_SIZE; > s->bmap_sector = header.offset_bmap / SECTOR_SIZE; > s->header = header; > > @@ -508,33 +505,29 @@ static int vdi_reopen_prepare(BDRVReopenState *state, > return 0; > } > > -static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) > +static int coroutine_fn vdi_co_block_status(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, int64_t bytes, > + int64_t *pnum, int64_t *map, > + BlockDriverState **file) > { > - /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */ > BDRVVdiState *s = (BDRVVdiState *)bs->opaque; > - size_t bmap_index = sector_num / s->block_sectors; > - size_t sector_in_block = sector_num % s->block_sectors; > - int n_sectors = s->block_sectors - sector_in_block; > + size_t bmap_index = offset / s->block_size; > + size_t index_in_block = offset % s->block_size; > uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]); > - uint64_t offset; > int result; > > - logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, pnum); > - if (n_sectors > nb_sectors) { > - n_sectors = nb_sectors; > - } > - *pnum = n_sectors; > + logout("%p, %" PRId64 ", %" PRId64 ", %p\n", bs, offset, bytes, pnum); may be not bad to update message, as you changed numbers, like logout("%p, offset %" PRId64 ", bytes %" PRId64 ", %p\n", bs, offset, bytes, pnum); but for me it is ok as is too. > + *pnum = MIN(s->block_size, bytes); looks like it should be MIN(s->block_size - index_in_block, bytes); with that: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > result = VDI_IS_ALLOCATED(bmap_entry); > if (!result) { > return 0; > } > > - offset = s->header.offset_data + > - (uint64_t)bmap_entry * s->block_size + > - sector_in_block * SECTOR_SIZE; > + *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size + > + index_in_block; > *file = bs->file->bs; > - return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset; > + return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > } > > static int coroutine_fn > @@ -902,7 +895,7 @@ static BlockDriver bdrv_vdi = { > .bdrv_child_perm = bdrv_format_default_perms, > .bdrv_create = vdi_create, > .bdrv_has_zero_init = bdrv_has_zero_init_1, > - .bdrv_co_get_block_status = vdi_co_get_block_status, > + .bdrv_co_block_status = vdi_co_block_status, > .bdrv_make_empty = vdi_make_empty, > > .bdrv_co_preadv = vdi_co_preadv,
On 11/30/2017 05:26 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.10.2017 21:59, Eric Blake wrote: >> We are gradually moving away from sector-based interfaces, towards >> byte-based. Update the vdi driver accordingly. Note that the >> TODO is already covered (the block layer guarantees bounds of its >> requests), and that we can remove the now-unused s->block_sectors. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> BDRVVdiState *s = (BDRVVdiState *)bs->opaque; >> - size_t bmap_index = sector_num / s->block_sectors; >> - size_t sector_in_block = sector_num % s->block_sectors; >> - int n_sectors = s->block_sectors - sector_in_block; >> + size_t bmap_index = offset / s->block_size; >> + size_t index_in_block = offset % s->block_size; >> uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]); >> - uint64_t offset; >> int result; >> >> - logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, >> pnum); >> - if (n_sectors > nb_sectors) { >> - n_sectors = nb_sectors; >> - } >> - *pnum = n_sectors; >> + logout("%p, %" PRId64 ", %" PRId64 ", %p\n", bs, offset, bytes, >> pnum); > > may be not bad to update message, as you changed numbers, like > > logout("%p, offset %" PRId64 ", bytes %" PRId64 ", %p\n", bs, offset, > bytes, pnum); > > but for me it is ok as is too. Debugging messages can be looked up in context when you enable debugging. I'm not bothering to change that. > >> + *pnum = MIN(s->block_size, bytes); > > looks like it should be MIN(s->block_size - index_in_block, bytes); Oh, good catch. You are right. > > with that: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Thanks again for a careful review throughout this series.
diff --git a/block/vdi.c b/block/vdi.c index 6f83221ddc..b30886823d 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -171,8 +171,6 @@ typedef struct { uint32_t *bmap; /* Size of block (bytes). */ uint32_t block_size; - /* Size of block (sectors). */ - uint32_t block_sectors; /* First sector of block map. */ uint32_t bmap_sector; /* VDI header (converted to host endianness). */ @@ -462,7 +460,6 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, bs->total_sectors = header.disk_size / SECTOR_SIZE; s->block_size = header.block_size; - s->block_sectors = header.block_size / SECTOR_SIZE; s->bmap_sector = header.offset_bmap / SECTOR_SIZE; s->header = header; @@ -508,33 +505,29 @@ static int vdi_reopen_prepare(BDRVReopenState *state, return 0; } -static int64_t coroutine_fn vdi_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) +static int coroutine_fn vdi_co_block_status(BlockDriverState *bs, + bool want_zero, + int64_t offset, int64_t bytes, + int64_t *pnum, int64_t *map, + BlockDriverState **file) { - /* TODO: Check for too large sector_num (in bdrv_is_allocated or here). */ BDRVVdiState *s = (BDRVVdiState *)bs->opaque; - size_t bmap_index = sector_num / s->block_sectors; - size_t sector_in_block = sector_num % s->block_sectors; - int n_sectors = s->block_sectors - sector_in_block; + size_t bmap_index = offset / s->block_size; + size_t index_in_block = offset % s->block_size; uint32_t bmap_entry = le32_to_cpu(s->bmap[bmap_index]); - uint64_t offset; int result; - logout("%p, %" PRId64 ", %d, %p\n", bs, sector_num, nb_sectors, pnum); - if (n_sectors > nb_sectors) { - n_sectors = nb_sectors; - } - *pnum = n_sectors; + logout("%p, %" PRId64 ", %" PRId64 ", %p\n", bs, offset, bytes, pnum); + *pnum = MIN(s->block_size, bytes); result = VDI_IS_ALLOCATED(bmap_entry); if (!result) { return 0; } - offset = s->header.offset_data + - (uint64_t)bmap_entry * s->block_size + - sector_in_block * SECTOR_SIZE; + *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size + + index_in_block; *file = bs->file->bs; - return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | offset; + return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; } static int coroutine_fn @@ -902,7 +895,7 @@ static BlockDriver bdrv_vdi = { .bdrv_child_perm = bdrv_format_default_perms, .bdrv_create = vdi_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_co_get_block_status = vdi_co_get_block_status, + .bdrv_co_block_status = vdi_co_block_status, .bdrv_make_empty = vdi_make_empty, .bdrv_co_preadv = vdi_co_preadv,
We are gradually moving away from sector-based interfaces, towards byte-based. Update the vdi driver accordingly. Note that the TODO is already covered (the block layer guarantees bounds of its requests), and that we can remove the now-unused s->block_sectors. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: rebase to interface tweak v3: no change v2: rebase to mapping flag --- block/vdi.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)