> On 29 Apr 2025, at 0:49, Eric Blake <ebl...@redhat.com> wrote:
>
> 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.
I kept the existing behavior, but I think present=false is relevant only if you
have a backing file (like qcow2), so for null-co it makes sense to behave like
file driver.
>
>
>>
>> % ./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).
If we want this we can remove read_zeros, since read_pattern support reading
zeros, and have:
*read_pattern: uint8 (default unset)
*block_status: “data"|”zero” (default data?)
*allocated: bool (default true)
We can replace reads-zeros=on with read-pattern=0 in tests and docs, but it may
break other users using null-co.
Maybe keep read_zeros for backward compatibility, and make it conflict with the
new options?
>
> 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
