On Wed, Jun 17, 2020 at 06:30:16PM +0800, Lin Ma wrote: > The get lba status wrapper based on the bdrv_block_status. The following > patches will add GET LBA STATUS 16 support for scsi emulation layer. > > Signed-off-by: Lin Ma <l...@suse.com> > --- > block/io.c | 43 +++++++++++++++++++++++++++++++++++++++ > include/block/block_int.h | 5 +++++ > 2 files changed, 48 insertions(+) > > diff --git a/block/io.c b/block/io.c > index df8f2a98d4..2064016b19 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2208,6 +2208,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild > *child, int64_t offset, > BDRV_REQ_ZERO_WRITE | flags); > } > > +int coroutine_fn > +bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes, > + uint32_t *num_blocks, uint32_t *is_deallocated)
Missing doc comments. > +{ > + BlockDriverState *bs = child->bs; Why does this function take a BdrvChild argument instead of BlockDriverState? Most I/O functions take BlockDriverState. > + int ret = 0; > + int64_t target_size, count = 0; > + bool first = true; > + uint8_t wanted_bit1 = 0; > + > + target_size = bdrv_getlength(bs); > + if (target_size < 0) { > + return -EIO; > + } > + > + if (offset < 0 || bytes < 0) { > + return -EIO; > + } > + > + for ( ; offset <= target_size - bytes; offset += count) { > + ret = bdrv_block_status(bs, offset, bytes, &count, NULL, NULL); > + if (ret < 0) { > + goto out; > + } > + if (first) { > + if (ret & BDRV_BLOCK_ZERO) { > + wanted_bit1 = BDRV_BLOCK_ZERO >> 1; > + *is_deallocated = 1; This is a boolean. Please use bool instead of uint32_t. Please initialize is_deallocated to false at the beginning of the function to avoid accidental uninitialized variable accesses in the caller. > + } else { > + wanted_bit1 = 0; > + } > + first = false; > + } > + if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) { > + (*num_blocks)++; If there is a long span of allocated/deallocated blocks then this function only increments num_blocks once without counting the number of blocks. I expected something like num_blocks += pnum / block_size. What is the relationship between bytes, count, and blocks in this function? > + } else { > + break; > + } > + } > +out: > + return ret; > +} > + > /* > * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or > not. > */ > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 791de6a59c..43f90591b9 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1296,6 +1296,11 @@ int coroutine_fn > bdrv_co_block_status_from_backing(BlockDriverState *bs, > int64_t *pnum, > int64_t *map, > BlockDriverState **file); > +int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child, > + int64_t offset, > + int64_t bytes, > + uint32_t *num_blocks, > + uint32_t *is_deallocated); Should this function be in include/block/block.h (the public API) so that any part of QEMU can call it? It's not an internal API.
signature.asc
Description: PGP signature