| Message ID | 20171207203036.14993-10-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 parallels driver accordingly. Note that > the internal function block_status() is still sector-based, because > it is still in use by other sector-based functions; but that's okay > because request_alignment is 512 as a result of those functions. > For now, no optimizations are added based on the mapping hint. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v5: fix pnum when return is 0 > v4: rebase to interface tweak, R-b dropped > v3: no change > v2: rebase to mapping parameter; it is ignored, so R-b kept > --- > block/parallels.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index 9545761f49..9a3d3748ae 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -280,23 +280,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs) > } > > > -static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, > - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) > +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs, > + bool want_zero, > + int64_t offset, > + int64_t bytes, > + int64_t *pnum, > + int64_t *map, > + BlockDriverState **file) > { > BDRVParallelsState *s = bs->opaque; > - int64_t offset; > + int count; > > + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); > qemu_co_mutex_lock(&s->lock); > - offset = block_status(s, sector_num, nb_sectors, pnum); > + offset = block_status(s, offset >> BDRV_SECTOR_BITS, > + bytes >> BDRV_SECTOR_BITS, &count); > qemu_co_mutex_unlock(&s->lock); > > + *pnum = count * BDRV_SECTOR_SIZE; > if (offset < 0) { > return 0; > } > > + *map = offset; oh, didn't notice previous time :(, should be *map = offset * BDRV_SECTOR_SIZE (also, if you already use ">> BDRV_SECTOR_BITS" in this function, would not it be more consistent to use "<< BDRV_SECTOR_BITS" for sector-to-byte conversion?) with that fixed (with "<<" or "*", up to you), Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > *file = bs->file->bs; > - return (offset << BDRV_SECTOR_BITS) | > - BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > + return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; > } > > static coroutine_fn int parallels_co_writev(BlockDriverState *bs, > @@ -793,7 +801,7 @@ static BlockDriver bdrv_parallels = { > .bdrv_open = parallels_open, > .bdrv_close = parallels_close, > .bdrv_child_perm = bdrv_format_default_perms, > - .bdrv_co_get_block_status = parallels_co_get_block_status, > + .bdrv_co_block_status = parallels_co_block_status, > .bdrv_has_zero_init = bdrv_has_zero_init_1, > .bdrv_co_flush_to_os = parallels_co_flush_to_os, > .bdrv_co_readv = parallels_co_readv,
On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote: > 07.12.2017 23:30, Eric Blake wrote: >> We are gradually moving away from sector-based interfaces, towards >> byte-based. Update the parallels driver accordingly. Note that >> the internal function block_status() is still sector-based, because >> it is still in use by other sector-based functions; but that's okay >> because request_alignment is 512 as a result of those functions. >> For now, no optimizations are added based on the mapping hint. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> { >> BDRVParallelsState *s = bs->opaque; >> - int64_t offset; >> + int count; >> >> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); >> qemu_co_mutex_lock(&s->lock); >> - offset = block_status(s, sector_num, nb_sectors, pnum); >> + offset = block_status(s, offset >> BDRV_SECTOR_BITS, >> + bytes >> BDRV_SECTOR_BITS, &count); >> qemu_co_mutex_unlock(&s->lock); >> >> + *pnum = count * BDRV_SECTOR_SIZE; >> if (offset < 0) { >> return 0; >> } >> >> + *map = offset; > > oh, didn't notice previous time :(, should be > > *map = offset * BDRV_SECTOR_SIZE > Oh, right. > (also, if you already use ">> BDRV_SECTOR_BITS" in this function, > would not it be more consistent to use "<< BDRV_SECTOR_BITS" > for sector-to-byte conversion?) > > with that fixed (with "<<" or "*", up to you), '>> BDRV_SECTOR_BITS' always works. You could also write '/ BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing by a power of 2, and use the shift instruction under the hood), but I find that a bit harder to reason about. On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size output as the input; if the left side is 32 bits, it risks overflowing. But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've learned (from past mistakes in other byte-conversion series) that the multiply form is less likely to introduce unintended truncation bugs. > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >
09.12.2017 19:39, Eric Blake wrote: > On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote: >> 07.12.2017 23:30, Eric Blake wrote: >>> We are gradually moving away from sector-based interfaces, towards >>> byte-based. Update the parallels driver accordingly. Note that >>> the internal function block_status() is still sector-based, because >>> it is still in use by other sector-based functions; but that's okay >>> because request_alignment is 512 as a result of those functions. >>> For now, no optimizations are added based on the mapping hint. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> { >>> BDRVParallelsState *s = bs->opaque; >>> - int64_t offset; >>> + int count; >>> >>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); >>> qemu_co_mutex_lock(&s->lock); >>> - offset = block_status(s, sector_num, nb_sectors, pnum); >>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS, >>> + bytes >> BDRV_SECTOR_BITS, &count); >>> qemu_co_mutex_unlock(&s->lock); >>> >>> + *pnum = count * BDRV_SECTOR_SIZE; >>> if (offset < 0) { >>> return 0; >>> } >>> >>> + *map = offset; >> oh, didn't notice previous time :(, should be >> >> *map = offset * BDRV_SECTOR_SIZE >> > Oh, right. > >> (also, if you already use ">> BDRV_SECTOR_BITS" in this function, >> would not it be more consistent to use "<< BDRV_SECTOR_BITS" >> for sector-to-byte conversion?) >> >> with that fixed (with "<<" or "*", up to you), > '>> BDRV_SECTOR_BITS' always works. You could also write '/ > BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing > by a power of 2, and use the shift instruction under the hood), but I > find that a bit harder to reason about. > > On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size > output as the input; if the left side is 32 bits, it risks overflowing. > But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've > learned (from past mistakes in other byte-conversion series) that the > multiply form is less likely to introduce unintended truncation bugs. hmm, interesting observation, thank you > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>
09.12.2017 19:39, Eric Blake wrote: > On 12/09/2017 06:31 AM, Vladimir Sementsov-Ogievskiy wrote: >> 07.12.2017 23:30, Eric Blake wrote: >>> We are gradually moving away from sector-based interfaces, towards >>> byte-based. Update the parallels driver accordingly. Note that >>> the internal function block_status() is still sector-based, because >>> it is still in use by other sector-based functions; but that's okay >>> because request_alignment is 512 as a result of those functions. >>> For now, no optimizations are added based on the mapping hint. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> { >>> BDRVParallelsState *s = bs->opaque; >>> - int64_t offset; >>> + int count; >>> >>> + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); >>> qemu_co_mutex_lock(&s->lock); >>> - offset = block_status(s, sector_num, nb_sectors, pnum); >>> + offset = block_status(s, offset >> BDRV_SECTOR_BITS, >>> + bytes >> BDRV_SECTOR_BITS, &count); >>> qemu_co_mutex_unlock(&s->lock); >>> >>> + *pnum = count * BDRV_SECTOR_SIZE; >>> if (offset < 0) { >>> return 0; >>> } >>> >>> + *map = offset; >> oh, didn't notice previous time :(, should be >> >> *map = offset * BDRV_SECTOR_SIZE >> > Oh, right. > >> (also, if you already use ">> BDRV_SECTOR_BITS" in this function, >> would not it be more consistent to use "<< BDRV_SECTOR_BITS" >> for sector-to-byte conversion?) >> >> with that fixed (with "<<" or "*", up to you), > '>> BDRV_SECTOR_BITS' always works. You could also write '/ > BDRV_SECTOR_SIZE' (the compiler should figure out that you are dividing > by a power of 2, and use the shift instruction under the hood), but I > find that a bit harder to reason about. > > On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size > output as the input; if the left side is 32 bits, it risks overflowing. > But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've > learned (from past mistakes in other byte-conversion series) that the > multiply form is less likely to introduce unintended truncation bugs. hm, now I doubt. I've wrote tiny program: #include <stdint.h> #include <stdio.h> int main() { uint32_t blocks = 1 << 20; int block_bits = 15; uint32_t block_size = 1 << block_bits; uint64_t a = blocks * block_size; uint64_t b = blocks << block_bits; uint64_t c = (uint64_t)blocks * block_size; uint64_t d = (uint64_t)blocks << block_bits; printf("%lx %lx %lx %lx\n", a, b, c, d); return 0; } and it prints 0 0 800000000 800000000 for me. So, no difference between * and <<... > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>
On 12/11/2017 09:24 AM, Vladimir Sementsov-Ogievskiy wrote: >> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size >> output as the input; if the left side is 32 bits, it risks overflowing. >> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've >> learned (from past mistakes in other byte-conversion series) that the >> multiply form is less likely to introduce unintended truncation bugs. > > hm, now I doubt. I've wrote tiny program: > > #include <stdint.h> > #include <stdio.h> > > int main() { > uint32_t blocks = 1 << 20; > int block_bits = 15; > uint32_t block_size = 1 << block_bits; > uint64_t a = blocks * block_size; > uint64_t b = blocks << block_bits; > uint64_t c = (uint64_t)blocks * block_size; Not the same. 'block_size' in your code is 'uint32_t', so your multiplication is still 32-bit if you don't cast blocks; while qemu has:: include/block/block.h:#define BDRV_SECTOR_BITS 9 include/block/block.h:#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS) and the 1ULL in the qemu definition forces 'unsigned long long' results whether or not you cast.
12.12.2017 19:32, Eric Blake wrote: > On 12/11/2017 09:24 AM, Vladimir Sementsov-Ogievskiy wrote: > >>> On the other hand, '<< BDRV_SECTOR_BITS' only produces the same size >>> output as the input; if the left side is 32 bits, it risks overflowing. >>> But '* BDRV_SECTOR_SIZE' always produces a 64-bit value. So I've >>> learned (from past mistakes in other byte-conversion series) that the >>> multiply form is less likely to introduce unintended truncation bugs. >> hm, now I doubt. I've wrote tiny program: >> >> #include <stdint.h> >> #include <stdio.h> >> >> int main() { >> uint32_t blocks = 1 << 20; >> int block_bits = 15; >> uint32_t block_size = 1 << block_bits; >> uint64_t a = blocks * block_size; >> uint64_t b = blocks << block_bits; >> uint64_t c = (uint64_t)blocks * block_size; > Not the same. 'block_size' in your code is 'uint32_t', so your > multiplication is still 32-bit if you don't cast blocks; while qemu has:: > > include/block/block.h:#define BDRV_SECTOR_BITS 9 > include/block/block.h:#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS) > > and the 1ULL in the qemu definition forces 'unsigned long long' results > whether or not you cast. > Ah, cool, missed this. And we can't do such thing for BDRV_SECTOR_BITS because it will be unspecified behavior. Thank you for explanation.
diff --git a/block/parallels.c b/block/parallels.c index 9545761f49..9a3d3748ae 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -280,23 +280,31 @@ static coroutine_fn int parallels_co_flush_to_os(BlockDriverState *bs) } -static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs, - int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file) +static int coroutine_fn parallels_co_block_status(BlockDriverState *bs, + bool want_zero, + int64_t offset, + int64_t bytes, + int64_t *pnum, + int64_t *map, + BlockDriverState **file) { BDRVParallelsState *s = bs->opaque; - int64_t offset; + int count; + assert(QEMU_IS_ALIGNED(offset | bytes, BDRV_SECTOR_SIZE)); qemu_co_mutex_lock(&s->lock); - offset = block_status(s, sector_num, nb_sectors, pnum); + offset = block_status(s, offset >> BDRV_SECTOR_BITS, + bytes >> BDRV_SECTOR_BITS, &count); qemu_co_mutex_unlock(&s->lock); + *pnum = count * BDRV_SECTOR_SIZE; if (offset < 0) { return 0; } + *map = offset; *file = bs->file->bs; - return (offset << BDRV_SECTOR_BITS) | - BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; + return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID; } static coroutine_fn int parallels_co_writev(BlockDriverState *bs, @@ -793,7 +801,7 @@ static BlockDriver bdrv_parallels = { .bdrv_open = parallels_open, .bdrv_close = parallels_close, .bdrv_child_perm = bdrv_format_default_perms, - .bdrv_co_get_block_status = parallels_co_get_block_status, + .bdrv_co_block_status = parallels_co_block_status, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_flush_to_os = parallels_co_flush_to_os, .bdrv_co_readv = parallels_co_readv,
We are gradually moving away from sector-based interfaces, towards byte-based. Update the parallels driver accordingly. Note that the internal function block_status() is still sector-based, because it is still in use by other sector-based functions; but that's okay because request_alignment is 512 as a result of those functions. For now, no optimizations are added based on the mapping hint. Signed-off-by: Eric Blake <eblake@redhat.com> --- v5: fix pnum when return is 0 v4: rebase to interface tweak, R-b dropped v3: no change v2: rebase to mapping parameter; it is ignored, so R-b kept --- block/parallels.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)