| Message ID | 20171207203036.14993-19-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | add byte-based block_status driver callbacks | expand |
07.12.2017 23:30, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. Update the vpc driver accordingly. Drop the now-unused > get_sector_offset(). > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir] > v4: rebase to interface tweak > v3: rebase to master > v2: drop get_sector_offset() [Kevin], rebase to mapping flag > --- > block/vpc.c | 43 ++++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 1576d7b595..0000b5b9b4 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -705,53 +705,54 @@ fail: > return ret; > } > > -static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) > +static int coroutine_fn vpc_co_block_status(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, int64_t bytes, > + int64_t *pnum, int64_t *map, > + BlockDriverState **file) > { > BDRVVPCState *s = bs->opaque; > VHDFooter *footer = (VHDFooter*) s->footer_buf; > - int64_t start, offset; > + int64_t image_offset; image_offset is used instead of both old start and offset vars > bool allocated; > - int64_t ret; > + int ret; > int n; > > if (be32_to_cpu(footer->type) == VHD_FIXED) { > - *pnum = nb_sectors; > + *pnum = bytes; > + *map = offset; > *file = bs->file->bs; > - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > - (sector_num << BDRV_SECTOR_BITS); > + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > } > > qemu_co_mutex_lock(&s->lock); > > - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, NULL); > - start = offset; ... instead of both.. > - allocated = (offset != -1); > + image_offset = get_image_offset(bs, offset, false, NULL); > + allocated = (image_offset != -1); > *pnum = 0; > ret = 0; > > do { > /* All sectors in a block are contiguous (without using the bitmap) */ > - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE) > - - sector_num; > - n = MIN(n, nb_sectors); > + n = ROUND_UP(offset + 1, s->block_size) - offset; > + n = MIN(n, bytes); > > *pnum += n; > - sector_num += n; > - nb_sectors -= n; > + offset += n; > + bytes -= n; > /* *pnum can't be greater than one block for allocated > * sectors since there is always a bitmap in between. */ > if (allocated) { > *file = bs->file->bs; > - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > + *map = image_offset; image_offset still unchanged as we can go here only on first loop iteration, so it's ok to use it instead of start. > + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > break; > } > - if (nb_sectors == 0) { > + if (bytes == 0) { > break; > } > - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, > - NULL); > - } while (offset == -1); > + image_offset = get_image_offset(bs, offset, false, NULL); > + } while (image_offset == -1); > > qemu_co_mutex_unlock(&s->lock); > return ret; > @@ -1097,7 +1098,7 @@ static BlockDriver bdrv_vpc = { > > .bdrv_co_preadv = vpc_co_preadv, > .bdrv_co_pwritev = vpc_co_pwritev, > - .bdrv_co_get_block_status = vpc_co_get_block_status, > + .bdrv_co_block_status = vpc_co_block_status, > > .bdrv_get_info = vpc_get_info, > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On Thu, 12/07 14:30, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. Update the vpc driver accordingly. Drop the now-unused > get_sector_offset(). No get_sector_offset change in the patch any more, it was removed by 778b087e513ea6fdc525c5a194ff7c9b8d3f53cb. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir] > v4: rebase to interface tweak > v3: rebase to master > v2: drop get_sector_offset() [Kevin], rebase to mapping flag > --- > block/vpc.c | 43 ++++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 1576d7b595..0000b5b9b4 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -705,53 +705,54 @@ fail: > return ret; > } > > -static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) > +static int coroutine_fn vpc_co_block_status(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, int64_t bytes, > + int64_t *pnum, int64_t *map, > + BlockDriverState **file) > { > BDRVVPCState *s = bs->opaque; > VHDFooter *footer = (VHDFooter*) s->footer_buf; > - int64_t start, offset; > + int64_t image_offset; > bool allocated; > - int64_t ret; > + int ret; > int n; > > if (be32_to_cpu(footer->type) == VHD_FIXED) { > - *pnum = nb_sectors; > + *pnum = bytes; > + *map = offset; > *file = bs->file->bs; > - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > - (sector_num << BDRV_SECTOR_BITS); > + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; > } > > qemu_co_mutex_lock(&s->lock); > > - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, NULL); > - start = offset; > - allocated = (offset != -1); > + image_offset = get_image_offset(bs, offset, false, NULL); > + allocated = (image_offset != -1); > *pnum = 0; > ret = 0; > > do { > /* All sectors in a block are contiguous (without using the bitmap) */ > - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE) > - - sector_num; > - n = MIN(n, nb_sectors); > + n = ROUND_UP(offset + 1, s->block_size) - offset; > + n = MIN(n, bytes); Should 'n' be updated to int64_t to match the types of offset and bytes? > > *pnum += n; > - sector_num += n; > - nb_sectors -= n; > + offset += n; > + bytes -= n; > /* *pnum can't be greater than one block for allocated > * sectors since there is always a bitmap in between. */ > if (allocated) { > *file = bs->file->bs; > - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > + *map = image_offset; > + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > break; > } > - if (nb_sectors == 0) { > + if (bytes == 0) { > break; > } > - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, > - NULL); > - } while (offset == -1); > + image_offset = get_image_offset(bs, offset, false, NULL); > + } while (image_offset == -1); > > qemu_co_mutex_unlock(&s->lock); > return ret;
On 12/28/2017 08:55 PM, Fam Zheng wrote: > On Thu, 12/07 14:30, Eric Blake wrote: >> We are gradually moving away from sector-based interfaces, towards >> byte-based. Update the vpc driver accordingly. Drop the now-unused >> get_sector_offset(). > > No get_sector_offset change in the patch any more, it was removed by > 778b087e513ea6fdc525c5a194ff7c9b8d3f53cb. Will tweak the commit message if I have reason to spin v7 (at least 9/20 had a real bug, which is probably easier for me to respin than to ask the maintainer to adjust). > >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> - int64_t start, offset; >> + int64_t image_offset; >> bool allocated; >> - int64_t ret; >> + int ret; >> int n; >> >> do { >> /* All sectors in a block are contiguous (without using the bitmap) */ >> - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE) >> - - sector_num; >> - n = MIN(n, nb_sectors); >> + n = ROUND_UP(offset + 1, s->block_size) - offset; >> + n = MIN(n, bytes); > > Should 'n' be updated to int64_t to match the types of offset and bytes? s->block_size is uint32_t, and a power of 2; therefore, rounding 'offset + 1' up to block size, then subtracting offset, can't exceed 32 bits. But that's tricky to audit for; I'm not opposed to changing the type of 'n' to 64-bits if you think that is easier to read.
diff --git a/block/vpc.c b/block/vpc.c index 1576d7b595..0000b5b9b4 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -705,53 +705,54 @@ fail: return ret; } -static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) +static int coroutine_fn vpc_co_block_status(BlockDriverState *bs, + bool want_zero, + int64_t offset, int64_t bytes, + int64_t *pnum, int64_t *map, + BlockDriverState **file) { BDRVVPCState *s = bs->opaque; VHDFooter *footer = (VHDFooter*) s->footer_buf; - int64_t start, offset; + int64_t image_offset; bool allocated; - int64_t ret; + int ret; int n; if (be32_to_cpu(footer->type) == VHD_FIXED) { - *pnum = nb_sectors; + *pnum = bytes; + *map = offset; *file = bs->file->bs; - return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | - (sector_num << BDRV_SECTOR_BITS); + return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID; } qemu_co_mutex_lock(&s->lock); - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, NULL); - start = offset; - allocated = (offset != -1); + image_offset = get_image_offset(bs, offset, false, NULL); + allocated = (image_offset != -1); *pnum = 0; ret = 0; do { /* All sectors in a block are contiguous (without using the bitmap) */ - n = ROUND_UP(sector_num + 1, s->block_size / BDRV_SECTOR_SIZE) - - sector_num; - n = MIN(n, nb_sectors); + n = ROUND_UP(offset + 1, s->block_size) - offset; + n = MIN(n, bytes); *pnum += n; - sector_num += n; - nb_sectors -= n; + offset += n; + bytes -= n; /* *pnum can't be greater than one block for allocated * sectors since there is always a bitmap in between. */ if (allocated) { *file = bs->file->bs; - ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; + *map = image_offset; + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; break; } - if (nb_sectors == 0) { + if (bytes == 0) { break; } - offset = get_image_offset(bs, sector_num << BDRV_SECTOR_BITS, false, - NULL); - } while (offset == -1); + image_offset = get_image_offset(bs, offset, false, NULL); + } while (image_offset == -1); qemu_co_mutex_unlock(&s->lock); return ret; @@ -1097,7 +1098,7 @@ static BlockDriver bdrv_vpc = { .bdrv_co_preadv = vpc_co_preadv, .bdrv_co_pwritev = vpc_co_pwritev, - .bdrv_co_get_block_status = vpc_co_get_block_status, + .bdrv_co_block_status = vpc_co_block_status, .bdrv_get_info = vpc_get_info,
We are gradually moving away from sector-based interfaces, towards byte-based. Update the vpc driver accordingly. Drop the now-unused get_sector_offset(). Signed-off-by: Eric Blake <eblake@redhat.com> --- v5: fix incorrect rounding in 'map' and bad loop condition [Vladimir] v4: rebase to interface tweak v3: rebase to master v2: drop get_sector_offset() [Kevin], rebase to mapping flag --- block/vpc.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)