[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status |
Date: |
Mon, 22 Jun 2020 11:25:58 +0100 |
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 <lma@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