On Mon, Apr 28, 2025 at 01:58:59AM +0300, Nir Soffer wrote:
> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
> confuse users or other programs:
>
> % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver':
> 'null-co', 'size': '1g'}}"
> [{ "start": 0, "length": 1073741824, "depth": 0, "present": false,
> "zero": false, "data": false, "compressed": false}]
>
> % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size':
> '1g'}}" &
>
> % nbdinfo --map nbd://127.0.0.1
> 0 1073741824 1 hole
>
> With this change we report DATA in this case:
>
> % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver':
> 'null-co', 'size': '1g'}}"
> [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero":
> false, "data": true, "compressed": false, "offset": 0}]
Do we really want to represent the region as present? I think we're
okay (we guarantee that reads from the region will return sensible
contents), and your next patch to allow control over what data is seen
makes it even better.
>
> % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co',
> 'size': '1g'}}" &
>
> % nbdinfo --map nbd://127.0.0.1
> 0 1073741824 0 data
>
> Signed-off-by: Nir Soffer <nir...@gmail.com>
> ---
> block/null.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/block/null.c b/block/null.c
> index dc0b1fdbd9..7ba87bd9a9 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -239,9 +239,7 @@ static int coroutine_fn
> null_co_block_status(BlockDriverState *bs,
> *map = offset;
> *file = bs;
>
> - if (s->read_zeroes) {
> - ret |= BDRV_BLOCK_ZERO;
> - }
> + ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
> return ret;
According to include/block/block-common.h,
* BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer
* BDRV_BLOCK_ZERO: offset reads as zero
* BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
Or read differently, BDRV_BLOCK_DATA says an area is not sparse. This
patch is changing the null driver to report the entire image as either
sparse and zero, or not sparse and not zero.
For testing purposes, I think that is reasonable enough; it matches
that file-posix.c always returns one or the other, when using only
lseek() to probe things. NBD has a bit finer-grained control (it has
two bits, one for sparseness which is effectively !BDRV_BLOCK_DATA,
and one for zero; where you can have something that is sparse but does
not read as zero, such as SCSI that lacks coherent uninitialized
read). In fact, you could even have DATA|ZERO if we know something is
allocated AND that it reads as zero (which will be the case in your
next patch if the user explicitly asks for a read pattern of 0).
But for all of that thinking, in the end your patch makes sense to me.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
