|
| From: | Lin Ma |
| Subject: | Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status |
| Date: | Thu, 25 Jun 2020 13:07:14 +0000 |
|
On 2020-06-25 20:59, Lin Ma wrote: > From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 6:25 PM
> ...
>> 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.
I'll add the comments.
>> +{
>> + BlockDriverState *bs = child->bs;
>
> Why does this function take a BdrvChild argument instead of
> BlockDriverState? Most I/O functions take BlockDriverState.
OK, I'll use BlockDriverState instead.
>> + 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.
As you mentioned in comment of patch 3/3, is_deallocated boolean is not enough
to represent 3 states. I'll keep the uint32_t *, but rename 'is_deallocated' to
'provisioning_status'
> Please initialize is_deallocated to false at the beginning of the
> function to avoid accidental uninitialized variable accesses in the
> caller.
It has already been initialized to 0 by 'data = "" 1);'
in function scsi_disk_emulate_get_lba_status of patch 3/3, Do I still need to
initialize it at the beginning of this function?
>> + } 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?
OK, I'll change it to '*num_blocks += pnum / bytes;'
The 'bytes' is the logical block size(default value is 512).
In fact, 'count' has the same meaning as 'pnum', It is the number of bytes
(including and immediately following the specified offset) that are known to
be in the same allocated/unallocated state. I'll rename the 'count' to 'pnum'.
Once this function returns, The number of contiguous logical blocks sharing
the same provisioning status as the specified logical block will be saved into
'num_blocks'.
>> + } 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.
|
| [Prev in Thread] | Current Thread | [Next in Thread] |