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 <[email protected]> > --- > 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
